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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for Etag headers on reads #489

Merged
merged 17 commits into from Jul 8, 2021

Conversation

@daniellehanks
Copy link
Contributor

@daniellehanks daniellehanks commented Jul 6, 2021

Support conditional requests based on ETag for read operations (reload, exists, download_*). My own testing seems to indicate that the JSON API does not support ETag If-Match/If-None-Match headers on modify requests (patch, delete, etc.), please correct me if I am mistaken.

This part two of #451. Part one in #488.
Fixes #451 馃

@tseaver tseaver changed the title Etag headers on reads feat: add support for Etag headers on reads Jul 6, 2021
Copy link
Contributor

@tseaver tseaver left a comment

Thanks very much for the patch!

google/cloud/storage/_helpers.py Show resolved Hide resolved
docs/generation_metageneration.rst Outdated Show resolved Hide resolved
google/cloud/storage/_helpers.py Outdated Show resolved Hide resolved
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
google/cloud/storage/bucket.py Outdated Show resolved Hide resolved
tests/unit/test__helpers.py Outdated Show resolved Hide resolved
tests/unit/test_blob.py Outdated Show resolved Hide resolved
tests/unit/test_client.py Outdated Show resolved Hide resolved
@daniellehanks
Copy link
Contributor Author

@daniellehanks daniellehanks commented Jul 6, 2021

Thanks very much for the patch!

Appreciate the preview review so I can address requested changes before writing system tests.

@daniellehanks
Copy link
Contributor Author

@daniellehanks daniellehanks commented Jul 6, 2021

PR feedback addressed. Still need to add system tests. Hoping to find some time tonight.

if value is not None:
if isinstance(value, str):
value = [value]
headers[header_name] = ", ".join(value)
Copy link
Contributor Author

@daniellehanks daniellehanks Jul 7, 2021

It seems to me (though I'm not a spec expert) that the GCS API isn't following spec putting quotes around the etag values. E.g. If-None-Match: COKaz4vVzfECEAE= works as intended but If-None-Match: "COKaz4vVzfECEAE=" does not. That said, the response header is also not quoted as it apparently should be etag: COKaz4vVzfECEAE= vs expected etag: "COKaz4vVzfECEAE=". Referencing this and this.

Copy link
Contributor Author

@daniellehanks daniellehanks Jul 7, 2021

Hmm, maybe its just the display layers or something (using Chrome dev tools and curl). Wikipedia does the same thing.

Copy link
Contributor Author

@daniellehanks daniellehanks Jul 7, 2021

microsoft.com has it quoted 馃

Copy link
Contributor

@tseaver tseaver Jul 7, 2021

RFC 7232 is the actual authoritative spec for conditional HTTP requests: If-Match and If-None-Match. The Collected ABNF appendix does indeed seem to require the double quote.

The GCS docs for the ETag header show the quotes as well.

Copy link
Contributor Author

@daniellehanks daniellehanks Jul 7, 2021

Yep, all the examples in the spec are also quoted. Fun times. Definitely out of scope for this PR. Just wanted to call it out. I would think the etag itself should have the quotes (i.e. blob.etag = '"Csdlei="'), so then this code would still function correctly. Of course getting etags quoted would be a far larger change spanning the backend, possibly ESF, and probably a fair amount of Hyrum's law.

It just stood out to me because I ran into a similar problem implementing the UI for BigQuery, which also doesn't follow the etag spec. In the end it meant we couldn't cache tables in the UI. But we also couldn't change the API without risking breaking clients. I'll leave it up to you if you want to file an internal ticket for that.

Copy link
Contributor

@tseaver tseaver Jul 7, 2021

Effectively we're stuck with the as-implemented back-end. @frankyn, @andrewsg, @tswast I'll leave it to y'all to open a ticket on the back-end (if one isn't there already) for the out-of-spec behavior.

@daniellehanks
Copy link
Contributor Author

@daniellehanks daniellehanks commented Jul 7, 2021

I added two system tests. The first for the client as requested. The second for blob crud modeled after generation crud (but without write operations). There wasn't an existing system test for bucket metageneration stuff, so I didn't add one there for etags. Please let me know if there are any other tests you would like to see added.

I'm removing draft status as this PR is now complete from my perspective.

@daniellehanks daniellehanks marked this pull request as ready for review Jul 7, 2021
@daniellehanks daniellehanks requested a review from Jul 7, 2021
@daniellehanks daniellehanks requested a review from as a code owner Jul 7, 2021
@daniellehanks daniellehanks requested a review from tseaver Jul 7, 2021
Copy link
Contributor

@tseaver tseaver left a comment

Thanks for the thoroughness of the systests you added!

tests/system/test_client.py Outdated Show resolved Hide resolved
tests/system/test_blob.py Outdated Show resolved Hide resolved
tests/system/test_blob.py Outdated Show resolved Hide resolved
@daniellehanks
Copy link
Contributor Author

@daniellehanks daniellehanks commented Jul 7, 2021

FYI I ran the system tests for 2.7 and caught a bug in the etag helper (instance check for str doesn't work for unicode type in 2.7) that the unit tests didn't catch (because they don't round trip the etag). Fixed by using six.string_types.

@daniellehanks daniellehanks requested a review from tseaver Jul 7, 2021
tseaver
tseaver approved these changes Jul 8, 2021
@google-cla
Copy link

@google-cla google-cla bot commented Jul 8, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

鈩癸笍 Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jul 8, 2021
@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 8, 2021

@googlebot I consent.

tests/system/test_client.py Outdated Show resolved Hide resolved
tests/unit/test_client.py Outdated Show resolved Hide resolved
tseaver
tseaver approved these changes Jul 8, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 741d3fd into googleapis:master Jul 8, 2021
5 checks passed
@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 8, 2021

Thanks, again, @daniellehanks for your effort!

cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
Support conditional requests based on ETag for read operations (`reload`, `exists`, `download_*`). My own testing seems to indicate that the JSON API does not support ETag If-Match/If-None-Match headers on modify requests (`patch`, `delete`, etc.), please correct me if I am mistaken.

This part two of googleapis#451. Part one in googleapis#488.
Fixes googleapis#451 馃
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
Support conditional requests based on ETag for read operations (`reload`, `exists`, `download_*`). My own testing seems to indicate that the JSON API does not support ETag If-Match/If-None-Match headers on modify requests (`patch`, `delete`, etc.), please correct me if I am mistaken.

This part two of googleapis#451. Part one in googleapis#488.
Fixes googleapis#451 馃
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants