Commit 5e0a2ce
authored
refactor(table): move the internal row routes onto the application boundary (#6809)
* refactor(table): move the single-row route onto the application boundary
The hottest table write path authorized in its own handler and queried the
database from the adapter — the two things a surface adapter must never do. It
now declares itself with defineInternalJsonRoute against the readRow, updateRow
and deleteRow use cases: 127 lines instead of 274, with no db import, no
drizzle import and no checkAccess.
Doing that surfaced why the violation existed. Write-provenance resolution
needs the canonical schema to map a caller's column key to the storage column
it certifies, and the adapter could only do that because it was already loading
the table illegally. The envelope is now split along the real seam: the adapter
reads the header and payload field, which is transport, and the use case
resolves the selections against the canonical table, which is domain.
That split has to preserve a distinction the defaulting logic would erase. An
internal caller that sends no envelope stays deliberately untracked; defaulting
it to an exact-empty stamp would certify "this write introduced no secrets" on
a runtime write that may well have introduced some. Only an interactive caller
certifies exact-empty, over the storage columns its write actually persists.
Two further changes fell out of it:
present() now receives the same { principal, input } pair its sibling hooks
responseHeaders and finalizeResponse already got. This route serves a session
and a workflow execution on one path and owes them different column keyings, so
rendering per caller kind is presentation rather than domain. That was a gap in
the builder, not a special case for this route.
tableRowWireSchema describes what the single-row routes actually return. The
contract claimed a full TableRow, carrying the executions sidecar and Date
objects — true of the list and query routes, and never true here. The hand-
rolled handler was never checked against its own contract, so the drift was
invisible until the builder started validating it.
Wire changes, both deliberate and both narrower than before: a cross-tenant
table now conceals as 404 where the old blanket handler answered 403, while an
in-workspace role denial still answers 403. Nothing in hooks/queries/tables.ts
branches on either.
Verified to fail: forcing one keying, dropping the actor, pre-resolving the
envelope, certifying an untracked internal write, and skipping the bundle
completeness check each turn the covering tests red.
* refactor(table): move the upsert route onto the application boundary
Same shape as the single-row route: declares itself against upsertTableRow,
hands the provenance envelope over unresolved, and derives its column keying
from the principal rather than assuming one.
The keying and presentation helpers the two routes shared are now in row-wire
beside the translators they wrap, so a third route does not restate them.
Two response details changed on purpose. The row now carries `position`, which
the use case always had and this route alone omitted — every other single-row
response already returned it, and the contract now describes one shape instead
of three. The upsert result also carries read-back provenance, which the route
previously assembled for itself.
The surface had no route-level tests; it has six now, covering both caller
keyings, both operations, and the envelope handover.
* refactor(table): move the enrichment-detail route onto the application boundary
The last internal adapter that queried the database for itself. It now runs
through readTableRowEnrichmentDetail, a new use case that shares
tableOperations.readRow — reading a cell's cascade breakdown is a projection of
the same row under the same role, not a second semantic operation.
Its tests move to the same seam and gain one the old suite could not express:
a cross-tenant table now conceals rather than confirming it exists.
* fix(table): mirror the storage rule when keying write provenance
storageKeyByWireKey mapped an unrecognised id-keyed key to null, but
rowDataToStorage persists that key when the caller is not writing strictly. A
cell would have been written with no provenance recorded, under a stamp still
marked complete — the same failure the bundle completeness check exists to
prevent, arriving through the keying map instead of the selection set.
The two wires genuinely differ and the code now says so: the name path drops an
unrecognised key, so it maps to null; the id path stores what it is given, so
every key it sends is a storage key.
Unreachable today, since no delegated surface uses id keying and a session
bundle is refused earlier. Fixed because the function's stated invariant — that
it mirrors how the row data itself is normalized — was not true.
Also corrects the delegated principal fixture in these tests, which used a kind
that is not in the Principal union, so the subject-id branch was never actually
exercised. It is now, and the scope check is asserted to receive the acting
principal's own subject id.
Verified to fail: restoring the schema-based lookup turns the covering test red.
* fix(table): restore executor access to the migrated row routes
The migration swapped checkSessionOrInternalAuth for the delegation policy,
and that broke every Table block call to these endpoints in two ways at once.
The old policy accepted a legacy internal token. The new one requires a
delegation token, which the executor only mints when the tool asks for it —
and none of the four table row tools did, so get/update/delete/upsert row
would each have failed with a 401. The knowledge tools already declare it,
because their routes migrated first.
Even with a valid token the operations denied the caller: readRow, updateRow,
deleteRow and upsertRow ran under a policy whose delegatedServices is
['copilot'], so the executor got a 403. They now use the tool-facing policy
that already existed for the group operations.
Neither was visible to the route tests, which mock the auth policy wholesale —
so the gap is closed at the layer that actually decides: one test pinning that
each tool requests delegation and that its operation admits the executor,
mutation-verified against both failure modes.
Also fixes findings from the review pass: the read surfaces no longer load an
executions sidecar none of them put on the wire (two readers rather than a
flag, so a caller cannot silently read an empty one); the provenance name
index is built once per batch instead of once per row; the uniqueness comment
now names the concurrent-insert race as well as the retro-added constraint;
and the presenter context gets NoInfer plus a note that the v2 builder passes
something different.
* fix(table): keep the lock on a 423 and pin the remaining wire changes
The rows error policy was built on the concealment base rather than the
lock-aware one, so a TableLockedError fell through to the generic handler and
the response lost its `lock` field — the only thing that tells a client which
lock to clear. A row write is exactly as lockable as a group mutation, so it
now shares that base.
Also pins the two wire changes the review found undocumented: a mismatched
workspace assertion answers 404 rather than 400, which is a superset of the
cross-tenant concealment already intended, and an unclassified failure answers
the builder's shared "Internal server error" rather than the old per-route
text. Both are consistent with the ~80 routes already on this builder; they are
asserted so they read as decisions rather than drift.
Verified to fail: reverting the policy base turns the lock test red.
* refactor(table): apply the quality pass
Four parallel reviews (reuse, simplification, efficiency, altitude). The
highest-value finding cut against the branch's own purpose: the rows error
policy sat in the barrel-exported route-policies module, so its import of the
1,200-line row use-case graph was paid by every one of the ~28 table routes
that can never throw a row error — the barrel's import cost went from ~1.1s to
~1.7s. row-route-policies.ts already existed for exactly this and is
deliberately not re-exported; the policy now lives there with its v2 sibling.
The upsert path still loaded an executions sidecar no surface puts on the wire,
and did it inside the write transaction, holding it open for a discarded
result. The read path got that fix earlier; the write path next to it did not.
rowKeyingForPrincipal fell through to name keying for anything that was not a
session. The operation policy admits API-key principals, so the first one to
reach these routes would have had every id-keyed cell dropped and the write
reported as successful. It is now an exhaustive switch over the two kinds the
auth policy yields — which immediately failed three tests using a principal
kind that exists nowhere in the repo, so those fixtures are real now too.
Also: reuses toWireTimestamp and createUnknownTableRowSecretProvenance instead
of re-inlining them; shares one helper for the provenance choice the update and
upsert use cases both make; keeps one canonical actorClientId doc with two
cross-references; merges two maps keyed by the same string into one; stops
round-tripping the principal through the legacy AuthType enum; hoists the
presenter function type out of both conditional branches; drops a subsumed
test; and freezes the shared locks fixture so a mutating test cannot poison its
siblings.1 parent 8bfaee4 commit 5e0a2ce
28 files changed
Lines changed: 1501 additions & 780 deletions
File tree
- apps/sim
- app/api/table
- [tableId]/rows
- [rowId]
- enrichment/[groupId]
- upsert
- lib
- api
- contracts
- server/routes
- table
- __tests__
- api
- application
- rows
- tools/table
- packages/testing/src/factories
Lines changed: 71 additions & 70 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
3 | 7 | | |
4 | | - | |
5 | 8 | | |
6 | 9 | | |
7 | | - | |
8 | 10 | | |
9 | | - | |
10 | | - | |
11 | | - | |
| 11 | + | |
| 12 | + | |
12 | 13 | | |
13 | 14 | | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
18 | | - | |
| 15 | + | |
| 16 | + | |
19 | 17 | | |
20 | | - | |
21 | | - | |
22 | | - | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
26 | 33 | | |
27 | 34 | | |
28 | | - | |
29 | | - | |
30 | | - | |
31 | | - | |
32 | | - | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
33 | 42 | | |
34 | 43 | | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | | - | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
53 | 48 | | |
54 | 49 | | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
55 | 60 | | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
64 | 68 | | |
65 | 69 | | |
66 | 70 | | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | | - | |
77 | | - | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
78 | 75 | | |
79 | 76 | | |
80 | 77 | | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
86 | 83 | | |
87 | 84 | | |
88 | | - | |
89 | | - | |
90 | | - | |
91 | | - | |
92 | | - | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
93 | 93 | | |
94 | 94 | | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
100 | 101 | | |
101 | 102 | | |
Lines changed: 24 additions & 43 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
2 | | - | |
3 | | - | |
4 | 1 | | |
5 | | - | |
6 | | - | |
7 | | - | |
8 | | - | |
9 | | - | |
10 | | - | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
11 | 7 | | |
12 | | - | |
13 | | - | |
14 | | - | |
15 | | - | |
16 | | - | |
| 8 | + | |
17 | 9 | | |
18 | 10 | | |
19 | 11 | | |
20 | 12 | | |
21 | | - | |
22 | | - | |
23 | | - | |
24 | | - | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
25 | 16 | | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
51 | 32 | | |
0 commit comments