fix: always fill the topic and sub names when creating from a PubSub object (#1816)

feywind · web-flow · commit be8ed530e327 · 2023-09-07T13:55:06.000-04:00
diff --git a/src/pubsub.ts b/src/pubsub.ts
@@ -561,6 +561,17 @@ export class PubSub {
           return;
         }
         subscription.metadata = resp!;
+
+        // If this is the first call we've made, the projectId might be empty still.
+        if (subscription.name?.includes(PROJECT_ID_PLACEHOLDER)) {
+          if (subscription.metadata && subscription.metadata.name) {
+            subscription.name = Subscription.formatName_(
+              this.projectId,
+              subscription.metadata.name
+            );
+          }
+        }
+
         callback!(null, subscription, resp!);
       }
     );
@@ -655,6 +666,14 @@ export class PubSub {
           return;
         }
         topic.metadata = resp!;
+
+        // If this is the first call we've made, the projectId might be empty still.
+        if (topic.name?.includes(PROJECT_ID_PLACEHOLDER)) {
+          if (topic.metadata && topic.metadata.name) {
+            topic.name = Topic.formatName_(this.projectId, topic.metadata.name);
+          }
+        }
+
         callback!(null, topic, resp!);
       }
     );
diff --git a/test/pubsub.ts b/test/pubsub.ts
@@ -33,7 +33,6 @@ const PKG = require('../../package.json');
 const sandbox = sinon.createSandbox();
 
 const fakeCreds = {} as gax.grpc.ChannelCredentials;
-sandbox.stub(gax.grpc.credentials, 'createInsecure').returns(fakeCreds);
 
 const subscriptionCached = subby.Subscription;
 
@@ -49,6 +48,11 @@ function Subscription(
   return new overrideFn(pubsub, name, options);
 }
 
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+(Subscription as any).formatName_ = (): string => {
+  return 'formatted';
+};
+
 let promisified = false;
 const fakeUtil = Object.assign({}, util, {
   promisifySome(
@@ -92,6 +96,10 @@ class FakeTopic {
   constructor(...args: Array<{}>) {
     this.calledWith_ = args;
   }
+
+  static formatName_(): string {
+    return 'foo';
+  }
 }
 
 let extended = false;
@@ -187,6 +195,11 @@ describe('PubSub', () => {
     googleAuthOverride = null;
     pubsub = new PubSub(OPTIONS);
     pubsub.projectId = PROJECT_ID;
+    sandbox.stub(gax.grpc.credentials, 'createInsecure').returns(fakeCreds);
+  });
+
+  afterEach(() => {
+    sandbox.restore();
   });
 
   describe('instantiation', () => {
@@ -554,13 +567,15 @@ describe('PubSub', () => {
 
       it('should return Subscription & resp to the callback', done => {
         const subscription = {};
-        pubsub.subscription = () => {
+        sandbox.stub(pubsub, 'subscription').callsFake(() => {
           return subscription as subby.Subscription;
-        };
+        });
 
-        pubsub.request = (config, callback: Function) => {
-          callback(null, apiResponse);
-        };
+        sandbox
+          .stub(pubsub, 'request')
+          .callsFake((config, callback: Function) => {
+            callback(null, apiResponse);
+          });
 
         function callback(
           err?: Error | null,
@@ -575,6 +590,31 @@ describe('PubSub', () => {
 
         pubsub.createSubscription?.(TOPIC_NAME, SUB_NAME, callback);
       });
+
+      it('should fill the subscription object name if projectId was empty', async () => {
+        const subscription = {};
+        pubsub.projectId = undefined;
+        sandbox.stub(pubsub, 'subscription').callsFake(() => {
+          // Simulate the project ID not being resolved.
+          const sub = subscription as subby.Subscription;
+          sub.name = '{{projectId}}/foo/bar';
+          return sub;
+        });
+
+        sandbox
+          .stub(pubsub, 'request')
+          .callsFake((config, callback: Function) => {
+            callback(null, apiResponse);
+          });
+
+        const [sub, resp] = await pubsub.createSubscription!(
+          TOPIC_NAME,
+          SUB_NAME
+        )!;
+        assert.strictEqual(sub, subscription);
+        assert.strictEqual(sub.name.includes('{{'), false);
+        assert.strictEqual(resp, apiResponse);
+      });
     });
   });
 
@@ -625,12 +665,17 @@ describe('PubSub', () => {
     });
 
     describe('success', () => {
-      const apiResponse = {};
+      const apiResponse = {
+        name: 'new-topic',
+      };
+      let requestStub: sinon.SinonStub<unknown[], unknown>;
 
       beforeEach(() => {
-        pubsub.request = (config, callback: Function) => {
-          callback(null, apiResponse);
-        };
+        requestStub = sandbox
+          .stub(pubsub, 'request')
+          .callsFake((config, callback: Function) => {
+            callback(null, apiResponse);
+          });
       });
 
       it('should return a Topic object', done => {
@@ -656,6 +701,33 @@ describe('PubSub', () => {
           done();
         });
       });
+
+      it('should fill the topic object name if projectId was empty', async () => {
+        const topicName = 'new-topic';
+        const topicInstance = {};
+
+        sandbox.stub(pubsub, 'topic').callsFake(name => {
+          assert.strictEqual(name, topicName);
+
+          // Simulate the project ID not being resolved.
+          const topic = topicInstance as Topic;
+          topic.name = 'projects/{{projectId}}/topics/new-topic';
+          return topic;
+        });
+
+        requestStub.restore();
+        sandbox
+          .stub(pubsub, 'request')
+          .callsFake((config, callback: Function) => {
+            pubsub.projectId = 'projectId';
+            callback(null, apiResponse);
+          });
+
+        const [topic, resp] = await pubsub.createTopic!(topicName)!;
+        assert.strictEqual(topic, topicInstance);
+        assert.strictEqual(topic.name.includes('{{'), false);
+        assert.strictEqual(resp, apiResponse);
+      });
     });
   });
 
