Skip to content

Commit b9060c5

Browse files
hotlongclaude
andauthored
refactor(service-storage): 摘除 Local/S3 适配器的 list(prefix) 实现与测试 (#5541) (#6061)
* refactor(service-storage): 摘除 Local/S3 适配器的 `list(prefix)` 实现与测试 (#5541) The implementation half of the #5540 contract retirement (ADR-0049 enforce-or-remove; analysis #5266). #5983 landed the spec member and the SwappableStorageService passthrough atomically; this removes what it left: - `LocalStorageAdapter.list` (single-level readdir, directories returned as files) and `S3StorageAdapter.list` (recursive ListObjectsV2, silently truncated at 1000 objects, IsTruncated/ContinuationToken never read); - the `'list'` label in each adapter's private `track()` metrics vocabulary, which no site can produce anymore; - the tests that pinned those two dialects, plus the dead `FakeAdapter.list` in the swappable proxy's test. Zero in-repo consumers: after #5983 the only surviving references were the two producers and their own tests. The absence is held by a new runtime pin, `storage-adapter-list-retirement.test.ts`. tsc cannot hold this line -- a class may carry members its interface does not declare, which is exactly what the #5540 changeset promised adapter authors -- verified by restoring both methods: the pin goes red on both adapters while `pnpm --filter @objectstack/service-storage build` (tsup DTS, i.e. tsc) still exits 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD * chore: 空提交换头 SHA,解救楔死的 CI 重投 (#5541) attempt-3 重投(run 31118691490/31118691377/31118691509)在平台故障期间 发起,恢复后 4 小时仍无 runner,且 API 拒绝取消: 「Cannot cancel a workflow re-run that has not yet queued」——重投请求 卡死在预入队状态,平台侧不可解。同期新建 run 秒级拿 runner,证明队列 本身健康。换头 SHA 让全套 check 全新起跑。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 92e9a11 commit b9060c5

7 files changed

Lines changed: 183 additions & 73 deletions
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
---
2+
"@objectstack/service-storage": patch
3+
---
4+
5+
refactor(service-storage): drop `list(prefix)` from the local and S3 adapters — the implementation half of the #5540 contract retirement (#5541)
6+
7+
`IStorageService.list?(prefix)` was removed from the contract in `@objectstack/spec` 5.x
8+
(#5540, ADR-0049 enforce-or-remove; analysis #5266). This removes what it left behind:
9+
the two shipped adapters' own implementations, the tests that pinned them, and the
10+
`'list'` label in each adapter's metrics vocabulary.
11+
12+
**Nothing in this repository ever called them.** The only in-repo call site was the
13+
`SwappableStorageService` pass-through, deleted with the contract member in #5540. After
14+
that deletion the surviving references were the two adapter methods and their own tests —
15+
four sites, all inside `@objectstack/service-storage`, all of them producers. REST, the
16+
CLI, the storage routes, the attachment/file-reference lifecycles and the backfill
17+
tooling never called `list` on either adapter, on the swappable proxy, or on the
18+
`file-storage` service. #5172 came closest and walked away: it planned to reclaim email
19+
attachments by listing `EMAIL_ATTACHMENT_KEY_PREFIX`, found the local adapter could not
20+
see one level down, and switched to queue-driven deferred work instead.
21+
22+
**What the two implementations actually did**, which is why aligning them was rejected:
23+
24+
| Adapter | Answered `list('a')` with |
25+
| --- | --- |
26+
| `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 |
27+
| `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 |
28+
29+
One contract method, two dialects, both silently incomplete, no signal on either.
30+
31+
**Migration.** Callers holding the contract type were already migrated by #5540 — the
32+
member is gone from `IStorageService`, so `storage.list(...)` stops type-checking there.
33+
This release also removes the method from the **concrete** classes, so a caller holding a
34+
`LocalStorageAdapter` or `S3StorageAdapter` directly loses it too:
35+
36+
| Wrote | Write instead |
37+
| --- | --- |
38+
| `new LocalStorageAdapter(...).list('attachments/task/')` | query the records you wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL |
39+
| `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 |
40+
| 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 |
41+
42+
Querying your own records is not a workaround for the missing method. It is the only form
43+
that was ever correct on both backends and past 1000 objects: the bucket was never the
44+
system of record for "which files exist" — the rows are.
45+
46+
**If enumeration ever comes back, it comes back cursor-shaped.** Not this signature. A
47+
prefix listing that cannot paginate is the wrong shape to inherit, so a future
48+
first-party need returns `list(prefix, { cursor, limit })` — a page plus a continuation
49+
token — with adapter-conformance cases (nested keys, directory entries, more than 1000
50+
objects) proving both backends agree *before* either ships. Maintainer ruling 2026-08-05
51+
on #5266 chose this over aligning the two adapters, which would have grown a conformance
52+
surface nobody walks.
53+
54+
Patch rather than major: the contract break was #5540's and shipped there. `tsc` cannot
55+
see this one — a class may carry members its interface does not declare, which is exactly
56+
why the #5540 changeset told adapter authors that leaving an implementation in place
57+
still compiles — so the absence is held by a runtime pin,
58+
`storage-adapter-list-retirement.test.ts`, instead.

packages/services/service-storage/src/local-storage-adapter.metrics.test.ts

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -29,31 +29,23 @@ describe('LocalStorageAdapter instrumentation', () => {
2929
expect(durations[0]).toBeGreaterThanOrEqual(0);
3030
});
3131

32-
it('records ok for get / head / list when the object is present', async () => {
32+
it('records ok for get / head when the object is present', async () => {
3333
const metrics = new InMemoryMetricsRegistry();
3434
const storage = new LocalStorageAdapter({ rootDir, metrics });
3535
await storage.upload('a/b.txt', Buffer.from('x'));
3636
await storage.download('a/b.txt');
3737
await storage.exists('a/b.txt');
3838
await storage.getInfo('a/b.txt');
39-
await storage.list('a');
4039

4140
expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'get', result: 'ok' })).toBe(1);
4241
expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'head', result: 'ok' })).toBe(2);
43-
expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'list', result: 'ok' })).toBe(1);
4442
});
4543

46-
it('list() does not double-count head per entry', async () => {
47-
const metrics = new InMemoryMetricsRegistry();
48-
const storage = new LocalStorageAdapter({ rootDir, metrics });
49-
await storage.upload('p/a.txt', Buffer.from('x'));
50-
await storage.upload('p/b.txt', Buffer.from('y'));
51-
metrics.reset();
52-
await storage.list('p');
53-
// Exactly one list operation; no head operations from inner stats.
54-
expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'list' })).toBe(1);
55-
expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'head' })).toBe(0);
56-
});
44+
// The `list() does not double-count head per entry` case was deleted with the
45+
// method it exercised (#5541). It pinned an internal detail of the removed
46+
// implementation (inline `stat` instead of `getInfo`), so with `list` gone it
47+
// could only have stayed green by asserting nothing. `op: 'list'` is no longer
48+
// in the adapter's `track()` vocabulary either, so no sample can carry it.
5749

5850
it('records errors_total{errorClass} on path-traversal rejection', async () => {
5951
const metrics = new InMemoryMetricsRegistry();

packages/services/service-storage/src/local-storage-adapter.test.ts

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@ describe('LocalStorageAdapter', () => {
3232
expect(typeof storage.exists).toBe('function');
3333
expect(typeof storage.getInfo).toBe('function');
3434
// `list` is deliberately absent: IStorageService no longer declares it
35-
// (#5540). The adapter's own `list` implementation goes in #5541.
35+
// (#5540) and the adapter no longer implements it (#5541). The absence is
36+
// pinned in `storage-adapter-list-retirement.test.ts`.
3637
});
3738

3839
it('should upload and download a file', async () => {
@@ -74,21 +75,12 @@ describe('LocalStorageAdapter', () => {
7475
expect(info.lastModified).toBeInstanceOf(Date);
7576
});
7677

77-
it('should list files in a directory', async () => {
78-
await createTempDir();
79-
await adapter.upload('docs/a.txt', Buffer.from('a'));
80-
await adapter.upload('docs/b.txt', Buffer.from('bb'));
81-
const files = await adapter.list('docs');
82-
expect(files).toHaveLength(2);
83-
const keys = files.map(f => f.key).sort();
84-
expect(keys).toEqual(['docs/a.txt', 'docs/b.txt']);
85-
});
86-
87-
it('should return empty array when listing non-existent directory', async () => {
88-
await createTempDir();
89-
const files = await adapter.list('nonexistent');
90-
expect(files).toEqual([]);
91-
});
78+
// The `should list files in a directory` and `should return empty array when
79+
// listing non-existent directory` cases were deleted with the method they
80+
// exercised (#5541). They pinned exactly the single-level, directories-as-files
81+
// behaviour the retirement removed — `docs/a.txt` + `docs/b.txt` are both one
82+
// level down, which is the only depth that implementation could see — so
83+
// keeping them green would have meant keeping the method.
9284

9385
it('should reject path traversal', async () => {
9486
await createTempDir();

packages/services/service-storage/src/local-storage-adapter.ts

Lines changed: 12 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ export class LocalStorageAdapter implements IStorageService {
8787
* Wrap a storage operation with metrics instrumentation. Never swallows
8888
* the underlying error; instrumentation failures are silently ignored.
8989
*/
90-
private async track<T>(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise<T>): Promise<T> {
90+
private async track<T>(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise<T>): Promise<T> {
9191
const started = Date.now();
9292
const baseLabels = { adapter: 'local', op } as const;
9393
try {
@@ -189,29 +189,17 @@ export class LocalStorageAdapter implements IStorageService {
189189
});
190190
}
191191

192-
async list(prefix: string): Promise<StorageFileInfo[]> {
193-
return this.track('list', async () => {
194-
const dirPath = this.resolvePath(prefix);
195-
try {
196-
const entries = await fs.readdir(dirPath);
197-
const results: StorageFileInfo[] = [];
198-
for (const entry of entries) {
199-
if (entry.startsWith('.')) continue;
200-
const fullKey = prefix ? `${prefix}/${entry}` : entry;
201-
try {
202-
// Inline stat to avoid double-counting `head` operations.
203-
const stat = await fs.stat(this.resolvePath(fullKey));
204-
results.push({ key: fullKey, size: stat.size, lastModified: stat.mtime });
205-
} catch {
206-
/* skip */
207-
}
208-
}
209-
return results;
210-
} catch {
211-
return [];
212-
}
213-
});
214-
}
192+
// `list(prefix)` is gone (#5541), following its removal from IStorageService
193+
// (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation was
194+
// a single-level `readdir` that reported subdirectories as files, so it and the
195+
// S3 adapter's recursive-but-truncated one answered the same call differently
196+
// and neither said so. Nothing in the repo called either. Enumerate the records
197+
// you wrote (`sys_file` / file references, paginated through ObjectQL) instead
198+
// of the bucket; if a first-party caller ever needs real bucket enumeration it
199+
// returns cursor-shaped — `list(prefix, { cursor, limit })` — with
200+
// adapter-conformance cases proving both backends agree before it ships.
201+
// Absence is pinned in `storage-adapter-list-retirement.test.ts`: an excess
202+
// method on a class is not a type error, so tsc cannot hold this line.
215203

216204
// ---------------------------------------------------------------------------
217205
// Presigned URL helpers

packages/services/service-storage/src/s3-storage-adapter.ts

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ export class S3StorageAdapter implements IStorageService {
7272
* Records ok/error counters, a duration histogram, and an error counter
7373
* keyed by error class on failure. Never swallows the underlying error.
7474
*/
75-
private async track<T>(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise<T>): Promise<T> {
75+
private async track<T>(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise<T>): Promise<T> {
7676
const started = Date.now();
7777
const baseLabels = { adapter: 's3', op } as const;
7878
try {
@@ -211,19 +211,19 @@ export class S3StorageAdapter implements IStorageService {
211211
});
212212
}
213213

214-
async list(prefix: string): Promise<StorageFileInfo[]> {
215-
return this.track('list', async () => {
216-
const client = await this.getClient();
217-
const s3 = await this.s3Mod();
218-
const cmd = new s3.ListObjectsV2Command({ Bucket: this.bucket, Prefix: prefix });
219-
const res = await client.send(cmd);
220-
return (res.Contents ?? []).map((item: any) => ({
221-
key: item.Key,
222-
size: item.Size ?? 0,
223-
lastModified: item.LastModified ?? new Date(),
224-
}));
225-
});
226-
}
214+
// `list(prefix)` is gone (#5541), following its removal from IStorageService
215+
// (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation
216+
// issued one `ListObjectsV2` and read neither `IsTruncated` nor
217+
// `ContinuationToken`, so past 1000 objects it returned the first page with
218+
// nothing to distinguish it from a complete answer — while the local adapter
219+
// answered the same call one level deep. Nothing in the repo called either.
220+
// Enumerate the records you wrote (`sys_file` / file references, paginated
221+
// through ObjectQL) instead of the bucket; if a first-party caller ever needs
222+
// real bucket enumeration it returns cursor-shaped —
223+
// `list(prefix, { cursor, limit })` — with adapter-conformance cases proving
224+
// both backends agree before it ships. Absence is pinned in
225+
// `storage-adapter-list-retirement.test.ts`: an excess method on a class is
226+
// not a type error, so tsc cannot hold this line.
227227

228228
// ---------------------------------------------------------------------------
229229
// Presigned URLs
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Retirement pin — `list(prefix)` on the shipped storage adapters.
5+
*
6+
* `IStorageService.list?(prefix)` was removed from the contract in #5540
7+
* (ADR-0049 enforce-or-remove; analysis #5266), and the two shipped adapters'
8+
* own implementations were removed in #5541. This file keeps the retired
9+
* surface retired.
10+
*
11+
* **Why a runtime pin and not `@ts-expect-error`.** On the *contract* side tsc
12+
* is the enforced channel and `packages/spec/src/contracts/storage-service.test.ts`
13+
* already carries both directives — reading `storage.list` is a type error, and
14+
* an object literal typed `IStorageService` that declares `list` is an excess
15+
* property. Neither reaches an adapter: a **class** that `implements` an
16+
* interface is only checked for the members the interface requires, so a class
17+
* may carry any number of extra methods without a single type error. That is
18+
* exactly what the retirement changeset promises adapter authors ("an
19+
* implementation left in place still compiles"), and it is also why tsc cannot
20+
* notice these two coming back. The pin has to read the shape at runtime.
21+
*
22+
* `SwappableStorageService` deliberately gets no pin here: it forwards to an
23+
* `inner` typed as `IStorageService`, so a re-added `list` passthrough fails to
24+
* compile — which is how #5540 found it in the first place. tsc holds that line
25+
* already; duplicating it here would pin nothing new.
26+
*
27+
* If prefix enumeration ever comes back it comes back cursor-shaped —
28+
* `list(prefix, { cursor, limit })` returning a page plus a continuation token,
29+
* with adapter-conformance cases (nested keys, directory entries, more than
30+
* 1000 objects) proving both backends agree. Restoring the old single-argument
31+
* shape to satisfy this file is the one fix that is not a fix.
32+
*/
33+
34+
import { describe, it, expect } from 'vitest';
35+
import { LocalStorageAdapter } from './local-storage-adapter';
36+
import { S3StorageAdapter } from './s3-storage-adapter';
37+
38+
/** Every method name reachable on an instance, own + prototype chain. */
39+
function reachableMethodNames(instance: object): string[] {
40+
const names = new Set<string>();
41+
for (
42+
let cursor: object | null = instance;
43+
cursor && cursor !== Object.prototype;
44+
cursor = Object.getPrototypeOf(cursor)
45+
) {
46+
for (const name of Object.getOwnPropertyNames(cursor)) names.add(name);
47+
}
48+
return [...names];
49+
}
50+
51+
describe('storage adapters no longer implement list(prefix) (#5540 / #5541)', () => {
52+
it('LocalStorageAdapter exposes no list member', () => {
53+
const adapter = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' });
54+
55+
expect(reachableMethodNames(adapter)).not.toContain('list');
56+
expect('list' in adapter).toBe(false);
57+
expect((adapter as unknown as Record<string, unknown>).list).toBeUndefined();
58+
});
59+
60+
it('S3StorageAdapter exposes no list member', () => {
61+
// The constructor only records options; the AWS SDK is imported lazily on
62+
// first use, so this never touches the network or the peer dependency.
63+
const adapter = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' });
64+
65+
expect(reachableMethodNames(adapter)).not.toContain('list');
66+
expect('list' in adapter).toBe(false);
67+
expect((adapter as unknown as Record<string, unknown>).list).toBeUndefined();
68+
});
69+
70+
it('still exposes the per-key contract members the retirement kept', () => {
71+
// Guards the pin against the mirror failure: a pin that passes because the
72+
// adapter has no methods at all would be green and meaningless.
73+
const local = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' });
74+
const s3 = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' });
75+
76+
for (const adapter of [local, s3]) {
77+
for (const member of ['upload', 'download', 'delete', 'exists', 'getInfo'] as const) {
78+
expect(typeof (adapter as unknown as Record<string, unknown>)[member]).toBe('function');
79+
}
80+
}
81+
});
82+
});

packages/services/service-storage/src/swappable-storage-service.test.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,9 @@ class FakeAdapter implements IStorageService {
2424
if (!b) throw new Error('not found');
2525
return { key, size: b.length, lastModified: new Date(), contentType: 'application/octet-stream' };
2626
}
27-
async list(prefix: string): Promise<StorageFileInfo[]> {
28-
return Array.from(this.store.keys())
29-
.filter((k) => k.startsWith(prefix))
30-
.map((k) => ({ key: k, size: this.store.get(k)!.length, lastModified: new Date() }));
31-
}
27+
// No `list(prefix)`: the contract dropped it in #5540 and the shipped adapters
28+
// dropped their implementations in #5541, so a fake that still advertised one
29+
// would model a surface no real adapter has.
3230
}
3331

3432
/** Adapter that omits the optional methods to exercise the proxy's

0 commit comments

Comments
 (0)