Fix _id where-clauses matching documents from other models - #411
Fix _id where-clauses matching documents from other models#411stickerdaniel wants to merge 1 commit into
Conversation
|
@stickerdaniel is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
findOne/findMany resolved _id where-clauses with a bare ctx.db.get, which returns whatever document the id points to regardless of the requested model. Resolve through ctx.db.normalizeId first so foreign ids and invalid id strings are treated as no match.
3b715a3 to
18caf0f
Compare
📝 WalkthroughWalkthroughThe adapter now normalizes string Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/adapter-utils.ts (1)
525-535: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect fix; consider extracting the duplicated normalize-and-get pattern.
The model-scoped
normalizeId→getpattern correctly treats foreign-table/invalid_idvalues as no-match rather than throwing or mismatching, per Convex's documentednormalizeIdcontract: "This does not guarantee that the ID exists (i.e. db.get(id) may return null)." and the underlying implementation confirms it returnsnullfor foreign-table/invalid ids rather than throwing.The unique-
eqbranch (Lines 529-535) and thein-clause branch (Lines 579-583) now implement identical logic (typeof value === "string" ? ctx.db.normalizeId(model, value) : null, then conditionallyctx.db.get). Extracting this into a small shared helper would keep both call sites in sync if this logic needs to change again later (e.g. supporting additional legacy ID formats).♻️ Suggested helper extraction
+const getByNormalizedId = async <T extends TableNamesInDataModel<GenericDataModel>>( + ctx: GenericQueryCtx<GenericDataModel>, + model: T, + value: unknown +) => { + const id = typeof value === "string" ? ctx.db.normalizeId(model, value) : null; + return id && (await ctx.db.get(model, id)); +}; + ... - const uniqueWhereId = - uniqueWhere.field === "_id" && typeof uniqueWhere.value === "string" - ? ctx.db.normalizeId(args.model as T, uniqueWhere.value) - : null; const doc = uniqueWhere.field === "_id" - ? uniqueWhereId && (await ctx.db.get(args.model, uniqueWhereId)) + ? await getByNormalizedId(ctx, args.model as T, uniqueWhere.value) : await ctx.db .query(args.model as any) ...const docs = await asyncMap(inWhere.value as any[], async (value) => { - const id = - typeof value === "string" - ? ctx.db.normalizeId(args.model as T, value) - : null; - return id && (await ctx.db.get(args.model, id)); + return getByNormalizedId(ctx, args.model as T, value); });Also applies to: 577-583
🤖 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-utils.ts` around lines 525 - 535, Extract the duplicated model-scoped normalize-and-get logic from the unique “_id” branch and the “in” clause branch into a shared helper near the relevant adapter utilities. Have the helper accept the model and value, normalize only string IDs, and conditionally call ctx.db.get so invalid or foreign-table IDs return no match; update both call sites to reuse it.
🤖 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-utils.ts`:
- Around line 525-535: Extract the duplicated model-scoped normalize-and-get
logic from the unique “_id” branch and the “in” clause branch into a shared
helper near the relevant adapter utilities. Have the helper accept the model and
value, normalize only string IDs, and conditionally call ctx.db.get so invalid
or foreign-table IDs return no match; update both call sites to reuse it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59350134-97bf-4ed8-b9c1-c3788b6b2fb9
📒 Files selected for processing (2)
src/client/adapter-utils.tssrc/test/adapter-factory/convex-custom.ts
Fixes #410.
findOne/findManywith an_idwhere-clause resolved the id with a barectx.db.get(value), which returns whatever document the id points to regardless of the requested model, so any valid id of another table matched._idvalues throughctx.db.normalizeId(args.model, value)first: foreign-table ids and invalid id strings both become "no match". This also covers the UUID values passed by better-auth's adapter tests that motivated the@convex-dev/explicit-table-idsexemption in use convex linter #347 (convex-test'snormalizeIdreturns null for them), so the exemption is removed. With it gone, everyctx.db.getin the package is table-scoped.inbranch, which since use convex linter #347 throwsInvalidTableon foreign ids instead of filtering them out.inthrows).Update/delete flows resolve their target through the same
paginatepath, so they are covered transitively. Side note: the two-argdb.get(table, id)overload needs convex 1.31+, which the package already de facto requires since #347 (the create and in-branch paths use it), while theconvexpeerDependency still says^1.25.0; might be worth bumping separately.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.