Repository navigation
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Carolx715
changed the title
751c129 to
c46f217
Compare
Carolx715
changed the title
|
I have read the CLA Document and I hereby sign the CLA |
Carolx715
changed the title
jp4a50
left a comment
jp4a50
left a comment
There was a problem hiding this comment.
Flushing review. Looked at the tests so far. Looking good!
Will come back and review the C++ later.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7092 +/- ##
==========================================
- Coverage 38.47% 38.42% -0.06%
==========================================
Files 862 865 +3
Lines 267336 270066 +2730
Branches 25184 25395 +211
==========================================
+ Hits 102868 103782 +914
- Misses 150393 152166 +1773
- Partials 14075 14118 +43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b7819ac to
a807b44
Compare
60d034e to
2d7e211
Compare
66e7ee2 to
94d9e94
Compare
The JSRPC methods on R2Bucket and R2MultipartUpload no longer create R2 user trace spans or set request/response tags. The HTTP-backed methods keep their spans unchanged. The JSRPC spans could not report cloudflare.r2.response.success or the R2 error tags, because the gateway's RPC results do not carry the backend status. Rather than emit spans that diverge from the HTTP transport, drop them until the RPC protocol can support parity. Drop addHttpMetadataRequestSpanTags and the TraceContext parameter of makeSerializableConditional, which only the JSRPC methods used. r2-jsrpc-test no longer attaches the instrumentation tail worker, and r2-instrumentation-test compares the HTTP transport's spans exactly.
|
|
||
| jsg::Ref<R2Bucket::HeadResult> headResultFromBackend( | ||
| jsg::Lock& js, R2Bucket::HeadBackendResult result, MissingMetadataPolicy policy) { | ||
| KJ_SWITCH_ONEOF(result) { |
There was a problem hiding this comment.
HeadBackendResult, GetBackendResult and ListBackendResult make the named JSRPC methods accept both R2 API-typed results and alternative object shapes that we supported before. I don't think we need the latter anymore.
I can see that this has probably been done to continue to support the test variants that manually construct return values directly (e.g. R2JsrpcResponseEntrypoint) rather than return the result of an R2 binding API call (ObservedR2Binding), but we don't need to support the former for the initial custom bindings milestone - we only need to support the "interceptor" custom bindings use case (tested by ObservedR2Binding).
SerializableHeadResult etc. can remain internal serialization payload types, but don't need to be accepted by the client as return types. You should just be able to replace HeadBackendResult's OneOf with the first variant: jsg::Ref<HeadResult>.
Could we also check in a TypeScript definition of the expected server-side interface, along the lines of D1’s D1BindingService and type-check an interceptor test implementation against it?
I'd suggest pulling out the test implementation of it (ObservedR2Binding) into a separate test-only TS file from which the tests can import the type where ObservedR2Binding is defined something like:
interface Env {
REAL_BUCKET: R2Bucket;
}
export class ObservedR2Binding
extends WorkerEntrypoint<Env>
implements R2BindingService
{
head(key: string): Promise<R2Object | null> {
return this.env.REAL_BUCKET.head(key);
}
async createMultipartUpload(
key: string,
options?: R2MultipartOptions,
): Promise<{ uploadId: string }>; {
const upload = await this.env.REAL_BUCKET.createMultipartUpload(key, options);
return { uploadId: upload.uploadId };
}
// Implement and type-check the remaining service methods here.
}
Note that, in that snippet, I'm also suggesting that we should have createMultipartUpload (and by extension resumeMultipartUpload) return an object with a named uploadId field to minimize the chance that a custom binding implementer passes back the key instead of the upload ID.
There was a problem hiding this comment.
If we keep R2BindingEntrypoint’s return contract in terms of the existing R2 API types, supporting arbitrary custom backends will require a way for their implementations (as well as r2-gateway-worker) to construct those native result types directly without obtaining them from an upstream R2 binding API call.
We can figure out how to do that in a follow-up PR: one approach could be to expose constructors for things like HeadResult to custom bindings user code.
There was a problem hiding this comment.
I don't think it's necessary to remove the Serializable* options from the Head-, List-, and GetBackendResults just because we won't be using them right now? We'll still definitely be calling R2-backed Bindings without an interceptor worker in between and that's what the serializable types help with, we will need it in the future anyway. Unless you mean that having the R2 API types (the Serializable* option) will make it more complicated to support these arbitrary custom backends, although I guess I don't really see the connection there
Yes, I'll add an R2BindingService and can return an object with named uploadId for the functions you mentioned
There was a problem hiding this comment.
I don't think it's necessary to remove the Serializable* options from the Head-, List-, and GetBackendResults just because we won't be using them right now?
It's not just that "we won't be using them right now". I'm not convinced that we should ever allow implementations of R2BindingEntrypoint to return them directly.
As per this comment, how we support a non-interceptor backend is a separate design problem and, whilst making the Serializable* types is perhaps one way we could potentially support non-interceptor backend implementations, I'm not convinced it's the best one. I think it'd be preferable to allow custom bindings implementers to construct the existing R2 binding resource types themselves and pass them back as JSRPC responses.
That may or may not turn out to be the right direction, but I think at the very least we should tackle that problem in a separate MR to unblock support for custom bindings which is its own deliverable.
94d9e94 to
05dcbab
Compare
05dcbab to
9906b15
Compare
|
This pull request was not reviewed automatically because its author does not have write access to this repository. A maintainer with write access can start a review by commenting |
| modules = .testModules, | ||
| bindings = [ | ||
| (name = "BUCKET", r2Bucket = "r2-test"), | ||
| (name = "R2_STRING_UPLOAD_TEST", text = "true"), |
There was a problem hiding this comment.
It seems like there is a fair bit of work involved in excluding the expensive string upload and body length tests from being run as part of the main test suite in r2-test.js (which is run in multiple configurations).
Moreover, two different approaches seem to be used to exclude these tests from being run in different places (either don't set some string like R2_STRING_UPLOAD_TEST or re-export only a subset of the test modules, excluding the string upload and body length tests). It's quite difficult to follow what's going on.
How about we instead break out the string upload test module into its own JS file so we don't have to use either technique? r2-test.js is getting pretty monolithic anyway.
| ( name = "fast-structs", | ||
| worker = ( | ||
| modules = [ | ||
| (name = "worker", esModule = "import { R2JsrpcResponseEntrypoint, testInvalidR2Responses } from './r2-test.js'; export { R2JsrpcResponseEntrypoint }; export const InvalidR2ResponseTests = { test: testInvalidR2Responses };"), |
There was a problem hiding this comment.
Another test organisation suggestion: perhaps it would be clearer if we moved R2JsrpcResponseEntrypoint, testInvalidR2Responses into their own file (r2-jsrpc-response-test.js) and have a default export which exposes what is currently testInvalidR2Responses as its test() handler?
Then both services in r2-jsrpc-response-test.wd-test could embed that file directly instead of constructing an inline wrapper module.
| @@ -0,0 +1,209 @@ | |||
| // Copyright (c) 2017-2026 Cloudflare, Inc. | |||
There was a problem hiding this comment.
This test suite seems to test a few different things:
- R2 binding transport-agnostic behaviour e.g. checking that you can pass
env.BUCKET.get(...).bodytoenv.BUCKET.put(...). I'd suggest moving that and any other assertions that fall into this category into the commonr2-test.jstest suite. - R2 resource serialization behaviour using
env.PLATFORMas an ordinary service binding to do JSRPC serialization round trips. I think that's reasonable to keep in here. - R2 binding JSRPC-specific behaviour e.g. checking that an R2 binding receives an error if the corresponding JSRPC backend call tries to return an object of the wrong type (e.g. returning an
R2ObjectBodyfromhead). This seems like it could belong in the tests run byr2-jsrpc-response-test.wd-test.
That would leave r2-jsrpc-serialization-test.js and its .wd-test focused on resource serialization, rather than mixing it with R2 binding behavior. Perhaps then we could rename it and its .wd-test configurations to r2-resource-serialization-test, to distinguish R2 resource serialization from testing the JSRPC-backed R2 binding.
| const jsg::TypeHandler<jsg::Promise<R2Bucket::HeadBackendResult>>& completeResultHandler) { | ||
| return js.evalNow([&] { | ||
| for (auto& part: uploadedParts) { | ||
| JSG_REQUIRE(part.partNumber >= 1 && part.partNumber <= 10000, TypeError, |
There was a problem hiding this comment.
As per internal discussions, we want the JSRPC-backed R2 binding implementation to entirely place the responsibility for input validation onto the JSRPC server, so we should remove this. check and any others like it from the client, whilst ensuring that "invalid" values can be serialized and passed over JSRPC without narrowing the input values (for example).
| SerializableGetOptions normalized; | ||
|
|
||
| KJ_IF_SOME(condition, o.onlyIf) { | ||
| // Parsed only for validation, so callers get the same errors as the non-RPC path (e.g. |
There was a problem hiding this comment.
We should be able to remove this input validation too?
| } | ||
| KJ_IF_SOME(requestedIncludes, o.include) { | ||
| for (auto& requested: requestedIncludes) { | ||
| JSG_REQUIRE(requested.value == "httpMetadata" || requested.value == "customMetadata", |
There was a problem hiding this comment.
Hmm I can see that the desire to keep input validation on the server is somewhat at odds with handling the r2_list_honor_include flag (which is specific to the client) in this case.
Here's something we could do:
- When the flag is enabled, forward options unchanged.
- When the flag is disabled and the caller's include list contains only supported values, replace
includewith both metadata fields. - When the flag is disabled but the caller's include list contains one or more unsupported values, forward the original options unchanged and let the backend decide the error. The native R2 passthrough already rejects unsupported values.
The backend's responsibility is then simply to:
- reject calls that specify invalid includes
- return exactly the requested includes when the include values are valid (including returning neither field when no includes or [] is specified)
There was a problem hiding this comment.
Also, you should be able to avoid copying all the other ListOptions fields into a new instance - just modify include on the provided options.
| } | ||
| } | ||
|
|
||
| KJ_IF_SOME(range, o.range) { |
| } | ||
| } | ||
|
|
||
| KJ_IF_SOME(ssecKey, buildSsecKey(kj::mv(o.ssecKey))) { |
| ssecKey = kj::mv(key); | ||
| } | ||
|
|
||
| rpcOptions = PutOptions{ |
There was a problem hiding this comment.
Unless I'm missing something, if we stop doing input validation on the client, we can just pass through the original put options unmodified to the RPC call.
| }; | ||
|
|
||
| R2MultipartUpload(kj::String key, kj::String uploadId, jsg::Ref<R2Bucket> bucket) | ||
| // The gateway's completion response omits create-time metadata. Created RPC uploads retain |
There was a problem hiding this comment.
Not sure exactly what this is saying but it appears that R2MultipartUpload's metadata field is now unused? I'm not sure there is a need for this state to exist beyond the creation RPC call.
| "R2 resources can only be deserialized for RPC."); | ||
| } | ||
|
|
||
| PreparedR2RpcBody prepareR2RpcBody(jsg::Lock& js, R2PutValue& value) { |
There was a problem hiding this comment.
Similar to input validation, I think we should forward R2PutValue variants without performing client-side validation. The variants are JSG-serializable with a couple of caveats:
- A stream whose identity length is known on the sender may not report that length when it is hydrated on the RPC receiver. That appears to be a generic JSRPC gap. Fixing it there should let us remove the known-length validation and extra
valueSizeparameter from the custom-binding RPC methods. An interceptor could then forward received streams directly to its native R2 binding. - JSRPC’s serialized-message size limit means forwarding large strings, buffers, or Blobs inline would regress support for large put/upload values.
I think it's reasonable to keep the conversion of large strings, buffers and Blobs into streams to address #2 but we should definitely try to address #1 with a generic JSRPC fix. I can look into that and raise a separate PR.
No description provided.