Skip to content

Commit 6443b79

Browse files
os-zhuangclaude
andauthored
fix(metadata-protocol): dotted-path SORT hint prescribes a stored field, not a formula (#6924) (#6996)
`assertSortFieldsExist` refuses a dotted `orderBy` and then told the author how to fix it: "Denormalise the value onto '<object>' (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 <formula> asc -> C A E B D 200 insertion order orderBy <formula> 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 <summary> 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. Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw Co-authored-by: Claude <noreply@anthropic.com>
1 parent 63f3b87 commit 6443b79

4 files changed

Lines changed: 144 additions & 6 deletions

File tree

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
fix(data): the dotted-path `400 INVALID_SORT` hint prescribes a **stored** field, not a formula (#6924)
6+
7+
`assertSortFieldsExist` refuses a dotted `orderBy` (`?sort=account.company_name`)
8+
and then told the author how to fix it: *"Denormalise the value onto '<object>'
9+
(a formula or rollup field that copies it into a real column) and sort by that."*
10+
That prescription cannot be built. Following it lands the author back inside the
11+
exact silent degradation the refusal had just saved them from.
12+
13+
Measured on a REAL `SqlDriver` (better-sqlite3) and on `InMemoryDriver`, with a
14+
`formula` field named directly in `orderBy` (non-dotted, so this gate lets it
15+
through):
16+
17+
```
18+
control orderBy title asc -> A B C D E a real column really sorts
19+
baseline no sort -> C A E B D insertion order
20+
orderBy <formula field> asc -> C A E B D 200 insertion order
21+
orderBy <formula field> desc -> C A E B D 200 direction-blind
22+
```
23+
24+
A `formula` field is virtual — `SqlDriver.createColumn` returns early for it and
25+
no column is created (sqlite answers `no such column`), the engine evaluates the
26+
expression *after* the driver returns, and the #3821 unknown-column backstop
27+
retries WITHOUT the sort. The response is `200`, every row present, order
28+
arbitrary: the failure mode #4226/#4256 exist to stop.
29+
30+
The hint now reads: *"Denormalise the value onto '<object>' (a stored field,
31+
written when the source changes) and sort by that. Not a formula field: it is
32+
virtual, no driver materialises a column for one, and ORDER BY on it is silently
33+
dropped."* — "stored" being the same word #6673 landed for the identical
34+
correction on the search axis.
35+
36+
`rollup`/`summary` is dropped from the hint for a different reason, and the
37+
measurement is worth recording because it contradicts the reported diagnosis: a
38+
`summary` field **does** get a real, maintained column (`orderBy <summary> desc`
39+
returned `E D C B A` over values `5 4 3 2 1`), so it is not unmaterializable. It
40+
simply cannot do this job — a rollup aggregates CHILD records
41+
(`count`/`sum`/`min`/`max`/`avg`) and so cannot carry a looked-up parent's column
42+
onto the queried object.
43+
44+
**This overturns a recorded decision.** #4256 (closed `completed`) explicitly
45+
chose the "formula or rollup" wording as its remedy for dotted-path sort, and its
46+
own still-pending changeset (`sort-dotted-path-rejected.md`) describes it; that
47+
file is left as the accurate record of what #4256 shipped, and this entry
48+
supersedes its prescription. `content/docs/protocol/objectql/query-syntax.mdx`
49+
("Sorting on Related Fields") taught the same denormalization and is corrected in
50+
the same change, so code and docs stop agreeing with each other about something
51+
untrue.
52+
53+
Not fixed here, filed separately: the platform still accepts a **non-dotted**
54+
`orderBy` naming a `formula` field and answers `200` in arbitrary order. That is
55+
an engine/driver-side refusal question, not hint text.

content/docs/protocol/objectql/query-syntax.mdx

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,12 +531,33 @@ no driver can order by it — `SqlDriver` would render it as
531531
`"account"."company_name"` against a table that was never joined, and until the
532532
path was refused, the unknown-column backstop retried **without the sort** and
533533
answered 200 with unordered rows. Denormalise the value onto the queried object
534-
(for example with a formula or rollup field) when you need to sort by it.
534+
as a **stored** field — one this object's own rows carry, written when the
535+
source changes — when you need to sort by it.
535536

536537
Internal callers reaching `engine.find()` directly are unaffected: a dotted
537538
`orderBy` there still falls through to the driver backstop and orders nothing.
538539
</Callout>
539540

541+
<Callout type="warn">
542+
**Do not denormalise onto a `formula` field to sort by it.** A `formula` field
543+
is virtual: no driver materialises a column for it (the engine evaluates it
544+
*after* the driver returns), so `ORDER BY` on one hits the same unknown-column
545+
backstop and is **silently dropped** — 200, every row present, arbitrary order.
546+
Measured on a real `SqlDriver` (better-sqlite3) and on `InMemoryDriver`: rows
547+
inserted `C A E B D` come back `C A E B D` for both `asc` and `desc`, while the
548+
same query on a stored column returns `A B C D E` / `E D C B A`.
549+
550+
A `rollup`/`summary` field *does* get a real, maintained column and can be
551+
sorted on — but it aggregates **child** records (`count`/`sum`/`min`/`max`/
552+
`avg`), so it cannot carry a looked-up parent's column such as
553+
`account.company_name`. For that, write the value onto a stored field of the
554+
queried object and keep it in sync (a trigger or flow on the source record).
555+
556+
This page taught the `formula`/`rollup` version until #6924, and so did the
557+
`400 INVALID_SORT` hint itself (#4256) — both are corrected together. The same
558+
correction on the search axis is #6673.
559+
</Callout>
560+
540561
---
541562

542563
## 4. Relationships (Expand)

packages/metadata-protocol/src/protocol.ts

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4819,6 +4819,41 @@ export class ObjectStackProtocolImplementation implements
48194819
* gets a message that says which relationship it tried to cross and
48204820
* prescribes what `query-syntax.mdx` has prescribed since #4240:
48214821
* denormalise the value onto the queried object and sort by that.
4822+
*
4823+
* [#6924] WHAT to denormalise onto was wrong, and this overturns #4256's
4824+
* own recorded wording. That issue chose "a formula or rollup field that
4825+
* copies it into a real column" — a prescription the platform cannot
4826+
* deliver, so the refusal handed the author a dead end at the exact moment
4827+
* they asked for help. Measured on a REAL `SqlDriver` (better-sqlite3) and
4828+
* on `InMemoryDriver`, with a `formula` field named directly (NOT dotted,
4829+
* so this gate lets it through):
4830+
*
4831+
* ```
4832+
* control orderBy title asc -> A B C D E (a real column sorts)
4833+
* baseline no sort -> C A E B D (insertion order)
4834+
* orderBy <formula field> asc -> C A E B D 200 (insertion order)
4835+
* orderBy <formula field> desc -> C A E B D 200 (direction-blind)
4836+
* ```
4837+
*
4838+
* No column exists to order by (`SqlDriver.createColumn` returns early for
4839+
* `formula`; sqlite answers `no such column`), the #3821 unknown-column
4840+
* backstop retries WITHOUT the sort, and the response is 200 with every
4841+
* row present in an arbitrary order — the very failure #4226/#4256 exist
4842+
* to stop. Following the old hint therefore landed the author back inside
4843+
* the defect they had just been refused for.
4844+
*
4845+
* `rollup`/`summary` was the other half of that wording and is NOT broken
4846+
* the same way — it does get a real, maintained column (`table.float`;
4847+
* measured: `orderBy <summary> desc` -> E D C B A over values 5 4 3 2 1).
4848+
* It is dropped from the hint because it cannot do THIS job: a rollup
4849+
* aggregates CHILD records (count/sum/min/max/avg), so it cannot carry a
4850+
* looked-up parent's column (`account.company_name`) onto this object.
4851+
* Wrong tool, not a broken one — naming it here still sends the author
4852+
* somewhere that cannot work.
4853+
*
4854+
* "Stored" is #6673's vocabulary for the same correction on the SEARCH
4855+
* axis (`validate-searchable-fields.ts`, "a stored text field"); the two
4856+
* axes deliberately say the same word.
48224857
*/
48234858
private assertSortFieldsExist(object: string, orderBy: ReadonlyArray<{ field: string }>, param: string): void {
48244859
if (orderBy.length === 0) return;
@@ -4855,8 +4890,10 @@ export class ObjectStackProtocolImplementation implements
48554890
+ "not values inside them")
48564891
+ (dotted.length > 1 ? ` (also: ${dotted.slice(1).join(', ')})` : ''),
48574892
{
4858-
hint: ` Denormalise the value onto '${object}' (a formula or rollup field that`
4859-
+ ' copies it into a real column) and sort by that.',
4893+
hint: ` Denormalise the value onto '${object}' (a stored field, written when the`
4894+
+ ' source changes) and sort by that. Not a formula field: it is virtual,'
4895+
+ ' no driver materialises a column for one, and ORDER BY on it is silently'
4896+
+ ' dropped.',
48604897
extra: { field: first, fields: dotted, object },
48614898
},
48624899
);

packages/objectql/src/query-expression-conformance.test.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -450,9 +450,34 @@ describe('#4226 — sort / select / expand on the list path (real ObjectQL engin
450450
});
451451
});
452452

