feat(deno-host): S3-compatible R2Bucket object storage adapter - #422
Conversation
Closes the last of the four demand-gated @dwk/deno-host gaps (#400): createS3Bucket({ client, endpoint }) maps put/get/head/delete onto the S3 REST verbs against an external S3-compatible provider, with httpMetadata and customMetadata round-tripped as Content-Type/x-amz-meta-* headers and streamed put bodies counted via a TransformStream instead of buffered. The injected S3ClientLike seam is a single fetch-shaped method (e.g. aws4fetch's AwsClient#fetch) rather than the AWS SDK's Command surface, keeping the package dependency-free. Closes #400 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011n3hX9afXK6racYWL738iG
|
The Generated by Claude Code |
…oint's trailing slash CodeQL flagged options.endpoint.replace(/\/+$/, "") as polynomial-time on a backtracking engine for a pathological all-slashes input. Replaced with a plain O(n) loop that strips the same trailing slashes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011n3hX9afXK6racYWL738iG
|
Fixed the CodeQL finding in 062bae6: Generated by Claude Code |
davidwkeith
left a comment
There was a problem hiding this comment.
Reviewed the diff (r2.ts, r2.test.ts, test-harness.ts's FakeS3Client, and the spec/README/changeset updates).
CONTRIBUTING.md conformance: looks good.
- PR opened from the template with
Summary/Packages affected/Checklistheadings intact; the one unchecked box (catalog/conformance) has a valid one-line reason (@dwk/deno-hostis alibrariesentry, not a mountable worker). - Spec (
spec/packages/deno-host.md,spec/deno-deploy-design.md) updated in the same PR alongside the code, matching "spec is the requirement" ground rule. - Colocated tests added (
r2.test.ts), covering round-trip, absent-keynulls, batch delete, stream body sizing, key encoding, andwriteHttpMetadata. - Changeset added (
minorbump for@dwk/deno-host). - PR title (
feat(deno-host): S3-compatible R2Bucket object storage adapter) matches the Conventional Commits style and precedent from #397–#399 (feat(deno-host): KV-backed durable at-least-once queue emulation, etc.). @cloudflare/workers-typesis imported type-only inr2.tswithout being declared inpackages/deno-host/package.json— but this matches the existing pattern already established ind1.tsin this same package, so it's not something this PR introduces or needs to fix.
Code observations (non-blocking):
S3Bucket#delete(r2.ts) issues oneDELETEper key sequentially inside aforloop rather than in parallel. For the batch-delete API this is a correctness no-op but a latency cost worth aPromise.allif a caller ever passes a large key batch (@dwk/storeGC sweep, e.g.).#objectBodyFromResponsealways setsbodyUsed: false, even after the returned body's.text()/.arrayBuffer()/etc. is called — a minor fidelity gap vs. realR2ObjectBody(harmless today since no@dwkconsumer inspectsbodyUsed, but worth a one-line doc note next to the other documented fidelity gaps if it's ever relied upon).
Neither blocks merge. All CI checks (build-test, integration, release-gate, CodeQL) were still in_progress as of this review — worth confirming green before merging.
Generated by Claude Code
Consolidates the five duplicated response-drain closures into one `drain()` helper, and adds tests for the previously-uncovered paths: a null put body, a Blob body, the R2ObjectBody accessor methods (arrayBuffer/json/blob/bytes), checksums.toJSON(), and drain's defensive catch when a response body fails to read. r2.ts is now at 100% function coverage, bringing the repo-wide function coverage back above the 94% gate the S3 adapter's original coverage (63.6%) had dropped it below. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011n3hX9afXK6racYWL738iG
|
Thanks for the review. Addressed both non-blocking observations, plus a real
All local gates green: lint, format:check, typecheck, build, full test suite (2776 tests), coverage. Generated by Claude Code |
davidwkeith
left a comment
There was a problem hiding this comment.
Reviewed the createS3Bucket diff (packages/deno-host/src/r2.ts + tests + docs) against the repo's architecture rules and CONTRIBUTING.md.
Code: No correctness or security issues found.
- Body draining, 404→
nullmapping, and error handling are consistent acrossput/head/get/delete. - The
ReadableStreamputpath streams through a countingTransformStreamrather than buffering, matching the non-functional "never buffer a full blob" rule (and the@dwk/cf-shimsprecedent it's explicitly modeled on). x-amz-meta-*custom-metadata lowercasing (a documented divergence from R2's case-preserving behavior) and the "not implemented" surface (list, multipart,onlyIf, range reads) are both accurately documented in the README/spec rather than silently gapped.- Trailing-slash stripping avoids a backtracking regex — reasonable, if maybe overcautious for this input size.
- Test coverage is solid: round-trip, absent-key
nulls, batch delete, stream/Blob/ArrayBuffer bodies, key percent-encoding, and a body-drain-failure case.
CONTRIBUTING.md conformance: PR title (feat(deno-host): ...) and body match the required Conventional Commits / template format. Checklist is complete; the one unchecked item ("catalog/conformance") correctly explains why it doesn't apply (library entry, no new mountable worker). Changeset present. Ran pnpm lint && pnpm format:check && pnpm build locally against this branch — all clean (CI's build-test/release-gate/integration are already green).
Not a code issue, FYI: the github-advanced-security check is failing, but the job log shows it's an infra-side failure in GitHub's own Copilot review agent (CAPIError: 400 model_not_supported for sweagent-capi:claude-opus-4.6), not a finding against this diff — safe to ignore or re-run.
No changes requested.
Generated by Claude Code
Summary
Closes the last of the four demand-gated
@dwk/deno-hostgaps (#400,tracked under #396).
createS3Bucket({ client, endpoint })presents anR2Bucket-shapedput/get/head/deletesurface (host-contract §3.4)over an external S3-compatible provider (R2 itself, MinIO, Backblaze B2,
Tigris, DigitalOcean Spaces, ...):
put/get/head/deletemap onto the S3 REST verbsPUT/GET/HEAD/DELETEagainst{endpoint}/{key}, with keyspercent-encoded per
/-delimited segment.httpMetadata.contentTyperound-trips asContent-Type;customMetadataround-trips as
x-amz-meta-*headers (lowercased on read-back — adocumented divergence from R2's case-preserving behavior, since HTTP
header names are case-insensitive).
ReadableStreamputvalue streams through a byte-countingTransformStreamrather than being buffered, so the returnedR2Object.sizeis known without reading the whole body into memoryfirst — the same no-buffering rule
@dwk/cf-shims' filesystem shimmeets by hashing while it writes.
S3ClientLikeseam is a singlefetch-shaped method(a signer such as
aws4fetch'sAwsClient#fetch, bound to theprovider's endpoint/region/credentials) rather than a structural subset
of the AWS SDK's
S3Client.send(Command)surface — that surface doesn'treduce to a small seam the way
@libsql/client's orDeno.Kv's do, andmodeling it would reintroduce an AWS SDK dependency just to type the
seam. This keeps the package dependency-free and runtime-agnostic (only
fetch/Headers/ReadableStream/TransformStream).list, multipart uploads, conditional operations (onlyIf), and rangereads are outside host-contract §3.4's required subset and are not
implemented, matching the existing
@dwk/cf-shimsprecedent.Also updates
spec/packages/deno-host.md(new "Design: R2Bucket-shapedobject storage adapter" section, live-verification items, non-goals) and
spec/deno-deploy-design.md(§3.4/§6 "Update" callouts) to record that allfour gaps this document scoped (#397–#400) are now implemented — whether an
actual deployed Deno Deploy app (Phase 1) gets built stays a separate,
still-demand-gated decision.
Closes #400
Packages affected
@dwk/deno-hostChecklist
spec/packages/and updated them ifbehaviour changed
src/*.test.ts)pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm testpnpm changeset) if this touches a publishablepackage
catalog.json/conformance/status.jsonif this adds a newmountable worker or changes conformance status — not applicable, no
new mountable worker (
@dwk/deno-hostis alibrariesentry, whosedescription I did update to mention the new object-storage shim)
Generated by Claude Code