fix(gateway): support suffix range requests #922
Conversation
| @@ -219,10 +219,11 @@ type ContentPathMetadata struct { | |||
| // - From >= 0 and To = nil: Get the file (From, Length) | |||
| // - From >= 0 and To >= 0: Get the range (From, To) | |||
| // - From >= 0 and To <0: Get the range (From, Length - To) | |||
| // - From < 0 and To = nil: Get the file (Length - From, Length) | |||
There was a problem hiding this comment.
@lidel the rules for this struct were not great from the get go. There's a similar struct that's more expansive (
Lines 98 to 108 in 2f225c4
Some options look like:
- Add a 4th rule that is the suffix requests (what's done here)
- Replace the 3rd rule with one proper for suffix requests (in theory even doable without changing the struct types) ... technically a breaking change although I suspect this isn't much used externally
- Just combine this with
DagBytesRangeand have a single struct
There was a problem hiding this comment.
option (1) sgtm – seems to be enough to fix ipfs/kubo#10808 and pass all new tests added in ipfs/gateway-conformance#213
| @@ -475,9 +475,18 @@ func (i *handler) seekToStartOfFirstRange(w http.ResponseWriter, r *http.Request | |||
| return true | |||
| } | |||
|
|
|||
| func seekToRangeStart(data io.Seeker, ra *ByteRange) error { | |||
| func seekToRangeStart(data io.Seeker, ra *ByteRange, size int64) error { | |||
There was a problem hiding this comment.
We should figure out how much testing is worth doing here vs just adding to gateway conformance and relying on that.
It's notable that there was a TODO around testing this area 🤦 https://github.com/ipfs/boxo/pull/922/files#diff-5b9dda4053dba85f902d2464389f79cdfca0976873fd206acaefcb09784d6e98R55, let's drop that TODO and replace with enough testing that we're happy 😅.
There was a problem hiding this comment.
Dropped todo + added e2e tests to ipfs/gateway-conformance#213, this way the same tests will be applied to inbrowser.link.
lidel
changed the title
|
Triage notes:
|
car backend did not support negative ranges, this attempts to fix that for raw blocks and dag-pb files
I will take a look with fresh eyes tomorrow, remaining work is to bubble up and land everything:
|
fixes ipfs/kubo#10808
Well ... the need for this PR is quite embarrassing. I thought we had this covered in gateway conformance but we don't.
@lidel IIRC we discussed this when this code path was implemented and maybe this was just supposed to not support suffix requests and return 200 with everything instead of 206 (as we do for ranges on things like dir-index-html responses), but given we found a user who could actually benefit from it it seemed reasonable to implement. Feel free to take over the PR and change as you wish, but figured I'd at least present an option to help get things moving.