Commit b69fdbd
authored
fix(api): give every v1 endpoint quota headers and errors that name the field (#6012)
* fix(api): give every v1 endpoint quota headers and errors that name the field
Three consistency gaps found by probing the live v1 surface end to end.
Rate-limit headers were only published by routes built on createApiResponse —
workflows, logs and audit-logs. Tables, files and knowledge are rate limited by
the same bucket and will return 429, but published no quota on success, so a
client discovered the ceiling only by hitting it. Adds a shared
rateLimitHeaders() builder, reused by createRateLimitResponse, and attaches it
to all 31 success responses on those three families.
Missing required fields did not name themselves. .min(1, '...') only fires for
a present-but-empty string, so an omitted field fell through to Zod's default
"Invalid input: expected string, received undefined". A previous pass fixed the
shared id schemas, but 22 of 23 v1 workspaceId declarations bypassed them, so
the fix reached almost nothing. Adds requiredFieldSchema(message) and routes
every v1 request input through it, preserving each site's existing, more
specific wording (for example "workspaceId query parameter is required")
instead of flattening them to the generic one. Response schemas are left alone
— "required" wording would be wrong there.
Validation failures on tables, files and knowledge reported the literal
"Validation error" and discarded the schema's message. Adds
v1ValidationErrorResponse, which surfaces the first issue while keeping
details, and wires it into the 19 parseRequest calls that had no handler.
Routes with deliberately specific wording keep theirs. The global default is
untouched, since routes outside v1 assert the current string.
* fix(api): finish the v1 consistency sweep at call level, not file level
Review found three places the first pass missed, all from filters that worked
on whole files instead of individual call sites.
- GET /api/v1/files/{fileId} returns the file bytes via `new Response`, not a
`success: true` JSON body, so the header pass skipped it. The download now
carries the same quota headers as the DELETE beside it.
- Four parseRequest calls in the table-row routes still reported the generic
"Validation error". The first pass skipped any file that already had a
handler anywhere in it, which excluded these two files wholesale. The check
is now per call site, and no bare call remains.
- POST /api/v1/tables takes its body from the shared tables contract, which
still used the bare `.min(1)` form, so an omitted workspaceId did not name
itself. Converted there and in the other v1-reachable contracts.
Scope note: roughly a thousand `.min(1, '... is required')` declarations remain
under contracts/tools/**. Those are block and tool definitions rather than the
public REST surface, and converting them belongs in its own change.
* refactor(api): publish quota headers from one chokepoint, not 32 call sites
A quality pass found the previous commit only made the happy path consistent.
Those three route families have 117 response sites; 32 got headers. The other
85 are the error paths — 400/403/404/500 — which are exactly the responses a
client is deciding whether to retry, and they published no quota at all.
`checkRateLimit` now records the bucket snapshot against the request, and
`withRouteHandler` attaches the headers next to the `x-request-id` it already
sets, on both the success and the unhandled-error branch. Every v1 response
carries the quota now, and a new v1 route gets it without remembering to. The
carrier is a WeakMap keyed by the request, so it needs no cleanup and routes
that never record a snapshot — everything outside v1 — are untouched.
This deletes more than it adds: the 32 decorations are gone, and so is the
`rateLimit` parameter that had been threaded into `handleBatchInsert` purely so
a business-logic helper could decorate its own response.
Also from the same pass:
- One definition of the header trio. `createApiResponse` had its own copy, so
after the last commit there were two; both now build from
`buildRateLimitHeaders`.
- `v1ValidationErrorResponse` delegates to the shared `validationErrorResponse`
instead of hand-rolling the same body, and takes a fallback message, which
collapses four route closures that differed only in that string.
- 36 sites wrote `requiredFieldSchema('Workspace ID is required')` — the
verbatim definition of the exported `workspaceIdSchema`. Using the primitive
is the whole point of having it; they now import it.
- Dropped TSDoc that had gone stale or contradicted the call sites it advised.
* docs(api): reattach the withRouteHandler docblock and drop stale wording
The comment pass caught a real casualty of the previous commit: inserting
`applyResponseHeaders` put it between `withRouteHandler`'s docblock and the
function itself, so the file's most-used export lost its documentation to the
new private helper. Reattached, and its header bullet now mentions the
rate-limit trio it also emits.
Remaining edits are wording only. The WeakMap rationale moved off
`RateLimitSnapshot` — three self-evident fields — onto the `snapshots`
declaration it actually describes. The record site no longer restates that
rationale; it keeps only the part unique to it. And three id-schema docs claimed
"same constraint as nonEmptyIdSchema", which stopped being true when that
schema was documented as deliberately message-less.
* docs(api): document the quota headers on every v1 success response
The spec already asserted, in the RateLimited description, that the
X-RateLimit-* trio accompanies every authenticated response. Before this branch
that was false for tables, files and knowledge; it is true now, but no operation
documented it — only 1 of 40 v1 success responses carried the headers.
All 40 now reference the shared header components. The shared BadRequest,
Forbidden and NotFound components are deliberately left alone: they are also
$ref-ed by non-v1 operations that publish no quota, so annotating them there
would over-claim. The RateLimited description carries the general rule instead,
now stating explicitly that the only responses without the headers are the ones
that failed authentication.
* fix(api): stop the last v1 validation paths from swallowing the message
Bugbot found GET /api/v1/tables/{tableId}/rows still answering with the bare
"Validation error". Its handler special-cases malformed filter/sort JSON and
then falls back to the shared helper — so the site looked handled to a check
that only asked whether a handler existed, which is why the earlier call-level
sweep passed over it.
Auditing the whole class turned up more of the same shape:
- The optional-body parses on deploy and rollback, where a bad `version` lost
"version must be a positive integer".
- Eleven catch-block `validationErrorResponseFromError` handlers across the
table routes, which discard the message of any ZodError thrown deeper.
Adds `v1ValidationErrorResponseFromError` as the v1 counterpart for unknown
caught values, and routes every remaining v1 validation path through the v1
helpers. No call to the generic helpers survives under app/api/v1 outside
admin, which keeps its own error envelope.1 parent 02311ca commit b69fdbd
39 files changed
Lines changed: 941 additions & 199 deletions
File tree
- apps
- docs
- sim
- app/api/v1
- audit-logs
- files
- [fileId]
- knowledge
- [id]
- documents
- [documentId]
- search
- logs
- tables
- [tableId]
- columns
- rows
- [rowId]
- upsert
- workflows
- [id]
- deploy
- rollback
- import
- lib
- api
- contracts
- v1
- knowledge
- tables
- server
- core/utils
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
| 29 | + | |
| 30 | + | |
29 | 31 | | |
30 | 32 | | |
31 | 33 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
27 | | - | |
| 27 | + | |
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
38 | | - | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
39 | 43 | | |
40 | 44 | | |
41 | 45 | | |
| |||
65 | 69 | | |
66 | 70 | | |
67 | 71 | | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
| 72 | + | |
76 | 73 | | |
77 | 74 | | |
78 | 75 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| 23 | + | |
| 24 | + | |
23 | 25 | | |
24 | 26 | | |
25 | 27 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
18 | 19 | | |
19 | 20 | | |
20 | 21 | | |
| |||
38 | 39 | | |
39 | 40 | | |
40 | 41 | | |
41 | | - | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
42 | 45 | | |
43 | 46 | | |
44 | 47 | | |
| |||
119 | 122 | | |
120 | 123 | | |
121 | 124 | | |
122 | | - | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
123 | 128 | | |
124 | 129 | | |
125 | 130 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| 25 | + | |
25 | 26 | | |
26 | 27 | | |
27 | 28 | | |
| |||
43 | 44 | | |
44 | 45 | | |
45 | 46 | | |
46 | | - | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
47 | 55 | | |
48 | 56 | | |
49 | 57 | | |
| |||
Lines changed: 7 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
| 14 | + | |
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | | - | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
32 | 34 | | |
33 | 35 | | |
34 | 36 | | |
| |||
117 | 119 | | |
118 | 120 | | |
119 | 121 | | |
120 | | - | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
121 | 125 | | |
122 | 126 | | |
123 | 127 | | |
| |||
Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
46 | 46 | | |
47 | 47 | | |
48 | 48 | | |
| 49 | + | |
| 50 | + | |
49 | 51 | | |
50 | 52 | | |
51 | 53 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
29 | | - | |
| 29 | + | |
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
47 | | - | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
48 | 50 | | |
49 | 51 | | |
50 | 52 | | |
| |||
99 | 101 | | |
100 | 102 | | |
101 | 103 | | |
102 | | - | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
103 | 107 | | |
104 | 108 | | |
105 | 109 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
| 16 | + | |
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
33 | 35 | | |
34 | 36 | | |
35 | 37 | | |
| |||
54 | 56 | | |
55 | 57 | | |
56 | 58 | | |
57 | | - | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
58 | 62 | | |
59 | 63 | | |
60 | 64 | | |
| |||
106 | 110 | | |
107 | 111 | | |
108 | 112 | | |
109 | | - | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
110 | 116 | | |
111 | 117 | | |
112 | 118 | | |
| |||
0 commit comments