fix(drizzle): do not read an uncastable equals operand as a null check - #17841
Open
creativoma wants to merge 1 commit into
Open
fix(drizzle): do not read an uncastable equals operand as a null check#17841creativoma wants to merge 1 commit into
creativoma wants to merge 1 commit into
Conversation
A scalar operand that cannot be cast to its column's type was coerced to null, and parseParams compiles a null value for equals/not_equals into IS NULL / IS NOT NULL. On a nullable column that silently answers a different question: `equals: 'not-a-uuid'` returned every document whose value was empty, and `not_equals` returned every document that had one. equals/not_equals now report such an operand as unmatchable, so the clause matches nothing rather than becoming a null check. Every other operator keeps coercing to null, which preserves the existing failure modes - notably `contains` against a native uuid column, and the free-text id search that parseParams rewrites from `like` to `equals`. Number fields had the identical defect via Number(val) -> NaN, and are fixed on the same path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
A scalar operand that cannot be cast to its column's type — a non-uuid against a
uuidcolumn, a non-numeric string against a numeric one — was coerced tonullinsanitizeQueryValue.parseParamsthen compiles a null value forequals/not_equalsintoIS NULL/IS NOT NULL, so the query silently answered a different question than the one asked.Those two operators now report the operand as unmatchable instead, and the clause resolves to "matches nothing" (
not_equals: "matches everything"). Every other operator keeps coercing to null exactly as before.Fixes #16483
Why?
On a nullable column the result is wrong data rather than an error. From the issue, on a
uuidadapter with a relationship field:equals: "<valid uuid, no matching documents>"→ no documents. Correct.equals: "invalid-something"→ every document whose relationship is empty.Nobody asked for the empty ones. The caller asked for
equals: "invalid-something", notequals: null.Two things worth adding to the issue's own analysis:
numberfields have the identical defect.equals: 'abc'on a number field isNumber('abc')→NaN→null→IS NULL. This fixes both paths.not_equalsis affected too, and more loudly: it becameIS NOT NULL, returning every document that has a value.On an
idcolumn the old behaviour happens to be harmless, since an id is never null —IS NULLmatches nothing, which is the right answer for a value that cannot exist. It is nullable columns where the conflation produces wrong rows.Why not throw?
The issue thread proposed rejecting an invalid uuid with a validation error. I tried that first, and this repository's own tests rule it out:
like - id should not crash(test/collections-rest/int.spec.ts) sendswhere[id][like]=words partialand asserts a 200. A free-text search box pointed at an id column is not a malformed query, and must not fail.equals/not_equalsdoes not help, becauseparseParamsrewritesliketoequalsfornumber,relationship,uploadandPgUUIDcolumns beforesanitizeQueryValueever sees it. At the point of the fix the two are indistinguishable.So the value has to resolve to a constraint, not an exception. "Matches nothing" also happens to be what the rewritten
likewants: the search found nothing, which is true.How?
UnmatchableValue, a new symbol inpackages/drizzle/src/utilities/, marks an operand that cannot be cast.sanitizeQueryValuereturns it early — before the branches that inspect the value, so nothing downstream reinterprets the symbol — andparseParamstranslates it next to the existing null checks:A new signal is needed because neither existing mechanism fits:
rawConstraintinjects a raw value and bypasses sanitising altogether, and returningnullfromsanitizeQueryValuemakesparseParamsbreakand drop the constraint — which matches everything, the opposite of what is wanted.Scoping it to
equals/not_equalsis deliberate, and keeps pre-existing failure modes intact.containsagainst a nativeuuidcolumn, for one, still surfaces Postgres' own "cannot ILIKE a uuid" error rather than quietly matching nothing —does not wrap a native PgUUID id column in unaccent()pins that, and it stays green.Two behaviour changes to call out explicitly, both beyond the letter of the issue:
not_equalswith an uncastable operand goes fromIS NOT NULLto matching every row, null ones included. "Not equal to a value that cannot exist" is every row; the old answer was wrong in a different direction.sanitizeQueryValuegrows a third possible value, and that function is shared by all four drizzle adapters.Tests
packages/drizzle/src/queries/sanitizeQueryValue.spec.ts(new) covers both column types: the unmatchable report, the operator preserved so the caller can negate,'null'and''still reading as null checks, valid operands untouched, and other operators still coercing to null.Verified locally:
build:core,test:unit,test:types, and thedatabase,collections-rest,fields-relationship,joins,query-presets,group-byandversionsint suites onpostgres,postgres-uuidandsqlite— 1087 integration tests, no failures.This targets
main(v4); the3.xbranch carries the same code if a backport is wanted.It also overlaps with #17839, which adds the same new spec file — that one fixes the array-operand half of this defect (
in/not_inwith an empty operand reaching the driver and 500ing). The two are independent and either can land first; I am happy to rebase whichever follows.