453-
it('the dotted rejection names the relationship it tried to cross and prescribes the fix', async () => {
454-
await expect(protocol.findData({ object: 'showcase_task', query: { sort: 'project_id.name' } }))
455-
.rejects.toThrow(/follows the relationship 'project_id'[\s\S]*formula or rollup/);
453+
it('the dotted rejection names the relationship it tried to cross and prescribes a STORED field', async () => {
454+
// [#6924] The prescription is part of the contract, not decoration: a
455+
// refusal that hands the author an unbuildable fix is the same dead end
456+
// as no hint at all. #4256 chose "a formula or rollup field that copies
457+
// it into a real column"; measured on a REAL SqlDriver (better-sqlite3)
458+
// and on InMemoryDriver, `orderBy` naming a `formula` field answers 200
459+
// with the rows in INSERTION order, identically for asc and desc — no
460+
// column exists, so the #3821 backstop retries without the sort. That
461+
// is the exact silent degradation this gate exists to stop, so the old
462+
// hint routed the author back into it.
463+
const err: any = await protocol
464+
.findData({ object: 'showcase_task', query: { sort: 'project_id.name' } })
465+
.then(() => null, (e: unknown) => e);
466+
expect(err).toBeTruthy();
467+
// ADR-0112 envelope — a rejection case asserts code AND status, not
468+
// merely that something was thrown.
469+
expect(err.status).toBe(400);
470+
expect(err.code).toBe('INVALID_SORT');
471+
expect(err.message).toMatch(/follows the relationship 'project_id'/);
472+
// The remedy must be a STORED field — #6673's vocabulary for the same
473+
// correction on the SEARCH axis, deliberately the same word here.
474+
expect(err.message).toMatch(/a stored field/);
475+
// ...and the old prescription must be gone, not merely joined.
476+
expect(err.message).not.toMatch(/formula or rollup/);
477+
// `formula` may still appear — but only as the named trap, never as the
478+
// thing to build. This is what separates the fix from a reword that
479+
// keeps the dead end in a subordinate clause.
480+
expect(err.message).toMatch(/Not a formula field/);
456481
});
457482

458483
it('a dotted path under a non-reference head is refused on the same axis, minus the relationship claim', async () => {

0 commit comments

Comments
 (0)