Skip to content

fix(policy): skip per-row create checks when the filter doesn't reference the row - #2842

Open
bgowers wants to merge 1 commit into
zenstackhq:devfrom
bgowers:fix/create-many-per-row-policy
Open

bgowers wants to merge 1 commit into
zenstackhq:devfrom
bgowers:fix/create-many-per-row-policy

Conversation

@bgowers

@bgowers bgowers commented Sep 16, 2026

Copy link
Copy Markdown

Fixes #2841.

Problem

createMany on a policy-protected model checks each row with its own select exists (...) round trip, awaited in sequence. When the create filter is built only from auth() it doesn't reference the row, so it compiles to a constant and every check re-asks the same question. A 32-row batch cost 32 serial queries, each evaluating where true.

Change

enforcePreCreatePolicy now builds the model-level create filter once and returns early when isTrueNode(filter) holds.

preCreateCheck already had a constant-policy short-circuit, but it goes through tryGetConstantPolicyisTrueExpr, which only matches a literal true written in the ZModel. An auth()-derived predicate is never a literal, so it never qualified. preUpdateCheck already tests the built node with isTrueNode; this applies the same test on the create path.

Row-dependent filters are unaffected — they don't produce a true node, so the per-row loop runs exactly as before. The many-to-many join-table path is left alone.

I kept this to the constant case. The issue also suggests checking a whole batch in one query for row-dependent filters, using the COUNT(1) = n shape already used for post-update verification — happy to follow up with that separately if you'd like it.

Tests

tests/regression/test/issue-2841.test.ts covers three cases:

  • an auth()-only create filter issues no policy check for a 5-row createMany, and all rows land
  • the same filter still rejects the batch when auth() makes it false
  • a row-referencing filter still runs one check per row, and a single offending row still rejects the whole batch

The second and third are the ones that would catch this change going too far.

Verification

  • New tests: 3 passed
  • tests/e2e/orm/policy: 46 files, 281 passed, 21 skipped, no type errors
  • tests/regression: 161 passed, 18 skipped, 1 failed — issue-2603 ("implicit m2m with models in different custom schemas"). I confirmed that one fails identically on a clean dev with this change reverted and the package rebuilt, so it's pre-existing and unrelated. It uses createTestClient with no policy plugin, so PolicyHandler isn't constructed at all in that test.
  • prettier --check clean on both files

Summary by CodeRabbit

  • Bug Fixes
    • Improved policy enforcement for batched record creation.
    • Batches governed by universally allowing policies can now be processed without unnecessary per-record checks.
    • Universally denying policies correctly reject the entire batch without creating records.
    • Policies that depend on individual record values continue to validate every record, rejecting the full batch if any record fails.

…ence the row

`createMany` on a policy-protected model checked each row with its own
`select exists (...)` round trip, awaited in sequence. When the create filter
is built only from `auth()` it carries no reference to the row, so it compiles
to a constant and every check re-asks the same question — a 32-row batch cost
32 serial queries evaluating `where true`.

`preCreateCheck` already short-circuits on a constant policy, but via
`tryGetConstantPolicy`, which only matches a literal `true` in the ZModel. This
tests the built filter with `isTrueNode` instead, the same way `preUpdateCheck`
already does.

Row-dependent filters are unaffected and still checked per row.

Fixes zenstackhq#2841
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 67b9d40e-0f48-4140-9e01-d23e38723516

📥 Commits

Reviewing files that changed from the base of the PR and between 53d0b9c and cb8e00a.

📒 Files selected for processing (2)
  • packages/plugins/policy/src/policy-handler.ts
  • tests/regression/test/issue-2841.test.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The create policy handler now skips per-row checks when a non-many-to-many model has a compiled filter that is always true. Regression tests cover constant allow and deny policies, row-dependent policies, query counts, and persisted records.

Changes

Batch create policy enforcement

Layer / File(s) Summary
Create policy short-circuit and regression coverage
packages/plugins/policy/src/policy-handler.ts, tests/regression/test/issue-2841.test.ts
enforcePreCreatePolicy builds the filter without an alias and returns early for true filters on non-many-to-many models. Many-to-many join tables retain per-row enforcement. Regression tests validate allowed batches, denied batches, row-dependent checks, policy-check queries, and persisted record counts.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: ymc9

Merge Risk: ⚪ Minimal · up to cb8e0

The batch-create optimization preserves rejection and row-dependent policy behavior while eliminating redundant checks for constant allowed policies.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: skipping redundant per-row create policy checks when the filter does not reference row data.
Linked Issues check ✅ Passed The changes satisfy the coding requirements in issue #2841. enforcePreCreatePolicy builds the compiled create filter before the row loop and returns when isTrueNode(filter) is true. This removes r…
Out of Scope Changes check ✅ Passed The changes are limited to the create-policy handler and regression tests for issue #2841. The handler change implements the constant-filter optimization. The tests verify its success, rejection, and …
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Some tools did not complete. Review the errors below.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/plugins/policy/src/policy-handler.ts

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

createMany under the policy plugin issues one serial round trip per row, even when the policy filter is constant

1 participant