pubsub.Topic.ID() panics on deleted topic - cf pubsub.SubscriptionConfig.Topic.ID()

[zachvictor]

If a reference exists to a Topic, and the Topic has been deleted, then Google Cloud returns a sentinel value for its name: _deleted-topic_. As this sentinel value has no package constant nor mention in the comments of exported package members, the published documentation gives no information about it. This is something of a landmine for client-library consumers, because the client library may return a reference to Topic that precipitates a panic on Topic.ID().

Topic.ID() panics if the name does not have at least one forward slash, a test that is evidently meant to ensure that the topic is a "fully-qualified name" (topic.go#L865). It's a common courtesy to warn of possible panics in the documentation of exported package members. The Topic.ID() documentation, however, does not inform the user that the function may panic. This lack of warning seems egregious, as it is the service that supplies the reference to a Topic named _deleted-topic_.

It would be better not to panic if the name is a known sentinel value—it would be better not to panic at all. In the context of the Go client library, certainly it would be more consistent if Topic.ID() did not panic, as both Topic.Exists() and TopicConfig.ID() consider the Topic name similarly, yet neither panics on "bad topic name":

• Topic.Exists() returns false specifically if the name is _deleted-topic_ (topic.go#L885).
• TopicConfig.ID() returns an empty string if the name doesn't have at least one forward slash (topic.go#L299).

In general, a user should not have to call Topic.Exists() to determine whether calling Topic.ID() on a topic that has been returned by the server will panic. It would contradict reasonable expectations. Worse, it would cost another service call (topic.go#L888). Alternatively, if the user is meant to know about special behavior related to a sentinel value, then that value should be assigned to a package-level constant that is exported and well commented. Or, better than those two "shoulds": a stringer-like function such as Topic.ID() should never panic.

Example scenario

A Subscription's Topic has been deleted. Get config via Subscription.Config(). Call SubscriptionConfiguration.Topic.ID(). The program panics. See attached example.

Suggestions

• Make a package constant for the sentinel value (e.g., const DeletedTopicName = "_deleted-topic_"). Use this constant in the source code, rather than hard-coding the sentinel value as a string literal every time it must be considered.
• Change Topic.ID() to return an empty string when the name is _deleted-topic_.
• Describe deleted-topic behavior in Topic.ID, Topic.String, TopicConfig.ID, and any other relevant contexts ("If the Topic has been deleted, then the function returns …").
• New receiver function Topic.Name() that returns the globally unique path-like name or a documented sentinel value. While there is indeed already a stringer that returns Topic.name (i.e., Topic.String() : topic.go#L874), it seems unintuitive and unfriendly to leverage a stringer as the sole interface to a globally unique identifier and what turns out to be a crucial piece of information.

I am not a contributor of this project. If these suggestions seem reasonable, and external contributes are allowed, I would be glad to submit a PR.

google-cloud-go/pubsub/topic.go

Line 868 in e9a8e3a

panic("bad topic name")

deleted-topic-panic-example.go.txt

[hongalex]

Hey,

Thanks for writing up this detailed report (and reproduction).

We are preparing to release a v2 of the pubsub client library soon. Part of the changes include removing the existing layer for Topic and Subscription admin operations (Create, Get, Update, Delete, etc), and replacing it with an autogenerated layer that will live in cloud.google.com/go/v2

Another change is that we will be renaming the existing Topic struct to Publisher, since it more accurately describes the operation

publisher := pubsub.Publisher("my-topic")
publisher.Publish(...)

The first set of changes makes it so that the provided example would become obsolete (since Subscription.Config will no longer be available, and calling GetSubscription directly doesn't return a topic reference).

I agree that panic on a deleted resource when calling a stringer function is poor, so I'll keep this issue open until that is addressed. However, we will likely make that change after the v2 is released.