Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use u64 for R2 range requests #564

Merged
merged 3 commits into from May 19, 2024
Merged

Use u64 for R2 range requests #564

merged 3 commits into from May 19, 2024

Conversation

kflansburg
Copy link
Member

Closes #560

Waiting for guidance on the behavior of limit without offset. For now I'd like to continue to match the JavaScript behavior.

@kflansburg
Copy link
Member Author

Looks like Rust 1.78 introduced some new clippy lints. I've fixed ours, but looks like wasm-bindgen may need to fix some as well. rustwasm/wasm-bindgen#3945

@kflansburg
Copy link
Member Author

@himikof How does this look? I'm still waiting to hear back on the intended behavior of the range API.

worker/src/r2/builder.rs Outdated Show resolved Hide resolved
@himikof
Copy link

himikof commented May 4, 2024

This change should fix the practical issue, allowing to use the API with large files.

But I still think that the API does not really match the JS from the user point of view, and does not feel like a proper Rust API. In JS, you just specify either some non-empty subset of {offset, length}, or suffix when specifying a request range, and just access the 3 fields (checking them for undefined) when reading a response range.
But in Rust, you must explicitly select one of the enum Variants in the first case (leading to questions like "what is the difference?", "what if I chose the wrong one?"), and even worse, handle all of them in the second case (leading to code duplication). I do not think that to match the JS behavior you need to copy the TypeScript type annotation (which is really an implementation detail many users won't care about) into the Rust API, making it very explicit for all users. Note how the impl TryFrom<R2RangeSys> for Range never constructs OffsetWithOptionalLength with Some length, or OptionalOffsetWithLength with Some offset: those fields are not needed to match JS semantics at all.

Furthermore, this is actually not some Cloudlare-specific API: I would expect it to match the well-known semantics of simple HTTP range header (with only one range). You would expect it to have 3 non-overlapping possibilities: you can requesting a specific range (offset+length), a suffix from the given offset, or a suffix of given length. It is well-known that the range header has this structure, so not having the API reflect this leads to unnecessary surprise.

As this PR already is a breaking change for this API (no implicit integer conversions in Rust), I ask to reconsider changing the Rust enum shape to remove this overlap, and better match the use cases/expectations.

@kflansburg
Copy link
Member Author

As this PR already is a breaking change for this API (no implicit integer conversions in Rust), I ask to reconsider changing the Rust enum shape to remove this overlap, and better match the use cases/expectations.

I'm waiting to hear back from the team on the actual behavior of these options. Without this information, I do not want to make a decision here.

@kflansburg
Copy link
Member Author

@himikof I've updated Range to have more clear options / documentation. I did keep 4 options, although you could achieve Prefix using OffsetWithLength { offset: 0, length N }, I think this is more clear.

@kflansburg kflansburg requested a review from himikof May 16, 2024 11:19
@kflansburg kflansburg merged commit 6bf6cf7 into main May 19, 2024
3 checks passed
@kflansburg kflansburg deleted the kflansburg/r2-range branch May 19, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Range requests in R2 are limited to 32 bit offset/length
2 participants