diff --git a/.changeset/sort-formula-field-refusal.md b/.changeset/sort-formula-field-refusal.md new file mode 100644 index 0000000000..83f4715427 --- /dev/null +++ b/.changeset/sort-formula-field-refusal.md @@ -0,0 +1,59 @@ +--- +"@objectstack/metadata-protocol": minor +--- + +fix(metadata-protocol): refuse a sort naming a `formula` field instead of dropping it silently (#6994) + +The list path's SORT gate (`assertSortFieldsExist`) refuses a sort naming a field +the object does not have (#4226) and a dotted path that would have to cross into +a related record (#4256). It did **not** refuse a name that is a real, +non-dotted field of the object whose **type** materialises no column — a +`formula` field is in the object's field map, so it passed the unknown check, +and it carries no dot, so it passed the dotted check. + +It then reached a driver that has no column for it. Re-measured on a real +`SqlDriver` (better-sqlite3, on-disk) driving a real `ObjectQL` engine with this +protocol on top, over five rows inserted `C A E B D` and a formula field +`sort_key` whose expression is `record.title`: + +``` +CONTROL orderBy title asc -> ["A","B","C","D","E"] a real column really sorts +BASELINE no sort -> ["C","A","E","B","D"] insertion order + +FORMULA orderBy sort_key asc -> ["C","A","E","B","D"] 5 rows, 200 + its sort_key values -> ["C","A","E","B","D"] +FORMULA orderBy sort_key desc -> ["C","A","E","B","D"] byte-identical to asc + +RAW SQL order by sort_key -> sqlite: no such column: sort_key +``` + +`asc` and `desc` coming back identical is what makes this a dropped sort rather +than a coincidence: `SqlDriver.createColumn` returns early for `formula` (it is +virtual — computed on read, after `driver.find` has already returned), sqlite +answers `no such column`, and the #3821 unknown-column backstop retries the +query **without** the `ORDER BY`. The response even carries the values it was +asked to order by, out of order, under a 200 — so it contradicts the request in +plain view and still reports success. `sort` + `top` is how a caller asks for +"the latest N", which this turned into an arbitrary N. + +**Now:** `400 INVALID_SORT`, naming the field and its type, and prescribing the +same remedy in the same words as the dotted refusal (#6924) and the SEARCH axis +(#6673) — denormalise onto a **stored field, written when the source changes**. +Precedence on this axis is `unknown` > `dotted` > unmaterializable, so both +older verdicts answer exactly what they answered before. + +**`summary` / `rollup` is not affected** and deliberately not in the refused +set: a summary field gets a real, maintained `float` column and genuinely sorts. +The spec's `COMPUTED_VALUE_TYPES` (`formula`/`summary`/`autonumber`) is the +WRITE contract and is the wrong set to gate a sort with — it would refuse two +types that work. + +**Scope.** This is an ingress gate, so it covers what reaches `findData`: the +REST list route, `POST /data/:object/query`, the export route, and the RPC +dispatcher. An internal caller that reaches `engine.find()` directly (hooks, +flows, reports) still gets the silent drop — closing that half means deciding +whether the engine refuses or keeps its documented internal-caller tolerance, +which is a separate contract decision and is tracked separately. + +If you were sorting a list by a formula field, that sort was never applied; the +call now fails loudly instead of returning rows in an arbitrary order. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index d4876b1a64..2e532013c6 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1555,6 +1555,42 @@ function unusableFilterError(param: string, detail: string): Error { return err; } +/** + * [#6994] Field types whose value NO driver materialises, so no driver can + * ORDER BY them. + * + * `formula` is the whole set today, and deliberately not a synonym for + * "computed": the three computed types diverge exactly here. + * + * | type | column | sortable | + * |---|---|---| + * | `formula` | none — `SqlDriver.createColumn` returns early; `driver-turso`'s transport skips it with the same `Virtual — no column` note | **no** | + * | `summary` | `table.float`, maintained by the engine | yes (measured #6924: `orderBy desc` -> E D C B A over 5 4 3 2 1) | + * | `autonumber` | `table.string`, engine-assigned | yes | + * + * So the spec's own `COMPUTED_VALUE_TYPES` (`formula`/`summary`/`autonumber`) + * is the WRITE contract — "never client-written" — and is the wrong set to + * gate a sort with: it would refuse the two types that sort correctly. + * + * This is a local set rather than a shared spec constant because the same fact + * is currently spelled in five places, none of them in `packages/spec`: + * `driver-sql`'s `fieldHasColumn` and `createColumn`, `driver-turso`'s + * `remote-transport`, `objectql`'s `planFormulaProjection` and + * `search-companion`, and `plugin-audit`'s `VIRTUAL_FIELD_TYPES`. + * Consolidating them is a cross-package change and is filed separately; adding + * a sixth local spelling here — with that ledger written down — keeps this fix + * inside one package rather than opening a spec-wide edit for one string. + * + * One deliberate divergence from `fieldHasColumn`: that helper short-circuits + * on `multiple` (a `multiple` field is a JSON column whatever its type), so it + * would answer "has a column" for a `multiple` formula. This set does not, + * because the questions differ — `fieldHasColumn` asks whether DDL emits a + * column, this asks whether there is a persisted VALUE to order by, and a + * formula's value is computed on read and never written, so that JSON column + * is always empty. Ordering by it degrades exactly as the bare case does. + */ +const UNMATERIALIZED_SORT_TYPES: ReadonlySet = new Set(['formula']); + /** * [#4226] A sort the normalizer cannot turn into a usable `SortNode[]`, or one * that names a field the object does not have — or, since #4256, a dotted path @@ -4815,8 +4851,9 @@ export class ObjectStackProtocolImplementation implements * copies it into a real column" — a prescription the platform cannot * deliver, so the refusal handed the author a dead end at the exact moment * they asked for help. Measured on a REAL `SqlDriver` (better-sqlite3) and - * on `InMemoryDriver`, with a `formula` field named directly (NOT dotted, - * so this gate lets it through): + * on `InMemoryDriver`, with a `formula` field named directly — which at the + * time was NOT dotted and NOT unknown, so this gate let it through + * (#6994 closes that, third verdict below): * * ``` * control orderBy title asc -> A B C D E (a real column sorts) @@ -4844,6 +4881,27 @@ export class ObjectStackProtocolImplementation implements * "Stored" is #6673's vocabulary for the same correction on the SEARCH * axis (`validate-searchable-fields.ts`, "a stored text field"); the two * axes deliberately say the same word. + * + * [#6994] The non-dotted half of that same defect, refused as the THIRD + * verdict below. It is the SORT axis finally growing the verdict its two + * neighbours in this class already have: `assertSearchFieldsExist` splits + * `unknown` from `unsearchable` (a known field whose TYPE search cannot + * scan) and `assertExpandFieldsExist` splits `unknown` from `notRelations` + * (a known field whose TYPE cannot be expanded). Sort had only `unknown` + * and `dotted`, so "known field, wrong type for this axis" was the one + * member of the family with no door — which is why a `formula` field + * reached a driver that has no column for it. + * + * SCOPE, stated because it is a real limit and not an oversight: this is an + * INGRESS gate, so it covers what reaches {@link findData} — the REST list + * route, `POST /data/:object/query`, the export route (which funnels its + * `$orderby` through here) and the RPC dispatcher. An internal caller that + * reaches `engine.find()` directly — hooks, flows, reports, expand + * sub-reads — still gets the silent drop, exactly as the projection and + * search axes note for themselves. Closing that half means deciding whether + * `engine.find` REFUSES or keeps its deliberate internal-caller tolerance, + * which is an engine-core contract decision rather than a gate fix; it is + * tracked separately. */ private assertSortFieldsExist(object: string, orderBy: ReadonlyArray<{ field: string }>, param: string): void { if (orderBy.length === 0) return; @@ -4866,25 +4924,78 @@ export class ObjectStackProtocolImplementation implements ); } const dotted = names.filter((f) => f.includes('.')); - if (dotted.length === 0) return; - const first = dotted[0]; - const head = first.split('.')[0]; - const headDef: any = gate.fields[head]; - const crossesRelation = headDef != null && REFERENCE_VALUE_TYPES.has(headDef.type); + if (dotted.length > 0) { + const first = dotted[0]; + const head = first.split('.')[0]; + const headDef: any = gate.fields[head]; + const crossesRelation = headDef != null && REFERENCE_VALUE_TYPES.has(headDef.type); + throw invalidSortError( + param, + (crossesRelation + ? `sorts by '${first}', which follows the relationship '${head}' into another object — ` + + `sort reaches only columns of '${object}' itself` + : `sorts by '${first}', a dotted path — sort reaches only whole columns of '${object}', ` + + "not values inside them") + + (dotted.length > 1 ? ` (also: ${dotted.slice(1).join(', ')})` : ''), + { + hint: ` Denormalise the value onto '${object}' (a stored field, written when the` + + ' source changes) and sort by that. Not a formula field: it is virtual,' + + ' no driver materialises a column for one, and ORDER BY on it is silently' + + ' dropped.', + extra: { field: first, fields: dotted, object }, + }, + ); + } + + // [#6994] The third verdict on this axis: a name that is a REAL, + // non-dotted field of this object and still cannot be ordered by, + // because its TYPE materialises no column ({@link + // UNMATERIALIZED_SORT_TYPES} — `formula`, today the whole set). + // + // This is the shape the doc comment above already describes and this + // gate already let through: being in `gate.known` is what carried it + // past the unknown check, being undotted is what carried it past the + // check just above. Re-measured on this branch's base (real `SqlDriver` + // over better-sqlite3, real `ObjectQL`, real protocol on top): + // + // ``` + // FORMULA orderBy sort_key asc -> ["C","A","E","B","D"] 5 rows, 200 + // its sort_key values -> ["C","A","E","B","D"] + // FORMULA orderBy sort_key desc -> ["C","A","E","B","D"] asc === desc + // RAW SQL order by sort_key -> sqlite: no such column: sort_key + // ``` + // + // The response literally carries the values it was asked to sort by, + // out of order, under a 200 — so the answer contradicts the request in + // plain view and still reports success. + // + // PRECEDENCE — `unknown` > `dotted` > this. It is last for the same + // reason the expand gate reports `unknown` before `not-a-reference`: + // identity errors first, then shape, then type. The two above are + // therefore unchanged verdict-for-verdict, and a dotted path whose head + // is a formula field keeps the dotted answer (it is wrong about the + // shape too, and the shape is what the caller wrote). + const unmaterialized = names.filter( + (f) => UNMATERIALIZED_SORT_TYPES.has(String(gate.fields[f]?.type ?? '')), + ); + if (unmaterialized.length === 0) return; + const virtualFirst = unmaterialized[0]; + const virtualType = String(gate.fields[virtualFirst]?.type); throw invalidSortError( param, - (crossesRelation - ? `sorts by '${first}', which follows the relationship '${head}' into another object — ` - + `sort reaches only columns of '${object}' itself` - : `sorts by '${first}', a dotted path — sort reaches only whole columns of '${object}', ` - + "not values inside them") - + (dotted.length > 1 ? ` (also: ${dotted.slice(1).join(', ')})` : ''), + `sorts by '${virtualFirst}', a ${virtualType} field on '${object}' — a ${virtualType} ` + + 'value is computed on read, so no driver materialises a column to order by' + + (unmaterialized.length > 1 ? ` (also: ${unmaterialized.slice(1).join(', ')})` : ''), { + // Deliberately the same remedy, in the same words, as the + // dotted refusal above and as #6673's SEARCH-axis correction: + // one vocabulary across the doors, so an author refused twice + // is not sent two different ways. hint: ` Denormalise the value onto '${object}' (a stored field, written when the` - + ' source changes) and sort by that. Not a formula field: it is virtual,' - + ' no driver materialises a column for one, and ORDER BY on it is silently' - + ' dropped.', - extra: { field: first, fields: dotted, object }, + + ' source changes) and sort by that. A formula field is virtual: with no' + + ' column behind it the ORDER BY reaches the driver, finds nothing, and is' + + ' dropped — the arbitrary order this refusal replaces.', + extra: { field: virtualFirst, fields: unmaterialized, object }, }, ); } diff --git a/packages/objectql/src/query-expression-conformance.test.ts b/packages/objectql/src/query-expression-conformance.test.ts index c3313d2e20..945f6b75ea 100644 --- a/packages/objectql/src/query-expression-conformance.test.ts +++ b/packages/objectql/src/query-expression-conformance.test.ts @@ -78,6 +78,28 @@ const taskObject = { // auto-default excludes by TYPE, which is its own rejection. notes: { name: 'notes', label: 'Notes', type: 'textarea' as const }, estimate: { name: 'estimate', label: 'Estimate', type: 'number' as const }, + // [#6994] The two "calculated" types that sort DIFFERENTLY, side by + // side, so the gate below is pinned on both edges at once. + // + // `sort_key` is virtual: no driver emits a column for a `formula`, its + // value is computed after `driver.find` returns, and an ORDER BY on it + // is dropped — the defect this axis' third verdict refuses. Its + // expression is `record.title`, so the value is VISIBLY the sort key + // the caller asked for, which is what makes the silent version so bad. + // + // `subtask_total` is not: `summary` gets a real, maintained float + // column (`SqlDriver.createColumn` → `table.float`) and genuinely + // sorts. It is the control that fails if this gate is ever widened to + // the spec's `COMPUTED_VALUE_TYPES` (`formula`/`summary`/`autonumber`), + // which is the WRITE contract and would refuse two working types. + sort_key: { + name: 'sort_key', label: 'Sort key', type: 'formula' as const, + expression: 'record.title', returnType: 'text' as const, + }, + subtask_total: { + name: 'subtask_total', label: 'Subtask total', type: 'summary' as const, + summaryOperations: { object: 'showcase_task', field: 'estimate', function: 'sum' as const }, + }, }, }; @@ -162,7 +184,20 @@ function makeStubDriver() { const sorted = applySort(matched, ast?.orderBy); const from = typeof ast?.offset === 'number' ? ast.offset : 0; const page = typeof ast?.limit === 'number' ? sorted.slice(from, from + ast.limit) : sorted.slice(from); - return project(page, ast?.fields); + // [#6994] Rows are COPIED out, as every real driver hands back rows + // it materialised from the wire rather than references into its own + // storage. Without this the double leaks engine-side mutation back + // into "the database": `applyFormulaPlan` writes each formula value + // onto the record it is given, so one read of an object carrying a + // `formula` field PERSISTED that value into the store, and the next + // read found a column no driver has and really sorted by it. + // + // Measured, and the reason this is a fix and not a preference: on a + // real `SqlDriver` (better-sqlite3) `orderBy asc` and + // `desc` come back BYTE-IDENTICAL, both in insertion order. Through + // this double they came back reversed on the second call. The + // double was contradicting the driver it stands in for. + return project(page, ast?.fields).map((r) => ({ ...r })); }, async findOne(object: string, ast: any) { const rows = await this.find(object, ast); @@ -232,6 +267,13 @@ describe('#4226 — sort / select / expand on the list path (real ObjectQL engin parent_id: letter === 'A' ? null : 't_A', owner_id: 'usr_1', created_at: '2026-07-30T00:00:00.000Z', + // [#6994] A permutation chosen so the summary control cannot + // hold vacuously: C=2 A=5 E=1 B=4 D=3 orders as `E C D B A` + // ascending and `A B D C E` descending, and neither matches + // insertion order (`C A E B D`) NOR title order (`A B C D E`). + // A control that agreed with either would pass against a driver + // that ignored `orderBy` entirely. + subtask_total: [2, 5, 1, 4, 3][i], }); }); stores.set('showcase_task', tasks); @@ -510,6 +552,131 @@ describe('#4226 — sort / select / expand on the list path (real ObjectQL engin })).rejects.toMatchObject({ status: 400, code: 'INVALID_SORT', field: 'project_id.created_at' }); }); + // ───────────────────────────────────────────────────────────── + // SORT — [#6994] a KNOWN, NON-DOTTED field whose TYPE materialises + // no column. The last shape on this axis that still degraded silently. + // ───────────────────────────────────────────────────────────── + + it('a summary field still sorts, in both directions — the family is `formula`, not "computed"', async () => { + // CONTROL, and the one that matters most here: `summary` is computed + // too, and it is NOT in this family. It gets a real maintained column + // (`table.float`) and orders correctly — measured on a real SqlDriver + // in #6924 (`orderBy desc` -> E D C B A over 5 4 3 2 1). + // + // This is what fails if the gate is ever widened from "materialises no + // column" to the spec's `COMPUTED_VALUE_TYPES` + // (`formula`/`summary`/`autonumber`) — that set is the WRITE contract + // ("never client-written") and refusing a sort with it would break two + // types that work. + expect(titles(await protocol.findData({ object: 'showcase_task', query: { sort: 'subtask_total' } }))) + .toEqual(['E', 'C', 'D', 'B', 'A']); + expect(titles(await protocol.findData({ object: 'showcase_task', query: { sort: '-subtask_total' } }))) + .toEqual(['A', 'B', 'D', 'C', 'E']); + }); + + it.each([ + ['bare string', { sort: 'sort_key' }], + ['descending', { sort: '-sort_key' }], + ['second of two', { sort: 'title,sort_key' }], + ['string array', { orderBy: ['sort_key'] }], + ['SortNode array', { orderBy: [{ field: 'sort_key', order: 'desc' }] }], + ['direction map', { orderBy: { sort_key: 'desc' } }], + ['OData spelling', { $orderby: 'sort_key' }], + ['with top — the "latest N" footgun', { sort: '-sort_key', top: 2 }], + ])('sorting by a formula field is a 400, not insertion order — %s', async (_label, query) => { + // `sort_key` is a REAL field of this object, so it is in `gate.known` + // and passed the #4226 unknown check; it carries no dot, so it passed + // the #4256 check as well. It then reached a driver with no column for + // it. Measured on a real `SqlDriver` (better-sqlite3) + real `ObjectQL` + // + this protocol, on the base of the branch that added this test: + // + // FORMULA orderBy sort_key asc -> ["C","A","E","B","D"] 5 rows, 200 + // its sort_key values -> ["C","A","E","B","D"] + // FORMULA orderBy sort_key desc -> ["C","A","E","B","D"] + // RAW SQL order by sort_key -> sqlite: no such column: sort_key + // + // `asc` and `desc` byte-identical is what makes it a DROPPED sort + // rather than a coincidence, and the response carrying the very values + // it was asked to order by, out of order, is what makes it invisible. + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ + status: 400, + code: 'INVALID_SORT', + field: 'sort_key', + object: 'showcase_task', + }); + }); + + it('the formula rejection names the type and prescribes the SAME stored field the dotted one does', async () => { + const err: any = await protocol + .findData({ object: 'showcase_task', query: { sort: 'sort_key' } }) + .then(() => null, (e: unknown) => e); + expect(err).toBeTruthy(); + // ADR-0112 envelope — a rejection case asserts code AND status, never + // merely that something was thrown. + expect(err.status).toBe(400); + expect(err.code).toBe('INVALID_SORT'); + // It must say WHICH type, or the author cannot tell this apart from a + // typo — the whole reason it needs its own verdict. + expect(err.message).toMatch(/a formula field on 'showcase_task'/); + expect(err.message).toMatch(/computed on read/); + // One vocabulary across the doors: #6924 fixed the dotted hint to + // prescribe "a stored field, written when the source changes", and + // #6673 says "a stored text field" on the SEARCH axis. An author + // refused on two axes must not be sent two different ways. + expect(err.message).toMatch(/a stored field, written when the source changes/); + // ...and it must never prescribe the thing it is refusing. + expect(err.message).not.toMatch(/formula or rollup/); + }); + + it('the two refusals agree word-for-word on the remedy', async () => { + // Pins the AGREEMENT itself rather than each wording separately: this + // goes red if either door's remedy is reworded without the other, which + // is exactly how #4256 and #6673 drifted apart in the first place. + const remedy = /Denormalise the value onto 'showcase_task' \(a stored field, written when the source changes\) and sort by that\./; + const dotted: any = await protocol + .findData({ object: 'showcase_task', query: { sort: 'project_id.name' } }) + .then(() => null, (e: unknown) => e); + const formula: any = await protocol + .findData({ object: 'showcase_task', query: { sort: 'sort_key' } }) + .then(() => null, (e: unknown) => e); + expect(dotted.message).toMatch(remedy); + expect(formula.message).toMatch(remedy); + }); + + it.each([ + ['unknown beats formula', { sort: 'no_such_field,sort_key' }, 'no_such_field'], + ['dotted beats formula', { sort: 'sort_key,project_id.name' }, 'project_id.name'], + ])('precedence is unknown > dotted > unmaterializable — %s', async (_label, query, field) => { + // Identity error first, then shape, then type — the same order the + // expand gate uses (`unknown` > `not-a-reference`). Deliberate, and + // pinned so it stays a decision rather than an accident: the two older + // verdicts keep answering exactly what they answered before this gate + // grew a third one. + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_SORT', field }); + }); + + it('RECORD OF A KNOWN HOLE — `engine.find` still drops a formula sort silently (#6994 is ingress-only)', async () => { + // NOT a defence of this behaviour: a pin on the half the ingress gate + // cannot reach, so it is measured rather than assumed closed. Internal + // callers (hooks, flows, reports, expand sub-reads) never pass through + // `findData`, so they still get the 200-in-arbitrary-order this axis + // refuses at the door. Closing it means deciding whether `engine.find` + // REFUSES or keeps its documented internal-caller tolerance — an + // engine-core contract decision, tracked separately. + // + // When that lands, this test SHOULD go red. Update it then; do not + // reach for it as evidence that the direct path is fine. + const asc = await engine.find('showcase_task', { orderBy: [{ field: 'sort_key', order: 'asc' }] }); + const desc = await engine.find('showcase_task', { orderBy: [{ field: 'sort_key', order: 'desc' }] }); + expect(asc.map((r: any) => r.title)).toEqual(INSERTION_ORDER); + // Direction-blind, and the rows carry the values they were meant to be + // ordered by — the exact signature from the issue's transcript. + expect(desc.map((r: any) => r.title)).toEqual(asc.map((r: any) => r.title)); + expect(asc.map((r: any) => r.sort_key)).toEqual(INSERTION_ORDER); + }); + // ───────────────────────────────────────────────────────────── // SELECT — control group, then rejected // ─────────────────────────────────────────────────────────────