fix(files): remove io.Seeker from File interface#1128
Conversation
Not all File implementations can seek. ReaderFile wrapping an HTTP multipart stream and WebFile both returned ErrNotSupported at runtime, which caused interface-sniffing bugs in downstream consumers like go-car (ipfs/kubo#9361). Seeking is now opt-in: implementations that support it (ReaderFile with a seekable reader, Symlink, ufsFile) still have a Seek method and callers type-assert to io.Seeker when needed. - files: remove io.Seeker from File interface - files: remove dummy WebFile.Seek that always returned ErrNotSupported - gateway: type-assert to io.Seeker before seeking in all handlers
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1128 +/- ##
==========================================
- Coverage 63.27% 63.16% -0.11%
==========================================
Files 267 267
Lines 26848 26867 +19
==========================================
- Hits 16987 16971 -16
- Misses 8145 8171 +26
- Partials 1716 1725 +9
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Update boxo to ipfs/boxo#1128 which removes io.Seeker from the files.File interface. Callers that need seeking now type-assert to io.Seeker. - core/commands/cat: type-assert before seeking - core/coreiface/tests: type-assert before seeking
gammazero
left a comment
gammazero
left a comment
There was a problem hiding this comment.
I think this all looks alright, but would like to any use of io.Seeker to io.Reader where ever possible.
| s, ok := f.(io.Seeker) | ||
| if !ok { | ||
| return nil, fmt.Errorf("file does not support seeking") | ||
| } | ||
| if _, err := s.Seek(from, io.SeekStart); err != nil { |
There was a problem hiding this comment.
Is it necessary to use io.Seeker here if from is never negative. Would it be OK to do a discarding read here?
There was a problem hiding this comment.
You would think so, but we seem to have layers upon layers and sometimes it means seek happens more than once. 🙈
Tried the slice in 8d0b535, broke gateway-conformance, reverted in 1cb4a9c.
iiuc the reader returned from this site is handed to serveCodecRaw, which seeks it again via seekToStartOfFirstRange before serveContent. With Seek both seeks land at the same absolute offset (idempotent). With blockData[from:] the second seek lands inside the slice, so the body comes from offset 2*from. The seek here isn't redundant: the codec path assumes the reader spans the full block.
Cleaner fix would be to drop it and let serveCodecRaw/serveFile own range positioning, but that's a separate boxo/gateway refactor, lets keep this PR a minimal refactor.
|
Triage: OK with cleaning this up. @lidel will rebase and merge. |
resolve conflicts in CHANGELOG.md, gateway/handler_block.go, and gateway/handler_codec.go. the handler conflicts both add an io.Seeker type assertion now that files.File no longer embeds io.Seeker, on top of the new size limit and roots-header logic from origin/main.
show the before/after migration pattern for callers and link to ipfs/kubo#11254 as a worked example of updating consumers.
Update boxo to ipfs/boxo#1128 which removes io.Seeker from the files.File interface. Callers that need seeking now type-assert to io.Seeker. - core/commands/cat: type-assert before seeking - core/coreiface/tests: type-assert before seeking
at the non-DagPb terminal-entity site, blockData is already in memory and from is non-negative once the negative case is converted to an offset from start. drop the io.Seeker assertion and Seek call in favor of NewBytesFile(blockData[from:]). clamp from to size to preserve the previous bytes.Reader.Seek behavior of yielding an empty body for out-of-range positive starts.
reverts 8d0b535 which broke gateway-conformance TestPlainCodec range request cases (silent body corruption on JSON/CBOR codecs). the codec path calls seekToStartOfFirstRange on the returned reader again before serveContent, and that second Seek expects offsets in the full block buffer, not a pre-sliced reader. add a comment explaining the constraint so future readers don't make the same mistake.
|
Merging, Sharness passes in ipfs/kubo#11254, will start passing in boxo as soon we land that PR too. |
Update boxo to ipfs/boxo#1128 which removes io.Seeker from the files.File interface. Callers that need seeking now type-assert to io.Seeker. - core/commands/cat: type-assert before seeking - core/coreiface/tests: type-assert before seeking
Update boxo to ipfs/boxo#1128 which removes io.Seeker from the files.File interface. Callers that need seeking now type-assert to io.Seeker. - core/commands/cat: type-assert before seeking - core/coreiface/tests: type-assert before seeking
2 participants
Not all File implementations can seek.
ReaderFilewrapping an HTTP multipart stream andWebFileboth returnedErrNotSupportedat runtime, which caused interface-sniffing bugs in downstream consumers like go-car (ipfs/kubo#9361).Not sure if this PR (removal of io.Seeker from File interface) is the "lesser evil", opening this mostly to have PR with code example.
With this PR seeking is now opt-in: implementations that support it (ReaderFile with a seekable reader, Symlink, ufsFile) still have a Seek method and callers type-assert to io.Seeker when needed.
files: removeio.SeekerfromFileinterfacefiles: remove dummyWebFile.Seekthat always returnedErrNotSupportedgateway: type-assert toio.Seekerbefore seeking in all handlersTest plan