feat: add support for Etag headers on reads #489

daniellehanks · 2021-07-06T06:10:24Z

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 🦕

google/cloud/storage/_helpers.py

tseaver

Thanks very much for the patch!

google/cloud/storage/_helpers.py

docs/generation_metageneration.rst

google/cloud/storage/_helpers.py

google/cloud/storage/blob.py

google/cloud/storage/bucket.py

tests/unit/test__helpers.py

tests/unit/test_blob.py

tests/unit/test_client.py

daniellehanks · 2021-07-06T17:28:50Z

Thanks very much for the patch!

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

daniellehanks · 2021-07-06T19:54:44Z

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

google/cloud/storage/_helpers.py

daniellehanks · 2021-07-07T04:46:57Z

google/cloud/storage/_helpers.py

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


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.

daniellehanks · 2021-07-07T05:59:18Z

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 · 2021-07-07T19:59:41Z

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.

google-cla · 2021-07-08T15:43:23Z

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.

tseaver · 2021-07-08T15:43:35Z

@googlebot I consent.

tests/system/test_client.py

tests/unit/test_client.py

tseaver · 2021-07-08T16:39:55Z

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

google/cloud/storage/_helpers.py Show resolved Hide resolved

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

google/cloud/storage/_helpers.py Show resolved Hide resolved

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 a review from a team July 7, 2021 05:59

daniellehanks requested a review from a team as a code owner July 7, 2021 05:59

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

daniellehanks added 2 commits July 7, 2021 13:57

Address PR feedback

7b05dbf

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

tests/system/test_client.py Outdated Show resolved Hide resolved

tseaver suggested changes Jul 8, 2021

View reviewed changes

tests/unit/test_client.py Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for Etag headers on reads #489

feat: add support for Etag headers on reads #489

daniellehanks commented Jul 6, 2021 •

edited

tseaver left a comment

daniellehanks commented Jul 6, 2021

daniellehanks commented Jul 6, 2021

daniellehanks Jul 7, 2021

daniellehanks Jul 7, 2021

daniellehanks Jul 7, 2021

tseaver Jul 7, 2021

daniellehanks Jul 7, 2021

tseaver Jul 7, 2021

daniellehanks commented Jul 7, 2021

daniellehanks commented Jul 7, 2021

google-cla bot commented Jul 8, 2021

tseaver commented Jul 8, 2021

tseaver commented Jul 8, 2021

feat: add support for Etag headers on reads #489

feat: add support for Etag headers on reads #489

Conversation

daniellehanks commented Jul 6, 2021 • edited

tseaver left a comment

Choose a reason for hiding this comment

daniellehanks commented Jul 6, 2021

daniellehanks commented Jul 6, 2021

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

tseaver Jul 7, 2021

Choose a reason for hiding this comment

daniellehanks Jul 7, 2021

Choose a reason for hiding this comment

tseaver Jul 7, 2021

Choose a reason for hiding this comment

daniellehanks commented Jul 7, 2021

daniellehanks commented Jul 7, 2021

google-cla bot commented Jul 8, 2021

tseaver commented Jul 8, 2021

tseaver commented Jul 8, 2021

daniellehanks commented Jul 6, 2021 •

edited