fix: raise a ValueError in BucketNotification.create() if a topic name is not set #617
Conversation
product-auto-label
bot
added
the
api: storage
google-cla
bot
added
the
cla: yes
cojenco
changed the title
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.
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.
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.
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.
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!
cojenco
added
the
do not merge
cojenco
removed
the
do not merge
…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://www.github.com/googleapis/python-storage/compare/v1.42.3...v1.43.0) (2021-11-15) ### Features * add ignore_flush parameter to BlobWriter ([#644](https://www.github.com/googleapis/python-storage/issues/644)) ([af9c9dc](https://www.github.com/googleapis/python-storage/commit/af9c9dc83d8582167b74105167af17c9809455de)) * add support for Python 3.10 ([#615](https://www.github.com/googleapis/python-storage/issues/615)) ([f81a2d0](https://www.github.com/googleapis/python-storage/commit/f81a2d054616c1ca1734997a16a8f47f98ab346b)) ### Bug Fixes * raise a ValueError in BucketNotification.create() if a topic name is not set ([#617](https://www.github.com/googleapis/python-storage/issues/617)) ([9dd78df](https://www.github.com/googleapis/python-storage/commit/9dd78df444d21af51af7858e8958b505a26c0b79)) ### Documentation * add contributing and authoring guides under samples/ ([#633](https://www.github.com/googleapis/python-storage/issues/633)) ([420591a](https://www.github.com/googleapis/python-storage/commit/420591a2b71f823dbe80f4a4405d8a514f87e0fb)) * add links to samples and how to guides ([#641](https://www.github.com/googleapis/python-storage/issues/641)) ([49f78b0](https://www.github.com/googleapis/python-storage/commit/49f78b09fed6d9f486639fd0a72542c30a0df084)) * add README to samples subdirectory ([#639](https://www.github.com/googleapis/python-storage/issues/639)) ([58af882](https://www.github.com/googleapis/python-storage/commit/58af882c047c31f59486513c568737082bca6350)) * update samples readme with cli args ([#651](https://www.github.com/googleapis/python-storage/issues/651)) ([75dda81](https://www.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
BucketNotificationaccording to the JSON API.Raise a
ValueErrorinBucketNotification.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 🦕