You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Storage resumable upload URI API not checking for (meta)generation match #201
The request returned status 200 even though the object I was requesting the upload URI for had different generation. So, does this API actually check for (meta)generation matches? Is this behavior intended?
If so we can avoid passing options to the StorageRpc.open method.
@aozarov I might be wrong but for non-resumable uploads you do not need to get an upload URI, you simply send the content in POST.
Here the problem is with the API that returns a resumable upload URI (i.e. an upload id). When I then do a POST to the retrieved resumable upload URI with ifGenerationMatch set I get a mismatch error as expected (as occurs with non-resumable uploads).
@mziccard Right, what I was suggesting is that non-resumable is also using the same POST command as you described (without the resumable type) and providing the content and/or the metadata. Because all is provided in one request the I would expect GCS to do the generation check then.
As for resumable uploads, I was theorizing that the check may happen when upload completes.
I don't think we need to supply the generation header again for the subsequent PUT request
after the "open". Do you see the apairy client doing that?
It seems like ifGenerationMatch is not checked by the first POST we do to get a resumable upload URI, but it is appended by server-side code to the returned resumable upload URI. So all subsequent POST (PUT?) to the resumable upload URI fail.
I would have expected the first POST (our open method) to fail in the first place.
Hope this is clear now.
I am not sure why we are using POST in the subsequent writes as it looks like we should
be using PUT.
Oh, I see, so the URL that is returned to us includes the generationMatch option...
In any case, I agree that the preferred behavior would be to fail on the first "open" request.
Lets wait to hear from @BrandonY though I wonder if we should change the POST writes to PUT (and if that changes anything)...
The GCS JSON API's current behavior is to wait until the final upload request (the one that will result in creating the new object) to verify the preconditions. It's a good suggestion that this check should also be made on the initial POST as well.
An initial POST to the URL, just like your example above.
A series of followup PUTs to the URL that comes back in the Location header. If there is to be more than one PUT, the "Content-Range" header should be set (in multiples of 256k). For example, the first upload should have a "Content-Range: bytes 0-524287/2000000" for a 20MB file.
These puts should all return 308, except for the last one, which will fail unless the generation has become equal to the precondition value in the interim.
Can you let me know if you are seeing a different behavior?
@BrandonY I confirm that intermediate PUTs do return 308 while the last one fails (my previous test used only one PUT so I assumed all of them were meant to fail).
It would be nice to have the first POST fail so that no unnecessary traffic is generated.
I mean, is there any action that we should take on our side? I think @BrandonY created an internal issue for adding a check in the first POST request. Until that happens (if it does) do you want to document it or just leave it as is?
Activity
aozarov commentedon Oct 2, 2015
Interesting. I would expect it to respect the ifGenerationMatch header (as specified).
Did you try it also with non-resumable upload?
I wonder if the enforcement for resumable happens only when upload completes...?
/cc @BrandonY
mziccard commentedon Oct 2, 2015
@aozarov I might be wrong but for non-resumable uploads you do not need to get an upload URI, you simply send the content in POST.
Here the problem is with the API that returns a resumable upload URI (i.e. an upload id). When I then do a POST to the retrieved resumable upload URI with
ifGenerationMatchset I get a mismatch error as expected (as occurs with non-resumable uploads).aozarov commentedon Oct 2, 2015
@mziccard Right, what I was suggesting is that non-resumable is also using the same POST command as you described (without the resumable type) and providing the content and/or the metadata. Because all is provided in one request the I would expect GCS to do the generation check then.
As for resumable uploads, I was theorizing that the check may happen when upload completes.
I don't think we need to supply the generation header again for the subsequent PUT request
after the "open". Do you see the apairy client doing that?
mziccard commentedon Oct 2, 2015
Multipart
Fails with:
Resumable
Succeeds with
HTTP/1.1 200 OKand returns an URI whereifGenerationMatchis set:Subsequent data uploads (seems like we are using POST) fail.
Fails with:
It seems like
ifGenerationMatchis not checked by the first POST we do to get a resumable upload URI, but it is appended by server-side code to the returned resumable upload URI. So all subsequent POST (PUT?) to the resumable upload URI fail.I would have expected the first POST (our
openmethod) to fail in the first place.Hope this is clear now.
aozarov commentedon Oct 2, 2015
Ok, so the non-resumable part fails as expected.
I am not sure why we are using POST in the subsequent writes as it looks like we should
be using PUT.
Oh, I see, so the URL that is returned to us includes the generationMatch option...
In any case, I agree that the preferred behavior would be to fail on the first "open" request.
Lets wait to hear from @BrandonY though I wonder if we should change the POST writes to PUT (and if that changes anything)...
mziccard commentedon Oct 2, 2015
@aozarov I did the above tests with PUT and obtained the same results. I also agree we should switch to PUT as it is the documented method.
aozarov commentedon Oct 2, 2015
Thanks, lets do that then.
I spoke with Brandon and he is trying to reproduce it and will get back to me.
BrandonY commentedon Oct 2, 2015
Hey @mziccard,
The GCS JSON API's current behavior is to wait until the final upload request (the one that will result in creating the new object) to verify the preconditions. It's a good suggestion that this check should also be made on the initial POST as well.
Uploads other than the final PUT do appear to be succeeding for me. Here is how I believe the transaction should go (per https://cloud.google.com/storage/docs/json_api/v1/how-tos/upload ):
These puts should all return 308, except for the last one, which will fail unless the generation has become equal to the precondition value in the interim.
Can you let me know if you are seeing a different behavior?
aozarov commentedon Oct 2, 2015
@BrandonY I assume we can use * instead of 2000000 except of the last chunk if the size is unknown, right?
BrandonY commentedon Oct 2, 2015
Yes, that should be the case.
mziccard commentedon Oct 2, 2015
@BrandonY I confirm that intermediate PUTs do return 308 while the last one fails (my previous test used only one PUT so I assumed all of them were meant to fail).
It would be nice to have the first POST fail so that no unnecessary traffic is generated.
aozarov commentedon Oct 23, 2015
@mziccard Do we need to leave this issue open?
I mean, is there any action that we should take on our side? I think @BrandonY created an internal issue for adding a check in the first POST request. Until that happens (if it does) do you want to document it or just leave it as is?
19 remaining items
chore(main): release 1.2.0 (#199)
chore(main): release 1.3.1 (#205)
chore(deps): update dependency com.google.cloud:google-iam-admin to v…
feat: update retry settings (#201)
chore(deps): update dependency com.google.cloud:libraries-bom to v13 (#…
chore(deps): update dependency com.google.cloud:libraries-bom to v9 (#…
Add codecov (#201)
feat: update retry settings (googleapis#201)
Add codecov (#201)
chore: release 1.17.0 (googleapis#201)
chore(deps): update dependency com.google.cloud:libraries-bom to v10 (#…
chore(deps): update dependency com.google.cloud:libraries-bom to v10 (#…
nit: fix typo in .kokoro/*.cfg (#201)
nit: fix typo in .kokoro/*.cfg (#201)
deps: update dependency com.google.http-client:google-http-client-bom…