refactor(drizzle): projected select in createDrizzleHandler (deletes both hand-written row remaps) - #91
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
fbd8f11 to
282b246
Compare
|
@claude review |
|
Claude finished @mxkaske's task in 3m 15s —— View job Review
Solid refactor — the diff matches the description closely and the regression tests are convincingly designed (the 1. 2. 3. Minor doc typo in interval.ts:4: "Thirteen rungs, from one second to twelve-and-a-half minutes" — 4. Handler construction is duplicated verbatim between route.ts and mcp/route.ts — same Everything else checks out:
Not verified: I could not run |
282b246 to
0b1597e
Compare
…n dropped
`handler.test.ts` read `row.uuid`, but `uuid` is not in `columnMapping` — it is
never filtered or sorted, so under this branch's "the projection IS the
mapping" rule it stops coming back unless a caller names it in `select`, which
is exactly what both production routes do. Every row therefore had
`uuid: undefined`, and `pages forward without overlap` compared `undefined` to
`undefined` and failed. Red in CI since the projection landed; only reproducible
there, since the suite is `skipIf(!hasDatabase)`.
The test helper now passes `select: { uuid }` like the routes do, and a new
case pins the contract that caused this — rows carry the mapped keys and
nothing else.
Also, both doc-only:
- `cursorColumn` now says that `scope.range`/`bucketMs` are read off it, so a
non-time cursor paginates correctly but hands aggregate callers a range built
from `new Date(id)`.
- The interval ladder's docstring said its thirteen rungs top out at
"twelve-and-a-half minutes". They top out at 384 minutes; 768 is the
fall-through above them. Ships into consumer trees verbatim.
Refs #91
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0b1597e to
6dd0263
Compare
…n dropped
`handler.test.ts` read `row.uuid`, but `uuid` is not in `columnMapping` — it is
never filtered or sorted, so under this branch's "the projection IS the
mapping" rule it stops coming back unless a caller names it in `select`, which
is exactly what both production routes do. Every row therefore had
`uuid: undefined`, and `pages forward without overlap` compared `undefined` to
`undefined` and failed. Red in CI since the projection landed; only reproducible
there, since the suite is `skipIf(!hasDatabase)`.
The test helper now passes `select: { uuid }` like the routes do, and a new
case pins the contract that caused this — rows carry the mapped keys and
nothing else.
Also, both doc-only:
- `cursorColumn` now says that `scope.range`/`bucketMs` are read off it, so a
non-time cursor paginates correctly but hands aggregate callers a range built
from `new Date(id)`.
- The interval ladder's docstring said its thirteen rungs top out at
"twelve-and-a-half minutes". They top out at 384 minutes; 768 is the
fall-through above them. Ships into consumer trees verbatim.
Refs #91
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6dd0263 to
8c28095
Compare
…llers a typed scope `createDrizzleHandler` fetched rows with a star select, so they came back keyed by Drizzle property names (`timingDns`) while the wire contract and the entire schema layer are keyed by schema keys (`"timing.dns"`). Every caller translated by hand on the way out. That remap was written twice, verbatim, and the two copies had already drifted: the MCP route emitted `date.toISOString()` where REST emitted a `Date`, and omitted `headers` and `percentile`. Its TODO already named the fix — the camelCase↔dot-notation bijection was right there in `columnMapping`. The handler now builds its select from that mapping, so rows arrive keyed by schema keys and both remaps are deleted. Columns that belong in the payload but are never filtered or sorted go in a new `select` option. `allConditions: SQL[]` is replaced by `scope: DrizzleQueryScope`. That leak was what made 110 lines of untested `db: any` SQL necessary: the three things such a caller needs — the resolved range, the bucket interval, and the composed WHERE — were all computed inside the handler and thrown away. `getChartData` keeps its aggregate SQL and stops re-deriving anything. Fixes a latent pagination bug. `getCursorValue` read `row[cursorCol.name]` — the SQL column name, not the JS key. They coincide for `date`. For any cursor column whose DB name differs (`timing_dns`), the lookup was `undefined`, so `getCursorValue` returned null, `boundaryValue` was null, the whole tie-snapping block was skipped, and `nextCursor` came back null — silently ending pagination after one page. A projected select makes this reachable to fix, and the regression test uses a table whose cursor column is deliberately renamed. Unmapped keys stop vanishing. `buildWhereConditions` and `buildOrderBy` both skipped a key missing from `columnMapping`, and `ColumnMapping` is `Record<string, Column>` so a typo was not a type error either — the filter just stopped filtering, with no error and no failing test. The handler now throws at construction, listing the missing keys with the snippet to paste. `evaluateIntervalMs` moves out of the demo route into the drizzle block, where its 13 rungs can be tested directly instead of through a chart query. Also fixed, found while testing: - `interval.ts` shipped in the block's code but was missing from the manifest, so `shadcn add data-table-drizzle` installed files importing a module it never wrote. `registry.test.ts` only checked alias imports; it now also resolves relative ones, which is the check that would have caught it. - The deleted remap normalized `message` with `?? undefined`. Projecting the column directly returns SQL NULL, and the sheet condition tested `!== undefined` — so rows with no message would have started rendering an empty field. The condition and the Zod schema now both accept null. Tests: the projection identity test is what makes deleting the remaps safe. `column-mapping.ts` is finally imported by a test — the suite had been substituting its own divergent copy that added a `message` entry production did not have. REST and MCP payloads are asserted to agree modulo the two deliberate differences. All of it runs ungated on PGLite. Refs #88 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n dropped
`handler.test.ts` read `row.uuid`, but `uuid` is not in `columnMapping` — it is
never filtered or sorted, so under this branch's "the projection IS the
mapping" rule it stops coming back unless a caller names it in `select`, which
is exactly what both production routes do. Every row therefore had
`uuid: undefined`, and `pages forward without overlap` compared `undefined` to
`undefined` and failed. Red in CI since the projection landed; only reproducible
there, since the suite is `skipIf(!hasDatabase)`.
The test helper now passes `select: { uuid }` like the routes do, and a new
case pins the contract that caused this — rows carry the mapped keys and
nothing else.
Also, both doc-only:
- `cursorColumn` now says that `scope.range`/`bucketMs` are read off it, so a
non-time cursor paginates correctly but hands aggregate callers a range built
from `new Date(id)`.
- The interval ladder's docstring said its thirteen rungs top out at
"twelve-and-a-half minutes". They top out at 384 minutes; 768 is the
fall-through above them. Ships into consumer trees verbatim.
Refs #91
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8c28095 to
a05c9bc
Compare
1 participant
Implements RFC #88. Stacked on #90 (RFC #87), which is stacked on #89 (RFC #86). This PR targets
feat/filter-semantics.Problem
createDrizzleHandlerfetched rows withdb.select(). That returns rows keyed by Drizzle property names (timingDns), but the wire contract and the whole schema layer are keyed by schema keys ("timing.dns"). So every caller translated, by hand, on the way out.The remap was written twice, verbatim — and the two copies had already drifted: the MCP route emitted
date.toISOString()where REST emitted aDate, and omittedheadersandpercentile. Its ownTODOalready named the fix:Fix
The handler builds its select from the mapping it already has, so rows come back keyed by schema keys and both remaps are deleted. Columns that belong in the payload but are never filtered or sorted go in a new
selectoption.allConditions: SQL[]→scope: DrizzleQueryScope:That leak is what made ~110 lines of untested
db: anySQL necessary in the first place — the three things such a caller needs (resolved range, bucket interval, composed WHERE) were all computed inside the handler and thrown away.getChartDatakeeps its aggregate SQL and stops re-deriving anything.Net −78 lines across the two routes.
Latent bug fixed
getCursorValuereadrow[cursorCol.name]— the SQL column name, not the JS key. They coincide fordate. For any cursor column whose DB name differs (timing_dns), the lookup wasundefined→getCursorValuereturned null →boundaryValuewas null → the entire tie-snapping block was skipped →nextCursorcame back null, silently ending pagination after one page.No test used a cursor column whose SQL name differed from its JS key. The regression test now builds one deliberately, and was mutation-verified: reverting that single line fails 7 tests.
Unmapped keys stop vanishing
buildWhereConditionsandbuildOrderByboth silently skipped a key missing fromcolumnMapping, andColumnMappingisRecord<string, Column>so a typo wasn't a type error either — the filter just stopped filtering, with no error, no warning, and no failing test. The handler now throws at construction, naming the missing keys with the snippet to paste.Also fixed, found while testing
interval.tsshipped in the block's code but was missing fromregistry.json.npx shadcn add data-table-drizzlewould have installed files importing a module it never wrote.registry.test.tsonly resolved alias imports; it now resolves relative ones too — the check that would have caught it. Verified to fail when the entry is removed.messagewith?? undefined; projecting the column directly returns SQL NULL, and the sheet condition tested!== undefined— so rows with no message would have started rendering an empty field. Condition and Zod schema now both accept null.Tests
All ungated on PGLite (available since #90).
columnMappingis present on a fetched row with the right value. This is the test that makes deleting the remaps safe.column-mapping.tsis finally imported by a test. The suite had been substituting its own divergent copy that added amessageentry production doesn't have — a mapping no test exercises is exactly where an unmapped key hides.Datevs ISO string;percentileis REST-only). Both real route modules are invoked end-to-end. This pins drift that had already happened, and was mutation-verified by making MCP dropheadersagain.2245 tests pass (1825 registry, 420 web). Typecheck, lint, prettier,
registry:buildclean.Note
The RFC suggested adding the replacement as a new entry point and deprecating the old one. I changed
createDrizzleHandlerin place:result.data's key shape is breaking, but registry blocks are copy-in so no installed project breaks silently, and the in-repo routes and docs land in the same change. Two parallel entry points would have meant maintaining the star-select path indefinitely.🤖 Generated with Claude Code