Pluggable StorageBackend API + extract @adobe/helix-shared-storage-s3 - #1260
Merged
Conversation
…-shared-storage-s3 HelixStorage/Bucket previously only knew how to talk to AWS S3, with R2 support hardcoded as a second S3Client mirrored via sendToS3andR2. This refactors @adobe/helix-shared-storage onto a pluggable StorageBackend interface (StorageBackend.d.ts, AbstractStorageBackend, MirroringBackend, Bucket facade) so other backends (Azure, GCS, ...) can ship as separate packages without core depending on any cloud SDK. HelixStorage is now configured with a single backendFactory closure instead of hardcoded S3/R2 options. The existing S3/R2 implementation is extracted into a new package, @adobe/helix-shared-storage-s3, which exports S3Backend, createDefaultBackendFactory, and a HelixStorageS3 subclass that pre-wires the default backendFactory so fromContext() keeps its single-argument signature. Along the way: MirroringBackend's error tagging is fixed to identify the failing backend by identity instead of by position (previously only correct by luck for exactly 2 clients), and S3Backend.copy() correctly merges raw backend-native passthrough options (copyOpts) without letting absent common fields clobber explicit values. Resolves #1258. Existing S3/R2 consumers migrate with a one-line import change: - import { HelixStorage } from '@adobe/helix-shared-storage'; + import { HelixStorageS3 as HelixStorage } from '@adobe/helix-shared-storage-s3'; No other call site changes are required for fromContext()-based usage. Tested: full workspace test/lint suite (17 projects) passes; both touched packages retain 100% line/branch/function/statement coverage. BREAKING CHANGE: `new HelixStorage(opts)` in @adobe/helix-shared-storage no longer defaults to an S3(+R2) backend - it requires an explicit `backendFactory`, or throws. Direct constructors must pass `backendFactory: createDefaultBackendFactory(env)` from the new @adobe/helix-shared-storage-s3 package. `HelixStorage.s3()` and the `HelixStorage.AWS_S3_SYSTEM_HEADERS` static are removed from core (the latter moves, unchanged, to `HelixStorageS3.AWS_S3_SYSTEM_HEADERS` in @adobe/helix-shared-storage-s3). `Bucket.client`/`HelixStorage.bucket()`'s second argument is now a generic options bag instead of a `disableR2` boolean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
see https://github.com/adobe/helix-admin/pull/3735 for the changes required in helix-admin to migrate |
tripodsan
commented
Aug 31, 2026
tripodsan
commented
Aug 31, 2026
Contributor
|
Two things:
|
Rename per PR review feedback:
- @adobe/helix-shared-storage's `HelixStorage` -> `Storage`
- @adobe/helix-shared-storage-s3's `HelixStorageS3` -> `StorageS3`
(file renamed HelixStorageS3.js -> StorageS3.js, likewise for its test)
Also removes every .d.ts file from both packages. Nothing in this
monorepo type-checks .d.ts against anything (only helix-shared-tokencache
even ships them), so a hand-written, unenforced type surface for just
these two packages wasn't worth maintaining. Both packages are now
plain JSDoc-commented JS, matching helix-shared-async/helix-shared-string/
etc. Dropped the "types" field from both package.json files and cleaned
up now-dangling `@typedef {import('./x.d')...}`/`@implements` JSDoc tags.
Also rewrote packages/helix-shared-storage/README.md, which still
documented the pre-refactor, S3-specific constructor API (this predates
the pluggable-backend work and was missed in the original PR).
No behavioral changes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This PR will trigger a major release when merged. |
Re-adds the type structures from the .d.ts files deleted in the previous commit, as explicit @typedef blocks colocated in the .js files that own each concept, following this repo's existing JSDoc-only convention (see helix-shared-git/src/GitUrl.js). - AbstractStorageBackend.js: CommonObjectMeta, PutOptions, CopyOptions (backend-level), RemoveOptions, RemoveResult, BulkRemoveResult, BackendListOptions, and the StorageBackend interface shape itself. - MirroringBackend.js: MirroringBackendOptions. - Bucket.js: BucketOptions, RawObjectMeta; per-method @param/@returns now reference the above plus storage.js's public types. - storage.js: ObjectInfo, ObjectFilter, CopyOptions (public, Bucket-facing), BrowseOptions, ListOptions, ListResult, BulkDeleteResult, StorageOptions, StorageContext, BucketMap. - helix-shared-storage-s3/S3Backend.js: S3BackendOptions; per-method @param/@returns reference core's types via the published package specifier. - helix-shared-storage-s3/createDefaultBackendFactory.js: CreateDefaultBackendFactoryOptions. - helix-shared-storage-s3/StorageS3.js: typed fromContext() override. Still no .d.ts files and no "types" field — this is documentation via JSDoc comments only, consistent with the rest of the monorepo. No behavioral changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dominique-pfister
approved these changes
Sep 1, 2026
putMeta was previously a generic default on AbstractStorageBackend, implemented as a self-copy composed through backend.copy(). That composition needed the caller's raw opts nested under a copyOpts key to reach copy(), while Bucket's own copy()/copyDeep() flatten copyOpts at the top level — the exact mismatch fixed in the prior commit. Removing the generic default removes that leaky abstraction entirely. putMeta now joins get/head/put/copy/remove/list as a mandatory primitive every backend must implement directly: - S3Backend.putMeta() implements the self-copy CopyObjectCommand itself (restoring the exact shape of the pre-refactor Bucket.putMeta()), independent of S3Backend.copy(). - S3Backend.copy() no longer needs to special-case a nested `opts.copyOpts` (nothing produces that shape anymore), so that accommodation is removed. - AbstractStorageBackend.putMeta() is now a throwing stub, matching the other 6 mandatory primitives. - MirroringBackend is unaffected — it already fans `putMeta` out generically regardless of whether it's mandatory or defaulted. Updated tests: AbstractStorageBackend.test.js's MinimalBackend no longer needs a copy() stub just to exercise putMeta's old delegation; Bucket.test.js's FakeBackend now implements putMeta directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Previously head()/put()/copy() spread the backend's raw native SDK
response (`...raw`) directly into the returned CommonObjectMeta
object, relying on lowerCamelCase-vs-PascalCase casing to avoid
collisions with the common fields. putMeta() returned the raw
response entirely unwrapped, with no common fields at all.
Both patterns leak backend-specific shape onto the top level of every
return value. Nest the raw response under a single `raw` property
instead, on S3Backend's head()/put()/copy()/putMeta():
return { etag: raw.ETag, versionId: raw.VersionId, ..., raw };
Callers who need backend-native fields now reach for `result.raw.X`
(e.g. `result.raw.ChecksumCRC64NVME`) instead of relying on an
implicit, casing-based non-collision guarantee.
Bucket.copy()'s pre-existing `result.CopyObjectResult ?? result`
narrowing (kept for byte-for-byte backward compat with the
pre-refactor Bucket.copy() return shape) now reads
`result.raw?.CopyObjectResult ?? result.raw ?? result` to match.
CommonObjectMeta's typedef documents `raw` as an explicit property
instead of an open-ended "plus any other raw fields" note; Bucket.js's
now-redundant `RawObjectMeta` alias (previously
`CommonObjectMeta & Record<string, unknown>`) collapses to a plain
`CommonObjectMeta` re-export, since there's nothing left to intersect.
Scope: head/put/copy/putMeta only. get()'s `meta` out-param keeps its
existing, separate mechanism (selectively copying named AWS_META_HEADERS
fields), left unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
StorageBackend#putMeta(key, meta, opts) leaked S3 semantics onto the common interface: `opts` was documented as "raw, backend-native fields merged into the underlying call" only because S3 has no dedicated metadata-update API — putMeta fakes one via a self-copy CopyObjectCommand, whose input happens to accept both system properties (ContentType, ContentDisposition, ...) and custom metadata together. Azure Blob Storage has no equivalent: it's two separate calls, setHTTPHeaders() for system properties and setMetadata() for custom metadata, with no way to merge arbitrary raw fields into either. The issue's own reference design for an Azure putMeta override was simply `setMetadata(meta)` - no opts at all. putMeta is now `(key, meta)` only, on both the StorageBackend interface and Bucket#putMeta - no raw-passthrough parameter. Within S3Backend#putMeta, `meta` keys matching a small SYSTEM_META_FIELDS map (contentType, contentDisposition, contentEncoding, contentLanguage - the same 4 fields StorageS3.AWS_S3_SYSTEM_HEADERS already names) are routed onto the corresponding CopyObjectCommand system field instead of becoming custom `x-amz-meta-*` metadata; everything else is custom metadata. This fakes Azure's two-call split within S3's one self-copy call, so a future AzureBlobBackend's putMeta can do the equivalent split against its own two real calls (setHTTPHeaders + setMetadata) instead of needing a raw-opts escape hatch. Drops the ability to set arbitrary raw S3 fields (e.g. Tagging) via putMeta through the common interface - tagging isn't a metadata concept on any backend, so this is intentional, not a regression to paper over. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This public static was carried over unchanged from core's pre-refactor HelixStorage.AWS_S3_SYSTEM_HEADERS, itself dead code even before this PR (defined, never read internally). S3Backend now has a real, functionally-used equivalent (SYSTEM_META_FIELDS in S3Backend.js, driving putMeta()'s system-vs-custom-metadata split), so keeping this inert, duplicate public reference around no longer serves any purpose. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a symmetric getMeta(key) alongside putMeta(key, meta), solving the "can't safely update metadata without knowing S3-specific system fields" problem: since head() already normalizes system properties into the same common lowerCamelCase names putMeta() recognizes, getMeta() is a purely generic AbstractStorageBackend default (no backend-specific code, same pattern as metadata()/listFolders()/ browse()) that merges head()'s recognized system fields with its custom metadata into one flat, putMeta()-compatible bag: const meta = await bucket.getMeta(key); meta.foo = 'updated'; await bucket.putMeta(key, meta); This round-trips correctly without the caller ever needing to distinguish "system" from "custom" fields. For this round-trip to actually be correct, head()'s common fields and putMeta()'s recognized system fields needed to be the *same* set - previously they were two different, only partly-overlapping lists (head() had contentType/contentEncoding/cacheControl/ contentDisposition/expires; putMeta() had contentType/ contentDisposition/contentEncoding/contentLanguage). Unified into one 6-field SYSTEM_META_FIELD_NAMES list (adding contentLanguage, keeping cacheControl/expires), exported from core and used consistently by S3Backend's head()/put()/copy()/putMeta() - also letting S3Backend derive its PascalCase field-name mapping programmatically instead of hand-duplicating the list four times. MirroringBackend now proxies getMeta() to primary alongside the other read-only methods (get/head/metadata/list/listFolders/browse). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
S3Backend#get()'s optional `meta` out-param was still copying raw AWS PascalCase field names (CacheControl, ContentType, ContentEncoding, ETag, Expires, LastModified) directly onto the caller-provided object, inconsistent with head()/put()/copy()/putMeta()/getMeta(), which all use the common lowerCamelCase vocabulary (SYSTEM_META_FIELD_NAMES) established earlier in this PR. GET_META_FIELDS is now derived from the existing SYSTEM_META_FIELDS map (reversed) plus the two read-only object attributes get() has always also surfaced (etag, lastModified), so get()'s meta output uses the exact same field names as head()'s CommonObjectMeta. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…AD-derived values
S3Backend#copy() only guarded against clobbering an explicit
copyOpts.ContentType (etc.) with an *absent* common-field value
(opts.contentType === undefined). It didn't guard the other
direction: when Bucket#_buildCopyOptions() populates the common
fields from the source's HEAD (the addMetadata/renameMetadata path,
needed to preserve system properties across the self-copy
REPLACE), a *present* HEAD-derived value silently overwrote an
explicit copyOpts override for the same field, since both travel
through the same opts.contentType slot.
Reproduced: bucket.copy(src, dst, { addMetadata: {...}, copyOpts:
{ ContentType: 'application/pdf' } }) against a source whose HEAD
reports 'text/plain' actually sent 'text/plain', silently discarding
the caller's explicit override.
Fix: only apply a common-field-derived value onto the raw
CopyObjectCommand input when that raw field isn't already set (i.e.
input[key] === undefined) - an explicit copyOpts value, however it
got into opts, now always wins over anything HEAD-derived.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…efined ones, into S3 CopyObjectCommand input Bucket._buildCopyOptions() unconditionally assigned head[field] onto copyOptions even when the source's HEAD lacked that system property, leaving explicit `undefined`-valued properties on the options bag. S3Backend.copy() then spread that whole options bag into the raw CopyObjectCommand input, so the resulting input carried both the lowerCamelCase common fields (contentType, metadata, metadataDirective, ...) and their mapped PascalCase equivalents side by side. Harmless to S3 (unrecognized fields are ignored) but confusing to debug. _buildCopyOptions() now only copies a system field from HEAD when it's actually defined, and S3Backend.copy() excludes the known common field names from the raw passthrough spread before mapping them onto their PascalCase equivalent, so the command input only ever contains the backend-native fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
🎉 This PR is included in version @adobe/helix-shared-storage-v3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Contributor
Author
|
🎉 This PR is included in version @adobe/helix-shared-storage-s3-v1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1258.
@adobe/helix-shared-storageonto a pluggableStorageBackendinterface (AbstractStorageBackend,MirroringBackend, a thinBucketfacade, all JSDoc-typed).Storage(renamed fromHelixStorage) is now configured with a singlebackendFactoryclosure instead of hardcoded S3/R2 constructor options, and core has zero cloud-SDK dependency (not even transitively/dev).@adobe/helix-shared-storage-s3, exportingS3Backend,createDefaultBackendFactory, and aStorageS3subclass (renamed fromHelixStorageS3) that pre-wires the defaultbackendFactorysofromContext()keeps its familiar single-argument call shape.@adobe/helix-shared-storage-azure(a reference implementation for a second backend family) is intentionally out of scope for this PR — deferred to a follow-up, per the issue's own phased design..d.tsfiles are removed from both packages; types are now JSDoc-only (@typedef/@property), matching the convention already used by most other packages in this repo (onlyhelix-shared-tokencacheships hand-written.d.ts, and nothing in the monorepo type-checks them).Correctness fixes and design cleanups surfaced during review:
MirroringBackend's fan-out error tagging now identifies the failing backend by identity instead of array position (the oldsendToS3andR2only inferred[S3]/[R2]correctly by luck, for exactly 2 clients).putMeta()is now a backend-owned, mandatory primitive (not acopy()-based generic default), and a new symmetricgetMeta()lets callers round-trip metadata updates without needing to know which fields are backend-specific "system properties" vs. custom metadata (meta.contentTypeauto-maps onto S3'sContent-Type, for example).CommonObjectMeta-shaped value (head/put/copy/putMeta) now nests the backend's native SDK response under a dedicatedrawproperty instead of spreading it into the top level, avoiding casing collisions with the common field vocabulary.S3Backend.copy()merges raw backend-native passthrough options (copyOpts) without letting an absent or head-derived common field (e.g.contentType) clobber an explicit raw value the caller passed (e.g.copyOpts.ContentType), in either direction, and without leaking the lowerCamelCase common fields (orundefined-valued placeholders) into the actualCopyObjectCommandinput.Migration for existing S3/R2 consumers
No other call-site changes are required for
fromContext()-based usage. Consumers constructingStoragedirectly (bypassingfromContext) pass an explicitbackendFactory: createDefaultBackendFactory(env).Breaking changes (major version bump for
@adobe/helix-shared-storage)HelixStorage→Storage(and, in the new package,HelixStorageS3→StorageS3).new Storage(opts)no longer defaults to an S3(+R2) backend — it requires an explicitbackendFactory, orbucket()/the named bus accessors throw.Storage.s3()is removed from core (usebucket().clientinstead).HelixStorage.AWS_S3_SYSTEM_HEADERSis removed entirely (it was a dead static, defined but never read anywhere in this repo).bucket()/contentBus()/codeBus()/sourceBus()/mediaBus()/configBus()'s second argument is now a generic, opaque options bag (e.g.{ disableR2: true }) forwarded verbatim to the backend factory, instead of a hardcodeddisableR2boolean parameter — core no longer needs to know about R2-specific vocabulary.bucket.head(key)no longer returns the raw S3 SDK response; it returns a backend-agnosticCommonObjectMeta(lowerCamelCase fields —contentType,contentEncoding,cacheControl,contentDisposition,expires,contentLanguage,etag,versionId,contentLength,lastModified,metadata), with the original raw SDK response available under.raw.bucket.get(key, meta)'s outputmetaobject is now populated with the same lowerCamelCase common field names ashead(), instead of the raw PascalCase S3 response field names (e.g.meta.contentTypeinstead ofmeta.ContentType).bucket.putMeta(path, meta, opts)drops the thirdoptsparameter (putMeta(path, meta)); it fully replaces metadata rather than merging, and now auto-maps recognized system-property keys inmetaonto the backend's native system properties instead of writing them as literal custom metadata. Use the newgetMeta()to round-trip an existing object's metadata safely..d.tsfiles are removed; TypeScript consumers relying on the previous hand-written types lose them (JSDoc types are exported via.jssource but not surfaced as a.d.tstype root).Test plan
npx nx run-many -t test,lintpasses for all 17 workspace projects.@adobe/helix-shared-storageretains 100% line/branch/function/statement coverage with a new fake-backend-based test suite (no HTTP, no cloud SDK).@adobe/helix-shared-storage-s3retains 100% coverage; the full pre-existing ~1820-line nock-based S3/R2 integration suite was migrated and passes unmodified in behavior (aside from documented, intentional changes noted above).grep -rn "@aws-sdk\|@smithy" packages/helix-shared-storage/package.json packages/helix-shared-storage/srcreturns nothing.packages/helix-shared-storage-s3/.releaserc.cjsis a real symlink to../../.releaserc.cjs, matching every other package's convention.🤖 Generated with Claude Code