fix(storage): document 200 responses for vector bucket CRUD endpoints - #1270
fix(storage): document 200 responses for vector bucket CRUD endpoints#1270grdsdev wants to merge 3 commits into
Conversation
The vector bucket routes (create/get/list/delete) only ever documented error responses, so the exported OpenAPI spec had no success schema at all for them - unusable for SDK generation. Adds a vectorBucketSchema component (mirroring bucketSchema/objectSchema) plus response wrappers for get/list, and explicit `type: 'null'` 200s for the void create/delete responses so generators don't try to decode an empty body as JSON. Also wires createOpenApiTransform/dedupeTrailingSlashPaths/ nameSchemaByDollarId into the swagger build (assigns stable operationIds from each route's config.operation, names swagger components by $id, and folds duplicate trailing-slash paths), and documents the 403 response every JWT-guarded route (including vector) can return. First slice of a larger OpenAPI/SDK-generation PR, split up by route family to keep review scoped.
Coverage Report for CI Build 30361736315Coverage decreased (-21.1%) to 59.184%Details
Uncovered Changes
Coverage Regressions2446 previously-covered lines in 105 files lost coverage.
Coverage Stats
💛 - Coveralls |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked vectorBucketSchema's use of the OpenAPI-style nullable: true keyword on creationTime (rather than the type: ['integer', 'null'] / anyOf pattern used everywhere else in src/storage/schemas) — it's a docs-only response schema and AJV ignores the unrecognized nullable keyword rather than erroring, so it doesn't break validation, just an inconsistency worth normalizing at some point.
Extended reasoning...
Confirmed via grep that every other nullable field in src/storage/schemas (bucket.ts, object.ts, upload.ts, multipart.ts) uses type: [x, 'null'] or anyOf, never the OpenAPI 3.0 'nullable' keyword. Since this schema is only used for response documentation/serialization (not request validation) and AJV in this codebase's default config doesn't run in strict mode that would throw on unknown keywords, this doesn't cause a functional bug — just a style inconsistency, not delaying approval on its own.
The onRoute hook was mutating routeOptions.schema.response directly,
which Fastify compiles into a real fast-json-stringify serializer
independent of docs generation. Iceberg's routes use a custom error
formatter ({ error: { message, type, code } }, no top-level
statusCode/message/code), so any 403 there (wrong-role JWT) would fail
serialization against errorSchema's required fields and come back as
a 500 instead. openapi-transform.ts's defaultErrorResponse already
documents a generic 4xx (covering 403) doc-only for every route, so no
replacement is needed.
| * leak collisions into each other when generated in the same process (see export-docs.ts). | ||
| */ | ||
| export function createOpenApiTransform() { | ||
| const seenIds = new Set<string>() |
There was a problem hiding this comment.
This makes ordering important so maintenance problem. If we change route ordering in a file, it will break SDKs. I would suggest adding explicit config directly to routes for documentation and validate for uniqueness. Auto generated head routes could receive Head suffix
config: {
operation: ROUTE_OPERATIONS.GET_AUTH_OBJECT,
openapiOperation: "yourid"
}
| metadata: { type: 'string' }, | ||
| file: { type: 'string', contentEncoding: 'binary' }, | ||
| }, | ||
| required: ['cacheControl', 'file'], |
There was a problem hiding this comment.
cacheControl isn't required, defaults to no-cache
| type: 'object', | ||
| properties: { | ||
| cacheControl: { type: 'string' }, | ||
| metadata: { type: 'string' }, |
There was a problem hiding this comment.
it also accepts userMetadata and contentType
… fields - operationId generation no longer silently resolves collisions by registration order. Routes can now set config.operationId to pin an explicit id; exposeHeadRoutes-derived HEAD routes get a deterministic Head suffix; any other collision throws instead of picking a winner based on file order, since that would make generated SDK method names depend on ordering that has nothing to do with the API contract. - documentMultipartUploadBody no longer marks cacheControl as required - the uploader defaults it to 'no-cache' when omitted.
| * regression, not a docs improvement. Document the multipart shape here instead, transform-only, | ||
| * where - like `defaultErrorResponse` above - it can never reach live request validation. | ||
| */ | ||
| function documentMultipartUploadBody(schema: FastifySchema, route: RouteOptions): FastifySchema { |
There was a problem hiding this comment.
we also support raw uploads so we need to document it as well?
I would suggest dropping multipart from this PR and could be added separately in a follow up
| properties: { | ||
| cacheControl: { type: 'string' }, | ||
| metadata: { type: 'string' }, | ||
| file: { type: 'string', contentEncoding: 'binary' }, |
There was a problem hiding this comment.
| file: { type: 'string', contentEncoding: 'binary' }, | |
| file: { type: 'string', format: 'binary' }, |
| * those ad-hoc replies (fast-json-stringify errors on a missing required property rather | ||
| * than dropping it). A transform can't affect request handling, so it can't cause that. | ||
| */ | ||
| function defaultErrorResponse(schema: FastifySchema | undefined): FastifySchema { |
There was a problem hiding this comment.
this is not aligned with iceberg
storage/src/http/routes/iceberg/index.ts
Line 41 in c57fbf2
| } as RouteOptions | ||
| } | ||
|
|
||
| describe('documentMultipartUploadBody (via transformOpenApiSchema)', () => { |
There was a problem hiding this comment.
would be nice to see a few tests vector response and error envelope
3 participants
Context
The SDK team is starting to use the generated OpenAPI spec to generate idiomatic API clients for the Supabase SDKs. We're starting with vector buckets for testing. This PR is the first of several that will make the fastify-generated OpenAPI spec ready for client generation, split by route family to keep each one reviewable.
Summary
First slice of #1215 (too large to review as one PR), split by route family. This PR covers only the vector bucket routes.
vectorBucketSchema,getVectorBucketResponseSchema,listVectorBucketsResponseSchema(mirroringbucketSchema/objectSchema), and wires them intocreate-bucket/delete-bucket/get-bucket/list-bucketsso the exported spec has success schemas for the vector CRUD endpoints, not just error responses.src/http/routes/openapi-transform.ts: derives stableoperationIds from each route's existingconfig.operation, names swagger components by$idinstead ofdef-N, folds duplicate trailing-slash paths, and documents a generic 4xx (doc-only, covering 403 etc.) for any route without one. Prerequisite infra for generating a usable client from any route family, starting here with vector.Deferred to follow-up PRs: bucket/object/iceberg/render/tus/cdn/health route documentation,
error-handler.tsformatter support,apikey.ts, typespec tooling additions.Test plan
tsc -noEmit— no errorsbiome checkon touched files — cleanvitest runonopenapi-transform.test.ts— 5/5 passing