Repository navigation
Fix AND where clauses being dropped when combined with OR clauses - #404
moritz-mode wants to merge 1 commit into
Conversation
|
@MauruschatM is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
|
Applied it as a patch file in my project and it's running smooth in production. |
📝 WalkthroughWalkthroughThe adapter's where-clause handling was reworked to correctly support mixed AND/OR semantics. New helper functions split incoming where clauses into OR-branches and shared AND-clauses, then combine each OR branch with all AND clauses for querying. This replaces prior logic that only handled OR when clauses were uniformly OR or when any clause was OR. The change affects Sequence Diagram(s)sequenceDiagram
participant Caller
participant Adapter
participant SplitWhereByConnector
participant ConvexQuery
Caller->>Adapter: findOne/findMany/count(where)
Adapter->>SplitWhereByConnector: splitWhereByConnector(where)
SplitWhereByConnector-->>Adapter: orClauses, andClauses
loop each orClause
Adapter->>ConvexQuery: query(orBranchWhere(orClause, andClauses))
ConvexQuery-->>Adapter: matching records
end
Adapter-->>Caller: combined/deduped results
Related PRs: None found. Suggested labels: bug, adapter, tests Suggested reviewers: get-convex/better-auth maintainers 🐰 A rabbit split its whiskers OR from AND, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/adapter.ts (1)
268-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: extract the shared OR-branch fetch/dedupe pattern.
The
asyncMap(orClauses, … handlePagination(… findMany({ where: parseWhere(orBranchWhere(w, andClauses)) })))followed bydedupeDocsById(results.flatMap((r) => r.docs))is now duplicated incollectIdsForOrWhere(here),findMany(Lines 342-355), andcount(Lines 385-395). A single helper returning the deduped docs for(orClauses, andClauses, { limit })would keep these branches from diverging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/adapter.ts` around lines 268 - 284, The OR-branch fetch/dedupe flow is duplicated across collectIdsForOrWhere, findMany, and count, so extract it into a shared helper that takes the OR/AND clauses plus optional pagination/limit inputs and returns the deduped docs or IDs. Update the logic in collectIdsForOrWhere, findMany, and count to call that helper instead of inlining asyncMap(handlePagination(ctx.runQuery(api.adapter.findMany, ...))) and dedupeDocsById(results.flatMap((r) => r.docs)), keeping parseWhere(orBranchWhere(...)) and getDocId usage centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/client/adapter.ts`:
- Around line 268-284: The OR-branch fetch/dedupe flow is duplicated across
collectIdsForOrWhere, findMany, and count, so extract it into a shared helper
that takes the OR/AND clauses plus optional pagination/limit inputs and returns
the deduped docs or IDs. Update the logic in collectIdsForOrWhere, findMany, and
count to call that helper instead of inlining
asyncMap(handlePagination(ctx.runQuery(api.adapter.findMany, ...))) and
dedupeDocsById(results.flatMap((r) => r.docs)), keeping
parseWhere(orBranchWhere(...)) and getDocId usage centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e270ea4-d926-424f-92f6-4fe9041fcd22
📒 Files selected for processing (2)
src/client/adapter.tssrc/test/adapter-factory/convex-custom.ts
Better Auth (and its SQL/Mongo adapters) treat clauses with connector "OR" as one OR group AND'd with all other clauses. The adapter ran every clause as its own OR branch, so AND clauses were silently dropped from findMany, count, updateMany and deleteMany (which then wrote/deleted rows outside the filter). findOne with an all-OR where and no match fell through to a query the component rejects instead of returning null. Run one component query per OR clause, each with all AND clauses, and union the results. Ports convex#404. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the fix. I found a case where I reproduced this with Could the mutation recheck the full filter before writing? A test could also cover a record that moves between OR alternatives but still matches the filter. |
Motivation
Better Auth where semantics: clauses with
connector: "OR"form a single OR-group that is AND'd with the remaining (implicitly AND) clauses —AND(...) AND OR(...). The Convex adapter currently unions every clause as an OR branch, so AND filters are silently dropped. For example, the dashboard's user search sends its search term as OR clauses combined with acreatedAtrange filter — the range filter is ignored and the query returns rows outside the range. This affectsfindOne,findMany,count, and (viacollectIdsForOrWhere)updateMany/deleteMany.Separately,
findOnewith an all-OR where and no matching branch fell through to a combined query that the component rejects (OR connector not supported with multiple where statements) instead of returningnull.Changes
whereis partitioned into the OR-group and the AND clauses (splitWhereByConnector); each OR branch runs as one component query AND'd with the AND clauses (orBranchWhere), and results are unioned/deduped.findOne,findMany,count, andupdateMany/deleteManyviacollectIdsForOrWhere.findOnereturnsnullwhen no OR branch matches instead of falling through.Behavior change
updateMany/deleteManywith a mixed AND/OR where now affect only rows matching(OR-group) AND (AND clauses)— previously they affected the broader all-OR union. That is the bug being fixed, but any caller relying on the old behavior will see fewer rows affected.Tests
New
convex-customtests: OR+AND combination forfindMany/count/findOne,findOne→nullwhen no OR branch matches,updateManywith mixed clauses.npm run build,npm run typecheck(src), and the full vitest suite pass: 10 files, 185 passed | 31 skipped (main baseline: 182 passed | 31 skipped).Note: this touches the same
findManyregion as the offset PR; whichever merges second will be rebased promptly.