pubsub: consider returning non-nil error from NewClient when projectID is invalid

[dmitshur]

Is your feature request related to a problem? Please describe.
This issue is motivated by my experience in golang/go#44948 (comment).

We had code that was publishing to a pub/sub topic that looked roughly like this:

// publishToTopic publishes body to the given topic.
// It returns the ID of the message published.
func publishToTopic(ctx context.Context, topic string, body []byte) (string, error) {
	client, err := pubsub.NewClient(ctx, projectID)
	if err != nil {
		return "", fmt.Errorf("pubsub.NewClient: %v", err)
	}

	t := client.Topic(topic)
	resp := t.Publish(ctx, &pubsub.Message{Data: body})
	id, err := resp.Get(ctx)
	if err != nil {
		return "", fmt.Errorf("topic.Publish: %v", err)
	}
	return id, nil
}

When the value of projectID became the empty string due to an environment misconfiguration, the publishToTopic function failed with the following error message:

topic.Publish: rpc error: code = InvalidArgument desc = You have passed an invalid argument to the service (argument=).

The error message said that I passed an invalid argument, and that the argument I passed was the empty string. With that information, it was quite hard to understand what the problem actually was. I wasn't providing any arguments in the high-level code shown above. (I'm guessing the projectID is one of the internal arguments used when publishing.)

Describe the solution you'd like
As far as I can tell there's no situation where pubsub.NewClient(ctx, "") is a non-error condition.

If that's the case, perhaps the readability of the error in this situation can be improved by having NewClient return something like errors.New("projectID must be a non-empty string") to make the error more readable:

topic.NewClient: projectID must be a non-empty string

Describe alternatives you've considered

• If the error check cannot be done in NewClient, then perhaps the error message returned by Publish can be improved to be more clear which argument was empty, like "You have passed an invalid value for the projectID argument to the service".

• If the case where a user accidentally calls pubsub.NewClient(ctx, projectID) with an empty string is deemed rare not to add more complexity to the client code, then another alternative solution is to close this for now and don't take any action unless this problem keeps happening.

Additional context
Issue #1294 may be related.

[hongalex]

Apologies for delay. I agree that the error returned is very obscure and difficult to interpret.NewClient calls are actually lazily loaded, so there is no network call to check permissions. This makes it difficult to check permissions with regards to arbitrary projects on client instantiation, and would have to be done when there's actually an RPC invoked.

With that said, we are looking to standardize detection of project IDs from various sources (the GOOGLE_CLOUD_PROJECT environment variable and Application Default Credentials). This will catch the empty-string project ID cases that have been reported in this issue. I'll come back and update this once that feature has been added to our clients.

[rxbynerd]

Just hit this again 18 months later inside a Cloud Function — it's an exceptionally confusing error, is it possible to do anything about this in the client without a server-side change?

[arikkfir]

Yeah seems a bit silly to wait 2 years for an error message fix 😄
But more importantly than the error message - detection of the project ID is not implement, and that's way more important IMO.

Any news on detecting project ID automatically from ADC?

[hongalex]

Since I found that we have precedent in Firestore, I think we're ok with making the change to return an error on empty string only. This does not fix the issue when other kinds of invalid projects are passed in, but I think that's fine.

ADC will be supported once the above PR is merged, thanks for your patience.