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

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

Merged
merged 5 commits into from
Oct 13, 2017
Merged

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

merged 5 commits into from
Oct 13, 2017

Conversation

mfschwartz
Copy link
Contributor

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 6, 2017
@mfschwartz mfschwartz mentioned this pull request Oct 9, 2017
Open
6 tasks
@tseaver 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
Copy link
Contributor

tseaver commented Oct 13, 2017

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

@tseaver 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 file Oct 13, 2017
@mfschwartz
Copy link
Contributor Author

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

@tseaver tseaver changed the title Datch checksum validation failure from download, delete corrupt file Catch checksum validation failure from download, delete corrupt file Oct 13, 2017
@tseaver
Copy link
Contributor

tseaver commented Oct 13, 2017

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

Note the linter failures in that output as well.

@dhermes
Copy link
Contributor

dhermes commented Oct 13, 2017

@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.

mfschwartz and others added 5 commits October 13, 2017 11:45
@mfschwartz @dhermes
Make file download catch checksum validation failure from new rev of …
ea8fa82 
…google-resumable-media-python library and delete corrupt file
@mfschwartz @dhermes
Changes in response to review comments
df76a62
@mfschwartz @dhermes
changes per dhermes comments
31cbc18
@dhermes
Updating the dependency on google-resumable-media.
51731c9
@dhermes
Moving DataCorruption exception catch to download_to_filename.
79ef564 
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
Copy link

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.

@googlebot 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
dhermes reviewed Oct 13, 2017
View reviewed changes
storage/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
Copy link
Contributor

dhermes commented Oct 13, 2017

@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
Copy link
Contributor

dhermes commented Oct 13, 2017

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

@dhermes dhermes merged commit 7c770c8 into googleapis:master Oct 13, 2017
dhermes pushed a commit that referenced this pull request Oct 13, 2017
@mfschwartz @dhermes
Cherry-pick of 7c770c8
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
@dhermes
Rolling back all storage changes aside from #4133 and #4180.
95ab8a5 
Had to make a few small manual tweaks as well.
@dhermes
Copy link
Contributor

dhermes commented Oct 13, 2017

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.

@tseaver tseaver mentioned this pull request Oct 16, 2017
Merged
@houglum houglum mentioned this pull request Jun 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhermes dhermes dhermes left review comments

@tseaver tseaver tseaver approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees

@lukesneeringer lukesneeringer

@tseaver tseaver

@theacodes theacodes

Labels
api: storage Issues related to the Cloud Storage API. cla: no This human has *not* signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mfschwartz @tseaver @dhermes @googlebot @lukesneeringer @theacodes