diff --git a/docs/reference/search.md b/docs/reference/search.md index daa9c0e7..31d3a777 100644 --- a/docs/reference/search.md +++ b/docs/reference/search.md @@ -703,6 +703,17 @@ policy produced it. A typed surface like GraphQL makes most of these unrepresentable; the port enforces them for everyone else (deployment `queryDefaults`, in-process callers, weaker-typed surfaces). +One rule is about a **value** rather than a field: a `date` range bound the +storage codec cannot read (`'yesterday'`, or a year past the ±271,821 window +`Date` covers) is an `unparseable-bound` issue naming the rejected bound. No +surface’s type system catches this – `String` is `String` – and it has to be +caught here because **no compiler can recover from it**: an engine filter +language has no term meaning “matches nothing”, so a criterion it cannot compile +either states no constraint (widening the search to everything) or vanishes from +the query (widening it to whatever the other clauses allow). Both answer a +question the caller did not ask, and only the caller can fix the bound – so +validation hands it back to them instead of guessing. + ### Lookup by IRI Every type is filterable on **`id`** – the document’s IRI – without declaring diff --git a/packages/search-typesense/src/query-compiler.ts b/packages/search-typesense/src/query-compiler.ts index 784bf023..4343e11b 100644 --- a/packages/search-typesense/src/query-compiler.ts +++ b/packages/search-typesense/src/query-compiler.ts @@ -71,7 +71,8 @@ export interface BuildSearchParamsOptions { * usable bound. Skipping keeps a malformed clause from reaching the engine * as garbage; supply this to log it instead of losing it silently. Through * the engine, a structurally invalid query throws up front - * (`assertValidQuery`), so there only the vacuous clauses reach this. + * (`assertValidQuery`), so there only the clauses that state no constraint + * reach this. */ readonly onIgnoredFilter?: (filter: Filter) => void; } @@ -234,8 +235,15 @@ function compileFilterBy( * unknown field, an operator that mismatches the field’s kind) is *false*, and * `false || X` is X, so it drops out and its siblings still stand. * - * A clause left with no terms either way is skipped and reported to - * `onIgnoredFilter`. + * Both readings hold **within** a clause. Neither survives a clause left with + * no terms: it is skipped and reported to `onIgnoredFilter`, and a clause + * missing from the conjunction constrains nothing – so a *false* criterion + * alone in its clause still WIDENS the query. `filter_by` has no term meaning + * “matches nothing” to compile it to instead. The engine never meets this + * (`assertValidQuery` rejects every malformed criterion, and `isUnsatisfiable` + * short-circuits the empty `id` membership before a query is dispatched); a + * direct caller should read `onIgnoredFilter` firing as “this query no longer + * asks what you asked”. */ function compileFilter( filter: Filter, @@ -359,14 +367,24 @@ function compileMembership( : `${field.name}:${list}`; } -/** An inclusive Typesense range clause, or `undefined` when neither bound is set. */ +/** An inclusive Typesense range clause, or `undefined` when neither bound is + * usable. Which of the two readings that is – the caller set no bounds, or the + * codec rejected the ones they set – is deliberately NOT decided here: a + * criterion dropped from a clause narrows the query while a clause dropped + * from the conjunction widens it, so the same reading cannot be right in both + * positions. `validateQuery` rejects an unreadable bound outright instead, + * which is the only answer that neither widens nor narrows. */ function compileRange( field: SearchField, range: { readonly min?: number | string; readonly max?: number | string }, ): string | undefined { const name = field.name; - const min = storedBound(field, range.min); - const max = storedBound(field, range.max); + // A bound a caller sent as `null` – a GraphQL variable left unfilled, which + // the surface passes through – is a bound NOT SET, not a bound of `null`. + // Read literally it reaches the engine as `datePosted:[null..…]`, which + // Typesense rejects outright. + const min = storedBound(field, range.min ?? undefined); + const max = storedBound(field, range.max ?? undefined); if (min !== undefined && max !== undefined) { return `${name}:[${min}..${max}]`; } diff --git a/packages/search-typesense/test/query-compiler.test.ts b/packages/search-typesense/test/query-compiler.test.ts index d8d7951c..cbcc475d 100644 --- a/packages/search-typesense/test/query-compiler.test.ts +++ b/packages/search-typesense/test/query-compiler.test.ts @@ -392,7 +392,14 @@ describe('buildSearchParams', () => { schema, ).filter_by, ).toBe(`datePosted:[${min}..${max}]`); - // An unparseable bound is dropped rather than compiled into garbage. + }); + + it('drops a date bound the codec cannot read rather than compiling garbage', () => { + // Unreachable through the engine – `assertValidQuery` rejects such a bound + // outright, which is the fix for it. What the compiler does when reached + // directly is only damage control: it cannot express “matches nothing”, so + // whichever bound it keeps, the query it sends is not the one asked for. + const max = Date.parse('2025-01-01T00:00:00Z') / 1000; expect( buildSearchParams( { @@ -413,6 +420,31 @@ describe('buildSearchParams', () => { ).toBe(`datePosted:<=${max}`); }); + it('reads a null bound as a bound not set, not as the literal null', () => { + // A GraphQL variable left unfilled arrives as `null` and the surface passes + // it through. Read literally it compiles to `datePosted:[null..…]`, which + // Typesense rejects – so the whole search fails on an unset filter. + const max = Date.parse('2025-01-01T00:00:00Z') / 1000; + expect( + buildSearchParams( + { + ...base, + where: [ + { + or: [ + { + field: 'datePosted', + range: { min: null, max: '2025-01-01T00:00:00Z' }, + } as never, + ], + }, + ], + }, + schema, + ).filter_by, + ).toBe(`datePosted:<=${max}`); + }); + it('reads an unpadded BCE bound as a year, so the range is not inverted', () => { const min = Date.parse('-001100-01-01T00:00:00.000Z') / 1000; const max = Date.parse('-000800-01-01T00:00:00.000Z') / 1000; diff --git a/packages/search-typesense/vite.config.ts b/packages/search-typesense/vite.config.ts index 998ce405..73d30aa6 100644 --- a/packages/search-typesense/vite.config.ts +++ b/packages/search-typesense/vite.config.ts @@ -24,7 +24,7 @@ export default mergeConfig( // projection naming what no lookup reaches are unreachable through // the port, since `assertValidQuery` rejects such a query first. // They hold for a direct caller, and are exercised as one. - branches: 94.54, + branches: 94.58, statements: 99.33, }, }, diff --git a/packages/search/src/query.ts b/packages/search/src/query.ts index 5e046158..4932ca20 100644 --- a/packages/search/src/query.ts +++ b/packages/search/src/query.ts @@ -2,6 +2,7 @@ import { fieldNamed, filterOperatorFor, ID_FIELD, + isoToUnixSeconds, type SearchSchema, type SearchType, } from './schema.js'; @@ -193,9 +194,10 @@ export function isUnsatisfiable(query: SearchQuery): boolean { /** * One structural problem {@link validateQuery} found: the query references a - * field the search type does not declare, or uses it in a role it does not - * opt into. Reported per **criterion**, so a clause carrying two criteria over - * unknown fields yields two issues, each naming its own field. + * field the search type does not declare, uses it in a role it does not opt + * into, or constrains it with a value that cannot be read. Reported per + * **criterion**, so a clause carrying two criteria over unknown fields yields + * two issues, each naming its own field. * * Vacuous-but-valid clauses (an empty `in` list, a `range` with no bound, a * clause with no criteria) are NOT issues – a compiler skips those as no-ops. @@ -203,6 +205,9 @@ export function isUnsatisfiable(query: SearchQuery): boolean { export interface QueryIssue { readonly part: 'where' | 'facets' | 'orderBy' | 'resolve'; readonly field: string; + /** The offending literal, on an issue about a **value** rather than a field: + * the bound of an `unparseable-bound`. Absent on every other reason. */ + readonly value?: string; readonly reason: | 'unknown-field' | 'not-filterable' @@ -214,7 +219,18 @@ export interface QueryIssue { * that is not `joinable`, or no join graph to resolve it against. */ | 'unknown-join' /** A projected reference is not a `lookup`, so nothing resolves it. */ - | 'not-resolvable'; + | 'not-resolvable' + /** + * A `date` range bound the storage codec ({@link isoToUnixSeconds}) cannot + * read, so there is no stored value to compare against – reported per + * bound, each issue naming its own {@link QueryIssue.value}. + * + * Caught here rather than left to a compiler because the alternatives both + * lie: a dropped bound answers a *wider* question than the caller asked, + * and a rejected criterion answers a narrower one. Neither is the query, + * and only the caller can fix the bound. + */ + | 'unparseable-bound'; } /** The `field` a joined issue is reported under: the path and the leaf name @@ -232,7 +248,10 @@ function issueField(criterion: Criterion): string { * always-filterable {@link ID_FIELD}, which takes `in`; every requested facet is * a declared, `facetable` field; every sort is `relevance` or a declared field. * Because each criterion is matched against its own field’s kind, one clause may - * range over fields of different kinds. Sorting deliberately + * range over fields of different kinds. A `date` range is checked one step + * further, on its **values**: a bound the storage codec cannot read is an + * `unparseable-bound`, the one place a criterion is rejected for what it says + * rather than for what it names. Sorting deliberately * checks declaration only, not the `sortable` flag: that flag means *publicly * selectable*, and a deployment policy may sort on a private tie-break field. * @@ -315,6 +334,24 @@ export function validateQuery( field, reason: 'operator-mismatch', }); + } else if (declared.kind === 'date' && 'range' in criterion) { + // A `date` bound is ISO 8601 at the edge and Unix seconds in storage, + // so a bound the codec cannot read has nothing to compare against. + // Both bounds are checked, so a caller who wrote two bad ones is told + // about both instead of fixing one to be told about the other. + for (const bound of [criterion.range.min, criterion.range.max]) { + if ( + typeof bound === 'string' && + isoToUnixSeconds(bound) === undefined + ) { + issues.push({ + part: 'where', + field, + value: bound, + reason: 'unparseable-bound', + }); + } + } } } } @@ -401,7 +438,10 @@ export function assertValidQuery( const issues = validateQuery(query, searchType, schema, joins); if (issues.length > 0) { const detail = issues - .map((issue) => `${issue.part}: “${issue.field}” (${issue.reason})`) + .map((issue) => { + const value = issue.value === undefined ? '' : `: “${issue.value}”`; + return `${issue.part}: “${issue.field}” (${issue.reason}${value})`; + }) .join(', '); throw new Error( `Invalid search query for “${searchType.name}”: ${detail}.`, diff --git a/packages/search/test/query.test.ts b/packages/search/test/query.test.ts index 2c338305..770ca651 100644 --- a/packages/search/test/query.test.ts +++ b/packages/search/test/query.test.ts @@ -107,6 +107,7 @@ describe('validateQuery', () => { fields: [ { name: 'status', kind: 'keyword', facetable: true, filterable: true }, { name: 'size', kind: 'integer', filterable: true }, + { name: 'datePosted', kind: 'date', filterable: true }, { name: 'license', kind: 'keyword' }, // declared, but no roles opted into { name: 'statusRank', kind: 'integer', sortable: true }, { name: 'creator', kind: 'reference', filterable: true }, @@ -413,6 +414,68 @@ describe('validateQuery', () => { ).toEqual([]); }); + describe('a `date` range bound', () => { + const dateRange = (range: { + min?: number | string; + max?: number | string; + }) => + validateQuery( + { ...base, where: [{ or: [{ field: 'datePosted', range }] }] }, + searchType, + schema, + ); + + it('accepts every form the storage codec reads', () => { + // An expanded and an unexpanded deep-time year, a plain one, a full + // timestamp – and a number, which is already the stored Unix seconds. + expect(dateRange({ min: '-250000', max: '25000-06-01' })).toEqual([]); + expect( + dateRange({ min: '1999', max: '2024-01-01T00:00:00.000Z' }), + ).toEqual([]); + expect(dateRange({ min: -7951405219200, max: 0 })).toEqual([]); + }); + + it('reports a bound the codec cannot read, naming the bound', () => { + // Past the ±271,821-year window `Date` covers… + expect(dateRange({ min: '-500000' })).toEqual([ + { + part: 'where', + field: 'datePosted', + value: '-500000', + reason: 'unparseable-bound', + }, + ]); + // …and not deep-time-specific: any unreadable literal is the same + // mistake. Without this rule the criterion compiled to no constraint and + // the search answered with everything. + expect(dateRange({ max: 'yesterday' })).toEqual([ + { + part: 'where', + field: 'datePosted', + value: 'yesterday', + reason: 'unparseable-bound', + }, + ]); + }); + + it('reports both bounds, so one fix does not uncover the next', () => { + expect(dateRange({ min: '-500000', max: '-400000' })).toEqual([ + { + part: 'where', + field: 'datePosted', + value: '-500000', + reason: 'unparseable-bound', + }, + { + part: 'where', + field: 'datePosted', + value: '-400000', + reason: 'unparseable-bound', + }, + ]); + }); + }); + it('treats a clause with no criteria as vacuous, not invalid', () => { // Like an empty `in` or a boundless `range`: it constrains nothing, so a // compiler skips it as a no-op rather than the query being rejected. @@ -436,6 +499,7 @@ describe('validateQuery', () => { searchable: { weight: 5 }, }, { name: 'country', kind: 'keyword', filterable: true }, + { name: 'issued', kind: 'date', filterable: true }, { name: 'note', kind: 'keyword' }, ...fields, ], @@ -507,6 +571,38 @@ describe('validateQuery', () => { ]); }); + it('reports an unreadable bound on a joined `date` under the full path', () => { + expect( + validateQuery( + { + ...base, + where: [ + { + or: [ + { on: ['dataset'], field: 'issued', range: { min: '1900' } }, + { + on: ['dataset', 'publisher'], + field: 'issued', + range: { max: 'yesterday' }, + }, + ], + }, + ], + }, + work, + joinedSchema, + joins, + ), + ).toEqual([ + { + part: 'where', + field: 'dataset.publisher.issued', + value: 'yesterday', + reason: 'unparseable-bound', + }, + ]); + }); + it('rejects a path an `id` criterion uses with the wrong operator', () => { expect( validateQuery( @@ -598,6 +694,22 @@ describe('validateQuery', () => { ).toThrow( 'Invalid search query for “Dataset”: where: “nonexistent” (unknown-field).', ); + // An issue about a value carries the literal that was rejected, so the + // caller is told which bound to fix and not just which field. + expect(() => + assertValidQuery( + { + ...base, + where: [ + { or: [{ field: 'datePosted', range: { min: 'yesterday' } }] }, + ], + }, + searchType, + schema, + ), + ).toThrow( + 'Invalid search query for “Dataset”: where: “datePosted” (unparseable-bound: “yesterday”).', + ); expect(() => assertValidQuery(base, searchType, schema)).not.toThrow(); }); }); diff --git a/packages/search/vite.config.ts b/packages/search/vite.config.ts index e3f60542..e64239d4 100644 --- a/packages/search/vite.config.ts +++ b/packages/search/vite.config.ts @@ -12,7 +12,7 @@ export default mergeConfig( thresholds: { functions: 100, lines: 100, - branches: 99.57, + branches: 99.58, statements: 100, }, },