diff --git a/.changeset/storage-adapter-list-implementations-removed.md b/.changeset/storage-adapter-list-implementations-removed.md new file mode 100644 index 0000000000..9fce433c92 --- /dev/null +++ b/.changeset/storage-adapter-list-implementations-removed.md @@ -0,0 +1,58 @@ +--- +"@objectstack/service-storage": patch +--- + +refactor(service-storage): drop `list(prefix)` from the local and S3 adapters — the implementation half of the #5540 contract retirement (#5541) + +`IStorageService.list?(prefix)` was removed from the contract in `@objectstack/spec` 5.x +(#5540, ADR-0049 enforce-or-remove; analysis #5266). This removes what it left behind: +the two shipped adapters' own implementations, the tests that pinned them, and the +`'list'` label in each adapter's metrics vocabulary. + +**Nothing in this repository ever called them.** The only in-repo call site was the +`SwappableStorageService` pass-through, deleted with the contract member in #5540. After +that deletion the surviving references were the two adapter methods and their own tests — +four sites, all inside `@objectstack/service-storage`, all of them producers. REST, the +CLI, the storage routes, the attachment/file-reference lifecycles and the backfill +tooling never called `list` on either adapter, on the swappable proxy, or on the +`file-storage` service. #5172 came closest and walked away: it planned to reclaim email +attachments by listing `EMAIL_ATTACHMENT_KEY_PREFIX`, found the local adapter could not +see one level down, and switched to queue-driven deferred work instead. + +**What the two implementations actually did**, which is why aligning them was rejected: + +| Adapter | Answered `list('a')` with | +| --- | --- | +| `LocalStorageAdapter` | one level of `readdir` — a nested key `a/b/c` was invisible, you got `a/b` — and every subdirectory `stat` succeeded on was returned as if it were a file, so `size` was a directory inode and `download()` could not fetch it | +| `S3StorageAdapter` | a recursive `ListObjectsV2` that read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the "all files under this prefix" you got was the first page, indistinguishable from a complete answer | + +One contract method, two dialects, both silently incomplete, no signal on either. + +**Migration.** Callers holding the contract type were already migrated by #5540 — the +member is gone from `IStorageService`, so `storage.list(...)` stops type-checking there. +This release also removes the method from the **concrete** classes, so a caller holding a +`LocalStorageAdapter` or `S3StorageAdapter` directly loses it too: + +| Wrote | Write instead | +| --- | --- | +| `new LocalStorageAdapter(...).list('attachments/task/')` | query the records you wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL | +| `new S3StorageAdapter(...).list(prefix)` | same; for a genuine bucket sweep, call `ListObjectsV2` through the AWS SDK yourself and handle `ContinuationToken`, which is the part the adapter never did | +| a custom adapter of your own with `list?(prefix)` | nothing breaks — an extra method on a class is not a type error; delete it whenever it suits you | + +Querying your own records is not a workaround for the missing method. It is the only form +that was ever correct on both backends and past 1000 objects: the bucket was never the +system of record for "which files exist" — the rows are. + +**If enumeration ever comes back, it comes back cursor-shaped.** Not this signature. A +prefix listing that cannot paginate is the wrong shape to inherit, so a future +first-party need returns `list(prefix, { cursor, limit })` — a page plus a continuation +token — with adapter-conformance cases (nested keys, directory entries, more than 1000 +objects) proving both backends agree *before* either ships. Maintainer ruling 2026-08-05 +on #5266 chose this over aligning the two adapters, which would have grown a conformance +surface nobody walks. + +Patch rather than major: the contract break was #5540's and shipped there. `tsc` cannot +see this one — a class may carry members its interface does not declare, which is exactly +why the #5540 changeset told adapter authors that leaving an implementation in place +still compiles — so the absence is held by a runtime pin, +`storage-adapter-list-retirement.test.ts`, instead. diff --git a/packages/services/service-storage/src/local-storage-adapter.metrics.test.ts b/packages/services/service-storage/src/local-storage-adapter.metrics.test.ts index 383823eda0..15cae46d03 100644 --- a/packages/services/service-storage/src/local-storage-adapter.metrics.test.ts +++ b/packages/services/service-storage/src/local-storage-adapter.metrics.test.ts @@ -29,31 +29,23 @@ describe('LocalStorageAdapter instrumentation', () => { expect(durations[0]).toBeGreaterThanOrEqual(0); }); - it('records ok for get / head / list when the object is present', async () => { + it('records ok for get / head when the object is present', async () => { const metrics = new InMemoryMetricsRegistry(); const storage = new LocalStorageAdapter({ rootDir, metrics }); await storage.upload('a/b.txt', Buffer.from('x')); await storage.download('a/b.txt'); await storage.exists('a/b.txt'); await storage.getInfo('a/b.txt'); - await storage.list('a'); expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'get', result: 'ok' })).toBe(1); expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'head', result: 'ok' })).toBe(2); - expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'list', result: 'ok' })).toBe(1); }); - it('list() does not double-count head per entry', async () => { - const metrics = new InMemoryMetricsRegistry(); - const storage = new LocalStorageAdapter({ rootDir, metrics }); - await storage.upload('p/a.txt', Buffer.from('x')); - await storage.upload('p/b.txt', Buffer.from('y')); - metrics.reset(); - await storage.list('p'); - // Exactly one list operation; no head operations from inner stats. - expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'list' })).toBe(1); - expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'head' })).toBe(0); - }); + // The `list() does not double-count head per entry` case was deleted with the + // method it exercised (#5541). It pinned an internal detail of the removed + // implementation (inline `stat` instead of `getInfo`), so with `list` gone it + // could only have stayed green by asserting nothing. `op: 'list'` is no longer + // in the adapter's `track()` vocabulary either, so no sample can carry it. it('records errors_total{errorClass} on path-traversal rejection', async () => { const metrics = new InMemoryMetricsRegistry(); diff --git a/packages/services/service-storage/src/local-storage-adapter.test.ts b/packages/services/service-storage/src/local-storage-adapter.test.ts index ef6678665b..2c4bca84a0 100644 --- a/packages/services/service-storage/src/local-storage-adapter.test.ts +++ b/packages/services/service-storage/src/local-storage-adapter.test.ts @@ -32,7 +32,8 @@ describe('LocalStorageAdapter', () => { expect(typeof storage.exists).toBe('function'); expect(typeof storage.getInfo).toBe('function'); // `list` is deliberately absent: IStorageService no longer declares it - // (#5540). The adapter's own `list` implementation goes in #5541. + // (#5540) and the adapter no longer implements it (#5541). The absence is + // pinned in `storage-adapter-list-retirement.test.ts`. }); it('should upload and download a file', async () => { @@ -74,21 +75,12 @@ describe('LocalStorageAdapter', () => { expect(info.lastModified).toBeInstanceOf(Date); }); - it('should list files in a directory', async () => { - await createTempDir(); - await adapter.upload('docs/a.txt', Buffer.from('a')); - await adapter.upload('docs/b.txt', Buffer.from('bb')); - const files = await adapter.list('docs'); - expect(files).toHaveLength(2); - const keys = files.map(f => f.key).sort(); - expect(keys).toEqual(['docs/a.txt', 'docs/b.txt']); - }); - - it('should return empty array when listing non-existent directory', async () => { - await createTempDir(); - const files = await adapter.list('nonexistent'); - expect(files).toEqual([]); - }); + // The `should list files in a directory` and `should return empty array when + // listing non-existent directory` cases were deleted with the method they + // exercised (#5541). They pinned exactly the single-level, directories-as-files + // behaviour the retirement removed — `docs/a.txt` + `docs/b.txt` are both one + // level down, which is the only depth that implementation could see — so + // keeping them green would have meant keeping the method. it('should reject path traversal', async () => { await createTempDir(); diff --git a/packages/services/service-storage/src/local-storage-adapter.ts b/packages/services/service-storage/src/local-storage-adapter.ts index 9b75168555..91b7f21865 100644 --- a/packages/services/service-storage/src/local-storage-adapter.ts +++ b/packages/services/service-storage/src/local-storage-adapter.ts @@ -87,7 +87,7 @@ export class LocalStorageAdapter implements IStorageService { * Wrap a storage operation with metrics instrumentation. Never swallows * the underlying error; instrumentation failures are silently ignored. */ - private async track(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise): Promise { + private async track(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise): Promise { const started = Date.now(); const baseLabels = { adapter: 'local', op } as const; try { @@ -189,29 +189,17 @@ export class LocalStorageAdapter implements IStorageService { }); } - async list(prefix: string): Promise { - return this.track('list', async () => { - const dirPath = this.resolvePath(prefix); - try { - const entries = await fs.readdir(dirPath); - const results: StorageFileInfo[] = []; - for (const entry of entries) { - if (entry.startsWith('.')) continue; - const fullKey = prefix ? `${prefix}/${entry}` : entry; - try { - // Inline stat to avoid double-counting `head` operations. - const stat = await fs.stat(this.resolvePath(fullKey)); - results.push({ key: fullKey, size: stat.size, lastModified: stat.mtime }); - } catch { - /* skip */ - } - } - return results; - } catch { - return []; - } - }); - } + // `list(prefix)` is gone (#5541), following its removal from IStorageService + // (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation was + // a single-level `readdir` that reported subdirectories as files, so it and the + // S3 adapter's recursive-but-truncated one answered the same call differently + // and neither said so. Nothing in the repo called either. Enumerate the records + // you wrote (`sys_file` / file references, paginated through ObjectQL) instead + // of the bucket; if a first-party caller ever needs real bucket enumeration it + // returns cursor-shaped — `list(prefix, { cursor, limit })` — with + // adapter-conformance cases proving both backends agree before it ships. + // Absence is pinned in `storage-adapter-list-retirement.test.ts`: an excess + // method on a class is not a type error, so tsc cannot hold this line. // --------------------------------------------------------------------------- // Presigned URL helpers diff --git a/packages/services/service-storage/src/s3-storage-adapter.ts b/packages/services/service-storage/src/s3-storage-adapter.ts index 29d5fcd221..97e98e9a06 100644 --- a/packages/services/service-storage/src/s3-storage-adapter.ts +++ b/packages/services/service-storage/src/s3-storage-adapter.ts @@ -72,7 +72,7 @@ export class S3StorageAdapter implements IStorageService { * Records ok/error counters, a duration histogram, and an error counter * keyed by error class on failure. Never swallows the underlying error. */ - private async track(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise): Promise { + private async track(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise): Promise { const started = Date.now(); const baseLabels = { adapter: 's3', op } as const; try { @@ -211,19 +211,19 @@ export class S3StorageAdapter implements IStorageService { }); } - async list(prefix: string): Promise { - return this.track('list', async () => { - const client = await this.getClient(); - const s3 = await this.s3Mod(); - const cmd = new s3.ListObjectsV2Command({ Bucket: this.bucket, Prefix: prefix }); - const res = await client.send(cmd); - return (res.Contents ?? []).map((item: any) => ({ - key: item.Key, - size: item.Size ?? 0, - lastModified: item.LastModified ?? new Date(), - })); - }); - } + // `list(prefix)` is gone (#5541), following its removal from IStorageService + // (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation + // issued one `ListObjectsV2` and read neither `IsTruncated` nor + // `ContinuationToken`, so past 1000 objects it returned the first page with + // nothing to distinguish it from a complete answer — while the local adapter + // answered the same call one level deep. Nothing in the repo called either. + // Enumerate the records you wrote (`sys_file` / file references, paginated + // through ObjectQL) instead of the bucket; if a first-party caller ever needs + // real bucket enumeration it returns cursor-shaped — + // `list(prefix, { cursor, limit })` — with adapter-conformance cases proving + // both backends agree before it ships. Absence is pinned in + // `storage-adapter-list-retirement.test.ts`: an excess method on a class is + // not a type error, so tsc cannot hold this line. // --------------------------------------------------------------------------- // Presigned URLs diff --git a/packages/services/service-storage/src/storage-adapter-list-retirement.test.ts b/packages/services/service-storage/src/storage-adapter-list-retirement.test.ts new file mode 100644 index 0000000000..9501210898 --- /dev/null +++ b/packages/services/service-storage/src/storage-adapter-list-retirement.test.ts @@ -0,0 +1,82 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Retirement pin — `list(prefix)` on the shipped storage adapters. + * + * `IStorageService.list?(prefix)` was removed from the contract in #5540 + * (ADR-0049 enforce-or-remove; analysis #5266), and the two shipped adapters' + * own implementations were removed in #5541. This file keeps the retired + * surface retired. + * + * **Why a runtime pin and not `@ts-expect-error`.** On the *contract* side tsc + * is the enforced channel and `packages/spec/src/contracts/storage-service.test.ts` + * already carries both directives — reading `storage.list` is a type error, and + * an object literal typed `IStorageService` that declares `list` is an excess + * property. Neither reaches an adapter: a **class** that `implements` an + * interface is only checked for the members the interface requires, so a class + * may carry any number of extra methods without a single type error. That is + * exactly what the retirement changeset promises adapter authors ("an + * implementation left in place still compiles"), and it is also why tsc cannot + * notice these two coming back. The pin has to read the shape at runtime. + * + * `SwappableStorageService` deliberately gets no pin here: it forwards to an + * `inner` typed as `IStorageService`, so a re-added `list` passthrough fails to + * compile — which is how #5540 found it in the first place. tsc holds that line + * already; duplicating it here would pin nothing new. + * + * If prefix enumeration ever comes back it comes back cursor-shaped — + * `list(prefix, { cursor, limit })` returning a page plus a continuation token, + * with adapter-conformance cases (nested keys, directory entries, more than + * 1000 objects) proving both backends agree. Restoring the old single-argument + * shape to satisfy this file is the one fix that is not a fix. + */ + +import { describe, it, expect } from 'vitest'; +import { LocalStorageAdapter } from './local-storage-adapter'; +import { S3StorageAdapter } from './s3-storage-adapter'; + +/** Every method name reachable on an instance, own + prototype chain. */ +function reachableMethodNames(instance: object): string[] { + const names = new Set(); + for ( + let cursor: object | null = instance; + cursor && cursor !== Object.prototype; + cursor = Object.getPrototypeOf(cursor) + ) { + for (const name of Object.getOwnPropertyNames(cursor)) names.add(name); + } + return [...names]; +} + +describe('storage adapters no longer implement list(prefix) (#5540 / #5541)', () => { + it('LocalStorageAdapter exposes no list member', () => { + const adapter = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' }); + + expect(reachableMethodNames(adapter)).not.toContain('list'); + expect('list' in adapter).toBe(false); + expect((adapter as unknown as Record).list).toBeUndefined(); + }); + + it('S3StorageAdapter exposes no list member', () => { + // The constructor only records options; the AWS SDK is imported lazily on + // first use, so this never touches the network or the peer dependency. + const adapter = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' }); + + expect(reachableMethodNames(adapter)).not.toContain('list'); + expect('list' in adapter).toBe(false); + expect((adapter as unknown as Record).list).toBeUndefined(); + }); + + it('still exposes the per-key contract members the retirement kept', () => { + // Guards the pin against the mirror failure: a pin that passes because the + // adapter has no methods at all would be green and meaningless. + const local = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' }); + const s3 = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' }); + + for (const adapter of [local, s3]) { + for (const member of ['upload', 'download', 'delete', 'exists', 'getInfo'] as const) { + expect(typeof (adapter as unknown as Record)[member]).toBe('function'); + } + } + }); +}); diff --git a/packages/services/service-storage/src/swappable-storage-service.test.ts b/packages/services/service-storage/src/swappable-storage-service.test.ts index cf3d52c776..04de3e706a 100644 --- a/packages/services/service-storage/src/swappable-storage-service.test.ts +++ b/packages/services/service-storage/src/swappable-storage-service.test.ts @@ -24,11 +24,9 @@ class FakeAdapter implements IStorageService { if (!b) throw new Error('not found'); return { key, size: b.length, lastModified: new Date(), contentType: 'application/octet-stream' }; } - async list(prefix: string): Promise { - return Array.from(this.store.keys()) - .filter((k) => k.startsWith(prefix)) - .map((k) => ({ key: k, size: this.store.get(k)!.length, lastModified: new Date() })); - } + // No `list(prefix)`: the contract dropped it in #5540 and the shipped adapters + // dropped their implementations in #5541, so a fake that still advertised one + // would model a surface no real adapter has. } /** Adapter that omits the optional methods to exercise the proxy's