From 84b3fe427e11a89981e474e86da2b1fa0ecb8e49 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 01:54:53 +0000 Subject: [PATCH 1/2] fix(metadata,objectql): drop the redundant `object` key from driver queries and the casts carrying it (#6231) `DriverQuery` (`Omit`) exists since #6076 and five drivers followed in #6075, but five call sites stayed as they were because they sat behind a cast the compiler could not see through. The key itself is inert -- `git grep 'query\.object' -- 'packages/drivers/*/src'` is zero, so no driver reads it. The cast was the cost: `as any` on a query argument switches off checking for `where` / `orderBy` / `fields` too, the account #5181's changeset opened (cloud#1030's `$like` reached runtime through exactly this hole). - metadata `DatabaseLoader._find/._findOne/._count`: declare `query: DriverQuery` and pass it to the driver unchanged and uncast (9 call sites re-checked). The ENGINE branch keeps its `as any` and says why in a comment: it is blocked by a spec-level divergence, not vestigial. - objectql `resolveSecret`: the `sys_secret` read loses both the key and the `as QueryAST` that existed only to satisfy it. - objectql `LifecycleService` governance counter: passes argument one only, and its hand-written driver shape becomes the named `CountCapableDriver` typed with `DriverQuery` instead of `Record`. Pins added at all three sites assert the shape the driver is actually handed; re-adding the key without a cast is now `TS2353`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw --- .../driver-query-redundant-object-callers.md | 44 +++++++++++++++++++ .../src/loaders/database-loader.test.ts | 43 ++++++++++++++++++ .../metadata/src/loaders/database-loader.ts | 25 ++++++++--- packages/objectql/src/engine.ts | 2 +- .../src/lifecycle/lifecycle-service.test.ts | 21 +++++++++ .../src/lifecycle/lifecycle-service.ts | 21 +++++++-- packages/objectql/src/secret-fields.test.ts | 26 ++++++++++- scripts/query-options-erasure-baseline.json | 2 +- 8 files changed, 170 insertions(+), 14 deletions(-) create mode 100644 .changeset/driver-query-redundant-object-callers.md diff --git a/.changeset/driver-query-redundant-object-callers.md b/.changeset/driver-query-redundant-object-callers.md new file mode 100644 index 0000000000..9aa2faf04f --- /dev/null +++ b/.changeset/driver-query-redundant-object-callers.md @@ -0,0 +1,44 @@ +--- +"@objectstack/metadata": patch +"@objectstack/objectql": patch +--- + +fix(metadata,objectql): stop restating the object name inside driver queries — and stop casting away the query's type to do it (#6231) + +`DriverQuery` (`Omit`) landed in #6076 and five drivers +followed in #6075, but five **call sites** stayed as they were, because they +were hidden behind a cast where the compiler could not see them. This removes +the redundant key at all five and, with it, the casts that existed only to +carry it. + +The redundant key was never the expensive half. `git grep 'query\.object' -- +'packages/drivers/*/src'` is zero: no driver reads it, so the key itself was +inert. **The cast was the cost.** `as any` on a query argument does not +suppress one key — it switches off checking for `where`, `orderBy` and +`fields` as well, which is precisely the account #5181's changeset opened +(cloud#1053 measured 20 such sites; cloud#1030's `$like` — an operator the +filter dialect does not have — survived compilation and reached the runtime +through exactly this hole). `packages/metadata`'s `DatabaseLoader` is the +main metadata read path, so it was the worst place to be running unchecked. + +The five sites: + +- `metadata` `DatabaseLoader._find` / `._findOne` / `._count` — each was + `driver.find(table, { object: table, ...query } as any)`. The helpers now + declare `query: DriverQuery` and hand it to the driver unchanged and uncast, + so all nine of their call sites' `where` / `orderBy` / `fields` are checked + again. +- `objectql` `ObjectQL.resolveSecret` — the `sys_secret` read was + `{ object: 'sys_secret', where: { id } } as QueryAST`, where the cast existed + only to satisfy the AST's then-required `object`. Both are gone. +- `objectql` `LifecycleService` governance counter — `count(obj.name, + { object: obj.name })` carried no cast; it was admitted by a hand-written + driver shape whose `query` was `Record`, which would equally + have admitted a `where` the dialect does not have. That shape is now the named + `CountCapableDriver` typed with `DriverQuery`, and the call passes argument + one only. + +No behaviour changes: the key was inert on every path, and the object name has +always travelled as the driver methods' first argument. What changes is that +these call sites are type-checked again, and that re-adding the key is now a +compile error (`TS2353`) rather than something a cast quietly absorbs. diff --git a/packages/metadata/src/loaders/database-loader.test.ts b/packages/metadata/src/loaders/database-loader.test.ts index 7cde975b23..e5f6315dab 100644 --- a/packages/metadata/src/loaders/database-loader.test.ts +++ b/packages/metadata/src/loaders/database-loader.test.ts @@ -179,6 +179,49 @@ describe('DatabaseLoader', () => { }); }); + // [#6231] The driver takes the object name as argument ONE, and `DriverQuery` + // is `Omit` — so the AST must never restate it. These + // three read helpers used to spell `{ object: table, ...query } as any`, and + // that cast did more than tolerate the redundant key: it switched off + // checking for `where` / `orderBy` / `fields` as well, which is the account + // #5181's changeset opened (cloud#1030's `$like` reached runtime through + // exactly this hole). The redundant key is inert — no driver reads it — so + // the pin is on the SHAPE the driver is handed, which is what a future + // re-add would change. + describe('driver query shape (#6231)', () => { + it('never restates the object name inside the query AST', async () => { + const seen: Array<{ method: string; table: unknown; query: unknown }> = []; + for (const method of ['find', 'findOne', 'count'] as const) { + const real = (mockDriver[method] as (...a: unknown[]) => unknown).bind(mockDriver); + (mockDriver as unknown as Record)[method] = ( + table: unknown, + query: unknown, + ...rest: unknown[] + ) => { + seen.push({ method, table, query }); + return real(table, query, ...rest); + }; + } + + // Every read path the loader owns: findOne (load/stat), find (loadMany/ + // list) and count (exists). + await loader.save('object', 'account', { name: 'account' }); + await loader.load('object', 'account'); + await loader.loadMany('object'); + await loader.exists('object', 'account'); + await loader.list('object'); + await loader.stat('object', 'account'); + + expect(seen.length).toBeGreaterThan(0); + for (const call of seen) { + // The object name travels as argument one… + expect(typeof call.table).toBe('string'); + // …and only there. + expect(call.query ?? {}).not.toHaveProperty('object'); + } + }); + }); + describe('schema bootstrapping', () => { it('should call syncSchema with SysMetadataObject on first operation', async () => { await loader.list('object'); diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index bb382d0f55..2165a73a31 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -22,7 +22,7 @@ import type { import { SysMetadataObject, SysMetadataHistoryObject } from '@objectstack/metadata-core'; import { applyConversionsToStoredItem } from '@objectstack/spec'; import { PLURAL_TO_SINGULAR } from '@objectstack/spec/shared'; -import type { IDataDriver, IDataEngine } from '@objectstack/spec/contracts'; +import type { IDataDriver, IDataEngine, DriverQuery } from '@objectstack/spec/contracts'; import type { MetadataLoader } from './loader-interface.js'; import { calculateChecksum } from '../utils/metadata-history-utils.js'; import { LRUCache } from '../utils/lru-cache.js'; @@ -225,25 +225,36 @@ export class DatabaseLoader implements MetadataLoader { // Internal CRUD helpers (driver vs engine) // ========================================== - private async _find(table: string, query: Record): Promise[]> { + // NOTE (#6231): the DRIVER branch below takes `query` unchanged and uncast — + // `DriverQuery` is `Omit`, so the object name travels as + // argument one only. The ENGINE branch still carries `as any`, and that cast + // is NOT vestigial: `EngineQueryOptionsSchema.search` admits only the + // structured `FullTextSearchSchema`, while `QueryAST.search` (hence + // `DriverQuery`) also admits the bare query string that ADR-0061 D1 calls the + // canonical Tier-1 spelling and that the engine actually serves. Until those + // two schemas agree, `DriverQuery` is not assignable to + // `EngineQueryOptionsParsed`. Filed as the follow-up named in the PR body; do + // not "fix" it by narrowing the cast. + + private async _find(table: string, query: DriverQuery): Promise[]> { if (this.engine) { return this.engine.find(table, query as any); } - return this.driver!.find(table, { object: table, ...query } as any); + return this.driver!.find(table, query); } - private async _findOne(table: string, query: Record): Promise | null> { + private async _findOne(table: string, query: DriverQuery): Promise | null> { if (this.engine) { return this.engine.findOne(table, query as any); } - return this.driver!.findOne(table, { object: table, ...query } as any); + return this.driver!.findOne(table, query); } - private async _count(table: string, query: Record): Promise { + private async _count(table: string, query: DriverQuery): Promise { if (this.engine) { return this.engine.count(table, query as any); } - return this.driver!.count(table, { object: table, ...query } as any); + return this.driver!.count(table, query); } private async _create(table: string, data: Record): Promise> { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 246bd10c13..344132188f 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -4132,7 +4132,7 @@ export class ObjectQL implements IObjectQLEngine { throw new Error('Cannot resolve secret: no CryptoProvider is registered (fail-closed).'); } const secretDriver = this.getDriver('sys_secret'); - const found = await secretDriver.find('sys_secret', { object: 'sys_secret', where: { id } } as QueryAST); + const found = await secretDriver.find('sys_secret', { where: { id } }); const secret: any = Array.isArray(found) ? found[0] : found; if (!secret) { throw new Error(`Cannot resolve secret: sys_secret row "${id}" not found (fail-closed).`); diff --git a/packages/objectql/src/lifecycle/lifecycle-service.test.ts b/packages/objectql/src/lifecycle/lifecycle-service.test.ts index 6f305b5061..ff82d79276 100644 --- a/packages/objectql/src/lifecycle/lifecycle-service.test.ts +++ b/packages/objectql/src/lifecycle/lifecycle-service.test.ts @@ -1063,6 +1063,27 @@ describe('LifecycleService.sweep — governance (P4)', () => { expect(deletes).toHaveLength(2); }); + // [#6231] `count()` takes the object name as argument ONE; the query AST is + // `DriverQuery` (`Omit`) and must not restate it. The + // governance counter used to pass `{ object: obj.name }` — carried not by a + // cast but by a hand-written driver shape whose `query` was + // `Record`, which would equally have accepted a `where` the + // filter dialect does not have. + it('counts by argument one only — the query never restates the object name', async () => { + const count = vi.fn(async (_object: string, _query?: unknown) => 5); + const driver = { name: 'default', count }; + const { engine } = captureEngine([TELEMETRY_OBJ], { driver }); + const settings = fakeSettings({ quotas: { sys_job_run: 1 } }); + + await service(engine, { getSettings: () => settings }).sweep(); + + expect(count).toHaveBeenCalled(); + for (const [object, query] of count.mock.calls) { + expect(object).toBe('sys_job_run'); + expect(query ?? {}).not.toHaveProperty('object'); + } + }); + it('quota defaults by class apply when no per-object quota is set', async () => { const driver = { name: 'default', count: async () => 50 }; const { engine } = captureEngine([TELEMETRY_OBJ], { driver }); diff --git a/packages/objectql/src/lifecycle/lifecycle-service.ts b/packages/objectql/src/lifecycle/lifecycle-service.ts index 4fec2a7e1a..b64aa9ead8 100644 --- a/packages/objectql/src/lifecycle/lifecycle-service.ts +++ b/packages/objectql/src/lifecycle/lifecycle-service.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { Lifecycle } from '@objectstack/spec/data'; +import type { DriverQuery } from '@objectstack/spec/contracts'; import { parseLifecycleDuration } from './duration.js'; import type { DanglingReferenceAuditOptions, @@ -298,6 +299,20 @@ interface RotationCapableDriver extends ReclaimCapableDriver { ): Promise<{ object: string; current: string; shards: string[]; dropped: string[] }>; } +/** + * Driver surface the governance counter (P4) uses. + * + * `query` is the driver contract's {@link DriverQuery}: the object name + * travels as argument ONE and is deliberately absent from the AST, so a + * caller cannot state it twice (objectstack#5181, #6231). Typing it as the + * contract rather than as a loose bag is the point — the previous + * `Record` accepted the redundant `object` key, and would + * equally have accepted a `where` the filter dialect does not have. + */ +interface CountCapableDriver { + count?(object: string, query?: DriverQuery, options?: unknown): Promise; +} + /** Driver surface the Archiver uses on both the hot and the cold store. */ interface ArchiveCapableDriver { name?: string; @@ -782,13 +797,11 @@ export class LifecycleService { const gov = this.governance; const nextCounts = new Map(); for (const obj of declared) { - const driver = engine.getDriverForObject(obj.name) as - | { count?(object: string, query?: Record): Promise } - | undefined; + const driver = engine.getDriverForObject(obj.name) as CountCapableDriver | undefined; if (!driver || typeof driver.count !== 'function') continue; let rowCount: number; try { - rowCount = await driver.count(obj.name, { object: obj.name }); + rowCount = await driver.count(obj.name); } catch { continue; } diff --git a/packages/objectql/src/secret-fields.test.ts b/packages/objectql/src/secret-fields.test.ts index 9bffff7b25..3de4916a99 100644 --- a/packages/objectql/src/secret-fields.test.ts +++ b/packages/objectql/src/secret-fields.test.ts @@ -137,7 +137,7 @@ async function buildEngine(withCrypto: boolean) { engine.registry.registerObject(dsObject); const crypto = makeFakeCrypto(); if (withCrypto) engine.setCryptoProvider(crypto.provider); - return { engine, stores, crypto }; + return { engine, stores, crypto, driver }; } describe('objectql secret-field channel', () => { @@ -180,6 +180,30 @@ describe('objectql secret-field channel', () => { expect(ctx.crypto.calls.decrypt).toBe(1); }); + // [#6231] `resolveSecret` reads `sys_secret` straight off the driver. That + // call used to spell `{ object: 'sys_secret', where: { id } } as QueryAST`, + // where the cast existed only to satisfy the AST's then-required `object`. + // With `DriverQuery` (`Omit`) the key is gone and so is + // the cast — so `where` is type-checked at this call site again. + it('resolveSecret reads sys_secret by argument one — the AST never restates the object name', async () => { + const created = await ctx.engine.insert('ext_datasource', { name: 'pg', db_password: 's3cr3t' }); + const stored = ctx.stores.get('ext_datasource')!.get(created.id) as any; + + const seen: Array<{ object: string; ast: any }> = []; + const realFind = ctx.driver.find.bind(ctx.driver); + ctx.driver.find = async (object: string, ast: any) => { + seen.push({ object, ast }); + return realFind(object, ast); + }; + + expect(await ctx.engine.resolveSecret(stored.db_password)).toBe('s3cr3t'); + + const secretReads = seen.filter((c) => c.object === 'sys_secret'); + expect(secretReads).toHaveLength(1); + expect(secretReads[0].ast).not.toHaveProperty('object'); + expect(secretReads[0].ast.where).toEqual({ id: expect.any(String) }); + }); + it('fail-closed: writing a secret field with no CryptoProvider throws', async () => { const bare = await buildEngine(false); await expect( diff --git a/scripts/query-options-erasure-baseline.json b/scripts/query-options-erasure-baseline.json index 02d75bac34..ad475bb005 100644 --- a/scripts/query-options-erasure-baseline.json +++ b/scripts/query-options-erasure-baseline.json @@ -35,7 +35,7 @@ "packages/core/src/security/resolve-authz-context.ts": 1, "packages/metadata-protocol/src/protocol.ts": 6, "packages/metadata-protocol/src/seed-loader.ts": 3, - "packages/metadata/src/loaders/database-loader.ts": 6, + "packages/metadata/src/loaders/database-loader.ts": 3, "packages/objectql/src/engine.ts": 9, "packages/plugins/plugin-approvals/src/approval-service.ts": 10, "packages/plugins/plugin-approvals/src/approver-org-scope.ts": 2, From 16859b84c3e404711a87a2f8991e509cdeac53d7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 01:56:42 +0000 Subject: [PATCH 2/2] docs(metadata): point the surviving engine-branch cast at its tracking issue (#7178) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw --- packages/metadata/src/loaders/database-loader.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index 2165a73a31..488d8a2d9b 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -233,8 +233,8 @@ export class DatabaseLoader implements MetadataLoader { // `DriverQuery`) also admits the bare query string that ADR-0061 D1 calls the // canonical Tier-1 spelling and that the engine actually serves. Until those // two schemas agree, `DriverQuery` is not assignable to - // `EngineQueryOptionsParsed`. Filed as the follow-up named in the PR body; do - // not "fix" it by narrowing the cast. + // `EngineQueryOptionsParsed`. Tracked as #7178; do not "fix" it here by + // narrowing the cast. private async _find(table: string, query: DriverQuery): Promise[]> { if (this.engine) {