Duplicate subscription names silently fail

[jasonwilson]

Environment details

• OS: OSX 10.13.3
• Node.js version: v8.9.1
• npm version: 5.6.0
• @google-cloud/pubsub version: 0.18.0

Steps to reproduce

1. Create topic foo
2. Create topic bar
3. Create subscription quix on topic foo
4. Create subscription quix on topic bar (this is successful?)

If a subscription name already exists for a topic the client doesn't ensure it matches the same topic for the existing resource.

This check allows the subscription to appear to be created successfully despite <topic, name> pair not matching the original resource:
https://github.com/googleapis/nodejs-pubsub/blob/master/src/index.js#L249

Error caught internally:

err is:  { Error: Resource already exists in the project (resource=quix).
    at /tmp/test/grpc/src/client.js:554:15
  code: 6,
  metadata: Metadata { _internal_repr: {} },
  note: 'Exception occurred in retry method that was not classified as transient' }

Ideally createSubscription would fail when the topic name doesn't match the existing resource.

[callmehiphop]

@stephenplusplus I'm not sure on the origin of this code, but it looks like its been around for quite some time. Do you have any insights on why we swallow this error?

[stephenplusplus]

It looks like it came from the big redesign: https://github.com/GoogleCloudPlatform/google-cloud-node/pull/2380/files

A code 6 is a 409 Conflict. I assume we're swallowing that error because if the user is trying to create something that already exists, the fact that it already does means "job done!"

@jasonwilson sorry, but I wasn't able to follow exactly what you were explaining. Maybe showing some code would help.

[callmehiphop]

@stephenplusplus we changed it from an http code to a grpc code in the redesign, but it has exsisted for much longer than that. I wasn't sure if it had something to do with the old mechanism of automatically creating subscription names or something.

[stephenplusplus]

Found the origin! googleapis/google-cloud-node#465 by @jonparrott.

We used to have an option, reuseExisting. Looks like we removed the option, but not the behavior. We can revert it so that if (409) return err, although at this point, that would be a breaking change.

[theacodes]

We're not v1.0.0 yet, right? If we want, let's remove it. It's surprising.

[marcusradell]

Adding a use case for clarity:

Creating the same subscription quix on the same topic foo more than once should not fail after a fix have been made.

[stephenplusplus]

I'm still a bit confused about the scenario reported in the first post. Would anyone be able to do an ELI5 on that for me?

[jasonwilson]

@stephenplusplus the client will seemingly allow the user to create two subscriptions that share the same name on different topics. Underneath the hood the client is saying "I've already got this resource you're good to go!" but the topic doesn't necessarily match what was passed into createSubscription.

[stephenplusplus]

Could you show code that demonstrates the issue?

[jasonwilson]

const PubSub = require(`@google-cloud/pubsub`);

// Creates a client
const pubsub = new PubSub();

(async () => {
  const subscriptionName = 'quix';
  const fooTopic = 'foo';
  const barTopic = 'bar';

  await pubsub.createTopic(fooTopic);
  await pubsub.createTopic(barTopic);

  // Create a subsciption 'quix' on topic foo
  const subscription1 = await pubsub.createSubscription(fooTopic, subscriptionName);

  // Create a subsciption 'quix' on topic bar
  // subscription2, despite being configured with a different topic returns successfully
  const subscription2 = await pubsub.createSubscription(barTopic, subscriptionName);

})();