fix: getObject with range request and then stat would not return the correct range size - #2267
Conversation
…size fix: getObject with range request and then stat would return the filesize
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesGetObject range and size handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GetObject
participant HTTPReader
participant Object
Caller->>GetObject: Create object with Range
GetObject->>HTTPReader: Fetch metadata or object data
HTTPReader-->>GetObject: Return response and object size
GetObject->>Object: Store totalSize
Caller->>Object: Call Stat
Object-->>Caller: Return remaining size or io.EOF
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object.go`:
- Around line 516-517: Remove the unused disableRange: true assignment in
api-get-object.go lines 516-517. Update the stale comment at lines 152-158 to
refer only to the initial Seek/StatObject behavior, and correct the disableRange
field comment at line 284 to describe disabling the range header for Seek
requests and fix its typo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 969cbd06-bd2c-4298-99ce-89e606b0c3bc
📒 Files selected for processing (1)
api-get-object.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object.go`:
- Around line 470-475: Update Stat() to create and return a copy of ObjectInfo
with Size adjusted to the remaining bytes, rather than mutating
o.objectInfo.Size. Preserve the canonical size stored in o.objectInfo so Seek()
continues using the original object boundary, including the related logic at the
additional Stat() location.
- Around line 159-167: Update the first-read/refetch flow around the Range
handling in GetObject so originalRangeHeader is restored before fetching after
Stat, including the paths corresponding to the noted additional locations. Do
not unconditionally delete the caller’s Range for offset-zero reads; preserve it
unless the caller explicitly supplies a different range or offset, while
retaining the existing behavior for stat operations that require the header
removed.
- Line 206: Update the response construction paths in the object GET handler,
including normal reads, metadata, empty-object handling, and RDMA, to always
populate object size and track whether it is present separately from its numeric
value. Revise Stat() to use the explicit size-present indicator rather than
checking totalSize != 0, preserving valid zero-byte object responses without
returning io.EOF.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f6e0457-2527-4be0-b3d7-cd93423a7fa6
📒 Files selected for processing (1)
api-get-object.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api-get-object.go (2)
222-242:⚠️ Potential issue | 🔴 CriticalRestore the caller’s range on the first refetch after
Stat().Line 222 now re-enters this path when
httpReader == nil, but the offset-zero branch at Lines 239-242 still deletes the caller’sRange. ThereforeGetObject(..., Range: bytes=100-1000); Stat(); Read()can fetch the full object instead of the requested range. RestoreoriginalRangeHeaderbefore this GET unless a later operation intentionally replaces it. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object.go` around lines 222 - 242, Update the refetch logic in the request path guarded by req.DidOffsetChange, !req.beenRead, or httpReader == nil so the offset-zero branch does not delete the caller’s original Range header after Stat(). Restore originalRangeHeader before issuing the GET, while preserving intentional range replacements from req.isReadAt and nonzero req.Offset.
307-307:⚠️ Potential issue | 🔴 CriticalPopulate
totalSizeon every applicable object path.
doGetRequestonly updatestotalSizewhenObjectSize != 0, while the non-first read response at Lines 278-283 omitsObjectSizeand the RDMA constructor does not initializetotalSize. AReadAt-first-then-Readsequence can therefore leavetotalSize == 0, causing a laterStat()to returnio.EOFfor a non-empty object. Initialize RDMAtotalSizeand propagate object size for full-range reads; use an explicit presence indicator if zero-byte objects must be distinguished from “unset.” (raw.githubusercontent.com)Also applies to: 326-326, 384-387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object.go` at line 307, Update doGetRequest and the RDMA object constructor so totalSize is populated on every applicable read path, including non-first/full-range responses and ReadAt followed by Read. Initialize the RDMA totalSize from the object metadata and propagate ObjectSize where currently omitted; use an explicit presence indicator if ObjectSize == 0 must remain distinguishable from an unset value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object.go`:
- Around line 476-482: Update Stat() to copy o.objectInfo into a local
ObjectInfo value before adjusting Size; return that copy for both the currOffset
> totalSize EOF path and the normal path. Preserve the canonical
o.objectInfo.Size unchanged so Seek() and setOffset() continue using the full
object boundary.
---
Outside diff comments:
In `@api-get-object.go`:
- Around line 222-242: Update the refetch logic in the request path guarded by
req.DidOffsetChange, !req.beenRead, or httpReader == nil so the offset-zero
branch does not delete the caller’s original Range header after Stat(). Restore
originalRangeHeader before issuing the GET, while preserving intentional range
replacements from req.isReadAt and nonzero req.Offset.
- Line 307: Update doGetRequest and the RDMA object constructor so totalSize is
populated on every applicable read path, including non-first/full-range
responses and ReadAt followed by Read. Initialize the RDMA totalSize from the
object metadata and propagate ObjectSize where currently omitted; use an
explicit presence indicator if ObjectSize == 0 must remain distinguishable from
an unset value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ced866e-6747-4c0e-9baf-f2eb4c232b70
📒 Files selected for processing (1)
api-get-object.go
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)
api-get-object.go (1)
152-159: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDo not use zero as the “ObjectSize unavailable” sentinel.
ObjectSize == 0is valid for an empty object, butdoGetRequestonly records non-zero values. The same value is also emitted for firstReadAtresponses where the total size is unknown, soStat()cannot distinguish an empty object from missing metadata and may report incorrect EOF/size results. Add an explicit size-present flag.Also applies to: 282-289, 313-313, 332-332, 390-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object.go` around lines 152 - 159, Replace the zero-value ObjectSize sentinel with an explicit size-present flag across doGetRequest and its getResponse handling. Mark the size as present when metadata reports an object, including valid zero-length objects, while leaving first ReadAt responses unknown when total size is unavailable. Propagate the flag through all response construction and update Stat() to distinguish absent metadata from a known empty object.
♻️ Duplicate comments (2)
api-get-object.go (2)
166-174: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the caller’s Range before refetching after Stat.
The Stat path preserves
originalRangeHeader, but the subsequent offset-zero refetch still unconditionally deletesopts.headers["Range"]. ThereforeGetObject(..., Range: bytes=100-1000); Stat(); Read()can fetch the full object instead of the requested range. Restore the original range in this branch unless the caller explicitly changed the range or offset.Proposed fix
} else { - delete(opts.headers, "Range") + if originalRangeHeader != "" { + opts.headers["Range"] = originalRangeHeader + } else { + delete(opts.headers, "Range") + } }Also applies to: 241-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object.go` around lines 166 - 174, Update the offset-zero refetch logic in the GetObject flow to restore originalRangeHeader after Stat instead of unconditionally deleting opts.headers["Range"]. Preserve an explicitly changed caller range or offset, and only remove the Range header when the original request had none.
482-487: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winReturn a copy from
Stat()instead of mutatingo.objectInfo.
Seek()and related offset checks use the canonicalo.objectInfo.Size. AfterStat()reduces that field to the remaining bytes, later seeks and reads can incorrectly stop at the reduced boundary. Copyo.objectInfo, adjust the copy’sSize, and return the copy in both branches.Also applies to: 507-512
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object.go` around lines 482 - 487, Update Stat() to preserve the canonical o.objectInfo by copying it into a local ObjectInfo value before adjusting Size. In both the o.currOffset > o.totalSize and o.currOffset <= o.totalSize branches, modify and return the copy rather than mutating or returning o.objectInfo, so Seek() and subsequent reads continue using the original size.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api-get-object.go`:
- Around line 152-159: Replace the zero-value ObjectSize sentinel with an
explicit size-present flag across doGetRequest and its getResponse handling.
Mark the size as present when metadata reports an object, including valid
zero-length objects, while leaving first ReadAt responses unknown when total
size is unavailable. Propagate the flag through all response construction and
update Stat() to distinguish absent metadata from a known empty object.
---
Duplicate comments:
In `@api-get-object.go`:
- Around line 166-174: Update the offset-zero refetch logic in the GetObject
flow to restore originalRangeHeader after Stat instead of unconditionally
deleting opts.headers["Range"]. Preserve an explicitly changed caller range or
offset, and only remove the Range header when the original request had none.
- Around line 482-487: Update Stat() to preserve the canonical o.objectInfo by
copying it into a local ObjectInfo value before adjusting Size. In both the
o.currOffset > o.totalSize and o.currOffset <= o.totalSize branches, modify and
return the copy rather than mutating or returning o.objectInfo, so Seek() and
subsequent reads continue using the original size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f91a0a9a-97fd-4a20-abda-537630e7941b
📒 Files selected for processing (1)
api-get-object.go
jiuker
changed the title
jiuker
changed the title
|
OK, looks good. Let's update the docs at Lines 460 to 461 in d92b1d2 If this is correct, something like this: // Stat returns the ObjectInfo structure describing Object.
// When requesting a partial object or reading has started,
// the size returned will reflect the remaining size.
func (o *Object) Stat() (ObjectInfo, error) { |
f5cecaf to
5018182
Compare
…stat-would-return-the-filesize
3 participants
fix: getObject with range request and then stat would return the filesize
fix #1813
context:
testGetObjectWithRange()Test SummaryAction Legend
GetObjectreader with the current Range configStat()and verify the remaining readable size equalsnnbytes usingRead()— advances the read offsetnbytes usingRead()(same as Read, just named differently in test)nbytes at the givenoffsetusingReadAt()— does NOT change the read offsetSeek()— changes the read offsetio.ReadAll()— consumes everything to EOFRange Settings
SetRange(100, 1000)SetRange(100, 0)SetRange(0, 1000)Operation Combinations (executed for each Range)
Summary by CodeRabbit
Summary of changes
Stat()to correctly report remaining size and detect end-of-object (EOF) after seek/read sequences.