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.

Sign up for GitHub

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

Jump to bottom

Fix return value of DownloadBuffer when HTTPRange count is less than data length #24111

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Fix return value of DownloadBuffer when HTTPRange count is less than data length #24111

wants to merge 18 commits into from
+137 −9

Conversation

ashruti-msft
Copy link
Contributor

Fixes Bug #23884

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to module CHANGELOG.md are included.
  • MIT license headers are included in each file.

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Storage Storage Service (Queues, Blobs, Files) labels Feb 12, 2025
@github-actions GitHub Actions
Copy link

Thank you for your contribution @ashruti-msft! We will review the pull request and get back to you soon.

@tanyasethi-msft tanyasethi-msft linked an issue Feb 13, 2025 that may be closed by this pull request
DownloadBuffer with HTTPRange return unexpected number of written bytes #23884
Open
@tamir-laminar
Copy link

@ashruti-msft Thanks for the fix! :)
I see you are fixing it for blob client, I just wanted to make sure that it is going to be fixed for fileshare client as well (github.com/Azure/azure-sdk-for-go/sdk/storage/azfile/), it's the same problem, might even worth doing it in the same PR

tamir-laminar
tamir-laminar reviewed Feb 15, 2025
View reviewed changes
sdk/storage/azblob/blob/client.go Outdated
err = body.Close()
return err
},
})
if err != nil {
return 0, err
}
return count, nil
return min(dataLength, count), nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering, why do we need to take the min of dataLength & count and not just returning dataLength?

looking at this seems like it is possible to ask for count and still download much more bytes then requested - if that's the case it sound like another bug that is worth solving.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datalength actually is the property of the blob which shows the full size its not related to what count was given by the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we shall be returning the number of bytes returned back by server as part of responses, as the data might be partial and not the full range we requested.

@ashruti-msft ashruti-msft requested review from chlowell, RickWinter and a team as code owners February 24, 2025 15:45
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

chlowell
chlowell requested changes Feb 24, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tamir-laminar tamir-laminar tamir-laminar left review comments

@chlowell chlowell chlowell requested changes

@souravgupta-msft souravgupta-msft Awaiting requested review from souravgupta-msft souravgupta-msft is a code owner

@jhendrixMSFT jhendrixMSFT Awaiting requested review from jhendrixMSFT jhendrixMSFT is a code owner

@gapra-msft gapra-msft Awaiting requested review from gapra-msft gapra-msft is a code owner

@vibhansa-msft vibhansa-msft Awaiting requested review from vibhansa-msft vibhansa-msft is a code owner

@tanyasethi-msft tanyasethi-msft Awaiting requested review from tanyasethi-msft tanyasethi-msft is a code owner

@RickWinter RickWinter Awaiting requested review from RickWinter

@joshfree joshfree Awaiting requested review from joshfree joshfree was automatically assigned from Azure/azure-sdk-write-identity

@billwert billwert Awaiting requested review from billwert billwert was automatically assigned from Azure/azure-sdk-write-identity

@g2vinay g2vinay Awaiting requested review from g2vinay g2vinay was automatically assigned from Azure/azure-sdk-write-identity

@schaabs schaabs Awaiting requested review from schaabs schaabs was automatically assigned from Azure/azure-sdk-write-identity

@mpodwysocki mpodwysocki Awaiting requested review from mpodwysocki mpodwysocki was automatically assigned from Azure/azure-sdk-write-identity

@minhanh-phan minhanh-phan Awaiting requested review from minhanh-phan minhanh-phan was automatically assigned from Azure/azure-sdk-write-identity

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DownloadBuffer with HTTPRange return unexpected number of written bytes
5 participants
@ashruti-msft @tamir-laminar @azure-sdk @chlowell @vibhansa-msft