googleapis / python-storage

Apache License 2.0
448 stars 154 forks source link

Misleading error message when creating notifications while specifying a notification ID. #1289

Closed JordanW1300 closed 5 months ago

JordanW1300 commented 5 months ago

Issue Summary: When following the Google documentation [1] but modifying the code to include a notification ID on the notification object before creating it you get an error stating that the notification ID exists. However when running the gcloud command to describe a notification with that same ID it actually does not exist.

When looking at the source for this method we can see this error message is returned anytime the notification ID on the notification object is set to anything other than None [2].

The reference documentation [3] also states that the notification ID is set by the server. This suggests this is working as intended and the creation of notifications when the ID is set to a value other than none is not allowed.

My Ask: Is there a specific reason why the error message states that a notification already exists with the ID?

This can be misleading and seems to imply that it is possible to create a notification with a custom ID however the one selected is already in use.

Possible Change: This error message is returned directly within the create method not from a method/function called within it. I would think it would be more informative to return a message stating that the notification ID must be set to None in order to create a new notification. Unless of course there is a specific reason for this message.

[1] https://cloud.google.com/storage/docs/reporting-changes#storage_create_bucket_notifications-python [2] https://github.com/googleapis/python-storage/blob/693f1954748154d895b3671339eeafe43dfa415d/google/cloud/storage/notification.py#L256 [3] https://cloud.google.com/python/docs/reference/storage/1.44.0/google.cloud.storage.notification.BucketNotification#google_cloud_storage_notification_BucketNotification_notification_id

Steps to reproduce

  1. Follow GCP tutorial using code snippet
  2. Modify it to specify a notification ID on the notification object before it is created
  3. Run the program and receive the error below.

Code example

Project, bucket, and topic names redacted for privacy

from google.cloud import storage

def create_bucket_notifications(bucket_name, topic_name, notification_id=None):

    storage_client = storage.Client('PROJECT-ID')
    bucket = storage_client.bucket(bucket_name)
    notification = bucket.notification(topic_name=topic_name, notification_id=notification_id)
    notification.create()

    print(f"Successfully created notification with ID {notification.notification_id} for bucket {bucket_name}")

create_bucket_notifications('MY-BUCKET', 'MY-TOPIC', 15)

Stack trace

Project, bucket, topic, and system user names redacted for privacy

Traceback (most recent call last): File "/home/user/bucket-notification/text.py", line 12, in create_bucket_notifications("MY-BUCKET", 'MY-TOPIC', 15) File "/home/wriker/bucket-notification/text.py", line 8, in create_bucket_notifications notification.create() File "/usr/local/lib/python3.10/dist-packages/google/cloud/storage/notification.py", line 257, in create raise ValueError( ValueError: Notification already exists w/ id: 15

tritone commented 5 months ago

Thanks for the issue. I think the reason for the error message being what it is currently is that you could run into this issue if you called create twice on the same notification object. However, you're correct that it could also happen by setting the notification_id kwarg. I have made a PR to clarify.