Conversation
…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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe 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. ChangesBatch create policy enforcement
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
packages/plugins/policy/src/policy-handler.tsESLint 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. Comment |
Fixes #2841.
Problem
createManyon a policy-protected model checks each row with its ownselect exists (...)round trip, awaited in sequence. When the create filter is built only fromauth()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 evaluatingwhere true.Change
enforcePreCreatePolicynow builds the model-level create filter once and returns early whenisTrueNode(filter)holds.preCreateCheckalready had a constant-policy short-circuit, but it goes throughtryGetConstantPolicy→isTrueExpr, which only matches a literaltruewritten in the ZModel. Anauth()-derived predicate is never a literal, so it never qualified.preUpdateCheckalready tests the built node withisTrueNode; 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) = nshape 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.tscovers three cases:auth()-only create filter issues no policy check for a 5-rowcreateMany, and all rows landauth()makes it falseThe second and third are the ones that would catch this change going too far.
Verification
tests/e2e/orm/policy: 46 files, 281 passed, 21 skipped, no type errorstests/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 cleandevwith this change reverted and the package rebuilt, so it's pre-existing and unrelated. It usescreateTestClientwith no policy plugin, soPolicyHandlerisn't constructed at all in that test.prettier --checkclean on both filesSummary by CodeRabbit