diff --git a/.changeset/retire-dispatcher-storage-bridge.md b/.changeset/retire-dispatcher-storage-bridge.md new file mode 100644 index 0000000000..b07f333bf9 --- /dev/null +++ b/.changeset/retire-dispatcher-storage-bridge.md @@ -0,0 +1,63 @@ +--- +"@objectstack/runtime": minor +"@objectstack/hono": minor +"@objectstack/plugin-dev": patch +--- + +fix(runtime,hono,plugin-dev): retire the dispatcher's `/storage` bridge — it never spoke the storage contract (#4087) + +`POST /api/v1/storage/upload` and `GET /api/v1/storage/file/:id` were a +dispatcher-side bridge to the `file-storage` service slot, written against a +service shape that does not exist: + +- **Upload** called the contract's `upload(key, data, options?)` as + `upload(file, { request })` — the parsed file object landed in the `key` + slot and `{ request }` in `data`. That is a `TypeError` against every + implementation in the repo (`S3StorageAdapter`, `LocalStorageAdapter`, + `SwappableStorageService`, plugin-dev's in-memory one), not a + near-miss: `Buffer.from({}) → ERR_INVALID_ARG_TYPE`, or an object used as + an S3 object key / `path.join` segment. +- **Download** branched on `result.url` / `result.redirect` / `result.stream` + / `result.mimeType` while the contract's `download(key)` resolves a + `Buffer`, so every branch fell through and the route answered a + JSON-serialized Buffer. + +Both routes are removed, along with `HttpDispatcher.handleStorage()`, the +`/storage` domain registration, the dispatcher-plugin mounts and the two route +ledger rows. + +**Migration.** There is nothing to migrate off in practice — neither route +could complete a request. (They were reachable: `service-storage` mounts +`/storage/upload/presigned`, not `/storage/upload`, so nothing shadowed them. +They simply had no caller — no SDK method builds those URLs.) +`/api/v1/storage` is `@objectstack/service-storage`'s surface and always was +the working one: + +- Upload — FROM `POST /api/v1/storage/upload` TO the presigned protocol + (`POST /storage/upload/presigned` → direct `PUT` to the returned URL → + `POST /storage/upload/complete`), or `client.storage.upload(file)`, which + runs all three steps. +- Download — FROM `GET /api/v1/storage/file/:id` TO + `GET /storage/files/:fileId/url` (`client.storage.getDownloadUrl(fileId)`) + for a signed URL, or `GET /storage/files/:fileId` for a stable browser URL + that 302s to it. + +Install `@objectstack/service-storage` to get those routes; without it +`/api/v1/storage` now has no handler, which is the same answer every other +uninstalled capability gives. + +Two follow-on corrections keep `declared === enforced`: + +- `@objectstack/hono` no longer mounts `app.all('/storage/*')`. That + wildcard claimed the whole `/storage` subtree for the two dead routes, so + every other path under it — service-storage's protocol above all — got the + bridge's own 404 rather than falling through. Storage is ordinary catch-all + traffic now. +- Discovery keeps gating `routes.storage` on `isServiceServeable` — the shared + `handlerReady` predicate #4058 step 2 introduced — and plugin-dev's in-memory + implementation now self-declares `handlerReady: false`. #4058 deliberately + left that one serving because the `/storage` bridge was still there to serve + it; with the bridge retired nothing routes HTTP to that slot, so `false` is + the honest value — the position `realtime` has held since ADR-0076 D12. The + implementation keeps working for in-process callers; it is simply no longer + advertised as a reachable HTTP capability. diff --git a/content/docs/api/plugin-endpoints.mdx b/content/docs/api/plugin-endpoints.mdx index 6ff76350aa..4463d58791 100644 --- a/content/docs/api/plugin-endpoints.mdx +++ b/content/docs/api/plugin-endpoints.mdx @@ -144,10 +144,43 @@ protocol `ai.chatStream` parses and owns message state for you. ### File Storage (`/storage`) — Plugin Required -| Method | Endpoint | Description | -|:-------|:---------|:------------| -| POST | `/storage/upload` | Upload a file | -| GET | `/storage/file/:id` | Download a file | +Provided by `@objectstack/service-storage`, which registers these routes on the +host HTTP server. Uploads are a three-step protocol — ask for a presigned +target, send the bytes straight to it, then commit — so the bytes never proxy +through the API server, and the same shape works for S3, local disk, or any +other adapter. + +| Method | Endpoint | SDK | Description | +|:-------|:---------|:----|:------------| +| POST | `/storage/upload/presigned` | `storage.getPresignedUrl` | Step 1 — mint an upload target for a new `sys_file` row | +| POST | `/storage/upload/complete` | `storage.upload` | Step 3 — commit the uploaded bytes | +| POST | `/storage/upload/chunked` | `storage.initChunkedUpload` | Open a chunked / resumable session | +| PUT | `/storage/upload/chunked/:uploadId/chunk/:chunkIndex` | `storage.uploadPart` | Send one chunk | +| POST | `/storage/upload/chunked/:uploadId/complete` | `storage.completeChunkedUpload` | Assemble the parts | +| GET | `/storage/upload/chunked/:uploadId/progress` | `storage.resumeUpload` | Read session progress (first step of a resume) | +| GET | `/storage/files/:fileId/url` | `storage.getDownloadUrl` | Resolve a short-lived signed download URL | +| GET | `/storage/files/:fileId` | — | Stable browser URL; 302s to the same signed URL (what file fields carry) | + +`storage.upload(file)` runs steps 1–3 for you, including the direct-to-storage +`PUT` in step 2. + + +**This table used to list two routes that never worked (#4087).** `POST +/storage/upload` and `GET /storage/file/:id` were a dispatcher-side bridge to +the `file-storage` service, written against a service shape that does not +exist: it called `upload(key, data, options?)` as `upload(file, { request })` — +a `TypeError` against every implementation in the repo — and read the `Buffer` +that `download(key)` resolves as if it were a `{ url | stream | mimeType }` +descriptor. They stayed mounted and reachable — nothing shadowed them — so +anyone who followed this table got a 500; everyone who followed the SDK used +the protocol above and never touched them. + +Both routes are retired. `/storage` is service-storage's surface — install that +package to get it. Discovery advertises the route only when the occupant of the +`file-storage` slot actually mounts HTTP handlers, so an in-memory dev +implementation now reports `handlerReady: false` and no `routes.storage` +instead of pointing at a path with nothing behind it. + --- diff --git a/docs/audits/2026-07-dispatcher-client-route-coverage.md b/docs/audits/2026-07-dispatcher-client-route-coverage.md index 9b212266b1..8f2b1e480a 100644 --- a/docs/audits/2026-07-dispatcher-client-route-coverage.md +++ b/docs/audits/2026-07-dispatcher-client-route-coverage.md @@ -111,6 +111,37 @@ had zero call sites in `objectstack` and `objectui`. dispatcher's two plain routes remain a low-level redirect/stream compat surface, deliberately outside the SDK. +### Amendment (#4087) — the storage half rested on its own bad premise + +The #3584 pass corrected one premise in this table and kept another it never +checked: that the dispatcher's two storage routes were *a working low-level +compat surface*. End-to-end verification found they were not, and never had +been. `POST /storage/upload` called `upload(key, data, options?)` as +`upload(file, { request })` — the file object landing in the `key` slot and +`{ request }` in `data` — which is a `TypeError` against every implementation +in the repo (`s3-storage-adapter`, `local-storage-adapter`, +`swappable-storage-service`, plugin-dev's in-memory one). `GET +/storage/file/:id` had the mirror-image defect one layer quieter: it branched +on `result.url` / `result.redirect` / `result.stream` / `result.mimeType` +while the contract's `download(key)` resolves a `Buffer`, so every branch fell +through to "serialize the Buffer as JSON". + +Neither could serve a request. What kept that hidden was not shadowing — +service-storage mounts `/storage/upload/presigned`, not `/storage/upload`, so +both bridge routes stayed mounted and reachable in a full stack — but the +absence of any caller: no SDK method, no console call — only the +`content/docs/api/plugin-endpoints` table advertising them to readers. The +tests that covered them mocked the shape the handler wanted rather than the +shape `IStorageService` declares, so the suite stayed green on a pair of routes +that answered 500. + +**Both routes are retired** (#4087) — the `/storage` domain, its +dispatcher-plugin mounts, the hono adapter's `/storage/*` wildcard (which had +been claiming the whole subtree for them) and the two ledger rows are gone. +Read the two storage rows in §4 as retired rather than as the `server-only` +disposition #3584 gave them: the surface belongs entirely to `service-storage`, +enumerated in `packages/services/service-storage/src/storage-route-ledger.ts`. + ## 5. Client-internal findings - **`trigger` vs `execute`** hit different URLs for the same intent diff --git a/packages/adapters/hono/src/__mocks__/runtime.ts b/packages/adapters/hono/src/__mocks__/runtime.ts index 137a7c13ad..5306d26684 100644 --- a/packages/adapters/hono/src/__mocks__/runtime.ts +++ b/packages/adapters/hono/src/__mocks__/runtime.ts @@ -7,7 +7,6 @@ export class HttpDispatcher { handleAuth = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: { ok: true } } }); handleMetadata = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: { objects: [] } } }); handleData = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: { records: [] } } }); - handleStorage = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: {} } }); constructor(_kernel: any) {} } diff --git a/packages/adapters/hono/src/hono.test.ts b/packages/adapters/hono/src/hono.test.ts index 7323c10112..1aa9b8bb05 100644 --- a/packages/adapters/hono/src/hono.test.ts +++ b/packages/adapters/hono/src/hono.test.ts @@ -8,7 +8,6 @@ const mockDispatcher = { getDiscoveryInfo: vi.fn().mockReturnValue({ version: '1.0', endpoints: [] }), handleAuth: vi.fn().mockResolvedValue({ handled: true, response: { body: { ok: true }, status: 200 } }), handleGraphQL: vi.fn().mockResolvedValue({ data: {} }), - handleStorage: vi.fn().mockResolvedValue({ handled: true, response: { body: {}, status: 200 } }), dispatch: vi.fn().mockResolvedValue({ handled: true, response: { body: { success: true }, status: 200 } }), }; @@ -486,6 +485,29 @@ describe('createHonoApp', () => { '/api', ); }); + + // [#4087] The retired `/storage/*` mount was a WILDCARD: it claimed the + // whole subtree for two dispatcher routes, so service-storage's protocol + // (`/storage/upload/presigned`, `/storage/files/:id/url`, …) — registered + // under the same prefix on the same server — could never be reached + // through this app. Storage is ordinary catch-all traffic now; nothing + // here parses formData or shadows the subtree. + it('POST /api/storage/upload/presigned reaches the catch-all, not a storage mount', async () => { + const res = await app.request('/api/storage/upload/presigned', { + method: 'POST', + body: JSON.stringify({ filename: 'a.txt' }), + headers: { 'Content-Type': 'application/json' }, + }); + expect(res.status).toBe(200); + expect(mockDispatcher.dispatch).toHaveBeenCalledWith( + 'POST', + '/storage/upload/presigned', + { filename: 'a.txt' }, + expect.any(Object), + expect.objectContaining({ request: expect.anything() }), + '/api', + ); + }); }); describe('Error Handling', () => { diff --git a/packages/adapters/hono/src/index.ts b/packages/adapters/hono/src/index.ts index be4b87c8e6..6db020306a 100644 --- a/packages/adapters/hono/src/index.ts +++ b/packages/adapters/hono/src/index.ts @@ -96,8 +96,8 @@ export function objectStackMiddleware(kernel: ObjectKernel) { /** * Creates a full-featured Hono app with all ObjectStack route dispatchers. * - * Only routes that need framework-specific handling (auth service, storage - * formData, GraphQL raw result, discovery wrapper) are registered explicitly. + * Only routes that need framework-specific handling (auth service, GraphQL + * raw result, discovery wrapper) are registered explicitly. * All other routes (meta, data, packages, analytics, automation, i18n, ui, * openapi, custom endpoints, and any future routes) are handled by a * catch-all that delegates to `HttpDispatcher.dispatch()`. @@ -347,24 +347,18 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { } }); - // --- Storage (needs formData parsing) --- - app.all(`${prefix}/storage/*`, async (c) => { - try { - const subPath = c.req.path.substring(`${prefix}/storage`.length); - const method = c.req.method; - - let file: any = undefined; - if (method === 'POST' && subPath === '/upload') { - const formData = await c.req.formData(); - file = formData.get('file'); - } - - const result = await dispatcher.handleStorage(subPath, method, file, { request: c.req.raw }); - return toResponse(c, result); - } catch (err: any) { - return errorJson(c, err.message || 'Internal Server Error', err.statusCode || 500); - } - }); + // --- Storage: deliberately NOT mounted (#4087) --- + // This used to be `app.all(prefix + '/storage/*')` feeding + // `dispatcher.handleStorage`. Two things were wrong with it. The handler it + // called spoke a storage contract that does not exist (it passed the parsed + // file as the `key` argument of `upload(key, data, options?)`, and read the + // Buffer that `download(key)` resolves as a `{ url | stream }` descriptor) — + // retired with this change. And the wildcard was wider than the two routes + // it served: it claimed the WHOLE `/storage` subtree, so every other path + // under it — the presigned / chunked / signed-URL protocol `service-storage` + // registers, above all — was answered by the bridge's own 404 instead of + // falling through to anything else this app might mount. Storage is ordinary + // catch-all traffic now, like every other domain. // ─── Catch-all: delegate to dispatcher.dispatch() ───────────────────────── // Handles meta, data, packages, analytics, automation, i18n, ui, openapi, diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 2291e371fb..52baa59182 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1480,10 +1480,13 @@ export class ObjectStackProtocolImplementation implements // registers the service (service-storage's own `/api/v1/storage` // routes, plugin-search, plugin-graphql, …) rather than to a dispatcher // domain, so `handlerReady` there says nothing about whether THAT route - // is mounted, and suppressing it would be a guess. `file-storage` is - // listed because the dispatcher does own a `/storage` bridge for the - // no-plugin case; when service-storage is installed it registers a real - // (unmarked) service, so the entry never fires. + // is mounted, and suppressing it would be a guess. `file-storage` stays + // listed as the one entry with no dispatcher domain behind it — #4087 + // retired the `/storage` bridge — because for that slot `handlerReady` + // is not a proxy for anything, it is the fact itself: an occupant that + // mounts no HTTP surface must not have `/api/v1/storage` advertised on + // its behalf. When service-storage is installed it registers a real + // (unmarked) service and mounts the routes, so the entry never fires. const DISPATCHER_GATED_SERVICES = new Set([ 'analytics', 'automation', 'notification', 'ai', 'i18n', 'file-storage', ]); diff --git a/packages/plugins/plugin-dev/src/dev-plugin.test.ts b/packages/plugins/plugin-dev/src/dev-plugin.test.ts index 5fe4a28a81..ee6d56f079 100644 --- a/packages/plugins/plugin-dev/src/dev-plugin.test.ts +++ b/packages/plugins/plugin-dev/src/dev-plugin.test.ts @@ -275,7 +275,11 @@ describe('DevPlugin', () => { // No HTTP/WS surface is mounted for these, so no handler can be ready — // independent of the implementation being real (ADR-0076 D12, realtime). - for (const name of ['cache', 'queue', 'job', 'realtime']) { + // `file-storage` joined them in #4087: the dispatcher's `/storage` bridge + // was the only thing that ever routed HTTP to this slot, and it was + // retired — `@objectstack/service-storage` owns that surface and mounts + // its own routes against its own adapters, never this implementation. + for (const name of ['cache', 'queue', 'job', 'realtime', 'file-storage']) { expect(readServiceSelfInfo(registeredServices.get(name))?.handlerReady, `${name} handlerReady`).toBe(false); } diff --git a/packages/plugins/plugin-dev/src/dev-plugin.ts b/packages/plugins/plugin-dev/src/dev-plugin.ts index 9e466f2a3e..929f815d6d 100644 --- a/packages/plugins/plugin-dev/src/dev-plugin.ts +++ b/packages/plugins/plugin-dev/src/dev-plugin.ts @@ -295,14 +295,18 @@ const DEV_STUB_FACTORIES: Record Record> = { * * `handlerReady` answers a different question: does an HTTP handler genuinely * serve this? It stays `false` for every `stub`, and also for `degraded` - * services with no HTTP surface at all (`realtime` — the D12 precedent). + * services with no HTTP surface at all (`realtime` — the D12 precedent, joined + * by `file-storage` in #4087: the dispatcher's `/storage` bridge — the one + * thing that ever routed HTTP to this slot — was retired, and the surface it + * claimed belongs to `@objectstack/service-storage`, which mounts its own + * routes and does not use this implementation). * * Entries are only needed for plugin-dev's OWN fakes. The wrapped kernel * fallbacks (`cache` / `queue` / `job` / `i18n`) already carry their own * `__serviceInfo`, and {@link applySelfInfo} never overwrites one. */ const DEV_STUB_SELF_INFO: Record = { - 'file-storage': { status: 'degraded', message: 'Dev in-memory file storage — really stores and returns bytes, but nothing survives a restart. Register a storage plugin for durable files.' }, + 'file-storage': { status: 'degraded', handlerReady: false, message: 'Dev in-memory file storage — really stores and returns bytes to in-process callers, but nothing survives a restart and no HTTP surface is mounted. Register @objectstack/service-storage for durable files and the upload/download protocol.' }, 'search': { status: 'degraded', message: 'Dev in-memory index — real substring matching over indexed documents, no ranking, analyzers, or persistence. Register a search plugin for the real engine.' }, 'metadata': { status: 'degraded', message: 'Dev in-memory metadata registry — real reads and writes, no persistence. Register MetadataPlugin for a persisted registry.' }, 'workflow': { status: 'degraded', message: 'Dev in-memory workflow state — transitions are recorded and read back, but NOTHING is validated: every transition is accepted and availableTransitions is always empty.' }, diff --git a/packages/runtime/src/dispatcher-plugin.error-envelope.test.ts b/packages/runtime/src/dispatcher-plugin.error-envelope.test.ts index 4790bc3b8a..eb92517389 100644 --- a/packages/runtime/src/dispatcher-plugin.error-envelope.test.ts +++ b/packages/runtime/src/dispatcher-plugin.error-envelope.test.ts @@ -4,7 +4,7 @@ * #3867 — the dispatcher-plugin error exit. * * `errorResponseBase` is the single error exit for EVERY route this plugin - * mounts (`/analytics`, `/packages`, `/i18n`, `/storage`, `/automation`, + * mounts (`/analytics`, `/packages`, `/i18n`, `/automation`, * `/auth`, `/notifications`, `/mcp`, …): each handler catches and calls it * rather than re-throwing. Two defects lived there, and neither is visible * until a real error actually reaches it: diff --git a/packages/runtime/src/dispatcher-plugin.ts b/packages/runtime/src/dispatcher-plugin.ts index b1b0ce64db..af123d906c 100644 --- a/packages/runtime/src/dispatcher-plugin.ts +++ b/packages/runtime/src/dispatcher-plugin.ts @@ -340,7 +340,7 @@ function sendResultBase( /** * The single error exit for EVERY dispatcher-plugin route — `/analytics`, - * `/packages`, `/i18n`, `/storage`, `/automation`, `/auth`, `/notifications`, + * `/packages`, `/i18n`, `/automation`, `/auth`, `/notifications`, * `/mcp`, … Each handler catches and calls here rather than re-throwing. * * [#3867] Two things were wrong with it, both invisible until a driver error @@ -432,9 +432,11 @@ function errorResponseBase(err: any, res: any, securityHeaders?: Record `/${req.params.id}/rollback`); // ── Storage ───────────────────────────────────────────────── - server.post(`${prefix}/storage/upload`, async (req: any, res: any) => { - try { - // For file uploads the body *is* the file (parsed by adapter) - const result = await dispatcher.handleStorage('upload', 'POST', req.body, { request: req }); - sendResult(result, res); - } catch (err: any) { - errorResponse(err, res); - } - }); - - server.get(`${prefix}/storage/file/:id`, async (req: any, res: any) => { - try { - const result = await dispatcher.handleStorage(`file/${req.params.id}`, 'GET', undefined, { request: req }); - sendResult(result, res); - } catch (err: any) { - errorResponse(err, res); - } - }); + // Nothing mounted here on purpose (#4087). The dispatcher used to + // bridge `POST /storage/upload` and `GET /storage/file/:id` to the + // `file-storage` service, off the contract both ends actually + // speak: `upload(key, data, options?)` was called as + // `upload(file, { request })` — a guaranteed TypeError against + // every implementation in the repo — and the download branch + // switched on a `{ url | stream | mimeType }` result no + // implementation has ever returned (`download(key)` resolves a + // Buffer). What kept that invisible is not that something shadowed + // these paths — service-storage mounts `/storage/upload/...`, not + // `/storage/upload`, so both stayed mounted and reachable — but + // that nothing speaks their dialect: no SDK method, no console + // call. They were reachable and broken, and the only thing that + // ever routed a request to them was a reader of the old docs. + // + // `/api/v1/storage` is service-storage's surface: it registers the + // presigned / chunked / signed-URL protocol on this same + // http-server (`storage-routes.ts`), and that protocol is what + // `@objectstack/client` speaks. Without that package the path + // simply has no handler — the honest answer, and the one an empty + // capability slot gives everywhere else. // ── i18n ──────────────────────────────────────────────────── // Route via dispatch() (not handleI18n directly) so the host diff --git a/packages/runtime/src/domain-handler-registry.test.ts b/packages/runtime/src/domain-handler-registry.test.ts index 0798c50983..4eb1f025a2 100644 --- a/packages/runtime/src/domain-handler-registry.test.ts +++ b/packages/runtime/src/domain-handler-registry.test.ts @@ -245,7 +245,7 @@ describe('HttpDispatcher extracted domains (PR-2)', () => { }); // --------------------------------------------------------------------------- -// PR-3 — keys / storage / ui extraction +// PR-3 — keys / storage / ui extraction (storage since retired, #4087) // --------------------------------------------------------------------------- describe('HttpDispatcher extracted domains (PR-3: keys/storage/ui)', () => { @@ -284,18 +284,34 @@ describe('HttpDispatcher extracted domains (PR-3: keys/storage/ui)', () => { expect(result.response?.body?.data?.key).toBeTruthy(); }); - it('/storage responds 501 when file-storage is not configured (extracted body keeps in-handler semantics)', async () => { - const result = await makeDispatcher().dispatch('POST', '/storage/upload', { blob: 1 }, {}, {} as any); - expect(result.handled).toBe(true); - expect(result.response?.status).toBe(501); - }); - - it('/storage/upload uploads through the file-storage service', async () => { - const upload = vi.fn().mockResolvedValue({ id: 'f1' }); - const result = await makeDispatcher({ 'file-storage': { upload, download: vi.fn() } }) + /** + * [#4087] `/storage` is no longer a dispatcher domain. The registry must + * not claim the prefix in either direction — with the `file-storage` slot + * empty AND with it filled, since the retired bridge's whole reason for + * existing was "a service is registered, so route to it". It called that + * service off-contract (`upload(key, data, options?)` invoked as + * `upload(file, { request })`), and `@objectstack/service-storage` — which + * mounts the real protocol on the http-server — never went through here. + */ + it('/storage is not claimed by the registry, filled slot or not', async () => { + // No domain matches, so dispatch's terminal exit answers — the same + // ROUTE_NOT_FOUND every unregistered path gets, not a 501/500 from a + // half-present bridge. + const empty = await makeDispatcher().dispatch('POST', '/storage/upload', { blob: 1 }, {}, {} as any); + expect(empty.response?.status).toBe(404); + expect(empty.response?.body?.error?.code).toBe('ROUTE_NOT_FOUND'); + + const upload = vi.fn(); + const download = vi.fn(); + const filled = await makeDispatcher({ 'file-storage': { upload, download } }) .dispatch('POST', '/storage/upload', { some: 'file' }, {}, {} as any); - expect(result.response?.status).toBe(200); - expect(upload).toHaveBeenCalledTimes(1); + expect(filled.response?.status).toBe(404); + expect(upload).not.toHaveBeenCalled(); + + const get = await makeDispatcher({ 'file-storage': { upload, download } }) + .dispatch('GET', '/storage/file/abc', undefined, {}, {} as any); + expect(get.response?.status).toBe(404); + expect(download).not.toHaveBeenCalled(); }); it('/ui/view/:object serves the protocol getUiView result; 503 without a protocol service', async () => { diff --git a/packages/runtime/src/domains/storage.ts b/packages/runtime/src/domains/storage.ts deleted file mode 100644 index ef7484290f..0000000000 --- a/packages/runtime/src/domains/storage.ts +++ /dev/null @@ -1,92 +0,0 @@ -// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * `/storage` domain — extracted dispatcher body (ADR-0076 D11 step ③, PR-3). - * Upload / download bridge to the `file-storage` service. Download results - * may be a redirect or a stream — those come back as `result:{type:...}` for - * the HTTP adapter to realize (the dispatcher envelope can't carry them). - * - * Routes (path is the sub-path after `/storage`): - * POST /upload → upload (body is the file/stream) - * GET /file/:id → download (redirect | stream | metadata) - */ - -import { CoreServiceName } from '@objectstack/spec/system'; -import { isServiceServeable } from '../service-serveable.js'; -import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; -import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; - -export function createStorageDomain(deps: DomainHandlerDeps): DomainRoute { - return { - prefix: '/storage', - handler: (req, context) => - handleStorageRequest(deps, req.path.substring(8), req.method, req.body, context), - }; -} - -/** Body kept signature-compatible with the legacy `HttpDispatcher.handleStorage`. */ -export async function handleStorageRequest( - deps: DomainHandlerDeps, - path: string, - method: string, - file: any, - context: HttpProtocolContext, -): Promise { - // The legacy body had `getService(...) || this.kernel.services?.['file-storage']`. - // The second leg was strictly redundant: resolveService's fallback chain - // already ends at the services map (and when `services` is a Map, the - // legacy index access returned undefined anyway), so it is dropped here. - const storageService = await deps.getService(CoreServiceName.enum['file-storage']); - // [#4058] Same 501 for an empty slot and for a slot filled by a - // self-declared non-handler (`handlerReady: false`, ADR-0076 D12) — "not - // configured" is the honest answer to both. NOT aimed at the in-memory dev - // file store: that one really stores and really reads back, so it declares - // `degraded` (`handlerReady: true`) and still serves here. The gate exists - // for the occupant that only *claims* to store. - if (!isServiceServeable(storageService)) { - return { handled: true, response: deps.error('File storage not configured', 501) }; - } - - const m = method.toUpperCase(); - const parts = path.replace(/^\/+/, '').split('/'); - - // POST /storage/upload - if (parts[0] === 'upload' && m === 'POST') { - if (!file) { - return { handled: true, response: deps.error('No file provided', 400) }; - } - const result = await storageService.upload(file, { request: context.request }); - return { handled: true, response: deps.success(result) }; - } - - // GET /storage/file/:id - if (parts[0] === 'file' && parts[1] && m === 'GET') { - const id = parts[1]; - const result = await storageService.download(id, { request: context.request }); - - // Result can be URL (redirect), Stream/Blob, or metadata - if (result.url && result.redirect) { - // Must be handled by adapter to do actual redirect - return { handled: true, result: { type: 'redirect', url: result.url } }; - } - - if (result.stream) { - // Must be handled by adapter to pipe stream - return { - handled: true, - result: { - type: 'stream', - stream: result.stream, - headers: { - 'Content-Type': result.mimeType || 'application/octet-stream', - 'Content-Length': result.size - } - } - }; - } - - return { handled: true, response: deps.success(result) }; - } - - return { handled: false }; -} diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 75bb234dff..da0437e963 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -949,54 +949,39 @@ describe('HttpDispatcher', () => { }); }); - describe('handleStorage with async service', () => { - it('should resolve storage service from Promise', async () => { - const mockStorage = { - upload: vi.fn().mockResolvedValue({ id: 'file_1', url: '/files/1' }), - }; - (kernel as any).getService = vi.fn().mockImplementation((name: string) => { - if (name === 'file-storage') return Promise.resolve(mockStorage); - return null; - }); - - const result = await dispatcher.handleStorage('/upload', 'POST', { name: 'test.txt' }, { request: {} }); - expect(result.handled).toBe(true); - expect(result.response?.status).toBe(200); - expect(mockStorage.upload).toHaveBeenCalled(); - }); - - it('should return 501 when storage service is not registered (async null)', async () => { - (kernel as any).getService = vi.fn().mockResolvedValue(null); - (kernel as any).services = new Map(); - - const result = await dispatcher.handleStorage('/upload', 'POST', {}, { request: {} }); - expect(result.handled).toBe(true); - expect(result.response?.status).toBe(501); - expect(result.response?.body?.error?.message).toBe('File storage not configured'); + /** + * [#4087] The `/storage` bridge is retired — these tests used to pin + * it, and they are the reason it survived: every one of them mocked a + * `file-storage` service shaped the way the handler wanted rather than + * the way `IStorageService` declares. `upload` was asserted only as + * "was called" (it was called as `upload(file, { request })` against a + * contract of `upload(key, data, options?)` — a TypeError on any real + * implementation), and `download` was mocked to resolve + * `{ data, mimeType }` when the contract resolves a `Buffer`. Green + * tests, zero working requests. + * + * What replaces them is the absence itself: no `handleStorage` method, + * no `/storage` domain, no dispatcher-plugin mount. + */ + describe('storage bridge retired (#4087)', () => { + it('exposes no handleStorage method', () => { + expect((dispatcher as any).handleStorage).toBeUndefined(); }); - it('should handle GET /storage/file/:id with async service', async () => { - const mockStorage = { - download: vi.fn().mockResolvedValue({ data: 'content', mimeType: 'text/plain' }), - }; + it('does not claim /storage — dispatch falls through to ROUTE_NOT_FOUND', async () => { + const mockStorage = { upload: vi.fn(), download: vi.fn() }; (kernel as any).getService = vi.fn().mockImplementation((name: string) => { if (name === 'file-storage') return Promise.resolve(mockStorage); return null; }); - const result = await dispatcher.handleStorage('/file/abc123', 'GET', null, { request: {} }); - expect(result.handled).toBe(true); - expect(mockStorage.download).toHaveBeenCalledWith('abc123', { request: {} }); - }); - - it('should return 400 when upload has no file', async () => { - const mockStorage = { upload: vi.fn() }; - (kernel as any).getService = vi.fn().mockResolvedValue(mockStorage); - - const result = await dispatcher.handleStorage('/upload', 'POST', null, { request: {} }); - expect(result.handled).toBe(true); - expect(result.response?.status).toBe(400); - expect(result.response?.body?.error?.message).toBe('No file provided'); + const result = await dispatcher.dispatch('POST', '/storage/upload', { name: 'a.txt' }, {}, { request: {} } as any); + expect(result.response?.status).toBe(404); + expect(result.response?.body?.error?.code).toBe('ROUTE_NOT_FOUND'); + // The service is registered and still never touched — the + // dispatcher has no business calling it over HTTP. + expect(mockStorage.upload).not.toHaveBeenCalled(); + expect(mockStorage.download).not.toHaveBeenCalled(); }); }); @@ -1129,17 +1114,9 @@ describe('HttpDispatcher', () => { expect((kernel as any).getServiceAsync).toHaveBeenCalledWith('automation'); }); - it('should prefer getServiceAsync over getService for file-storage', async () => { - const asyncStorage = { - upload: vi.fn().mockResolvedValue({ id: 'file_1', url: '/files/1' }), - }; - (kernel as any).getServiceAsync = vi.fn().mockResolvedValue(asyncStorage); - - const result = await dispatcher.handleStorage('/upload', 'POST', { name: 'test.txt' }, { request: {} }); - expect(result.handled).toBe(true); - expect(result.response?.status).toBe(200); - expect((kernel as any).getServiceAsync).toHaveBeenCalledWith('file-storage'); - }); + // The `file-storage` variant of this trio went with the `/storage` + // bridge (#4087) — `resolveService`'s async preference is the same one + // code path for every slot, and auth / automation above still pin it. it('should resolve protocol service via getServiceAsync for handleMetadata', async () => { const asyncProtocol = { @@ -1286,19 +1263,9 @@ describe('HttpDispatcher', () => { ).rejects.toThrow('Query timeout'); }); - it('should propagate storage upload error', async () => { - const badStorage = { - upload: vi.fn().mockRejectedValue(new Error('Disk full')), - }; - (kernel as any).getService = vi.fn().mockImplementation((name: string) => { - if (name === 'file-storage') return Promise.resolve(badStorage); - return null; - }); - - await expect( - dispatcher.handleStorage('/upload', 'POST', { data: 'file' }, { request: {} }) - ).rejects.toThrow('Disk full'); - }); + // The storage-upload variant went with the `/storage` bridge (#4087); + // service-method errors reach the same exit from every domain, and the + // analytics case above pins it. }); // ═══════════════════════════════════════════════════════════════ @@ -2449,29 +2416,16 @@ describe('HttpDispatcher', () => { expect(result.response?.body?.data?.flows).toEqual(['flow_a']); }); - // Storage answers 501 for an empty slot rather than `handled: false`, so - // "same answer as an empty slot" means the same 501 here. - it('/storage — a stub slot answers the not-configured 501 without being called', async () => { - const stub = stubbed({ upload: vi.fn(), download: vi.fn() }); - serveOnly('file-storage', stub); - - const result = await dispatcher.handleStorage('upload', 'POST', { name: 'f.txt' }, { request: {} }); - expect(result.handled).toBe(true); - expect(result.response?.status).toBe(501); - expect(stub.upload).not.toHaveBeenCalled(); - }); - - // The half that protects plugin-dev's in-memory file store: it really - // stores and really reads back, so it declares `degraded` and serves. - it('/storage — a degraded in-memory store keeps serving', async () => { - const svc = degraded({ upload: vi.fn().mockResolvedValue({ key: 'f.txt' }) }); - serveOnly('file-storage', svc); - - const result = await dispatcher.handleStorage('upload', 'POST', { name: 'f.txt' }, { request: {} }); - expect(result.handled).toBe(true); - expect(result.response?.status).toBe(200); - expect(svc.upload).toHaveBeenCalled(); - }); + // [#4087] The two `/storage` cases this block carried are gone with the + // domain. Gating the bridge was correct as far as it went, but the + // thing being gated could not serve a request either way: it called + // `upload(key, data, options?)` as `upload(file, { request })`. The + // "degraded store keeps serving" case is the shape of the problem — + // asserting a 200 off `upload` mocked to resolve `{ key }`, a return + // value the contract (`Promise`) does not have. `file-storage` + // keeps its slot and its `handlerReady` gate on the ADVERTISEMENT + // (`routes.storage`, pinned in the discovery block above); what it no + // longer has is a dispatcher handler to gate. it('/i18n — a stub slot answers the not-available 501; a degraded provider serves', async () => { const stub = stubbed({ getLocales: vi.fn().mockReturnValue(['xx']) }); diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 6386b4931e..3c37bf3ff4 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -17,7 +17,6 @@ import { createI18nDomain, handleI18nRequest } from './domains/i18n.js'; import { createNotificationsDomain, handleNotificationRequest } from './domains/notifications.js'; import { createSecurityDomain, handleSecurityRequest } from './domains/security.js'; import { createKeysDomains, handleKeysRequest } from './domains/keys.js'; -import { createStorageDomain, handleStorageRequest } from './domains/storage.js'; import { createUiDomain, handleUiRequest } from './domains/ui.js'; import { createShareLinksDomain, handleShareLinksRequest } from './domains/share-links.js'; import { createPackagesDomain, handlePackagesRequest } from './domains/packages.js'; @@ -345,7 +344,10 @@ export class HttpDispatcher { this.domainRegistry.register(createNotificationsDomain(this.domainDeps)); this.domainRegistry.register(createSecurityDomain(this.domainDeps)); for (const route of createKeysDomains(this.domainDeps)) this.domainRegistry.register(route); - this.domainRegistry.register(createStorageDomain(this.domainDeps)); + // No `/storage` domain (#4087): the dispatcher's two-route bridge was + // retired. `/api/v1/storage` is service-storage's surface — it mounts + // the presigned / chunked / signed-URL protocol autonomously on the + // host http-server. See route-ledger.ts for the disposition. this.domainRegistry.register(createUiDomain(this.domainDeps)); this.domainRegistry.register(createShareLinksDomain(this.domainDeps)); this.domainRegistry.register(createPackagesDomain(this.domainDeps)); @@ -873,6 +875,14 @@ export class HttpDispatcher { // Same predicate ⇒ same answer. #4000 did this for `analytics` alone; // #4058 extended it to the rest of the dispatcher-owned domains. // + // [#4087] `file-storage` is the one slot here whose surface is NOT a + // dispatcher domain any more — the `/storage` bridge was retired and + // service-storage mounts `/api/v1/storage` itself. The predicate still + // holds, for the same reason and one step removed: `handlerReady` is + // exactly "does an HTTP handler serve this slot", so an occupant that + // mounts nothing (plugin-dev's in-memory implementation) must not have + // `${prefix}/storage` advertised on its behalf. + // // A `degraded` implementation keeps its route: `handlerReady` defaults // to `true` for it and its domain keeps serving it (that is the whole // point of the `stub` / `degraded` split — see plugin-dev's markers). @@ -911,6 +921,8 @@ export class HttpDispatcher { packages: `${prefix}/packages`, auth: hasAuth ? `${prefix}/auth` : undefined, ui: hasUi ? `${prefix}/ui` : undefined, + // Not a dispatcher domain since #4087 — this advertises + // service-storage's own mount. Gate unchanged (see above). storage: hasFiles ? `${prefix}/storage` : undefined, analytics: hasAnalytics ? `${prefix}/analytics` : undefined, automation: hasAutomation ? `${prefix}/automation` : undefined, @@ -1200,11 +1212,6 @@ export class HttpDispatcher { } } - /** Thin delegate — body extracted to `./domains/storage.ts` (D11③ PR-3). */ - async handleStorage(path: string, method: string, file: any, context: HttpProtocolContext): Promise { - return handleStorageRequest(this.domainDeps, path, method, file, context); - } - /** Thin delegate — body extracted to `./domains/ui.ts` (D11③ PR-3). */ async handleUi(path: string, query: any, _context: HttpProtocolContext): Promise { return handleUiRequest(this.domainDeps, path, query, _context); diff --git a/packages/runtime/src/route-ledger.ts b/packages/runtime/src/route-ledger.ts index d195082a7b..f26d3764aa 100644 --- a/packages/runtime/src/route-ledger.ts +++ b/packages/runtime/src/route-ledger.ts @@ -139,10 +139,20 @@ export const ROUTE_LEDGER: readonly RouteLedgerEntry[] = [ note: 'raw secret returned once; user_id pinned server-side' }, // ── storage ─────────────────────────────────────────────────────────────── - { route: 'POST /storage/upload', domain: '/storage', disposition: 'server-only', - note: 'low-level compat surface; the SDK deliberately speaks the storage protocol (/upload/presigned, /upload/complete, /upload/chunked/*) that service-storage registers autonomously on any http-server — direct-to-cloud, chunked/resumable, auth-gated (#3584)' }, - { route: 'GET /storage/file/:id', domain: '/storage', disposition: 'server-only', - note: 'ditto — the SDK downloads via /storage/files/:id/url from the service-storage protocol (authorization-gated signed URLs); this route stays as the low-level redirect/stream path (#3584)' }, + // RETIRED (#4087) — deliberately no rows, and the `/storage` domain is gone + // from the registry with them. #3584 kept `POST /storage/upload` and + // `GET /storage/file/:id` as a "low-level redirect/stream compat surface" + // outside the SDK; re-verification found that surface never existed. The + // upload branch called `upload(key, data, options?)` as `upload(file, + // { request })` — a TypeError against every implementation — and the + // download branch switched on a `{ url | redirect | stream | mimeType }` + // result no implementation returns (`download(key)` resolves a Buffer). + // Two routes ledgered as a compat path, neither of which could serve one + // request. `/api/v1/storage` is service-storage's protocol, enumerated in + // `packages/services/service-storage/src/storage-route-ledger.ts` — the + // surface the SDK actually speaks. A dispatcher `/storage` row coming back + // means a bridge is being rebuilt; make it speak the contract or don't + // build it. // ── ui ──────────────────────────────────────────────────────────────────── { route: 'GET /ui/view/:object/:type?', domain: '/ui', disposition: 'sdk', client: 'meta.getView', diff --git a/packages/types/src/error-leak.ts b/packages/types/src/error-leak.ts index 5bc29f4e9f..df55b1ef02 100644 --- a/packages/types/src/error-leak.ts +++ b/packages/types/src/error-leak.ts @@ -5,7 +5,7 @@ * * ObjectStack has more than one HTTP boundary. `@objectstack/rest` guards the * REST data routes inside `mapDataError`; the dispatcher-plugin routes - * (`/analytics`, `/packages`, `/i18n`, `/storage`, `/automation`, …) exit + * (`/analytics`, `/packages`, `/i18n`, `/automation`, …) exit * through `errorResponseBase`. Before #3867 only the first of those sanitised * anything, so a driver error raised under `/analytics/query` reached the * client verbatim — a real SQL statement in the response body: diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index 711c28d211..2e6a373fa6 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -204,7 +204,10 @@ const DISPATCHER_DOMAINS = { 'notifications.ts': { handBuilt: 0 }, 'packages.ts': { handBuilt: 0 }, 'security.ts': { handBuilt: 0 }, - 'storage.ts': { handBuilt: 0 }, + // No `storage.ts` — the `/storage` domain was retired in #4087 (it called + // the storage contract with the wrong arity and read a Buffer as a + // descriptor). `/api/v1/storage` is service-storage's surface; its envelope + // is audited in that package's own routes, not here. 'ui.ts': { handBuilt: 0 }, // Kind 1 — enveloped, hand-built only because the helper cannot say it.