fix: refuse filters and sorts that cannot be compiled to SQL#1160
Open
Helveg wants to merge 3 commits into
Open
fix: refuse filters and sorts that cannot be compiled to SQL#1160Helveg wants to merge 3 commits into
Helveg wants to merge 3 commits into
Conversation
Three ways to ask for something the database cannot do, each of which reached it and
failed there instead of being refused:
`$eq` bypassed the operator allowlist. `filterableColumns: { age: [FilterOperator.GT] }`
still accepted `filter.age=5`, so a column could not be narrowed to the operators it
supports — the README's promise to reject "a non-whitelisted filter operator" did not
hold for the one operator every filter defaults to.
`$eq` cannot simply be required, though: it is the operator a suffix negates and a
quantifier quantifies, and it is implicit. `[FilterSuffix.NOT]` with `$not:John`, and
`[FilterQuantifier.NONE]` with `$none:red`, name no operator at all, and would become
undeclarable. A whitelisted modifier therefore licenses the `$eq` it modifies, spelled
out or not; a standalone `$eq` is whitelisted like any other operator.
A value filter on a `json` column compiled to `col = :v` — `operator does not exist:
json = unknown`. Postgres `json` has no equality or comparison operators at all (that is
`jsonb`), so no allowlist can make this work, and `filterableColumns: { col: true }`
could not be guarded against. Only emptiness is askable of the column itself; its
contents are reached through a key path, which is unaffected.
Sorting by a column that names no single value emitted `column
distinctAlias.__root_<fk> does not exist`. A path ending on a relation has no value of
its own, and a `json` column has no ordering operators; either compiles to an ORDER BY
over a column the paginated DISTINCT subquery never selects. Ordering *through* a
to-many (`toys.id`) does name a value and stays allowed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Refusing to sort a `json` column closed the crash (`could not identify an ordering operator for type json`) but shipped an inconsistency: the `jsonb` column beside it sorts fine, because `jsonb` has a total order and `json` has none. The cast between them is exact, so order the `json` column as `jsonb`. It now sorts like its neighbour instead of falling back to the default sort. The cast makes the ORDER BY a raw expression, which the query builder no longer escapes, so the identifier is escaped here — otherwise Postgres folds it to lower case and cannot find the column. Only a path that ends on a relation is still dropped: that names no value at all, and no cast can give it one. Covers ppetzold#1109 with a regression test: `$eq:1` against a `text[]` column, which the allowlist fix now refuses before it can reach the database as `malformed array literal`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ordering by `<col>::jsonb` works on a bare query and breaks on a paginated one:
`take` wraps the query in a DISTINCT subquery, and the query builder resolves every
ORDER BY back to a column it selected. A pre-escaped identifier is not one it recognises,
so it looks for an alias literally named `"__root"`, quotes included, and fails with
TypeORMError: ""__root"" alias was not found. Maybe you forgot to join it?
Select the cast under an alias and order by that instead — the same shape the virtual and
jsonb-path sorts already use.
The earlier test only sorted a bare query, so it never reached the subquery path. Cover
the shape that does: a json sort with a relation joined and a limit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Three ways to ask for something the database cannot do. Each reached the database and failed there, instead of being answered.
Fixes #1111
Fixes #1109
1.
$eqbypassed the operator allowlist (#1111)A column could not be narrowed to the operators it actually supports. The README promises to reject "a non-whitelisted filter operator/suffix"; that did not hold for the one operator every filter defaults to.
It is also how #1109 happens:
filterableColumns: { list: [CONTAINS] }still lets$eq:1through to anint[]column, which dies asmalformed array literal. Refusing the operator refuses the 500.$eqcannot simply be required, though — it is the operator a suffix negates and a quantifier quantifies, and it is implicit:Requiring
EQin the list would make[FilterSuffix.NOT]and[FilterQuantifier.NONE]undeclarable. So: a whitelisted modifier licenses the$eqit modifies —$not:Johnand$not:$eq:Johnalike — and licenses nothing else. A standalone$eq, bare or spelled out, must be whitelisted like any other operator.2. A value filter on a top-level
jsoncolumn emitted invalid SQLTo be clear about what is already there:
jsonis cast tojsonbtoday.JsonContainsemits<col>::jsonb @> :value, exactly asJSON_COLUMN_TYPESdocuments. But it is only reached for a columnresolveJsonbPathrecognises, and that function returns early on a single-segment name:So a key path (
metadataJson.length) and a relation-prefixed column (underCoat.metadataJson) are both cast, and both work. A top-levelmetadataJsonis not: it falls through to a plainEqual()→col = :v→ the error above. Sorting it fails the same way (could not identify an ordering operator for type json).Widening that early return so a top-level JSON column is recognised would route it through
JsonContainsand fix the crash. It would also change what$eqmeans on a top-level barejsonbcolumn — from equality (=, what it does today) to containment (@>, whatunderCoat.metadatadoes today). That divergence is worth resolving, but not silently, inside this PR.So this PR takes the narrow option and refuses a value filter on the column itself. Its contents are still reached through a key path, and its emptiness through
$null. Whole-document equality is also not something$in/$btwcan spell — their values are comma-separated, and a JSON document contains commas.3. Sorting by a column that names no single value emitted invalid SQL
A path that ends on a relation has no value of its own, so it is dropped like any other unsortable column and falls back to
defaultSortBy.A
jsoncolumn does name a value — Postgres just cannot order it, whilejsonbhas a total order and the cast between them is exact. So it is ordered asjsonb, and sorts like thejsonbcolumn beside it.Ordering through a to-many (
toys.id) names a value and is unaffected — covered by a test.Tests
Each behaviour is covered: the allowlist rule and the
$not/$noneidioms it must keep working; the #1109 array case; the json column as filter target and as sort target, against itsjsonbtwin; and the relation-terminated sort.Green on sqlite (350), postgres (381) and mariadb (348). The one failing Postgres test is the pre-existing virtual-column sort tie, which fails identically on an untouched
masterand is fixed by #1158.🤖 Generated with Claude Code