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

blob/azureblob: Do not panic if Content-Length and Content-Range are missing #3445

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Jun 12, 2024

I found that when we're downloading an a gzipped object with Content-Encoding: gzip, AZB is transparently sending back an uncompressed object rather than the gzipped object, and when this happens, I also discovered that go-cloud fails and panics because the Content-Length and the Content-Range fields are not set in this situation.

To gracefully handle this, check if the ContentLength field is nil before using it, and return 0 if both ContentRange and ContentLength are missing. This unfortunately does mean Size() may return 0 in some cases, but I don't see a good way to fix that without a bigger change in design. With this patch, I can confirm our issue is resolved.

Unfortunately, I did not see a good reference for how I could write a unit test for this. It seems existing conformance tests rely on a live bucket, and there's no unit tests that mock the underlying response or server for any of the existing drivers that I could reference either. If you have a suggestion for testing, let me know, but currently no new tests are added as part of this PR.

blob/azureblob/azureblob.go Outdated Show resolved Hide resolved
…missing

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/azure_blob_panic branch from cc3250a to 8799b3b Compare June 13, 2024 15:15
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.43%. Comparing base (ba58ec7) to head (8799b3b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3445      +/-   ##
==========================================
+ Coverage   73.40%   73.43%   +0.02%     
==========================================
  Files         113      113              
  Lines       14948    14950       +2     
==========================================
+ Hits        10973    10978       +5     
+ Misses       3201     3198       -3     
  Partials      774      774              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vangent vangent merged commit e0142cf into google:master Jun 13, 2024
5 checks passed
@chancez chancez deleted the pr/chancez/azure_blob_panic branch June 13, 2024 17:07
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.

None yet

3 participants