Repository navigation
fix: match unset optional fields against null where clauses - #425
suhailopensource wants to merge 1 commit into
Conversation
|
@syedsuhail-07 is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: get-convex/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe adapter’s 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
Convex omits optional fields that were never written, so they read back as
undefined and `eq null` never matched them. Every other first-party adapter
matches an unset optional field against `eq null` (SQL `IS NULL`, Mongo
`{ field: null }`), and plugins rely on it: @better-auth/oauth-provider creates
oauthRefreshToken rows with no `revoked` key, then rotates them with an
`eq null` compare-and-swap, so every refresh_token grant failed with 400
invalid_grant.
Treat an absent field as null for `eq` and the mirrored `ne`, following the
existing local-predicate pattern in filterByWhere (isLessThan for lt/lte,
isGreaterThan for gt/gte), both of which already coalesce null and undefined.
Behavior only changes when the where value is null.
Closes convex#399
818c53d to
107db27
Compare
Convex omits optional fields that were never written, and filterByWhere compared them strictly, so eq null never matched them and ne null did. Better Auth 1.7's incrementOne fallback guards on the row snapshot with eq null for every absent field it touches, so for example the two factor lockout (set lockedUntil) failed with a "contention" error. Treat undefined as null for eq/ne null. Beyond convex#425, also apply eq null statically instead of through an index range or a unique-field lookup, which only match explicit nulls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Treat absent fields as null in static eq-null and ne-null comparisons. Better Auth's refresh rotation guards an unset revoked field with eq null, so valid tokens were rejected even with native incrementOne. Add regression cases for absent, explicit-null and set fields, including repeated guarded claims. Indexed null and in/not_in behavior remain separate. Credit @suhailopensource for the same fix in convex#425, addressing the report by @a-fatah in convex#399. Co-authored-by: suhailopensource <114918598+suhailopensource@users.noreply.github.com>
Closes #399.
Problem
filterByWhereimplementedeqas strict equality (value === w.value). Convex omits optional fields that were never written, so such a field reads back asundefinedand never matchedeq null. Every other first-party adapter treats an unset optional field as matching — SQLIS NULL, Mongo{ field: null }— so plugins written against that contract broke only here.adapterWhereValidatoralready acceptsv.null()forvalue, so the input was supported; nothing handled it foreq/ne.Impact:
@better-auth/oauth-providercreatesoauthRefreshTokenrows with norevokedkey, then guards rotation with a CAS on[{ field: "id" }, { field: "revoked", operator: "eq", value: null }]. With no nativeincrementOne, core falls back tofindMany({ where, limit: 1 }); therevoked eq nullleg never matched the fresh row, so everyrefresh_tokengrant failed with 400invalid_grant, makingoffline_accessunusable. Device-authorization uses the same create-without-the-field-then-match-eq nullpattern withuserId.Fix
Treat an absent field as null when the where value is null, for
eqand the mirroredne. This follows the local-predicate pattern already infilterByWhere—isLessThanforlt/lte,isGreaterThanforgt/gte, nowisNullishforeq/ne; both existing helpers already coalescenullandundefinedthe same way. Behavior changes only when the where value isnull.Tests
Two cases in
convexCustomTestSuite, beside the existingshould preserve null to non-null range comparisons:should match unset optional fields against eq null— anaccountcreated with noaccessTokenExpiresAtkey matches viafindOneand via thefindManyshape core'sincrementOnefallback issues; an explicitly-null field still matches; a field with a value does not.should not match unset optional fields against ne null— the mirror.account.accessTokenExpiresAtis an unindexed nullable timestamp, the same shape asrevoked, so the clause is applied as a static filter rather than pushed into an index range. Without the fix these two are the only failures in the adapter suite (2 failed | 131 passed); with it,133 passed.Upstream
@better-auth/test-utilsnull coverage (eq operator with null value (single condition) should use IS NULL, the AND/OR group variants,update — ... where condition uses null value) passes both before and after this change — it exercises explicitly written nulls; the gap was only fields Convex omits.Known gap (pre-existing, not addressed here)
A null-valued
eqon an indexed field is pushed into the index range instead, and Convex index ranges match only explicit nulls:Closing it means truncating the index range at the first null-valued
eqand re-applying it (and everything after) statically, which also touchespaginate's unique short-circuit. That felt separate from this fix, and it isn't needed for #399:revokedis unindexed and the rotation CAS goes through the_idshort-circuit, both landing infilterByWhere. Happy to fold it in if you'd prefer.in/not_inshare the same asymmetry and are likewise unchanged.Verification
On Node 24 (
src/client/adapter.test.tsself-skips below it):npm test— 184 passed, 31 skipped, no type errors ·npm run typecheck— clean, including all example projects ·npm run lint— 0 errors (3 pre-existingreact-hooks/exhaustive-depswarnings insrc/react/index.tsx) ·npm run build— clean.