diff --git a/.changeset/dotted-projection-refused.md b/.changeset/dotted-projection-refused.md new file mode 100644 index 0000000000..15ed48b12c --- /dev/null +++ b/.changeset/dotted-projection-refused.md @@ -0,0 +1,49 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): refuse a dotted `fields`/`$select` entry instead of widening the response to every field (#7532) + +`POST /api/v1/data/:object/query` with `{"fields":["name","account.name"]}` answered +`200` carrying **every** business field — strictly more data than was asked for — and +no resolved `account.name`. `GET /api/v1/data/:object?$select=name,account.name` did +the same. A parameter whose entire purpose is to return LESS had "return more" as its +failure mode, pointing away from both FLS and data minimisation. + +`assertProjectionFieldsExist` validated only `f.split('.')[0]`, so a dotted entry +cleared the #4226 unknown-name gate on its **head** segment (`account` really is a +field) and travelled on to the driver as a projection column. Measured on a real +`SqlDriver` (better-sqlite3): + +``` +no projection -> account amount created_at id name status updated_at +fields ['name'] -> name (a plain name narrows) +fields ['name','account.name'] -> account amount created_at id name status updated_at +fields ['account.name'] -> account amount created_at id name status updated_at +``` + +The dotted rows are byte-identical to no projection at all. Knex renders +`"account"."name"` against a table that was never joined, sqlite answers `no such +column`, and the driver's #3821 recovery ladder retries `select('*')` because rows +matter more than the projection. + +A dotted entry on this axis is now `400 INVALID_FIELD`, with the `unknown` > `dotted` +precedence {@link assertSortFieldsExist} already applies, so the two axes report the +same complaint first. The message names the relationship it tried to cross and sends +the caller to `expand` — the sanctioned door for related data here — or to +denormalising the value onto the queried object. A dotted path whose head is a real +but non-reference column gets its own wording, since `expand` would be the wrong +prescription for it. + +This also settles the second half of the report: an unknown **plain** column was a +`400` while an unknown **dotted** one was a `200` with every field, so one mistake got +opposite verdicts on one endpoint depending on how it was spelled. Both doors — +`POST /query` body `fields` and `GET ?$select=` — fold into the same slot before the +gate and are pinned separately. + +Nothing that worked stops working: no driver ever resolved these paths. Plain +projections still narrow, unknown plain columns still refuse per #4226, an unknown +head still gets the unknown-name verdict with its did-you-mean, and `expand` still +delivers related records. The engine's internal-caller projection tolerance and +`SqlDriver`'s recovery ladder are deliberately untouched — refusing at ingress is what +stops a request reaching them with a projection no driver can apply. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7ee497373a..5e47dee351 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -5429,6 +5429,52 @@ export class ObjectStackProtocolImplementation implements * narrowing it cost no caller anything; nothing equivalent has been measured * for the projection axis, and this sentence is not a licence to assume it. * + * [#7532] The DOTTED leg, which this gate used to pass on its head + * segment. `f.split('.')[0]` is what let `fields=['name','account.name']` + * through: `account` IS a field, so the entry cleared the unknown-name + * check above and reached the driver as a projection column. Measured at + * that commit on a REAL `SqlDriver` (better-sqlite3), against the same + * object the card reports: + * + * ``` + * no projection -> account amount created_at id name status updated_at + * fields ['name'] -> name (a plain name narrows) + * fields ['name','account.name'] -> account amount created_at id name status updated_at + * fields ['account.name'] -> account amount created_at id name status updated_at + * ``` + * + * The dotted rows are BYTE-IDENTICAL to no projection at all — the exact + * "asked for less, received more" this axis' first paragraph describes, + * reached by a different route. Knex renders `"account"."name"` against a + * table that was never joined, sqlite answers `no such column`, and + * `SqlDriver`'s #3821 recovery ladder retries `select('*')` because rows + * matter more than the projection. That ladder is a DRIVER-side tolerance + * for internal callers and is deliberately left alone here (filed + * separately as defence-in-depth); refusing at this ingress is what stops a + * request from reaching it carrying a projection no driver can apply. + * + * It also settles the card's second complaint: an unknown PLAIN column was + * a 400 while an unknown DOTTED one was a 200 with every field, so one + * mistake got opposite verdicts on one endpoint depending on spelling. + * + * The governing precedent is #5918 on the analytics MEASURES axis, which + * faced this exact shape and ruled the same way: refuse the dotted member + * loudly, naming the caller's original spelling, *because there is no + * correct answer to converge on*. That is the distinction from #5739, where + * refusing would have rejected queries that already compiled correctly. + * Here — as there — nothing resolved these paths, so both the typo + * (`titel.name`) and the genuine traversal intent (`account.name`) eat this + * 400: the two are not separable at this door, and the alternative is the + * over-return above. + * + * NOT a removal of a working feature — nothing resolved these paths. The + * spec's `fields` description, `query-syntax.mdx`, `data/query.mdx` and the + * `query.joins` / nested-select retirement prescriptions all still offer a + * dotted `fields` path as the way to read one related column; every one of + * them describes behaviour no driver implements. Aligning that prose with + * `expand` is spec/docs surface with its own blast radius and is called out + * on the PR rather than smuggled in here. + * * [#4196] It also owns the projection's SHAPE, which is a different * question from its names and is answered first — see below. */ @@ -5452,9 +5498,15 @@ export class ObjectStackProtocolImplementation implements ? ' The nested-select object form `{ field, fields, alias }` was removed in ' + '@objectstack/spec 17 (#4196) — no engine or driver ever read it.' : '') - + " Select related records with `expand` (`expand=owner` / `{ expand: { owner: " - + "{ object: 'user', fields: ['name'] } } }`), or name one related column with a " - + 'dotted path (`select=owner.name`).', + // [#7532] The dotted-path half of this prescription is GONE. + // It pointed at a spelling this same gate now refuses — and + // before that refusal it pointed at a spelling no driver + // resolves, which answered with every field. Naming it here + // sent the author from one refusal straight into the widening + // defect, the same dead end #6924 removed from the SORT axis' + // hint. `expand` is the one door for related data on this axis. + + " Select related records with `expand` (`expand=owner`, or `{ expand: { owner: " + + "{ object: 'user', fields: ['name'] } } }` to choose its columns).", ); err.code = 'INVALID_FIELD'; err.status = 400; @@ -5464,24 +5516,68 @@ export class ObjectStackProtocolImplementation implements } const gate = this.resolveQueryFields(object); if (!gate) return; - const unknown = (fields as string[]).filter((f) => !gate.known.has(f.split('.')[0])); - if (unknown.length === 0) return; - const first = unknown[0]; - const err: any = new Error( - `Unknown field '${first}' on object '${object}'` - + (unknown.length > 1 ? ` (also: ${unknown.slice(1).join(', ')})` : '') - + `. '${param}' chooses which fields to return; dropping an unknown one silently ` - + 'answered a NARROWER projection with a WIDER one — a projection naming no known ' - + 'field fell all the way back to every field.' - + suggestFieldName(first, gate.declared), + const names = fields as string[]; + const unknown = names.filter((f) => !gate.known.has(f.split('.')[0])); + if (unknown.length > 0) { + const first = unknown[0]; + const unknownErr: any = new Error( + `Unknown field '${first}' on object '${object}'` + + (unknown.length > 1 ? ` (also: ${unknown.slice(1).join(', ')})` : '') + + `. '${param}' chooses which fields to return; dropping an unknown one silently ` + + 'answered a NARROWER projection with a WIDER one — a projection naming no known ' + + 'field fell all the way back to every field.' + + suggestFieldName(first, gate.declared), + ); + unknownErr.code = 'INVALID_FIELD'; + unknownErr.status = 400; + unknownErr.field = first; + unknownErr.fields = unknown; + unknownErr.object = object; + unknownErr.param = param; + throw unknownErr; + } + // [#7532] The DOTTED verdict — the leg the head-segment check above + // does not cover, and the one that made this axis fail in the very + // direction its own docblock warns about. + // + // Ordered `unknown` > `dotted`, the same precedence + // {@link assertSortFieldsExist} applies, so the two axes agree about + // which complaint a caller hears first when an entry is both. + // + // It sits AFTER the `gate` early-return for the same reason the sort + // axis' dotted verdict does: the relation-vs-not split below reads + // `gate.fields`, and a registry-less host has no field map to read. + 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); + const dottedErr: any = new Error( + (crossesRelation + ? `Field '${first}' on object '${object}' follows the relationship '${head}' into ` + + `another object — '${param}' reaches only columns of '${object}' itself` + : `Field '${first}' on object '${object}' is a dotted path — '${param}' reaches only ` + + `whole columns of '${object}', not values inside them`) + + (dotted.length > 1 ? ` (also: ${dotted.slice(1).join(', ')})` : '') + + '. No driver resolves it: the path reaches the driver as a column name, matches no ' + + 'column, and the projection falls back to EVERY field — a narrower request answered ' + + 'with a wider response, which is the same failure the unknown-name refusal above ' + + 'exists to stop.' + + (crossesRelation + ? ` Read the related record with 'expand' (\`expand=${head}\`, or ` + + `\`{ expand: { ${head}: { object: '', fields: [''] } } }\` to ` + + `choose its columns), or denormalise the value onto '${object}' (a stored ` + + 'field, written when the source changes) and name that.' + : ` Name the whole column ('${head}') and read into its value in the caller.`), ); - err.code = 'INVALID_FIELD'; - err.status = 400; - err.field = first; - err.fields = unknown; - err.object = object; - err.param = param; - throw err; + dottedErr.code = 'INVALID_FIELD'; + dottedErr.status = 400; + dottedErr.field = first; + dottedErr.fields = dotted; + dottedErr.object = object; + dottedErr.param = param; + throw dottedErr; } /** diff --git a/packages/objectql/src/query-expression-conformance.test.ts b/packages/objectql/src/query-expression-conformance.test.ts index 9ea5b47d35..127293f932 100644 --- a/packages/objectql/src/query-expression-conformance.test.ts +++ b/packages/objectql/src/query-expression-conformance.test.ts @@ -901,13 +901,137 @@ describe('#4226 — sort / select / expand on the list path (real ObjectQL engin })).rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', param: 'fields' }); }); - it('a dotted path is still accepted — the replacement the rejection prescribes', async () => { - // The string form covers the readable half of what the object form - // claimed: the head segment is validated here, the tail resolved - // downstream. The rejection above points at this and at `expand`. + it('[#7532] a dotted path is REFUSED — it was never resolved, only widened', async () => { + // This test used to assert the OPPOSITE ("a dotted path is still + // accepted — the replacement the rejection prescribes"), on the + // reasoning that the head segment is validated here and the tail + // resolved downstream. The tail is resolved NOWHERE: measured on a real + // `SqlDriver` (better-sqlite3), a dotted projection comes back + // byte-identical to no projection at all, because the path reaches the + // driver as a column name, matches none, and the #3821 ladder retries + // `select('*')`. What this test protected was therefore not a narrower + // projection with a resolved relation — it was EVERY field. + // + // The shape rejection above no longer points here; it points at + // `expand` alone, and its hint was corrected to match. await expect(protocol.findData({ object: 'showcase_task', query: { select: 'parent_id.title' }, - })).resolves.toBeDefined(); + })).rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', param: 'select' }); + }); + + // ───────────────────────────────────────────────────────────── + // [#7532] DOTTED PROJECTION — the leg #4226's head-segment check + // did not cover. Controls first, then the rejections. + // ───────────────────────────────────────────────────────────── + + // GUARD (green before and after): the plain spelling still narrows, and + // narrows to EXACTLY these keys. An over-return defect passes any assertion + // written as "does not contain X", so the whole point of this axis has to + // be pinned as an equality on the key SET or it pins nothing. + it('[#7532 GUARD] a plain projection still narrows to exactly the named columns', async () => { + const r: any = await protocol.findData({ + object: 'showcase_task', query: { fields: ['title', 'status'] }, + }); + expect(Object.keys(r.records[0]).sort()).toEqual(['status', 'title']); + }); + + it('[#7532 GUARD] the same plain control through the GET door', async () => { + const r: any = await protocol.findData({ + object: 'showcase_task', query: { $select: 'title,status' }, + }); + expect(Object.keys(r.records[0]).sort()).toEqual(['status', 'title']); + }); + + // GUARD: #4226's own verdict is untouched — an unknown PLAIN column is + // still the 400 it has been since that card. If this ever goes red the + // change below stopped being additive. + it('[#7532 GUARD] an unknown plain column is still refused (#4226 intact)', async () => { + await expect(protocol.findData({ + object: 'showcase_task', query: { fields: ['no_such_field'] }, + })).rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', field: 'no_such_field' }); + }); + + // BOTH DOORS. The card measured `POST /query` body `fields` and + // `GET ?$select=` widening identically; both fold into `fields` through + // `WIRE_QUERY_ALIAS_SLOTS` before the gate, and both are pinned here so a + // future change that closes one and not the other cannot pass. + it.each([ + ['POST /query body fields', { fields: ['title', 'project_id.name'] }], + ['GET ?$select=', { $select: 'title,project_id.name' }], + ['GET ?select=', { select: 'title,project_id.name' }], + ['fields as a comma string', { fields: 'title,project_id.name' }], + ])('a dotted projection is refused, not answered with every field — %s', async (_label, query) => { + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ + status: 400, + code: 'INVALID_FIELD', + field: 'project_id.name', + object: 'showcase_task', + }); + }); + + it('a projection that is ONLY a dotted path is refused too', async () => { + // The card's worst shape: every entry unresolvable, so the projection + // emptied and fell all the way back to `*`. + await expect(protocol.findData({ + object: 'showcase_task', query: { fields: ['project_id.name'] }, + })).rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', field: 'project_id.name' }); + }); + + it('the refusal names the relationship it tried to cross and sends the caller to `expand`', async () => { + // A refusal that does not say where to go next is how #6924 described + // the SORT axis' dead end. `expand` is the sanctioned door for related + // data on this axis, so the message must name it. + await expect(protocol.findData({ + object: 'showcase_task', query: { fields: ['project_id.name'] }, + })).rejects.toThrow(/relationship 'project_id'.*expand/s); + }); + + it('a dotted path whose head is NOT a relationship gets the other message', async () => { + // `title` is a real column, so this clears the unknown check — but it + // is text, not a reference, so "follows the relationship" would be a + // lie and `expand` would be the wrong prescription. + await expect(protocol.findData({ + object: 'showcase_task', query: { fields: ['title.something'] }, + })).rejects.toMatchObject({ + status: 400, code: 'INVALID_FIELD', field: 'title.something', + }); + await expect(protocol.findData({ + object: 'showcase_task', query: { fields: ['title.something'] }, + })).rejects.toThrow(/dotted path.*whole columns/s); + }); + + // GUARD: precedence. An UNKNOWN head is still reported as the unknown-name + // verdict (with its did-you-mean), not as the dotted one — the same + // `unknown` > `dotted` order the sort axis applies. Green before and after: + // this shape was already a 400, and this pins that the new branch did not + // steal it. + it('[#7532 GUARD] an unknown HEAD is still the unknown-field verdict, not the dotted one', async () => { + await expect(protocol.findData({ + object: 'showcase_task', query: { fields: ['no_such.name'] }, + })).rejects.toMatchObject({ + status: 400, code: 'INVALID_FIELD', field: 'no_such.name', + }); + await expect(protocol.findData({ + object: 'showcase_task', query: { fields: ['no_such.name'] }, + })).rejects.toThrow(/Unknown field/); + }); + + // GUARD: the door the refusal points at actually works. A rejection that + // prescribes `expand` is only honest if `expand` delivers the related + // column — otherwise this card just closed the last route to it. + it('[#7532 GUARD] `expand` still delivers the related record the refusal prescribes', async () => { + // Exactly what a caller following the refusal writes: keep the + // reference COLUMN in the projection and expand it. Projecting it away + // (`fields: ['title']` alone) leaves expansion nothing to resolve — the + // relation is carried by the foreign key, so a narrowed projection must + // retain it. Measured while writing this test, and worth pinning: it is + // the one sharp edge in the remedy this card now prescribes. + const r: any = await protocol.findData({ + object: 'showcase_task', query: { fields: ['title', 'project_id'], expand: 'project_id' }, + }); + expect(Object.keys(r.records[0]).sort()).toEqual(['project_id', 'title']); + expect(r.records[0].project_id).toMatchObject({ id: 'p1', name: 'Apollo' }); }); it('the system columns the registry injected still project', async () => {