Catch checksum validation failure from download, delete corrupt file by mfschwartz · Pull Request #4133 · googleapis/google-cloud-python

mfschwartz · 2017-10-06T19:56:54Z

This change depends on googleapis/google-resumable-media-python#31

tseaver · 2017-10-13T13:22:38Z

Needs a new unit test for the case where the download object raises resumable_media.DataCorruption.

mfschwartz · 2017-10-13T14:30:27Z

@tseaver - there's an integration test for this case

tseaver · 2017-10-13T14:37:49Z

@mfschwartz We require 100% unit test coverage: see the current CI failure.

Note the linter failures in that output as well.

dhermes · 2017-10-13T17:06:28Z

@tseaver @mfschwartz I'm happy to take up the testing mantle. Before this can go in I need to get an LGTM on

googleapis/google-resumable-media-python#33

and cut a release there.

…google-resumable-media-python library and delete corrupt file

This is the only method where we **know** we have created the file, so it is safe to delete it. In `download_to_file`, the user could pass in a non-file but file-like object, so there would be nothing to delete. Also - Adding a unit test for corrupted data - Just calling `raise` rather than re-wrapping the exception. Using `exceptions.from_http_response` or `exceptions.from_http_status` wouldn't make sense because the corruption will have happened with a status of 200 or 206.

googlebot · 2017-10-13T18:46:25Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Sign in to view

+ except resumable_media.DataCorruption as exc:
+ # Delete the corrupt downloaded file.
+ os.remove(filename)
+ raise


dhermes · 2017-10-13T18:51:32Z

@mfschwartz RE:

there's an integration test for this case

He (Tres) was referring to tests in this library, not in google-resumable-media (which is in some sense an opaque dependency).

dhermes · 2017-10-13T19:45:27Z

@jonparrott @lukesneeringer @tseaver Can I get an LGTM?

Done via: $ git cherry-pick 7c770c8 Catch checksum validation failure from download, delete corrupt file (#4133)

Had to make a few small manual tweaks as well.

dhermes · 2017-10-13T23:34:29Z

Since @tseaver has made a lot of user_project changes since 1.4.0 I am toying with doing a release for this with a cherry-pick branch (though I'm not that excited about it).

I made one branch just by starting from the storage-1.4.0 tag, cherry-picking #4133 and #4180 and then "fixing" it up (build).

I made another branch from master and then just did a git show storage-1.4.0:${NAME} > ${NAME} for all the changed files, then made sure the changes from #4133 and #4180 stayed in while git add-ing (build).

Both approaches will make figuring out what has changed for the next release difficult. It will at least be difficult via git log storage-1.4.1..HEAD since they will no longer share a merge base.

So the question I put to @lukesneeringer and @jonparrott, is it worth it to cherry-pick this?

The question I put to @tseaver, could we just release the user_project stuff as-is? If yes, please take the lead on writing the release notes.

…4133)

mfschwartz requested a review from lukesneeringer as a code owner October 6, 2017 19:56

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 6, 2017

mfschwartz mentioned this pull request Oct 9, 2017
Add (optional) hash validation of objects #15633
Closed
6 tasks

tseaver added api: storage Issues related to the Cloud Storage API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 13, 2017

tseaver changed the title ~~Make file download catch checksum validation failure from new rev of google-resumable-media-python library and delete corrupt file~~Datch checksum validation failure from download, delete corrupt fileOct 13, 2017

tseaver changed the title ~~Datch checksum validation failure from download, delete corrupt file~~Catch checksum validation failure from download, delete corrupt fileOct 13, 2017

mfschwartzand others added 5 commits October 13, 2017 11:45

Make file download catch checksum validation failure from new rev of …
ea8fa82
…google-resumable-media-python library and delete corrupt file

Changes in response to review comments
df76a62

changes per dhermes comments
31cbc18

Updating the dependency on google-resumable-media.
51731c9

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Oct 13, 2017

dhermes reviewed Oct 13, 2017
View reviewed changes

Comment threadstorage/google/cloud/storage/blob.py
except resumable_media.DataCorruption as exc:
# Delete the corrupt downloaded file.
os.remove(filename)
raise
This comment was marked as spam.
Sign in to view
This comment was marked as spam.
Sign in to view

dhermes assigned lukesneeringer, tseaver and theacodes Oct 13, 2017

tseaver approved these changes Oct 13, 2017
View reviewed changes

dhermes merged commit 7c770c8 into googleapis:masterOct 13, 2017

dhermes pushed a commit that referenced this pull request Oct 13, 2017
Cherry-pick of7c770c8
8a68acb
Done via: $ git cherry-pick 7c770c8 Catch checksum validation failure from download, delete corrupt file (#4133)

dhermes added a commit that referenced this pull request Oct 13, 2017
Rolling back all storage changes aside from#4133and#4180.
95ab8a5
Had to make a few small manual tweaks as well.

tseaver mentioned this pull request Oct 16, 2017
Prep 'storage-1.5.0' release. #4202
Merged

houglum mentioned this pull request Jun 5, 2018
For cloud storage client library, consider adding integrity checking functionality to compute checksums for downloads and uploads googleapis/google-cloud-cpp#660
Closed

parthea pushed a commit that referenced this pull request Mar 9, 2026
Catch checksum validation failure from download, delete corrupt file (#…
5c848ec
…4133)

Navigation Menu

Uh oh!

Catch checksum validation failure from download, delete corrupt file - #4133

Catch checksum validation failure from download, delete corrupt file#4133
dhermes merged 5 commits into
googleapis:masterfrom
mfschwartz:master

mfschwartz commented Oct 6, 2017

Uh oh!

tseaver commented Oct 13, 2017

Uh oh!

mfschwartz commented Oct 13, 2017

Uh oh!

tseaver commented Oct 13, 2017

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

googlebot commented Oct 13, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

mfschwartz commented Oct 6, 2017

Uh oh!

tseaver commented Oct 13, 2017

Uh oh!

mfschwartz commented Oct 13, 2017

Uh oh!

tseaver commented Oct 13, 2017

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

googlebot commented Oct 13, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

dhermes commented Oct 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants