diff --git a/.changeset/expand-nested-fields-join-key.md b/.changeset/expand-nested-fields-join-key.md new file mode 100644 index 0000000000..4c054a0c69 --- /dev/null +++ b/.changeset/expand-nested-fields-join-key.md @@ -0,0 +1,44 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): `expand` no longer silently no-ops when the nested `fields` omits `id` (#7537) + +`expand: { account: { object: 'showcase_account', fields: ['name'] } }` answered +`200` with `account` still holding the **raw foreign-key id** — the expansion did +not happen, and nothing in the response said so. Adding `"id"` to the nested +projection made it work. The failing spelling is not an exotic one: it is the form +**prescribed verbatim** by two spec retirement messages, +`FIELD_NODE_OBJECT_FORM_REMOVED` and `QUERY_JOINS_REMOVED`, which both tell authors +migrating off the retired nested-select object form and off `query.joins` to write +`expand: { owner: { object: 'user', fields: ['name'] } }`. A caller who did exactly +what the error message instructed got a silent no-op. + +**Cause.** `expandRelatedRecords` forwarded `nestedAST.fields` to the sub-read +verbatim and then keyed its lookup map on `rec.id`. A projection that did not name +`id` therefore produced rows with no `id`, an empty map, and an injection that fell +through `recordMap.get(String(val)) ?? val` — writing the original foreign key back. +Because the fallback is the caller's own id, "expanded" and "not expanded" were +indistinguishable in the response. + +**Fix.** The join key is machinery, not a caller-chosen column, so it is now added to +the sub-read's projection unconditionally and **stripped back out** of the emitted +nested record when the caller did not ask for it. The prescribed spelling is made to +work rather than refused — refusing would turn two retirement messages' own migration +target into an error. `id` is the only join key there is: a reference field names a +target *object* (`referenceTargetOf`) and carries no target-column metadata, so the +batch filter is `{ id: { $in } }` by construction. + +This covers every expand entry point (`find`, `findOne`, and recursive nested +expands), since all of them route through the one helper. The strip runs after the +recursive pass, which still needs the key to rebuild its map. + +**Visible change beyond the fix.** A nested projection that omits `id` now emits a +nested record without `id`, matching the columns the caller named. Previously the +result depended on the driver: `SqlDriver` honours a projection exactly +(`builder.select(query.fields)`) and produced the no-op above, while +`InMemoryDriver.projectFields` force-adds `id` to every projection and so returned an +expanded record that carried an unrequested `id`. Both now emit exactly the projected +columns. Callers that read `.id` off such a record should add `id` to the nested +`fields` — on any SQL-backed store there was nothing to read there before, since the +value was still a plain foreign-key string. diff --git a/content/docs/protocol/objectql/query-syntax.mdx b/content/docs/protocol/objectql/query-syntax.mdx index e1a112dff7..01fc2dc783 100644 --- a/content/docs/protocol/objectql/query-syntax.mdx +++ b/content/docs/protocol/objectql/query-syntax.mdx @@ -670,6 +670,12 @@ The nested `where` is **AND-merged** with the batch `$in` the engine uses to loa records, so a related record is attached only when it also matches your filter. A foreign key whose target is filtered out is left as the raw id (unresolved) rather than dropped. +The nested `fields` is a **projection of the related record**, and you do not have to name +`id` in it to make the expansion work. `id` is the join key the batch `$in` re-attaches by, +so the engine adds it to its own sub-read and strips it back out when you did not ask for +it — the attached record carries exactly the columns you listed. Name `id` explicitly when +you want it (e.g. to link to the related record). + **Per-parent shaping (`limit` / `offset` / `orderBy`) is not honored on the expand path.** The engine batch-loads every parent's related records in a single `$in` query and then diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index 0239497ae6..1ac2b30981 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -1785,6 +1785,156 @@ describe('ObjectQL Engine', () => { expect(expandCall[1]).not.toHaveProperty('top'); }); + describe('[#7537] nested `fields` that omits the join key', () => { + // The nested projection is forwarded to the sub-read and the lookup + // map is keyed on `rec.id`, so a projection that did not name `id` + // produced rows with no `id`, an EMPTY map, and an injection that + // fell through `recordMap.get(...) ?? val` — writing the original FK + // back. `200`, and the field still holds a valid-looking id. + // + // That spelling is PRESCRIBED VERBATIM by two retirement messages + // (`FIELD_NODE_OBJECT_FORM_REMOVED`, `QUERY_JOINS_REMOVED`), so it + // is made to work rather than refused: `id` is forced into the + // sub-read and stripped from the emitted record when unrequested. + // + // These mocks return exactly the requested columns — what a + // projection-honouring driver does (`SqlDriver`: + // `builder.select(query.fields)`). The real-driver end of this pins + // at `runtime/src/expand-nested-fields-join-key.integration.test.ts`. + // The surrounding file reaches `vi.mocked(SchemaRegistry.getObject)` + // directly, which tsc reports twice per site (TS2339 — `getObject` + // is not on the static type — plus TS7006 on the untyped `name`). + // Those sites are frozen TEST_DEBT; this block routes through one + // typed helper instead of adding four more to the pile. + const mockSchema = (impl: (name: string) => any) => { + vi.mocked((SchemaRegistry as any).getObject).mockImplementation(impl); + }; + + const registerTaskUser = () => { + mockSchema((name: string) => { + if (name === 'task') return { + name: 'task', + fields: { + assignee: { type: 'lookup', reference: 'user' }, + title: { type: 'text' }, + }, + } as any; + if (name === 'user') return { + name: 'user', + fields: { name: { type: 'text' }, email: { type: 'text' } }, + } as any; + return undefined; + }); + }; + + it('forces the join key into the sub-read projection and expands', async () => { + registerTaskUser(); + vi.mocked(mockDriver.find) + .mockResolvedValueOnce([ + { id: 't1', title: 'Task 1', assignee: 'u1' }, + { id: 't2', title: 'Task 2', assignee: 'u2' }, + ]) + // A projection-honouring driver, given `['name','id']`. + .mockResolvedValueOnce([ + { name: 'Alice', id: 'u1' }, + { name: 'Bob', id: 'u2' }, + ]); + + const result = await engine.find('task', { + expand: { assignee: { object: 'user', fields: ['name'] } }, + }); + + // The sub-read asked for the join key even though the caller did not. + const expandCall = vi.mocked(mockDriver.find).mock.calls[1]; + expect((expandCall[1] as any).fields).toEqual(['name', 'id']); + + // SUBSTANCE: the nested value is the related record projected to + // `name` — not the raw FK, and not carrying the machinery column. + expect(result[0].assignee).toEqual({ name: 'Alice' }); + expect(result[1].assignee).toEqual({ name: 'Bob' }); + expect(result[0].assignee).not.toHaveProperty('id'); + expect(result[0].assignee).not.toHaveProperty('email'); + }); + + it('leaves the projection untouched when the caller already asked for `id`', async () => { + registerTaskUser(); + vi.mocked(mockDriver.find) + .mockResolvedValueOnce([{ id: 't1', title: 'Task 1', assignee: 'u1' }]) + .mockResolvedValueOnce([{ id: 'u1', name: 'Alice' }]); + + const result = await engine.find('task', { + expand: { assignee: { object: 'user', fields: ['id', 'name'] } }, + }); + + const expandCall = vi.mocked(mockDriver.find).mock.calls[1]; + expect((expandCall[1] as any).fields).toEqual(['id', 'name']); + expect(result[0].assignee).toEqual({ id: 'u1', name: 'Alice' }); + }); + + it('strips the join key from a MULTIPLE (array-valued) expansion too', async () => { + mockSchema((name: string) => { + if (name === 'task') return { + name: 'task', + fields: { watchers: { type: 'lookup', reference: 'user', multiple: true } }, + } as any; + if (name === 'user') return { name: 'user', fields: { name: { type: 'text' } } } as any; + return undefined; + }); + + vi.mocked(mockDriver.find) + .mockResolvedValueOnce([{ id: 't1', watchers: ['u1', 'u2'] }]) + .mockResolvedValueOnce([ + { name: 'Alice', id: 'u1' }, + { name: 'Bob', id: 'u2' }, + ]); + + const result = await engine.find('task', { + expand: { watchers: { object: 'user', fields: ['name'] } }, + }); + + expect(result[0].watchers).toEqual([{ name: 'Alice' }, { name: 'Bob' }]); + }); + + it('keeps the join key available to a RECURSIVE nested expand, then strips it', async () => { + // The recursion rebuilds the map keyed on `rec.id`, so the strip + // must happen after it — a strip done at sub-read time would + // break the inner level exactly the way the outer one was broken. + mockSchema((name: string) => { + if (name === 'task') return { + name: 'task', + fields: { assignee: { type: 'lookup', reference: 'user' } }, + } as any; + if (name === 'user') return { + name: 'user', + fields: { name: { type: 'text' }, team: { type: 'lookup', reference: 'team' } }, + } as any; + if (name === 'team') return { name: 'team', fields: { label: { type: 'text' } } } as any; + return undefined; + }); + + vi.mocked(mockDriver.find) + .mockResolvedValueOnce([{ id: 't1', assignee: 'u1' }]) + // user sub-read, projection ['name','team','id'] + .mockResolvedValueOnce([{ name: 'Alice', team: 'g1', id: 'u1' }]) + // team sub-read, projection ['label','id'] + .mockResolvedValueOnce([{ label: 'Core', id: 'g1' }]); + + const result = await engine.find('task', { + expand: { + assignee: { + object: 'user', + fields: ['name', 'team'], + expand: { team: { object: 'team', fields: ['label'] } }, + }, + }, + }); + + expect(result[0].assignee).toEqual({ name: 'Alice', team: { label: 'Core' } }); + expect(result[0].assignee).not.toHaveProperty('id'); + expect(result[0].assignee.team).not.toHaveProperty('id'); + }); + }); + it('should expand master_detail fields', async () => { vi.mocked(SchemaRegistry.getObject).mockImplementation((name) => { if (name === 'order_item') return { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 0ce09e2517..99dd18a5c6 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -6282,6 +6282,39 @@ export class ObjectQL implements IObjectQLEngine { // applies to both. `expand` is intentionally omitted from this query so // `find` does not re-expand — nested relations recurse below under the // depth guard. + // [#7537] The join key is MACHINERY, not a caller-chosen column. This + // sub-read's projection is forwarded from `nestedAST.fields`, and the + // map built below is keyed on `rec.id` — so a nested projection that did + // not name `id` returned rows carrying no `id`, built an EMPTY map, and + // fell through the `recordMap.get(...) ?? val` injection, writing the + // original foreign key back. The expansion was then indistinguishable + // from never having been requested: `200`, and the field still holds a + // valid-looking id. + // + // The failing spelling is the one the platform itself PRESCRIBES: both + // `FIELD_NODE_OBJECT_FORM_REMOVED` and `QUERY_JOINS_REMOVED` + // (`query.zod.ts`) tell authors migrating off the retired nested-select + // object form and off `query.joins` to write + // `expand: { owner: { object: 'user', fields: ['name'] } }`. Refusing the + // mismatch would turn two retirement messages' own migration target into + // an error, so the prescribed form is made to WORK instead: `id` is added + // to the sub-read unconditionally and stripped back out of the emitted + // nested record when the caller did not ask for it. + // + // `id` is the only join key there is, literally: `referenceTargetOf` + // yields the target OBJECT and a reference field carries no + // target-column metadata, so the batch filter above is `{ id: { $in } }` + // by construction. There is no configurable key to add instead. + // + // Only a driver that honours a projection exactly could ever show this — + // `SqlDriver` emits `builder.select(query.fields)` verbatim, while + // `InMemoryDriver.projectFields` force-adds `id` to every projection, + // which is why mock/in-memory suites could not reach the defect and a + // better-sqlite3 QA run (#7463) could. + const nestedFields = Array.isArray(nestedAST.fields) && nestedAST.fields.length > 0 + ? nestedAST.fields + : undefined; + const joinKeyRequested = !nestedFields || nestedFields.includes('id'); const relatedRecords = await this.find( referenceObject, { @@ -6289,7 +6322,9 @@ export class ObjectQL implements IObjectQLEngine { // [#6300] The `as any` these two carried is gone: `find` takes the // author state now, and the parsed nodes a `QueryAST` holds are // valid author input (a present `order` is legal to write). - ...(nestedAST.fields ? { fields: nestedAST.fields } : {}), + ...(nestedAST.fields + ? { fields: joinKeyRequested ? nestedAST.fields : [...nestedFields!, 'id'] } + : {}), ...(nestedAST.orderBy ? { orderBy: nestedAST.orderBy } : {}), context: { ...(execCtx ?? {}), __expandRead: true } as ExecutionContext, }, @@ -6319,6 +6354,20 @@ export class ObjectQL implements IObjectQLEngine { } } + // [#7537] Strip the join key back out when the caller did not name it. + // It was added above for THIS function's own lookup, so the emitted + // nested record carries exactly the columns the projection asked for — + // the same contract a top-level `fields` gets. Deliberately AFTER the + // recursive expand: that block rebuilds `recordMap` keyed on `rec.id` + // and still needs the key present. Each related record is stripped once + // here rather than per-parent, because one record object is injected + // into every parent that points at it. + if (!joinKeyRequested) { + for (const rec of recordMap.values()) { + if (rec && typeof rec === 'object') delete rec.id; + } + } + // Inject expanded records back into the original result set for (const record of records) { const val = record[fieldName]; diff --git a/packages/runtime/src/expand-nested-fields-join-key.integration.test.ts b/packages/runtime/src/expand-nested-fields-join-key.integration.test.ts new file mode 100644 index 0000000000..b904ed1b72 --- /dev/null +++ b/packages/runtime/src/expand-nested-fields-join-key.integration.test.ts @@ -0,0 +1,173 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7537 — `expand` must not degrade to a no-op when the nested `fields` omits `id`. + * + * `expandRelatedRecords` forwards `nestedAST.fields` to the sub-`find` verbatim + * and then keys its `recordMap` on `rec.id`. A nested projection that does not + * name `id` therefore produced rows with no `id`, an EMPTY map, and an injection + * that fell through `recordMap.get(String(val)) ?? val` — writing the original + * foreign key back. Nothing in the response distinguished "expanded" from "not + * expanded": the field just still held an id, which is a valid-looking value. + * + * ## Why this needs a REAL driver + * + * The defect is invisible to the drivers most suites reach for. `InMemoryDriver` + * force-adds the primary key to every projection ("Always include id if not + * explicitly listed", `memory-driver.ts` `projectFields`), so its rows always + * carry the join key and the map is never empty. `SqlDriver` emits the requested + * column list verbatim (`builder.select(query.fields...)`), so it is the side + * that actually drops `id` — which is why the QA run that found this (#7463) hit + * it on better-sqlite3 and why the engine's mock-driver unit pins could not. + * This wires the REAL {@link ObjectQL} engine to the REAL {@link SqlDriver}. + * + * ## Why the no-`id` spelling is the one that must work + * + * It is PRESCRIBED VERBATIM by two spec retirement messages — + * `FIELD_NODE_OBJECT_FORM_REMOVED` and `QUERY_JOINS_REMOVED` (`query.zod.ts`) + * both tell the author to write `expand: { owner: { object: 'user', fields: + * ['name'] } }`. A caller who does exactly what the error message instructed got + * a silent no-op, so refusing the mismatch would turn the documented migration + * target into an error. The join key is added to the sub-read unconditionally + * (it is machinery, not a caller-chosen column) and stripped from the emitted + * nested record when the caller did not ask for it. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; + +const ACCOUNT = { + name: 'showcase_account', + fields: { + name: { type: 'text' }, + industry: { type: 'text' }, + }, +}; + +const INVOICE = { + name: 'showcase_invoice', + fields: { + name: { type: 'text' }, + account: { type: 'lookup', reference: 'showcase_account' }, + }, +}; + +// The second object pair from the issue's repro — the report confirmed 2/2, so +// both pairs are pinned rather than only the one that was written up first. +const PROJECT = { + name: 'showcase_project', + fields: { + name: { type: 'text' }, + }, +}; + +const TASK = { + name: 'showcase_task', + fields: { + name: { type: 'text' }, + project: { type: 'lookup', reference: 'showcase_project' }, + }, +}; + +describe('#7537 expand with a nested `fields` that omits the join key (REAL SqlDriver)', () => { + let engine: ObjectQL | null = null; + let dir: string | null = null; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + engine = null; + if (dir) { rmSync(dir, { recursive: true, force: true }); dir = null; } + }); + + async function boot() { + dir = mkdtempSync(join(tmpdir(), 'os-expand-7537-')); + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + await driver.initObjects([ACCOUNT, INVOICE, PROJECT, TASK]); + engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + for (const obj of [ACCOUNT, INVOICE, PROJECT, TASK]) { + engine.registry.registerObject(obj as any); + } + return engine; + } + + it('expands with `fields: ["name"]` — the spelling the retirement messages prescribe', async () => { + const e = await boot(); + const acct: any = await e.insert('showcase_account', { name: 'Acme', industry: 'Manufacturing' }); + await e.insert('showcase_invoice', { name: 'INV-001', account: acct.id }); + + const rows = await e.find('showcase_invoice', { + expand: { account: { object: 'showcase_account', fields: ['name'] } }, + }); + + expect(rows).toHaveLength(1); + // SUBSTANCE: the nested value is the RELATED RECORD, not the raw FK id. + expect(typeof rows[0].account).toBe('object'); + expect(rows[0].account.name).toBe('Acme'); + // Projected to exactly what was asked for: the unrequested column is absent… + expect(rows[0].account).not.toHaveProperty('industry'); + // …and so is the join key the engine added for its own use. + expect(rows[0].account).not.toHaveProperty('id'); + expect(rows[0].account).toEqual({ name: 'Acme' }); + }); + + it('keeps `id` when the caller DID ask for it (`fields: ["id","name"]` — the workaround spelling)', async () => { + const e = await boot(); + const acct: any = await e.insert('showcase_account', { name: 'Acme', industry: 'Manufacturing' }); + await e.insert('showcase_invoice', { name: 'INV-001', account: acct.id }); + + const rows = await e.find('showcase_invoice', { + expand: { account: { object: 'showcase_account', fields: ['id', 'name'] } }, + }); + + expect(rows[0].account).toEqual({ id: acct.id, name: 'Acme' }); + }); + + it('second object pair (showcase_task → project) — the issue confirmed 2/2', async () => { + const e = await boot(); + const proj: any = await e.insert('showcase_project', { name: 'Apollo' }); + await e.insert('showcase_task', { name: 'Design', project: proj.id }); + + const rows = await e.find('showcase_task', { + expand: { project: { object: 'showcase_project', fields: ['name'] } }, + }); + + expect(rows[0].project).toEqual({ name: 'Apollo' }); + }); + + it('findOne takes the same path (single-record entry point)', async () => { + const e = await boot(); + const acct: any = await e.insert('showcase_account', { name: 'Acme', industry: 'Manufacturing' }); + const inv: any = await e.insert('showcase_invoice', { name: 'INV-001', account: acct.id }); + + const row: any = await e.findOne('showcase_invoice', { + where: { id: inv.id }, + expand: { account: { object: 'showcase_account', fields: ['name'] } }, + }); + + expect(row.account).toEqual({ name: 'Acme' }); + }); + + it('no projection at all still returns the whole related record (unchanged behaviour)', async () => { + const e = await boot(); + const acct: any = await e.insert('showcase_account', { name: 'Acme', industry: 'Manufacturing' }); + await e.insert('showcase_invoice', { name: 'INV-001', account: acct.id }); + + const rows = await e.find('showcase_invoice', { + expand: { account: { object: 'showcase_account' } }, + }); + + expect(rows[0].account.id).toBe(acct.id); + expect(rows[0].account.name).toBe('Acme'); + expect(rows[0].account.industry).toBe('Manufacturing'); + }); +});