-
Notifications
You must be signed in to change notification settings - Fork 48
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
fix: accept 201 Created
as valid upload initiation response
#125
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
Left a question before continuing.
@@ -452,7 +452,7 @@ def _process_initiate_response(self, response): | |||
""" | |||
_helpers.require_status_code( | |||
response, | |||
(http_client.OK,), | |||
(http_client.OK, http_client.CREATED), |
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.
Hi @lisandromc, thanks again for sending this change. Could you show me an example of when this fails? At the moment a 201 isn't expected when a resumable session is initialized: https://cloud.google.com/storage/docs/performing-resumable-uploads#initiate-session
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.
Friendly ping on this question.
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.
Hey Frank. I was so busy dealing with other stuff that I overlooked your question, sorry about that.
curl --location --request POST 'https://storage.googleapis.com/dam-staging/Ingest/Blink/Testing/Quinta%20Prueba%20de%20Lich/Photo/Palermo/20191005/dolomites.jpg?GoogleAccessId=digitalassetsmanagement%40production-backup-194719.iam.gserviceaccount.com&Expires=1590596284&Signature=kc1xIWCbHeqr100pSOtlA5GpkO1tLoWgU%2FFUyN3QwGQOOGSOwOoO1XCfqBFAdvYywpTi0QdVjIBnB77nq302o0fkzObgpPCUmCq9uWSS2z9ibkiAQdpwWlR9n4XfC1hn25GYumFjZ161ANxC2M5yrjR91jdYtFu2Z4VGFTN%2BhEl7xtYgQ8GHA%2FWSjfCCEfU%2FY0B4p8GMpiLHL3GZtOmBJvqJsuZdTIKb%2FscqctBNX75VLVw5FaWadkj8s0CyIRLM69jy00aPfNO%2BSrfcm4z7PHE6FV8yeJdewoXvJnRDgT0CZDAU7UzgPA4rhqSutcjQT0VkPrN6CHqniEMvhTWaLg%3D%3D' \
> --header 'x-goog-resumable: start' \
> --header 'x-upload-content-type: application/octet-stream' -D -
HTTP/2 201
content-type: text/plain; charset=utf-8
x-guploader-uploadid: AAANsUm8Q-L1Oh6afXYzfU15-IxWUj4x_JvH8fXvbdCmv-fEdThwSBo4kh3nTyb-RSuyUoelAOLT94NpMy_aZ7YM89k
location: https://storage.googleapis.com/dam-staging/Ingest/Blink/Testing/Quinta%20Prueba%20de%20Lich/Photo/Palermo/20191005/dolomites.jpg?GoogleAccessId=digitalassetsmanagement%40production-backup-194719.iam.gserviceaccount.com&Expires=1590596284&Signature=kc1xIWCbHeqr100pSOtlA5GpkO1tLoWgU%2FFUyN3QwGQOOGSOwOoO1XCfqBFAdvYywpTi0QdVjIBnB77nq302o0fkzObgpPCUmCq9uWSS2z9ibkiAQdpwWlR9n4XfC1hn25GYumFjZ161ANxC2M5yrjR91jdYtFu2Z4VGFTN%2BhEl7xtYgQ8GHA%2FWSjfCCEfU%2FY0B4p8GMpiLHL3GZtOmBJvqJsuZdTIKb%2FscqctBNX75VLVw5FaWadkj8s0CyIRLM69jy00aPfNO%2BSrfcm4z7PHE6FV8yeJdewoXvJnRDgT0CZDAU7UzgPA4rhqSutcjQT0VkPrN6CHqniEMvhTWaLg%3D%3D&upload_id=AAANsUm8Q-L1Oh6afXYzfU15-IxWUj4x_JvH8fXvbdCmv-fEdThwSBo4kh3nTyb-RSuyUoelAOLT94NpMy_aZ7YM89k
content-length: 0
date: Wed, 20 May 2020 16:19:47 GMT
server: UploadServer
alt-svc: h3-27=":443"; ma=2592000,h3-25=":443"; ma=2592000,h3-T050=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q049=":443"; ma=2592000,h3-Q048=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"
The URL for Google Storage is provided by a separate web service, so I don't have direct control over it. Anyway, the only apparent difference would seem to be the uploadType=resumable
param.
I tried adding that parameter, but nothing changed. Still 201 Created
.
Also, notice that repeatedly issuing an upload initiation request always yields 201, while one might assume a 201 response for the first time and a 200 for subsequent repeated requests, as it seems to be idempotent.
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.
Thanks for clarifying that. This change LGTM!
@lisandromc pending rebasing or merging master into PR and passing tests. |
Merged! |
There are still test failures that need to be addressed. Could you take a look at them? |
@lisandromc apologies, tests needs to be updated and master must be merged again. |
201 Created
as valid upload initiation response
🤖 I have created a release \*beep\* \*boop\* --- ## [0.7.0](https://www.github.com/googleapis/google-resumable-media-python/compare/v0.6.0...v0.7.0) (2020-07-23) ### Features * add configurable checksum support for uploads ([#139](https://www.github.com/googleapis/google-resumable-media-python/issues/139)) ([68264f8](https://www.github.com/googleapis/google-resumable-media-python/commit/68264f811473a5aa06102912ea8d454b6bb59307)) ### Bug Fixes * accept `201 Created` as valid upload response ([#141](https://www.github.com/googleapis/google-resumable-media-python/issues/141)) ([00d280e](https://www.github.com/googleapis/google-resumable-media-python/commit/00d280e116d69f7f8d8a4b970bb1176e338f9ce0)), closes [#125](https://www.github.com/googleapis/google-resumable-media-python/issues/125) [#124](https://www.github.com/googleapis/google-resumable-media-python/issues/124) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Accepts HTTP 201 Created as a valid upload initiation response
Fixes #124