-
Notifications
You must be signed in to change notification settings - Fork 151
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: raise a ValueError in BucketNotification.create() if a topic name is not set #617
Conversation
google/cloud/storage/notification.py
Outdated
""" | ||
if self.notification_id is not None: | ||
raise ValueError( | ||
"Notification already exists w/ id: {}".format(self.notification_id) | ||
) | ||
|
||
if self.topic_name is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the error from the service currently if the topic name is not included?
This might be fine, but in general we want to avoid overdoing it on client-side validation unless we have a good reason-- we want to let the service itself own most of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's two parts to answer your question.
If the topic is not included, the server returns a 400 with error message "Invalid Google Cloud Pub/Sub topic. It should look like '//pubsub.googleapis.com/projects/*/topics/*.'"
In our client, on line 270, we preprocess the request body topic
through string interpolation. So topic becomes "//pubsub.googleapis.com/projects/<project-identifier>/topics/None"
if topic_name=None
. Thus we get a 400 for another reason "Cloud Pub/Sub topic '//pubsub.googleapis.com/projects/project/topics/None' not found, ..."
To avoid overdoing client-side validation, I could instead change the logic around preparing/formatting the topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay-- yeah I think in this case since we are constructing the topic client-side using topic_name
, that's a good reason to just do the validation ourselves. It would be confusing if users thought that they had to supply the whole path in the error message you show as topic_name
. Let's just try to make as clear a message as possible for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed the logic in constructing the topic. I like the idea of letting the service itself own the validation and aligning with the JSON API. Now the error from the server, if any, will bubble up directly. Will be merging the latest changes if no further questions. Thanks for the review @tritone!
…e is not set (googleapis#617) * fix: topic name is required to create a notification. add check and tests * revise conf tests * fix topic formatting and remove extra validation * blacken lint * update docstrings
…e is not set (googleapis#617) * fix: topic name is required to create a notification. add check and tests * revise conf tests * fix topic formatting and remove extra validation * blacken lint * update docstrings
🤖 I have created a release \*beep\* \*boop\* --- ## [1.43.0](https://github.com/googleapis/python-storage/compare/v1.42.3...v1.43.0) (2021-11-15) ### Features * add ignore_flush parameter to BlobWriter ([#644](https://github.com/googleapis/python-storage/issues/644)) ([af9c9dc](https://github.com/googleapis/python-storage/commit/af9c9dc83d8582167b74105167af17c9809455de)) * add support for Python 3.10 ([#615](https://github.com/googleapis/python-storage/issues/615)) ([f81a2d0](https://github.com/googleapis/python-storage/commit/f81a2d054616c1ca1734997a16a8f47f98ab346b)) ### Bug Fixes * raise a ValueError in BucketNotification.create() if a topic name is not set ([#617](https://github.com/googleapis/python-storage/issues/617)) ([9dd78df](https://github.com/googleapis/python-storage/commit/9dd78df444d21af51af7858e8958b505a26c0b79)) ### Documentation * add contributing and authoring guides under samples/ ([#633](https://github.com/googleapis/python-storage/issues/633)) ([420591a](https://github.com/googleapis/python-storage/commit/420591a2b71f823dbe80f4a4405d8a514f87e0fb)) * add links to samples and how to guides ([#641](https://github.com/googleapis/python-storage/issues/641)) ([49f78b0](https://github.com/googleapis/python-storage/commit/49f78b09fed6d9f486639fd0a72542c30a0df084)) * add README to samples subdirectory ([#639](https://github.com/googleapis/python-storage/issues/639)) ([58af882](https://github.com/googleapis/python-storage/commit/58af882c047c31f59486513c568737082bca6350)) * update samples readme with cli args ([#651](https://github.com/googleapis/python-storage/issues/651)) ([75dda81](https://github.com/googleapis/python-storage/commit/75dda810e808074d18dfe7915f1403ad01bf2f02)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
A PubSub topic (topic name) is required to insert a
BucketNotification
according to the JSON API.Raise a
ValueError
inBucketNotification.create()
iftopic_name = None
. This adds a guard check and further prevents requests with invalid NoneType string interpolation.Add tests and revise conformance tests.
Fixes #616 🦕