fix(objectql): force the join key into the expand sub-read, strip it when unrequested (#7537)

.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.

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).
+
 <Callout type="warn">
 **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

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 {

packages/objectql/src/engine.ts

@@ -6282,14 +6282,49 @@ 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,
           {
             where,
             // [#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];