-
Notifications
You must be signed in to change notification settings - Fork 60
Return error if blob download is incomplete by validating hash digest. #1833
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work; you've solved the essence of the issue.
See in-line for some other comments.
Also — how might we test this? It's a fairly rare case for the error to come up naturally, so the new code path won't be exercised much unless we add test(s). I have some ideas that we can talk about, but first I'd like you to noodle on it some.
Co-authored-by: Reid Priedhorsky <[email protected]>
Co-authored-by: Reid Priedhorsky <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. The logic seems quite solid to me. I have some style recommendations in-line.
One oversight, unless I’ve missed something: request()
only checks the digest if the response is going to a file, i.e. out != None
. I think this is fine, but the arguments need to be checked. As written, if out == None
and expdigest != None
, the latter will be silently ignored. It should instead fail noisily. assert
would be fine I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I didn’t see an assert so I added one.
Closes #1758