diff --git a/.changeset/engine-query-options-search-union.md b/.changeset/engine-query-options-search-union.md new file mode 100644 index 0000000000..76705fcca3 --- /dev/null +++ b/.changeset/engine-query-options-search-union.md @@ -0,0 +1,50 @@ +--- +"@objectstack/spec": minor +"@objectstack/metadata": patch +--- + +fix(spec): `EngineQueryOptionsSchema.search` accepts the bare query string ADR-0061 D1 calls canonical (#7178) + +Two sibling schemas in `packages/spec` described the same key and disagreed. +`BaseQuerySchema.search` (`query.zod.ts`, hence `QueryAST`, hence `DriverQuery`) +has been `z.union([z.string(), FullTextSearchSchema])` since its own drift +repair, with a doc comment saying why: the bare string **is** the canonical +Tier-1 contract (ADR-0061 D1 — "the client sends only the query text; the server +resolves which fields to search from object metadata"), it is what every surface +sends, and it is what the dogfood HTTP proof pins. +`EngineQueryOptionsSchema.search` — the options type of `IDataEngine.find` / +`findOne` — declared the structured `FullTextSearchSchema` **only**. + +The runtime never agreed with that narrowing. `expandSearchOnAst` +(`objectql/src/engine.ts`) reads `search` through `normalizeSearch`, whose first +line is `if (typeof raw === 'string') return { query: raw }`, and +`protocol-data.test.ts` asserts the protocol layer hands the engine a bare +string. So the type forbade what the engine serves, and callers paid the +standard price: `as any` on the query argument — which does not suppress +`search` alone, it switches off checking for `where` / `orderBy` / `fields` in +the same literal. Since this schema is not `.strict()`, an unknown key there is +**silently dropped**, so the cast this divergence forced was precisely the cast +`check:query-options-erasure` exists to stop. + +This is the same-family drift REPAIR, not a new dialect — the identical fix +`BaseQuerySchema.search` already carries, for the identical reason. On the query +side the divergence surfaced as a validation failure the moment #3899 started +validating request bodies; here it surfaced as a type error, when #6231 retyped +`DatabaseLoader`'s read helpers to `DriverQuery` and the **engine** branch alone +refused to compile (TS2345 — `DriverQuery` not assignable to +`EngineQueryOptionsParsed`, purely because of `search`; nothing else differs). + +Consumer census before landing, per the card's own guard: every site that reads +object-form members off an engine-options `search` already narrows with `typeof` +— `engine.ts` (`typeof raw === 'object' ? raw?.fields : undefined`), +`search-filter.ts` `normalizeSearch`, and `metadata-protocol/protocol.ts`'s +`searchFields` ingress gate. No consumer needed a guard added, and none changes +behavior: they were all written for the union already. `count` is untouched — +`EngineCountOptionsSchema` declares no `search` key at all. + +With the schemas agreed, the casts the divergence forced are deleted: +`DatabaseLoader`'s three engine-branch `as any` (`_find` / `_findOne` / +`_count`), which restores real `where` / `orderBy` / `fields` checking on the +metadata main read path, and the seven `as any` in +`engine-findone-contract.test.ts` that were passing the canonical spelling. +`scripts/query-options-erasure-baseline.json` is ratcheted down accordingly. diff --git a/content/docs/kernel/contracts/data-engine.mdx b/content/docs/kernel/contracts/data-engine.mdx index 0113942445..f9e57f5af0 100644 --- a/content/docs/kernel/contracts/data-engine.mdx +++ b/content/docs/kernel/contracts/data-engine.mdx @@ -110,7 +110,9 @@ interface EngineQueryOptions { limit?: number; // LIMIT offset?: number; // OFFSET top?: number; // Alias for limit (OData compat) - search?: FullTextSearch; // Full-text search + search?: string | FullTextSearch; // Full-text search — the bare query text + // is canonical (ADR-0061 D1); the object + // form carries the Tier-2 knobs (#7178) expand?: Record; // Recursive relation loading context?: ExecutionContext; // Identity, tenant, transaction — any subset } diff --git a/content/docs/references/data/data-engine.mdx b/content/docs/references/data/data-engine.mdx index 2da68e20b1..75cdfa0115 100644 --- a/content/docs/references/data/data-engine.mdx +++ b/content/docs/references/data/data-engine.mdx @@ -519,7 +519,7 @@ QueryAST-aligned query options for IDataEngine.find() operations | **offset** | `number` | optional | | | **top** | `number` | optional | | | **cursor** | `never` | optional | [REMOVED] `query.cursor` was removed in @objectstack/spec 17 (#4286, ADR-0049) — no driver ever implemented keyset pagination, so the cursor was accepted and ignored and every page came back identical (a caller looping "until hasMore is false" never terminates). Delete the key; `QueryBuilder.cursor()` was removed with it. Express the keyset as an ordinary `where` predicate on your sort key — `where: { created_at: { $gt: last.created_at } }` with the matching `orderBy` — which every driver executes with canonicalised comparands. A first-class cursor, if ever built, will be a response-minted opaque token, not this caller-built record. | -| **search** | `{ query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | | +| **search** | `string \| { query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | | | **searchFields** | `string[]` | optional | | | **expand** | `Record` | optional | | | **distinct** | `never` | optional | [REMOVED] `query.distinct` was removed in @objectstack/spec 17 (#4286, ADR-0049 / ADR-0078) — no driver ever rendered SELECT DISTINCT; the flag's only observable effect was MIS-WIRED: the REST list path treated a distinct query as not countable and silently degraded `total`/`hasMore` to a page-local estimate while still returning duplicate rows. Delete the key; `QueryBuilder.distinct()` was removed with it, and the count suppression is gone (`total` is truthful again). For unique values of one column use the SQL/memory drivers' `distinct(object, field)` door; for unique combinations, `groupBy`; for a deduplicated count, the `count_distinct` aggregation. | diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index 488d8a2d9b..70fd1b6552 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -225,34 +225,41 @@ export class DatabaseLoader implements MetadataLoader { // Internal CRUD helpers (driver vs engine) // ========================================== - // NOTE (#6231): the DRIVER branch below takes `query` unchanged and uncast — - // `DriverQuery` is `Omit`, so the object name travels as - // argument one only. The ENGINE branch still carries `as any`, and that cast - // is NOT vestigial: `EngineQueryOptionsSchema.search` admits only the - // structured `FullTextSearchSchema`, while `QueryAST.search` (hence - // `DriverQuery`) also admits the bare query string that ADR-0061 D1 calls the - // canonical Tier-1 spelling and that the engine actually serves. Until those - // two schemas agree, `DriverQuery` is not assignable to - // `EngineQueryOptionsParsed`. Tracked as #7178; do not "fix" it here by - // narrowing the cast. + // NOTE (#6231, closed out by #7178): BOTH branches below now take `query` + // unchanged and uncast. `DriverQuery` is `Omit`, so the + // object name travels as argument one only — that was always enough for the + // driver branch. The ENGINE branch used to carry `as any`, for one reason: + // `EngineQueryOptionsSchema.search` admitted only the structured + // `FullTextSearchSchema`, while `QueryAST.search` (hence `DriverQuery`) also + // admits the bare query string that ADR-0061 D1 calls the canonical Tier-1 + // spelling and that the engine actually serves, so `DriverQuery` was not + // assignable to `EngineQueryOptionsParsed`. #7178 aligned the two schemas; + // the casts are now genuinely vestigial and are gone, which restores real + // `where`/`orderBy`/`fields` checking on the metadata main read path — this + // schema is not `.strict()`, so an unknown key here is SILENTLY DROPPED + // (`check:query-options-erasure`'s own rationale) and the erased type was + // the only thing standing between a typo and that silence. + // + // If a future edit makes one of these stop compiling, the honest fix is to + // reconcile the two schemas again — not to reinstate the cast. private async _find(table: string, query: DriverQuery): Promise[]> { if (this.engine) { - return this.engine.find(table, query as any); + return this.engine.find(table, query); } return this.driver!.find(table, query); } private async _findOne(table: string, query: DriverQuery): Promise | null> { if (this.engine) { - return this.engine.findOne(table, query as any); + return this.engine.findOne(table, query); } return this.driver!.findOne(table, query); } private async _count(table: string, query: DriverQuery): Promise { if (this.engine) { - return this.engine.count(table, query as any); + return this.engine.count(table, query); } return this.driver!.count(table, query); } diff --git a/packages/objectql/src/engine-findone-contract.test.ts b/packages/objectql/src/engine-findone-contract.test.ts index c195011eaf..fc5a9e52ca 100644 --- a/packages/objectql/src/engine-findone-contract.test.ts +++ b/packages/objectql/src/engine-findone-contract.test.ts @@ -141,13 +141,13 @@ describe('findOne executes what it declares and refuses an empty predicate (#441 // ── (1) `search` is a predicate on findOne, not a dropped key ──────── it('findOne({search}) matches the searched record, not the first row', async () => { - const row = await engine.findOne('crm_account', { search: 'Two' } as any); + const row = await engine.findOne('crm_account', { search: 'Two' }); expect(row?.name).toBe('Two'); expect(row?.id).not.toBe(one.id); }); it('the search term reaches the driver as a $contains predicate — `search` never does', async () => { - await engine.findOne('crm_account', { search: 'Two' } as any); + await engine.findOne('crm_account', { search: 'Two' }); const { ast } = lastRead(); expect(ast.where).toBeTruthy(); expect(JSON.stringify(ast.where)).toContain('$contains'); @@ -160,13 +160,13 @@ describe('findOne executes what it declares and refuses an empty predicate (#441 // `industry`, only the latter can hit — and a narrowed miss must be a // miss, not a fall-back to an unpredicated read. const narrowed = { searchFields: ['industry'] }; - expect(await engine.findOne('crm_account', { search: 'Two', ...narrowed } as any)).toBeNull(); - expect((await engine.findOne('crm_account', { search: 'Metals', ...narrowed } as any))?.id) + expect(await engine.findOne('crm_account', { search: 'Two', ...narrowed })).toBeNull(); + expect((await engine.findOne('crm_account', { search: 'Metals', ...narrowed }))?.id) .toBe(two.id); }); it('find({search}) is unchanged — the expansion moved, it did not fork', async () => { - const rows = await engine.find('crm_account', { search: 'Two' } as any); + const rows = await engine.find('crm_account', { search: 'Two' }); expect(rows.map((r: any) => r.name)).toEqual(['Two']); }); @@ -176,7 +176,7 @@ describe('findOne executes what it declares and refuses an empty predicate (#441 // "predicate resolved to empty" shape the issue names. Before #4419 the // forced `limit: 1` turned it into the object's first row. for (const term of ['', ' ']) { - await expect(engine.findOne('crm_account', { search: term } as any)) + await expect(engine.findOne('crm_account', { search: term })) .rejects.toThrow(/selects no particular record/); } expect(reads).toHaveLength(0); @@ -293,7 +293,7 @@ describe('findOne executes what it declares and refuses an empty predicate (#441 it('a miss is still null — the guard did not turn "not found" into an error', async () => { expect(await engine.findOne('crm_account', { where: { id: 'nope' } } as any)).toBeNull(); - expect(await engine.findOne('crm_account', { search: 'nope' } as any)).toBeNull(); + expect(await engine.findOne('crm_account', { search: 'nope' })).toBeNull(); }); // ── (3) drift pin: every declared findOne option is executed ──────── diff --git a/packages/spec/src/data/data-engine.test.ts b/packages/spec/src/data/data-engine.test.ts index 4407bb5a48..6cbb694ee9 100644 --- a/packages/spec/src/data/data-engine.test.ts +++ b/packages/spec/src/data/data-engine.test.ts @@ -25,6 +25,7 @@ import { DataEngineRequestSchema, DroppedFieldsEventSchema, } from './data-engine.zod'; +import { QuerySchema } from './query.zod'; describe('DataEngineFilterSchema', () => { it('should accept simple key-value filter', () => { @@ -376,6 +377,53 @@ describe('EngineQueryOptionsSchema', () => { expect(options.expand!.owner.object).toBe('user'); }); + // ── `search`: both spellings, canonical one first (#7178) ──────────── + + it('accepts the BARE query string — the canonical ADR-0061 D1 spelling (#7178)', () => { + // This is the pin that was RED before #7178: the schema declared only the + // structured form, so the spelling the executor actually serves, every + // surface sends, and `BaseQuerySchema.search` already accepts was rejected + // here — and every engine caller wanting it had to `as any` the whole query. + const options = EngineQueryOptionsSchema.parse({ search: 'acme corp' }); + expect(options.search).toBe('acme corp'); + }); + + it('still accepts the structured FullTextSearch form — the Tier-2 knobs (#7178)', () => { + const options = EngineQueryOptionsSchema.parse({ + search: { query: 'acme corp', fields: ['name', 'industry'] }, + }); + expect(typeof options.search).toBe('object'); + expect((options.search as { query: string }).query).toBe('acme corp'); + expect((options.search as { fields?: string[] }).fields).toEqual(['name', 'industry']); + }); + + it('accepts search alongside searchFields, in both spellings (#7178)', () => { + expect(EngineQueryOptionsSchema.parse({ + search: 'acme', searchFields: ['name'], + }).searchFields).toEqual(['name']); + expect(EngineQueryOptionsSchema.parse({ + search: { query: 'acme' }, searchFields: ['name'], + }).searchFields).toEqual(['name']); + }); + + it('rejects a search that is neither a string nor a FullTextSearch (#7178)', () => { + // The union widens the accept face by exactly one spelling — it does not + // open the key to anything. + expect(() => EngineQueryOptionsSchema.parse({ search: 42 })).toThrow(); + expect(() => EngineQueryOptionsSchema.parse({ search: { fields: ['name'] } })).toThrow(); + }); + + it('matches BaseQuerySchema.search — the two sibling schemas agree (#7178)', () => { + // The whole point of the repair: what QuerySchema accepts for `search`, + // the engine options schema accepts too. `DriverQuery` (= Omit) is assignable to `EngineQueryOptionsParsed` again because of + // this, which is what lets `database-loader`'s engine branch drop its casts. + for (const search of ['acme corp', { query: 'acme corp', fields: ['name'] }]) { + expect(QuerySchema.parse({ object: 'crm_account', search })).toBeDefined(); + expect(EngineQueryOptionsSchema.parse({ search })).toBeDefined(); + } + }); + it('rejects the removed cursor/distinct keys with the query.* prescriptions (#4286)', () => { expect(() => EngineQueryOptionsSchema.parse({ cursor: { id: 'x' } })) .toThrow(/query\.cursor.*removed/s); diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts index d4f4f9c181..efc17a59de 100644 --- a/packages/spec/src/data/data-engine.zod.ts +++ b/packages/spec/src/data/data-engine.zod.ts @@ -115,8 +115,29 @@ export const EngineQueryOptionsSchema = lazySchema(() => BaseEngineOptionsSchema /** Keyset cursor — REMOVED (#4286); same tombstone as `QuerySchema.cursor`. */ cursor: retiredKey(QUERY_CURSOR_REMOVED), - /** Full-text search configuration */ - search: FullTextSearchSchema.optional(), + /** + * Full-Text Search. + * + * The bare string IS the canonical Tier-1 contract (ADR-0061 D1: "the + * client sends only the query text; the server resolves which fields to + * search from object metadata") — it is what every surface sends, what the + * engine's `$search` expansion actually serves, and what the dogfood HTTP + * proof (`showcase-search.dogfood.test.ts`) pins. The structured + * `FullTextSearchSchema` form remains for the declared Tier-2 knobs. + * + * The union is schema-side drift REPAIR, not a new dialect — the same + * repair `BaseQuerySchema.search` (`query.zod.ts`) already carries, and for + * the same reason: this schema declared only the object form while the + * executor and the ADR's own conformance ledger served the string. Here the + * divergence surfaced as a type error rather than a validation failure + * (#7178): `DriverQuery` (= `Omit`, which inherits the + * union) was not assignable to `EngineQueryOptionsParsed` purely because of + * this key, so every engine caller wanting the canonical spelling had to + * `as any` the whole query — switching off `where`/`orderBy`/`fields` + * checking too, and, since this schema is not `.strict()`, arming exactly + * the silent-key-drop that `check:query-options-erasure` exists to stop. + */ + search: z.union([z.string(), FullTextSearchSchema]).optional(), /** * Fields the `search` expansion may match against — intersected with the diff --git a/scripts/query-options-erasure-baseline.json b/scripts/query-options-erasure-baseline.json index ad475bb005..cd30ad7d1d 100644 --- a/scripts/query-options-erasure-baseline.json +++ b/scripts/query-options-erasure-baseline.json @@ -35,7 +35,6 @@ "packages/core/src/security/resolve-authz-context.ts": 1, "packages/metadata-protocol/src/protocol.ts": 6, "packages/metadata-protocol/src/seed-loader.ts": 3, - "packages/metadata/src/loaders/database-loader.ts": 3, "packages/objectql/src/engine.ts": 9, "packages/plugins/plugin-approvals/src/approval-service.ts": 10, "packages/plugins/plugin-approvals/src/approver-org-scope.ts": 2, @@ -51,6 +50,6 @@ "packages/services/service-settings/src/settings-service.ts": 2 }, "testSurface": { - "sites": 256 + "sites": 249 } }