fix(storage): Allow custom R2 endpoints for jurisdictional buckets#14848
fix(storage): Allow custom R2 endpoints for jurisdictional buckets#14848remihuigen wants to merge 5 commits intotoeverything:canaryfrom
Conversation
|
|
📝 WalkthroughWalkthroughR2 storage provider now omits public Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant R2Provider
participant EndpointBuilder as "Endpoint Builder"
participant S3Provider
Test->>R2Provider: construct(accountId, bucket, jurisdiction?)
R2Provider->>EndpointBuilder: derive account host (accountId[ + ".eu" if jurisdiction==='eu'])
EndpointBuilder-->>R2Provider: computed host
R2Provider->>S3Provider: instantiate with endpoint=https://{host}/ and forcePathStyle=true
Test->>S3Provider: inspect endpointUrl -> contains bucket path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/server/src/base/storage/providers/r2.ts (1)
18-25:⚠️ Potential issue | 🟠 MajorMake
accountIdoptional inR2StorageConfigto match the new runtime contract.The constructor now accepts configs that provide only
endpoint(noaccountId), but the type still declaresaccountId: stringas required. TypeScript consumers configuring a jurisdictional bucket with justendpoint— the main use case of this PR — will hit a type error, defeating the fix. The runtimeasserton line 35-38 already enforces the "at least one" invariant, so the static type should be loosened accordingly.🔧 Proposed fix
export interface R2StorageConfig extends S3StorageConfig { - accountId: string; + accountId?: string; usePresignedURL?: { enabled: boolean; urlPrefix?: string; signKey?: string; }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/base/storage/providers/r2.ts` around lines 18 - 25, The R2StorageConfig type incorrectly requires accountId while the runtime constructor accepts configs with only endpoint; change accountId to optional in the R2StorageConfig interface so the static type matches the runtime invariant enforced by the constructor's assert (refer to R2StorageConfig and the constructor/assert that checks endpoint/accountId). Update the interface definition to make accountId?: string and leave the existing runtime assert (lines that validate endpoint or accountId) unchanged so callers can pass only endpoint without a TypeScript error.
🧹 Nitpick comments (1)
packages/backend/server/src/base/storage/__tests__/r2.spec.ts (1)
29-47: Consider an endpoint-only test case.Both tests pass
accountId; neither exercises the new "endpoint without accountId" path that this PR enables. Adding a case with onlyendpointset would (a) guard against regressions of theassertbranch, and (b) catch theR2StorageConfigtype issue flagged inr2.ts(requiredaccountId).🧪 Suggested additional test
+test('R2 provider should accept endpoint without accountId', t => { + const provider = new R2StorageProvider( + { + endpoint: 'https://test-account.eu.r2.cloudflarestorage.com', + region: 'auto', + credentials: { + accessKeyId: 'test', + secretAccessKey: 'test', + }, + }, + 'test-bucket' + ); + + t.is( + endpointOf(provider), + 'https://test-account.eu.r2.cloudflarestorage.com/test-bucket' + ); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/base/storage/__tests__/r2.spec.ts` around lines 29 - 47, Add a new unit test that covers the "endpoint without accountId" path: instantiate R2StorageProvider with a config that sets only endpoint (no accountId) plus region/credentials and the same 'test-bucket', then assert endpointOf(provider) equals the endpoint + '/test-bucket'; this will exercise the assert branch and surface the R2StorageConfig typing issue flagged in r2.ts (refer to R2StorageProvider, endpointOf, and R2StorageConfig).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/backend/server/src/base/storage/providers/r2.ts`:
- Around line 18-25: The R2StorageConfig type incorrectly requires accountId
while the runtime constructor accepts configs with only endpoint; change
accountId to optional in the R2StorageConfig interface so the static type
matches the runtime invariant enforced by the constructor's assert (refer to
R2StorageConfig and the constructor/assert that checks endpoint/accountId).
Update the interface definition to make accountId?: string and leave the
existing runtime assert (lines that validate endpoint or accountId) unchanged so
callers can pass only endpoint without a TypeScript error.
---
Nitpick comments:
In `@packages/backend/server/src/base/storage/__tests__/r2.spec.ts`:
- Around line 29-47: Add a new unit test that covers the "endpoint without
accountId" path: instantiate R2StorageProvider with a config that sets only
endpoint (no accountId) plus region/credentials and the same 'test-bucket', then
assert endpointOf(provider) equals the endpoint + '/test-bucket'; this will
exercise the assert branch and surface the R2StorageConfig typing issue flagged
in r2.ts (refer to R2StorageProvider, endpointOf, and R2StorageConfig).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bc89415-a0d7-4ab7-b8f7-30368d00e9e8
📒 Files selected for processing (3)
packages/backend/server/src/base/storage/__tests__/r2.spec.tspackages/backend/server/src/base/storage/providers/index.tspackages/backend/server/src/base/storage/providers/r2.ts
|
i prefer to add a dedicated option specifically to indicate the region but not the endpoint, e.g., |
|
@darkskygit I removed the custom endpoint override path for cloudflare-r2 and introduced a dedicated jurisdiction option instead ( I also updated the related schema/docs/tests so everything is aligned. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.docker/selfhost/schema.json (1)
490-496: Jurisdiction enum is tightly coupled to the TS type — keep both in sync.The
enum: ["eu"]here (and in the duplicated blob/copilot blocks at lines 688–693 and 1335–1340) mirrors thejurisdiction?: 'eu'type inR2StorageProvider. Cloudflare R2 also exposes afedrampjurisdiction; if/when that is added toR2StorageConfig, remember to extend all three schema occurrences as well. No action required now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.docker/selfhost/schema.json around lines 490 - 496, The schema's "jurisdiction" enum is duplicated in three places and must be kept in sync with the TypeScript type R2StorageProvider / R2StorageConfig; when adding a new jurisdiction value (e.g., "fedramp") to the TS type, update the "enum" arrays in all three schema occurrences (the primary jurisdiction block and the duplicated blob/copilot blocks) so they include the new value; ensure the enum strings exactly match the TS literal values.packages/backend/server/src/base/storage/__tests__/r2.spec.ts (2)
10-47: Consider covering theassert(config.accountId, ...)path.The constructor in
r2.tsassertsaccountIdis required, but there's no test for that failure mode, nor for the non-EU default path explicitly distinguishing "no jurisdiction field" from "jurisdiction: undefined". A singlet.throws(() => new R2StorageProvider({ ... } as any, 'b'))case would lock in the current contract cheaply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/base/storage/__tests__/r2.spec.ts` around lines 10 - 47, Add a test that asserts the constructor's required accountId path by calling new R2StorageProvider with a config lacking accountId (e.g., { region: 'auto', credentials: {...} } as any) and expect it to throw (use t.throws(() => new R2StorageProvider(..., 'b'))); also add a short test that passes jurisdiction: undefined explicitly to R2StorageProvider and assert endpointOf(...) matches the non-EU default to distinguish "no jurisdiction field" from "jurisdiction: undefined".
5-8:endpointOfhelper couples tests to internal S3 Compat client shape.The test accesses
client.endpoint(a private field of the S3Compat instance) via anunknowncast, relying on undocumented internal structure. The endpoint is correctly composed with the bucket viabuildEndpoint()in the s3-compat library, so the test assertions are accurate. However, this tight coupling to the client's internal shape means the test will break silently if the s3-compat implementation changes (e.g., ifendpointis removed or renamed).Consider exposing a protected getter on
S3StorageProvider(e.g.,get endpointUrl()) and asserting against that instead — it decouples the test from library internals while maintaining the same test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/base/storage/__tests__/r2.spec.ts` around lines 5 - 8, The test helper endpointOf currently reaches into the private S3Compat shape via a cast; add a protected (or public for test access) getter on S3StorageProvider named endpointUrl that returns the composed endpoint URL string (delegating to the provider's existing endpoint-building logic), then update endpointOf to call provider.endpointUrl (or cast to S3StorageProvider and call get endpointUrl) instead of accessing client.endpoint; this removes dependency on s3-compat internals while preserving existing assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.docker/selfhost/schema.json:
- Around line 490-496: The schema's "jurisdiction" enum is duplicated in three
places and must be kept in sync with the TypeScript type R2StorageProvider /
R2StorageConfig; when adding a new jurisdiction value (e.g., "fedramp") to the
TS type, update the "enum" arrays in all three schema occurrences (the primary
jurisdiction block and the duplicated blob/copilot blocks) so they include the
new value; ensure the enum strings exactly match the TS literal values.
In `@packages/backend/server/src/base/storage/__tests__/r2.spec.ts`:
- Around line 10-47: Add a test that asserts the constructor's required
accountId path by calling new R2StorageProvider with a config lacking accountId
(e.g., { region: 'auto', credentials: {...} } as any) and expect it to throw
(use t.throws(() => new R2StorageProvider(..., 'b'))); also add a short test
that passes jurisdiction: undefined explicitly to R2StorageProvider and assert
endpointOf(...) matches the non-EU default to distinguish "no jurisdiction
field" from "jurisdiction: undefined".
- Around line 5-8: The test helper endpointOf currently reaches into the private
S3Compat shape via a cast; add a protected (or public for test access) getter on
S3StorageProvider named endpointUrl that returns the composed endpoint URL
string (delegating to the provider's existing endpoint-building logic), then
update endpointOf to call provider.endpointUrl (or cast to S3StorageProvider and
call get endpointUrl) instead of accessing client.endpoint; this removes
dependency on s3-compat internals while preserving existing assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf2e587e-af98-4fa2-a203-ad9bdb705ebb
📒 Files selected for processing (4)
.docker/selfhost/schema.jsonpackages/backend/server/src/base/storage/__tests__/r2.spec.tspackages/backend/server/src/base/storage/providers/index.tspackages/backend/server/src/base/storage/providers/r2.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/backend/server/src/base/storage/providers/r2.ts
- packages/backend/server/src/base/storage/providers/index.ts
… handling and decoupling tests from S3 internals
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/backend/server/src/base/storage/providers/index.ts (1)
96-105: FilterforcePathStyleout of the R2 schema too.
R2StorageConfigomits bothendpointandforcePathStyle, andR2StorageProvideralways forcesforcePathStyle: true. This helper only removesendpoint, so thecloudflare-r2JSON schema still advertises a no-op/unsupported option.Suggested schema alignment
-const S3ConfigPropertiesWithoutEndpoint = Object.fromEntries( +const R2ConfigPropertiesFromS3 = Object.fromEntries( Object.entries( ( S3ConfigSchema as { type: 'object'; properties?: Record<string, JSONSchema>; } ).properties ?? {} - ).filter(([key]) => key !== 'endpoint') + ).filter(([key]) => key !== 'endpoint' && key !== 'forcePathStyle') ) as Record<string, JSONSchema>;- ...S3ConfigPropertiesWithoutEndpoint, + ...R2ConfigPropertiesFromS3,Also applies to: 152-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/base/storage/providers/index.ts` around lines 96 - 105, The helper S3ConfigPropertiesWithoutEndpoint only filters out 'endpoint' from S3ConfigSchema but must also exclude 'forcePathStyle' so the R2StorageConfig/R2StorageProvider schema does not expose the unsupported option; update the filter used when building S3ConfigPropertiesWithoutEndpoint (and the other analogous helper later in the file) to remove both 'endpoint' and 'forcePathStyle' (e.g., change the filter predicate to exclude keys === 'endpoint' || keys === 'forcePathStyle') so the cloudflare-r2 JSON schema matches R2StorageConfig and R2StorageProvider behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/backend/server/src/base/storage/providers/index.ts`:
- Around line 96-105: The helper S3ConfigPropertiesWithoutEndpoint only filters
out 'endpoint' from S3ConfigSchema but must also exclude 'forcePathStyle' so the
R2StorageConfig/R2StorageProvider schema does not expose the unsupported option;
update the filter used when building S3ConfigPropertiesWithoutEndpoint (and the
other analogous helper later in the file) to remove both 'endpoint' and
'forcePathStyle' (e.g., change the filter predicate to exclude keys ===
'endpoint' || keys === 'forcePathStyle') so the cloudflare-r2 JSON schema
matches R2StorageConfig and R2StorageProvider behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20d73c27-52ce-4a78-b792-b0a3f59af15a
📒 Files selected for processing (4)
packages/backend/server/src/base/storage/__tests__/r2.spec.tspackages/backend/server/src/base/storage/providers/index.tspackages/backend/server/src/base/storage/providers/r2.tspackages/backend/server/src/base/storage/providers/s3.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/server/src/base/storage/providers/r2.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #14848 +/- ##
==========================================
- Coverage 57.41% 56.95% -0.46%
==========================================
Files 3125 3125
Lines 169161 169231 +70
Branches 25032 24912 -120
==========================================
- Hits 97120 96387 -733
- Misses 68667 69577 +910
+ Partials 3374 3267 -107
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
darkskygit
force-pushed
the
canary
branch
5 times, most recently
from
3403237 to
78a9942
Compare
3 participants
Summary
This PR fixes
cloudflare-r2storage configuration so jurisdictional R2 endpoints (for example EU buckets) work correctly.Closes #14847
Problem
cloudflare-r2currently ignoresconfig.endpointand always uses:https://<accountId>.r2.cloudflarestorage.comThat breaks uploads for jurisdictional buckets that require endpoints like:
https://<accountId>.eu.r2.cloudflarestorage.comChanges
R2StorageProviderendpoint resolution:config.endpointwhen providedhttps://${accountId}.r2.cloudflarestorage.comforcePathStyle: truebehavior unchangedaccountIdorendpointBackward Compatibility
accountIdcontinue to work exactly as before.config.endpointis explicitly set.Tests
packages/backend/server/src/base/storage/__tests__/r2.spec.tsDisclaimer: parts of this PR were implemented with AI assistance.
Summary by CodeRabbit
New Features
Documentation
Tests