refactor: put object storage behind a swappable StorageAdapter seam

🟢 open · #403 · open-legal-products/mike ← open-legal-products/mike · opened 10d ago by amal66 · self · +487-213 across 4 files · ↗ on GitHub

From the PR description

Summary

Splits backend/src/lib/storage.ts into policy and transport so the object-storage backend can be swapped in one new file plus one boot-time call - without editing any shipped file. The default S3 adapter (R2 in production, RustFS locally, MinIO in CI) is the existing code moved verbatim; every public export keeps its exact signature and semantics, so zero of the ~66 call sites change.

Context: a community request for pluggable backends (Azure at work, lighter stacks at home). The storage module was the clearest fork-forcer: the abstraction shape was already right - six exported functions, S3-protocol-neutral - but the S3Client was constructed inline in the shared module, so a Blob/GCS/disk port had nowhere to plug in and had to edit storage.ts itself, turning every upstream change into a merge conflict.

Storage seam demo

Live A/B: on main, setStorageAdapter doesn't exist - swapping backends means forking. On this branch, a one-file Azure-Blob-style adapter is injected and upload/download/list/signed-URL all flow through it via the unchanged facade.

What changed

  • backend/src/lib/storage/adapter.ts (new) - the StorageAdapter type: enabled, configurationHint, and five transport methods (upload, download, list, delete, signed URL). getSignedUrl receives a fully built Content-Disposition header value, because S3 (response-content-disposition) and Azure SAS (rscd) echo the same format - the facade builds it once, transports just forward it.
  • backend/src/lib/storage/s3.ts (new) - the existing R2/S3 code moved verbatim: same lazy client cache, same R2_* env vars, same pagination loop.
  • backend/src/lib/storage.ts - now the facade. Owns the policy every backend shares: not-configured degradation (reads return null/empty, writes throw), error logging (exact log strings preserved), Content-Disposition building, and the storage-key layout. Exports setStorageAdapter() and keeps storageEnabled as a live let binding so existing if (!storageEnabled) checks in callers observe a swap.
  • backend/src/lib/__tests__/storageAdapter.test.ts (new) - injects a fake adapter and locks the seam contract: delegation, live storageEnabled updates, disposition building, shared degradation policy, error-swallowing on download/signing.

Why

The DB/auth layer aside, storage was the one subsystem where "Azure port" genuinely meant "fork": ~66 call sites all go through six functions, but the transport had no indirection point. This converts that fork axis into an extension point at the cost of ~40 lines of interface. An Azure Blob adapter becomes a ~150-line file using @azure/storage-blob, registered at boot - no upstream file edited.

Reproduce the base behavior on main

  1. git checkout main && npm ci --prefix backend && npm run build --prefix backend
  2. node -e "console.log(typeof require('./backend/dist/lib/storage.js').setStorageAdapter)"undefined. There is no seam; targeting a non-S3 store requires editing backend/src/lib/storage.ts (the inline new S3Client(...)).

Verify on this branch

  1. Same setup on this branch; the same command prints function.
  2. npm test --prefix backend -- src/lib/__tests__/storageAdapter.test.ts src/lib/__tests__/storage.test.ts src/lib/__tests__/storageErrors.test.ts → 3 files pass. The two pre-existing storage test files pass untouched - they pin the degradation semantics and log format the refactor had to preserve.
  3. Behavior-swap check (what the GIF records): require the compiled facade, call setStorageAdapter() with a small in-memory adapter, then drive uploadFile/downloadFile/listFiles/getSignedUrl - all operations flow through the injected backend, storageEnabled reflects it live, and the signed URL carries the facade-built Content-Disposition.
  4. S3 path unchanged: the normal compose stack (RustFS) and CI (MinIO) exercise the default adapter with identical env vars and behavior.

Tradeoffs & design decisions

  • Programmatic seam, no STORAGE_PROVIDER env switch. With exactly one in-tree backend, a config switch would be dead code; selection-by-config can land together with a second in-tree adapter (e.g. Azure Blob). Until then, forks/embedders call setStorageAdapter() from their own boot file.
  • R2_* env names kept. Renaming to STORAGE_* (with back-compat) would be a breaking-ish config change and belongs in its own PR; the names are cosmetic, not architectural.
  • storageEnabled stays an exported boolean, now a live let binding. Keeping the export shape avoids touching its 12 importers; ES-module imports observe the post-swap value. The documented contract is to swap adapters at boot, before the first request.
  • Disabled-write error message is now adapter-supplied (configurationHint), so a swapped backend names its missing configuration. For the default S3 adapter the message is byte-identical to main's.
  • Storage still isn't validated at boot (missing R2 vars silently degrade to storageEnabled=false + runtime 503s, same as main). Tightening that changes operator-facing behavior and is deliberately out of scope.

Testing performed

  • npm test --prefix backend - full suite, 846 passed / 25 skipped (includes the 3 storage test files, 44 tests)
  • npm run build --prefix backend (tsc clean)
  • Live A/B demo against compiled builds of main and this branch (GIF above)
  • git diff --check clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01BpdjmF3BsrGfjVX6qEx2G2

Our analysis

Add a pluggable storage adapter seam — read the full analysis →

Think the analysis missed something the PR description covers?

Capture this PR into my fork

Download a Markdown prompt that tells Claude how to port every commit in this PR into your working tree. Run it via claude -p < capture-pull-403.md from inside the repo you want the changes in.

⬇ Download capture-pull-403.md