From cb18f2986a9a963c59d873727b3f886fae04c65e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 08:08:20 +0000 Subject: [PATCH] fix(metadata-protocol): dotted-path SORT hint prescribes a stored field, not a formula (#6924) `assertSortFieldsExist` refuses a dotted `orderBy` and then told the author how to fix it: "Denormalise the value onto '' (a formula or rollup field that copies it into a real column) and sort by that." Following that lands the author back inside the exact silent degradation the refusal saved them from. Measured on a REAL SqlDriver (better-sqlite3) and on InMemoryDriver, with a `formula` field named directly in `orderBy` (non-dotted, so the gate lets it through): control orderBy title asc -> A B C D E a real column really sorts baseline no sort -> C A E B D insertion order orderBy asc -> C A E B D 200 insertion order orderBy desc -> C A E B D 200 direction-blind `SqlDriver.createColumn` returns early for `formula` (no column), sqlite answers "no such column", the #3821 backstop retries WITHOUT the sort, and the response is 200 with every row present in arbitrary order. `rollup`/`summary` is dropped for a DIFFERENT reason, and the measurement contradicts the reported diagnosis: a summary field does get a real, maintained column (orderBy desc -> E D C B A over values 5 4 3 2 1). It simply cannot do this job, since a rollup aggregates CHILD records and cannot carry a looked-up parent's column onto the queried object. This overturns #4256's recorded wording choice (closed `completed`), which explicitly picked the "formula or rollup" phrasing as its remedy. The docs callout at content/docs/protocol/objectql/query-syntax.mdx taught the same denormalization, so code and docs agreed with each other about something untrue; both move here. "Stored" is #6673's vocabulary for the identical correction on the search axis. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw --- .../sort-hint-prescribes-stored-field.md | 55 +++++++++++++++++++ .../docs/protocol/objectql/query-syntax.mdx | 23 +++++++- packages/metadata-protocol/src/protocol.ts | 41 +++++++++++++- .../src/query-expression-conformance.test.ts | 31 ++++++++++- 4 files changed, 144 insertions(+), 6 deletions(-) create mode 100644 .changeset/sort-hint-prescribes-stored-field.md diff --git a/.changeset/sort-hint-prescribes-stored-field.md b/.changeset/sort-hint-prescribes-stored-field.md new file mode 100644 index 0000000000..f347ecd21b --- /dev/null +++ b/.changeset/sort-hint-prescribes-stored-field.md @@ -0,0 +1,55 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(data): the dotted-path `400 INVALID_SORT` hint prescribes a **stored** field, not a formula (#6924) + +`assertSortFieldsExist` refuses a dotted `orderBy` (`?sort=account.company_name`) +and then told the author how to fix it: *"Denormalise the value onto '' +(a formula or rollup field that copies it into a real column) and sort by that."* +That prescription cannot be built. Following it lands the author back inside the +exact silent degradation the refusal had just saved them from. + +Measured on a REAL `SqlDriver` (better-sqlite3) and on `InMemoryDriver`, with a +`formula` field named directly in `orderBy` (non-dotted, so this gate lets it +through): + +``` +control orderBy title asc -> A B C D E a real column really sorts +baseline no sort -> C A E B D insertion order +orderBy asc -> C A E B D 200 insertion order +orderBy desc -> C A E B D 200 direction-blind +``` + +A `formula` field is virtual — `SqlDriver.createColumn` returns early for it and +no column is created (sqlite answers `no such column`), the engine evaluates the +expression *after* the driver returns, and the #3821 unknown-column backstop +retries WITHOUT the sort. The response is `200`, every row present, order +arbitrary: the failure mode #4226/#4256 exist to stop. + +The hint now reads: *"Denormalise the value onto '' (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."* — "stored" being the same word #6673 landed for the identical +correction on the search axis. + +`rollup`/`summary` is dropped from the hint for a different reason, and the +measurement is worth recording because it contradicts the reported diagnosis: a +`summary` field **does** get a real, maintained column (`orderBy desc` +returned `E D C B A` over values `5 4 3 2 1`), so it is not unmaterializable. It +simply cannot do this job — a rollup aggregates CHILD records +(`count`/`sum`/`min`/`max`/`avg`) and so cannot carry a looked-up parent's column +onto the queried object. + +**This overturns a recorded decision.** #4256 (closed `completed`) explicitly +chose the "formula or rollup" wording as its remedy for dotted-path sort, and its +own still-pending changeset (`sort-dotted-path-rejected.md`) describes it; that +file is left as the accurate record of what #4256 shipped, and this entry +supersedes its prescription. `content/docs/protocol/objectql/query-syntax.mdx` +("Sorting on Related Fields") taught the same denormalization and is corrected in +the same change, so code and docs stop agreeing with each other about something +untrue. + +Not fixed here, filed separately: the platform still accepts a **non-dotted** +`orderBy` naming a `formula` field and answers `200` in arbitrary order. That is +an engine/driver-side refusal question, not hint text. diff --git a/content/docs/protocol/objectql/query-syntax.mdx b/content/docs/protocol/objectql/query-syntax.mdx index 638ecfb9eb..640fe8b433 100644 --- a/content/docs/protocol/objectql/query-syntax.mdx +++ b/content/docs/protocol/objectql/query-syntax.mdx @@ -531,12 +531,33 @@ no driver can order by it — `SqlDriver` would render it as `"account"."company_name"` against a table that was never joined, and until the path was refused, the unknown-column backstop retried **without the sort** and answered 200 with unordered rows. Denormalise the value onto the queried object -(for example with a formula or rollup field) when you need to sort by it. +as a **stored** field — one this object's own rows carry, written when the +source changes — when you need to sort by it. Internal callers reaching `engine.find()` directly are unaffected: a dotted `orderBy` there still falls through to the driver backstop and orders nothing. + +**Do not denormalise onto a `formula` field to sort by it.** A `formula` field +is virtual: no driver materialises a column for it (the engine evaluates it +*after* the driver returns), so `ORDER BY` on one hits the same unknown-column +backstop and is **silently dropped** — 200, every row present, arbitrary order. +Measured on a real `SqlDriver` (better-sqlite3) and on `InMemoryDriver`: rows +inserted `C A E B D` come back `C A E B D` for both `asc` and `desc`, while the +same query on a stored column returns `A B C D E` / `E D C B A`. + +A `rollup`/`summary` field *does* get a real, maintained column and can be +sorted on — but it aggregates **child** records (`count`/`sum`/`min`/`max`/ +`avg`), so it cannot carry a looked-up parent's column such as +`account.company_name`. For that, write the value onto a stored field of the +queried object and keep it in sync (a trigger or flow on the source record). + +This page taught the `formula`/`rollup` version until #6924, and so did the +`400 INVALID_SORT` hint itself (#4256) — both are corrected together. The same +correction on the search axis is #6673. + + --- ## 4. Relationships (Expand) diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7773db4c4f..d649120aea 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -4819,6 +4819,41 @@ export class ObjectStackProtocolImplementation implements * gets a message that says which relationship it tried to cross and * prescribes what `query-syntax.mdx` has prescribed since #4240: * denormalise the value onto the queried object and sort by that. + * + * [#6924] WHAT to denormalise onto was wrong, and this overturns #4256's + * own recorded wording. That issue chose "a formula or rollup field that + * 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): + * + * ``` + * control orderBy title asc -> A B C D E (a real column sorts) + * baseline no sort -> C A E B D (insertion order) + * orderBy asc -> C A E B D 200 (insertion order) + * orderBy desc -> C A E B D 200 (direction-blind) + * ``` + * + * No column exists to order by (`SqlDriver.createColumn` returns early for + * `formula`; sqlite answers `no such column`), the #3821 unknown-column + * backstop retries WITHOUT the sort, and the response is 200 with every + * row present in an arbitrary order — the very failure #4226/#4256 exist + * to stop. Following the old hint therefore landed the author back inside + * the defect they had just been refused for. + * + * `rollup`/`summary` was the other half of that wording and is NOT broken + * the same way — it does get a real, maintained column (`table.float`; + * measured: `orderBy desc` -> E D C B A over values 5 4 3 2 1). + * It is dropped from the hint because it cannot do THIS job: a rollup + * aggregates CHILD records (count/sum/min/max/avg), so it cannot carry a + * looked-up parent's column (`account.company_name`) onto this object. + * Wrong tool, not a broken one — naming it here still sends the author + * somewhere that cannot work. + * + * "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. */ private assertSortFieldsExist(object: string, orderBy: ReadonlyArray<{ field: string }>, param: string): void { if (orderBy.length === 0) return; @@ -4855,8 +4890,10 @@ export class ObjectStackProtocolImplementation implements + "not values inside them") + (dotted.length > 1 ? ` (also: ${dotted.slice(1).join(', ')})` : ''), { - hint: ` Denormalise the value onto '${object}' (a formula or rollup field that` - + ' copies it into a real column) and sort by that.', + 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 }, }, ); diff --git a/packages/objectql/src/query-expression-conformance.test.ts b/packages/objectql/src/query-expression-conformance.test.ts index 7f8488969c..c3313d2e20 100644 --- a/packages/objectql/src/query-expression-conformance.test.ts +++ b/packages/objectql/src/query-expression-conformance.test.ts @@ -450,9 +450,34 @@ describe('#4226 — sort / select / expand on the list path (real ObjectQL engin }); }); - it('the dotted rejection names the relationship it tried to cross and prescribes the fix', async () => { - await expect(protocol.findData({ object: 'showcase_task', query: { sort: 'project_id.name' } })) - .rejects.toThrow(/follows the relationship 'project_id'[\s\S]*formula or rollup/); + it('the dotted rejection names the relationship it tried to cross and prescribes a STORED field', async () => { + // [#6924] The prescription is part of the contract, not decoration: a + // refusal that hands the author an unbuildable fix is the same dead end + // as no hint at all. #4256 chose "a formula or rollup field that copies + // it into a real column"; measured on a REAL SqlDriver (better-sqlite3) + // and on InMemoryDriver, `orderBy` naming a `formula` field answers 200 + // with the rows in INSERTION order, identically for asc and desc — no + // column exists, so the #3821 backstop retries without the sort. That + // is the exact silent degradation this gate exists to stop, so the old + // hint routed the author back into it. + const err: any = await protocol + .findData({ object: 'showcase_task', query: { sort: 'project_id.name' } }) + .then(() => null, (e: unknown) => e); + expect(err).toBeTruthy(); + // ADR-0112 envelope — a rejection case asserts code AND status, not + // merely that something was thrown. + expect(err.status).toBe(400); + expect(err.code).toBe('INVALID_SORT'); + expect(err.message).toMatch(/follows the relationship 'project_id'/); + // The remedy must be a STORED field — #6673's vocabulary for the same + // correction on the SEARCH axis, deliberately the same word here. + expect(err.message).toMatch(/a stored field/); + // ...and the old prescription must be gone, not merely joined. + expect(err.message).not.toMatch(/formula or rollup/); + // `formula` may still appear — but only as the named trap, never as the + // thing to build. This is what separates the fix from a reword that + // keeps the dead end in a subordinate clause. + expect(err.message).toMatch(/Not a formula field/); }); it('a dotted path under a non-reference head is refused on the same axis, minus the relationship claim', async () => {