Skip to content

Commit 4384921

Browse files
os-zhuangclaude
andauthored
fix(spec,drivers): the three drivers type-check — and most of their 292 errors were the types, not the tests (#4311) (#4407)
* fix(spec,driver-sql,driver-memory): the three drivers type-check — and 165 of their 292 errors were the types, not the tests (#4311) `driver-sql` (241), `driver-sqlite-wasm` (27) and `driver-memory` (23) were filed as one playbook: author-tier literals handed to a parsed-tier parameter. That is what 127 of them were. The other 165 were the types being wrong, and only a tsc that had never run could have kept them invisible: - **`bypassTenantAudit` was never on `DriverOptionsSchema`** (118 errors). `SqlDriver.auditMissingTenant` reads it, its own warning text tells callers to pass it, `ObjectQLEngine` sets it for system-context calls, and both `service-settings` and `service-datasource` send it on global-scope writes. Declared now, with the limit stated: it silences a diagnostic, it never changes which rows a write touches. The driver read it — and `timezone`, `tenantId`, `tenantIds`, `preserveAudit`, all four long since declared — through `(options as any)`; those seven casts are gone, so the next undeclared option fails the build instead of hiding behind one. - **`findOne(object, id)` was on no contract** (41). An undeclared `typeof query === 'string'` branch in `SqlDriver`, used by nothing outside this package's own tests, and answered differently by the other two drivers: `MemoryDriver` spreads the string (`{0:'t',1:'1'}`), `MongoDriver` reads `query.where` (undefined → an arbitrary row). It also bypassed `find()`, so it skipped field selection, temporal coercion and the deterministic-order tie-breaker `IDataDriver.find` promises. Removed; the call sites spell the lookup as `{ object, where: { id } }`. - **`initObjects` did not declare the `tenancy` it consumes** (4) — each object flows into `computeAndRecordTenantField`, which reads `obj.tenancy` to pick the tenant column and set the sticky opt-out. `registerExternalObject` had it right all along. - **`AnalyticsQuery` has no author tier** (19+1). `timezone` is `.default('UTC')` and `public` is `.default(false)`, so the parsed types require both; the tests wrote author-tier literals. Added `AnalyticsQueryInput` beside `AnalyticsQuery` (the `QueryInput`/`QueryAST` pattern) and routed the literals through the schema, so the parse itself is the proof the default lands — `defineCube` does the same job for the cube. - **`InMemoryDriver.create` declared no return type** (3), so TS inferred the literal it builds and every other column of the created row vanished from the caller's view. The remaining 127 are the filed playbook: `object` added to the query literal (the driver reads the name from parameter 1 — the AST field is required and redundant there, filed separately), and `count`/`find` calls likewise. Six call sites the type checker could not see at all wrote `findOne('users', alice.id as any)`; the cast is what let an off-contract call survive a narrowing. They surfaced as runtime failures the moment the branch was removed, which is the point. All three packages now declare `typecheck` and leave the DEBT ledger: 60/77 packages covered, 17 ledgered. 1069 tests green across the three. Claude-Session: https://claude.ai/code/session_011m13wbaZziPveBdtQthrXd Co-authored-by: Claude <noreply@anthropic.com> * fix(driver-sql,driver-sqlite-wasm,driver-memory): 111 `as any` casts were opting the driver call sites out of the check they had just been given (#4311) Onboarding a package is supposed to make its `typecheck` mean something. These three declared it while 111 driver-call arguments were cast to `any` — the gate went green over call sites the type checker was not allowed to read. Stripping the casts produced 66 fresh errors, every one real: - 65 were a missing `object` on the query literal, the same defect the first commit fixed 127 times in the sites that were visible. - 1 was a silent runtime bug. `sql-driver-aggregate-temporal-output.test.ts` asked for `orderBy: [['id', 'asc']]`; the driver reads `item.field`, a tuple has none, so `if (item.field)` was false and the sort never reached SQL. The helper is named `viaFind` and exists to read a column in order — it had been reading whatever order the rows came back in, in the one file whose subject is output ordering. The tuple form appears nowhere else in the repo. 43 of the casts were needed by nothing at all. Exactly 2 are load-bearing and stay: `sql-driver-filter-no-silent-drop` and `memory-filter-ast-vocabulary` both feed `where` shapes `isFilterAST()` refuses, deliberately, to prove the driver throws rather than dropping the condition — those now say `where as FilterCondition` next to a comment, so the one honest cast in the package is not camouflaged by a hundred idle ones. Verified: `turbo run typecheck` 122/122 (three new), `pnpm test` 132/132, eslint, the 12 lint.yml check gates, coverage ratchet self-test 18/18. Claude-Session: https://claude.ai/code/session_011m13wbaZziPveBdtQthrXd Co-authored-by: Claude <noreply@anthropic.com> * docs(spec): regenerate the DriverOptions reference for `bypassTenantAudit` (#4311) `content/docs/references/data/driver.mdx` is generated from the Zod schema, so declaring a new `DriverOptions` key leaves it stale until `gen:docs` runs. `@objectstack/spec check:docs` caught it — the generated table is what a driver author reads to learn which options exist, and it had been complete for every key except the one this branch adds. `pnpm --filter @objectstack/spec gen:schema && gen:docs`; the diff is the one row. 252 generated files back in sync. Claude-Session: https://claude.ai/code/session_011m13wbaZziPveBdtQthrXd Co-authored-by: Claude <noreply@anthropic.com> * docs(spec): record `AnalyticsQueryInput` in the api-surface snapshot (#4311) `check:api-surface` reports every public export against a committed snapshot, so a new type — additive, 0 breaking — still has to be acknowledged rather than appearing unannounced in a consumer's build. One line. Also ran the other fifteen `@objectstack/spec` gates against this branch rather than waiting to meet them one CI round at a time: docs, generated, skill-refs, skill-docs, exported-any, authorable-surface, spec-changes, upgrade-guide, liveness, empty-state, variant-docs, strictness-ledger, react-blocks, react-conformance, skill-examples — plus `check:i18n` / `check:i18n-coverage` and `check:generated --reconcile-only`, which are the remaining steps of the CI job that failed. All green; only this snapshot needed regenerating. The pattern behind both this and the previous commit: `pnpm build` does not run the generators, so a schema edit type-checks and tests clean locally while two committed artifacts derived from it sit stale. Worth knowing before the next spec change — the generators are `gen:*` in `packages/spec`, and their gates are the `check:*` beside them. Claude-Session: https://claude.ai/code/session_011m13wbaZziPveBdtQthrXd Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9881074 commit 4384921

60 files changed

Lines changed: 539 additions & 338 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/driver-sql": patch
4+
"@objectstack/driver-sqlite-wasm": patch
5+
"@objectstack/driver-memory": patch
6+
---
7+
8+
fix(spec,drivers): `bypassTenantAudit` becomes a declared driver option, and `findOne` stops accepting a bare id (#4311)
9+
10+
Three drivers built with `tsup` and tested with `vitest`, so no `tsc` had ever
11+
read them. Onboarding them to the #4311 type-check ratchet surfaced 292 errors,
12+
and most of what looked like sloppy test fixtures was the types being wrong.
13+
14+
**`DriverOptions.bypassTenantAudit` is now declared.** It has been live for a
15+
long time without being on the schema: `SqlDriver.auditMissingTenant` reads it
16+
to suppress the "tenant-scoped write without `tenantId`" warning, the driver's
17+
own warning text tells callers to set it, `ObjectQLEngine` sets it for
18+
system-context calls, and `service-settings` / `service-datasource` pass it on
19+
every global-scope write. Because the schema never had it, the driver read it
20+
through `(options as any)` and no caller was type-checked. The declaration
21+
states the limit as well: it silences a diagnostic and MUST NOT change which
22+
rows a write touches — suppressing an audit warning is not a permission.
23+
24+
The same cast covered `timezone`, `tenantId`, `tenantIds` and `preserveAudit`,
25+
all long since declared. Those reads now go through `DriverOptions`, so the next
26+
undeclared option fails the build instead of hiding behind an existing cast.
27+
28+
**`SqlDriver.findOne(object, id)` is removed.** An undeclared
29+
`typeof query === 'string' | 'number'` branch accepted a bare id. It was on no
30+
contract, nothing outside that package's own tests used it, and the other two
31+
drivers answered the identical call differently — `MemoryDriver` spreads the
32+
string into `{0:'t',1:'1'}`, `MongoDBDriver` reads `query.where` as `undefined`
33+
and returns an arbitrary row. It also bypassed the shared `findRows()` path, so
34+
it skipped field selection, temporal coercion, unknown-column recovery and the
35+
`singleRowLookup` ORDER BY decision. Spell an id lookup as the query it is:
36+
37+
```ts
38+
- await driver.findOne('task', 't1');
39+
+ await driver.findOne('task', { object: 'task', where: { id: 't1' } });
40+
```
41+
42+
**`SqlDriver.initObjects` declares the `tenancy` it consumes.** Each object is
43+
fed to `computeAndRecordTenantField`, which reads `obj.tenancy` to pick the
44+
tenant column and to set or clear the sticky explicit-opt-out — but the
45+
parameter type listed only `{ name, fields }`, so a caller that spelled the key
46+
correctly was rejected while the driver read it anyway.
47+
`registerExternalObject` already had it.
48+
49+
**`AnalyticsQueryInput` joins `AnalyticsQuery`.** `timezone` is
50+
`.default('UTC')`, so the parsed type requires it and an authored literal does
51+
not have it — the same two-tier split `QueryInput`/`QueryAST` already names on
52+
the query side. `InMemoryDriver.create`/`bulkCreate` also declare their
53+
`IDataDriver` return types; without them TS inferred the literal the method
54+
builds and every other column of the created row disappeared from the caller's
55+
view.
56+
57+
One silent runtime bug fell out of the same pass: a driver test asked for
58+
`orderBy: [['id', 'asc']]`, the driver reads `item.field`, a tuple has none, and
59+
the sort never reached SQL. The tuple spelling appears nowhere else.

content/docs/references/data/driver.mdx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ const result = DriverCapabilities.parse(data);
9898
| **tenantIds** | `string[]` | optional | Union tenant access set (group posture): native read scoping widens to organization_id IN (...); inserts still stamp from tenantId |
9999
| **timezone** | `string` | optional | Business reference timezone (IANA) for date-dependent generation, e.g. autonumber date tokens |
100100
| **preserveAudit** | `boolean` | optional | Historical import: keep a supplied updated_at instead of force-stamping now (from ExecutionContext.preserveAudit) |
101+
| **bypassTenantAudit** | `boolean` | optional | Suppress the driver tenant-audit warning for a deliberately global write on a tenant-scoped object (diagnostics only — never changes what the write touches) |
101102

102103

103104
---

packages/plugins/driver-memory/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@
1515
"scripts": {
1616
"build": "tsup --config ../../../tsup.config.ts",
1717
"dev": "tsc -w",
18-
"test": "vitest run"
18+
"test": "vitest run",
19+
"typecheck": "tsc --noEmit"
1920
},
2021
"dependencies": {
2122
"@objectstack/core": "workspace:*",

packages/plugins/driver-memory/src/memory-analytics.test.ts

Lines changed: 58 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,21 @@
33
import { describe, it, expect, beforeEach } from 'vitest';
44
import { InMemoryDriver } from './memory-driver.js';
55
import { MemoryAnalyticsService } from './memory-analytics.js';
6-
import type { Cube } from '@objectstack/spec/data';
6+
import { AnalyticsQuerySchema, defineCube } from '@objectstack/spec/data';
7+
import type { AnalyticsQuery, AnalyticsQueryInput, Cube } from '@objectstack/spec/data';
8+
9+
/**
10+
* Author-tier literal → the parsed `AnalyticsQuery` the service contract takes.
11+
*
12+
* `timezone` is `.default('UTC')` on the schema, so it is optional to write and
13+
* required on the parsed type — the two tiers are genuinely different types. A
14+
* real query reaches `query()` through the schema (the REST layer parses the
15+
* request body), so these tests take the same route rather than hand-writing
16+
* the filled-in default: the parse IS the proof that the default lands. Until
17+
* #4311 no tsc read this file, so 19 author-tier literals sat unnoticed in a
18+
* parameter that had required `timezone` all along.
19+
*/
20+
const asQuery = (input: AnalyticsQueryInput): AnalyticsQuery => AnalyticsQuerySchema.parse(input);
721

822
describe('MemoryAnalyticsService', () => {
923
let driver: InMemoryDriver;
@@ -160,10 +174,10 @@ describe('MemoryAnalyticsService', () => {
160174

161175
describe('query', () => {
162176
it('should execute a simple count query', async () => {
163-
const result = await service.query({
177+
const result = await service.query(asQuery({
164178
cube: 'orders',
165179
measures: ['orders.count']
166-
});
180+
}));
167181

168182
expect(result.rows).toHaveLength(1);
169183
expect(result.rows[0]['orders.count']).toBe(5);
@@ -173,11 +187,11 @@ describe('MemoryAnalyticsService', () => {
173187
});
174188

175189
it('should group by a dimension', async () => {
176-
const result = await service.query({
190+
const result = await service.query(asQuery({
177191
cube: 'orders',
178192
measures: ['orders.count'],
179193
dimensions: ['orders.status']
180-
});
194+
}));
181195

182196
expect(result.rows).toHaveLength(3); // completed, pending, cancelled
183197

@@ -187,34 +201,34 @@ describe('MemoryAnalyticsService', () => {
187201
});
188202

189203
it('should calculate sum aggregation', async () => {
190-
const result = await service.query({
204+
const result = await service.query(asQuery({
191205
cube: 'orders',
192206
measures: ['orders.totalAmount'],
193207
dimensions: ['orders.customer']
194-
});
208+
}));
195209

196210
const aliceRow = result.rows.find(r => r['orders.customer'] === 'Alice');
197211
expect(aliceRow).toBeDefined();
198212
expect(aliceRow!['orders.totalAmount']).toBe(250); // 100 + 150
199213
});
200214

201215
it('should calculate average aggregation', async () => {
202-
const result = await service.query({
216+
const result = await service.query(asQuery({
203217
cube: 'products',
204218
measures: ['products.avgPrice'],
205219
dimensions: ['products.category']
206-
});
220+
}));
207221

208222
const electronicsRow = result.rows.find(r => r['products.category'] === 'electronics');
209223
expect(electronicsRow).toBeDefined();
210224
expect(electronicsRow!['products.avgPrice']).toBe(512); // (999 + 25) / 2
211225
});
212226

213227
it('should support multiple measures', async () => {
214-
const result = await service.query({
228+
const result = await service.query(asQuery({
215229
cube: 'orders',
216230
measures: ['orders.count', 'orders.totalAmount', 'orders.avgAmount']
217-
});
231+
}));
218232

219233
expect(result.rows).toHaveLength(1);
220234
expect(result.rows[0]['orders.count']).toBe(5);
@@ -223,71 +237,71 @@ describe('MemoryAnalyticsService', () => {
223237
});
224238

225239
it('should apply filters via canonical `where`', async () => {
226-
const result = await service.query({
240+
const result = await service.query(asQuery({
227241
cube: 'orders',
228242
measures: ['orders.count', 'orders.totalAmount'],
229243
where: { 'orders.status': 'completed' },
230-
});
244+
}));
231245

232246
expect(result.rows).toHaveLength(1);
233247
expect(result.rows[0]['orders.count']).toBe(3);
234248
expect(result.rows[0]['orders.totalAmount']).toBe(600); // 100+200+300
235249
});
236250

237251
it('should accept FilterCondition (short-form equality)', async () => {
238-
const result = await service.query({
252+
const result = await service.query(asQuery({
239253
cube: 'orders',
240254
measures: ['orders.count', 'orders.totalAmount'],
241255
where: { status: 'completed' },
242-
});
256+
}));
243257

244258
expect(result.rows).toHaveLength(1);
245259
expect(result.rows[0]['orders.count']).toBe(3);
246260
expect(result.rows[0]['orders.totalAmount']).toBe(600);
247261
});
248262

249263
it('should accept FilterCondition with $in operator', async () => {
250-
const result = await service.query({
264+
const result = await service.query(asQuery({
251265
cube: 'orders',
252266
measures: ['orders.count'],
253267
where: { status: { $in: ['completed', 'pending'] } },
254-
});
268+
}));
255269

256270
expect(result.rows[0]['orders.count']).toBe(4); // 3 completed + 1 pending
257271
});
258272

259273
it('should accept FilterCondition with $gte operator', async () => {
260-
const result = await service.query({
274+
const result = await service.query(asQuery({
261275
cube: 'orders',
262276
measures: ['orders.count'],
263277
where: { amount: { $gte: 200 } },
264-
});
278+
}));
265279

266280
expect(result.rows[0]['orders.count']).toBe(2); // 200, 300
267281
});
268282

269283
it('should support sorting', async () => {
270-
const result = await service.query({
284+
const result = await service.query(asQuery({
271285
cube: 'orders',
272286
measures: ['orders.totalAmount'],
273287
dimensions: ['orders.customer'],
274288
order: { 'orders.totalAmount': 'desc' }
275-
});
289+
}));
276290

277291
expect(result.rows[0]['orders.customer']).toBe('Charlie'); // 300
278292
expect(result.rows[1]['orders.customer']).toBe('Alice'); // 250
279293
expect(result.rows[2]['orders.customer']).toBe('Bob'); // 250
280294
});
281295

282296
it('should support limit and offset', async () => {
283-
const result = await service.query({
297+
const result = await service.query(asQuery({
284298
cube: 'orders',
285299
measures: ['orders.count'],
286300
dimensions: ['orders.customer'],
287301
order: { 'orders.customer': 'asc' },
288302
limit: 2,
289303
offset: 1
290-
});
304+
}));
291305

292306
expect(result.rows).toHaveLength(2);
293307
expect(result.rows[0]['orders.customer']).toBe('Bob');
@@ -299,7 +313,10 @@ describe('MemoryAnalyticsService', () => {
299313
// 'amount' of type 'sum'). Clients that build measure names from
300314
// (field, function) pairs send 'amount_sum' — the resolver should
301315
// accept that alias and produce the same aggregate value.
302-
const aliasCube: Cube = {
316+
// `defineCube` rather than a bare `: Cube` literal: `public` is
317+
// `.default(false)`, so it is required on the parsed `Cube` type and
318+
// absent here — the factory is the spec's own answer to that split.
319+
const aliasCube = defineCube({
303320
name: 'opps',
304321
title: 'Opps',
305322
sql: 'orders',
@@ -309,14 +326,14 @@ describe('MemoryAnalyticsService', () => {
309326
dimensions: {
310327
status: { name: 'status', label: 'Status', type: 'string', sql: 'status' },
311328
},
312-
};
329+
});
313330
const aliasService = new MemoryAnalyticsService({ driver, cubes: [aliasCube] });
314331

315-
const aliased = await aliasService.query({
332+
const aliased = await aliasService.query(asQuery({
316333
cube: 'opps',
317334
measures: ['amount_sum'],
318335
dimensions: ['status'],
319-
});
336+
}));
320337

321338
const completed = aliased.rows.find(r => r.status === 'completed');
322339
expect(completed).toBeDefined();
@@ -325,18 +342,18 @@ describe('MemoryAnalyticsService', () => {
325342

326343
it('should throw error for unknown cube', async () => {
327344
await expect(async () => {
328-
await service.query({
345+
await service.query(asQuery({
329346
cube: 'unknown',
330347
measures: ['unknown.count']
331-
});
348+
}));
332349
}).rejects.toThrow('Cube not found: unknown');
333350
});
334351

335352
it('should include SQL in result for debugging', async () => {
336-
const result = await service.query({
353+
const result = await service.query(asQuery({
337354
cube: 'orders',
338355
measures: ['orders.count']
339-
});
356+
}));
340357

341358
expect(result.sql).toBeDefined();
342359
expect(result.sql).toContain('orders');
@@ -345,56 +362,56 @@ describe('MemoryAnalyticsService', () => {
345362

346363
describe('generateSql', () => {
347364
it('should generate SQL for a simple query', async () => {
348-
const result = await service.generateSql({
365+
const result = await service.generateSql(asQuery({
349366
cube: 'orders',
350367
measures: ['orders.count']
351-
});
368+
}));
352369

353370
expect(result.sql).toContain('SELECT');
354371
expect(result.sql).toContain('COUNT(*)');
355372
expect(result.sql).toContain('FROM orders');
356373
});
357374

358375
it('should generate SQL with GROUP BY', async () => {
359-
const result = await service.generateSql({
376+
const result = await service.generateSql(asQuery({
360377
cube: 'orders',
361378
measures: ['orders.count'],
362379
dimensions: ['orders.status']
363-
});
380+
}));
364381

365382
expect(result.sql).toContain('GROUP BY status');
366383
});
367384

368385
it('should generate SQL with WHERE clause', async () => {
369-
const result = await service.generateSql({
386+
const result = await service.generateSql(asQuery({
370387
cube: 'orders',
371388
measures: ['orders.count'],
372389
where: { 'orders.status': 'completed' },
373-
});
390+
}));
374391

375392
expect(result.sql).toContain('WHERE');
376393
expect(result.sql).toContain('status');
377394
});
378395

379396
it('should generate SQL with ORDER BY', async () => {
380-
const result = await service.generateSql({
397+
const result = await service.generateSql(asQuery({
381398
cube: 'orders',
382399
measures: ['orders.count'],
383400
dimensions: ['orders.status'],
384401
order: { 'orders.status': 'asc' }
385-
});
402+
}));
386403

387404
expect(result.sql).toContain('ORDER BY');
388405
expect(result.sql).toContain('ASC');
389406
});
390407

391408
it('should generate SQL with LIMIT and OFFSET', async () => {
392-
const result = await service.generateSql({
409+
const result = await service.generateSql(asQuery({
393410
cube: 'orders',
394411
measures: ['orders.count'],
395412
limit: 10,
396413
offset: 5
397-
});
414+
}));
398415

399416
expect(result.sql).toContain('LIMIT 10');
400417
expect(result.sql).toContain('OFFSET 5');

0 commit comments

Comments
 (0)