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.

Sign up for GitHub

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

Jump to bottom

Making all Blob/Bucket getters fail gracefully if keys don't exist. #793

Merged
merged 1 commit into from
Apr 6, 2015
Merged

Making all Blob/Bucket getters fail gracefully if keys don't exist. #793

merged 1 commit into from
Apr 6, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Apr 1, 2015

Also replacing uses of dict.copy() (shallow) with copy.deepcopy().

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 1, 2015
@dhermes dhermes added the api: storage Issues related to the Cloud Storage API. label Apr 1, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 873ec53 on dhermes:prevent-property-key-error-in-storage into 3af46b1 on GoogleCloudPlatform:master.

tseaver
tseaver reviewed Apr 1, 2015
View reviewed changes
gcloud/exceptions.py
@@ -38,7 +39,7 @@ def __init__(self, message, errors=()):
super(GCloudError, self).__init__()
# suppress deprecation warning under 2.6.x
self.message = message
self._errors = [error.copy() for error in errors]
self._errors = errors

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Apr 1, 2015

ISTM that returning None from the getters breaks their contract a bit: we could either change the :rtype: to append or None, or else return defaults which match the documented type.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 1, 2015

@tseaver I imagine a future where people use their own projection or fields, e.g.

blob.reload(fields=['generation', 'metageneration'])

and then all other values are unset.

I think changing the rtype is the right way to go here.

@dhermes
Making all Blob/Bucket getters fail gracefully if keys don't exist.
2971484 
Also
- replacing uses of dict.copy() (shallow) with copy.deepcopy().
- Making getters honor contracts (e.g. return an integer if supposed to).
  This is necessary because int64 -> JSON becomes a string, even though
  we want an integer. This is due to only one numeric type "number" in
  JavaScript.
@dhermes dhermes force-pushed the prevent-property-key-error-in-storage branch from 873ec53 to 2971484 Compare April 2, 2015 03:21
@dhermes
Copy link
Contributor Author

dhermes commented Apr 2, 2015

@tseaver I updated the docstrings and pushed again. I also coerced some values to ints, since int64 in JSON is a string even though it should be an int (due to JS precision problems).

I also wanted to convert the timestamps to datetime.datetime but was worried that using

datetime.datetime.strptime(value, '%Y-%m-%dT%H:%M:%S.%fZ')

would be insufficient for values returned by the backend (AFAIK the only contract is that they will be valid RFC3339).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2971484 on dhermes:prevent-property-key-error-in-storage into fd06f16 on GoogleCloudPlatform:master.

tseaver
tseaver reviewed Apr 3, 2015
View reviewed changes
gcloud/storage/blob.py
@@ -553,7 +583,8 @@ def metadata(self, value):

See: https://cloud.google.com/storage/docs/json_api/v1/objects

:type value: dict
:type value: dict or ``NoneType``
:param value: The blob metadata to set.

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Apr 3, 2015

Aside from the worry about PATCH w/ None, LGTM.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 3, 2015

PATCH with None effectively "unsets" the property. But we would only send None if it was explicitly set by the user.

I'm AFK now but would like to test out what happens if None is sent for a required property.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 6, 2015

@tseaver What is the implication of my answer? How to move forward?

@tseaver
Copy link
Contributor

tseaver commented Apr 6, 2015

We should figure out what happens when {'metadata': null} is the payload for a PATCH request where metadata had already been set. If the back-end barfs, we shouldn't allow None as a value in the setter.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 6, 2015

@tseaver I was already pretty sure what happened, but I verified via https://developers.google.com/apis-explorer/#p/storage/v1/storage.objects.patch by sending "metadata: null" after previously setting metadata to {'foo': 'bar'}. It did as expected: it simply "unset" the metadata value.

@tseaver
Copy link
Contributor

tseaver commented Apr 6, 2015

OK. ISTM we can just mirror that, then (the json module already marshalls Python None as null).

@dhermes
Copy link
Contributor Author

dhermes commented Apr 6, 2015

Yep, we get it for free. Remaining hangups?

@tseaver
Copy link
Contributor

tseaver commented Apr 6, 2015

Nope.

dhermes added a commit that referenced this pull request Apr 6, 2015
@dhermes
Merge pull request #793 from dhermes/prevent-property-key-error-in-st…
1536f09 
…orage

Making all Blob/Bucket getters fail gracefully if keys don't exist.
@dhermes dhermes merged commit 1536f09 into googleapis:master Apr 6, 2015
@dhermes dhermes deleted the prevent-property-key-error-in-storage branch April 6, 2015 17:25
This was referenced Apr 8, 2015
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dhermes @coveralls @tseaver @googlebot