Repository navigation
Fix $not where clauses matching no rows - #365
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe where builder now applies middleware to the top-level clause and builds nested clauses without applying middleware again. ChangesWhere Clause Handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change corrects negated predicates and prevents repeated middleware application. No concrete merge-blocking risk remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The corrected predicates preserve the demonstrated soft-delete filter and restore intended negation behavior. Custom middleware now runs only on the complete predicate, rather than on each logical branch. Production integrations were not established, leaving limited uncertainty about policy compatibility. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
A `[$not]` clause came out as SQL that could not match any row, so
queries using it silently did nothing. Two bugs compounded in
buildWhere:
The recursive call re-ran applyWhereMiddleware, so middleware injected
its conditions inside the NOT as well as at the top level. Negating them
there contradicts the top-level copy — `deleted_at IS NULL AND NOT
(deleted_at IS NULL AND ...)` is never true. Middleware now applies once,
to the top-level clause, with an inner buildWhereClause handling the
recursion. This also stops $and and $or repeating the conditions in
every branch, where they were redundant rather than wrong.
`NOT ${subclause}` was also unbracketed, so NOT bound to the first
conjunct only: a $not over several plain clauses was mis-negated with no
middleware involved at all.
Covers both with unit tests on the generated SQL, and an end-to-end
deleteMany regression test under soft-delete middleware.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ea3d923 to
d6ae947
Compare
Coverage Report
|
1 participant
A
[$not]clause in awherecame out of the ORM as SQL that cannot match any row, so queries using it silently did nothing. Fixes CK-108.Two bugs compounded in
buildWhere.Middleware was re-applied on every recursion. The recursive call ran
applyWhereMiddlewareagain, so middleware injected its conditions inside theNOTas well as at the top level. Negating them there contradicts the top-level copy:Middleware now applies once, to the top-level clause, with a private
buildWhereClausehandling the recursion. This also stops$andand$orrepeating the conditions in every branch, where they were redundant rather than wrong.NOTwas unbracketed.sql`NOT ${subclause}`letNOTbind to the first conjunct only, so what was asked for asNOT (field_id IN (...))came out asNOT (deleted_at IS NULL) AND field_id IN (...).$andand$oralready bracket their subclauses;$notnow does too. This one mis-negates a$notover several plain clauses with no middleware involved at all.Tests
buildWhere.test.tsgets awhere middleware with logical operatorsblock asserting the generated SQL for$not,$and,$orand a nested combination under an injecting middleware — the combination the ticket called out as untested — plus a$notover multiple conditions for the bracketing bug on its own. The existingbuilds NOT clauseexpectation becomesNOT ("u"."active" IS TRUE).orm.middleware.where.test.tsgets an end-to-enddeleteManytest using[$not]: { id: { [$in]: [...] } }under soft-delete middleware, asserting the right rows actually go.Verified by stashing the fix: 7 assertions fail, the end-to-end one with
expected +0 to be 2— the reported symptom. With the fix the full suite, typecheck, lint and format are clean.Notes for reviewers
$and/$ornow throws "must not be empty" where middleware could previously have filled it in. That follows from applying middleware once, but it is a visible change if anyone relied on it. Nothing in the suite did..changeset/is configured but no recent commit includes one, so I matched current practice. Happy to add one.savePersonType.procedure.test.ts > leaves the dropped field's formField row behindshould be flipped to assert the row is gone and its comment removed, and the other four affected call sites want coverage. Tracked on the ticket.🤖 Generated with Claude Code
Summary by CodeRabbit