feat: add support for Etag headers on reads by daniellehanks · Pull Request #489 · googleapis/python-storage · GitHub

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 🦕

Thanks very much for the patch!

Thanks very much for the patch!

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

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

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.

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

microsoft.com has it quoted 🤔

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.

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.

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.

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.

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.

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.

@googlebot I consent.

Thanks, again, @daniellehanks for your effort!

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 🦕

daniellehanks added 8 commits July 5, 2021 22:06

Supports downloads conditional on etag

98fe6d6

Move etag parameter to header conversion to _helpers

9a41f9a

Support conditional by etag on Blob.exists

ab2a1e7

Support reload conditional on etag

c5b19d2

blacken

ef0d4e6

Support Bucket.exists conditional on etag

4d23548

Rename duplicate test

5336fd5

Pipe etag parameters through Bucket.get_blob

ce4d348

product-auto-label Bot added the api: storage Issues related to the googleapis/python-storage API. label Jul 6, 2021

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 6, 2021

daniellehanks commented Jul 6, 2021

View reviewed changes

Comment thread google/cloud/storage/_helpers.py

daniellehanks mentioned this pull request Jul 6, 2021

Support ETag headers #451

Closed

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

tseaver suggested changes Jul 6, 2021

View reviewed changes

tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 6, 2021

yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 6, 2021

Merge branch 'master' into etag-headers

2b4f7f4

Address PR feedback

2f720e7

daniellehanks commented Jul 7, 2021

View reviewed changes

Comment thread google/cloud/storage/_helpers.py

daniellehanks commented Jul 7, 2021

View reviewed changes

daniellehanks added 2 commits July 6, 2021 23:41

Add system test for client download conditional on etag

97f46c0

Add system test for blob crud with etag

db1ab12

daniellehanks marked this pull request as ready for review July 7, 2021 05:59

daniellehanks requested review from a team and tseaver July 7, 2021 05:59

Make etag helper 2.7 compatible

7a03371

daniellehanks requested a review from tseaver July 7, 2021 20:00

Update second blob variable name

c388fe8

tseaver approved these changes Jul 8, 2021

View reviewed changes

tseaver added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 8, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2021

chore: blacken

6b3b6ab

tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2021

google-cla Bot 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 Jul 8, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2021

google-cla Bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 8, 2021

tseaver reviewed Jul 8, 2021

View reviewed changes

Comment thread tests/system/test_client.py Outdated

tseaver suggested changes Jul 8, 2021

View reviewed changes

Comment thread tests/unit/test_client.py Outdated

chore: remove unused import

3e32a3e

tseaver approved these changes Jul 8, 2021

View reviewed changes

tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2021

gcf-merge-on-green Bot merged commit 741d3fd into googleapis:master Jul 8, 2021

gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 8, 2021

release-please Bot mentioned this pull request Jan 12, 2022

chore(main): release 3.0.0 #688

Closed

cojenco mentioned this pull request Apr 8, 2025

Add support for JSON API headers and query string parameters #30

Closed

+                      if value is not None:
+                          if isinstance(value, str):
+                              value = [value]
+                          headers[header_name] = ", ".join(value)

feat: add support for Etag headers on reads by daniellehanks · Pull Request #489 · googleapis/python-storage · GitHub

Uh oh!

feat: add support for Etag headers on reads - #489

feat: add support for Etag headers on reads#489gcf-merge-on-green[bot] merged 17 commits intogoogleapis:mastergoogleapis/python-storage:masterfrom daniellehanks:etag-headersdaniellehanks/python-storage:etag-headersCopy head branch name to clipboard

Conversation

daniellehanks commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tseaver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniellehanks commented Jul 6, 2021

Uh oh!

daniellehanks commented Jul 6, 2021

Uh oh!

Uh oh!

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

tseaver Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

tseaver Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

daniellehanks commented Jul 7, 2021

Uh oh!

daniellehanks commented Jul 7, 2021

Uh oh!

google-cla Bot commented Jul 8, 2021

Uh oh!

tseaver commented Jul 8, 2021

Uh oh!

Uh oh!

Uh oh!

tseaver commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: add support for Etag headers on reads#489
gcf-merge-on-green[bot] merged 17 commits into
googleapis:masterfrom
daniellehanks:etag-headers

daniellehanks commented Jul 6, 2021 •
edited
Loading