From 208b680bea796eeafe44574265a27d3794bfafec Mon Sep 17 00:00:00 2001 From: Takuya Igei Date: Sun, 11 Jan 2026 23:09:51 +0900 Subject: [PATCH 1/3] fix(data-connect): Include connector in DataConnect cache key The `DataConnectService.getDataConnect()` method was caching DataConnect instances using only `location` and `serviceId` as the cache key, ignoring the `connector` field. This caused a bug where calling `getDataConnect()` with different connector configs but the same location and serviceId would return the same (first) cached instance instead of creating separate instances for each connector. **Problem:** ```typescript const dc1 = getDataConnect({ location: 'us-west2', serviceId: 'svc', connector: 'public' }); const dc2 = getDataConnect({ location: 'us-west2', serviceId: 'svc', connector: 'user' }); // dc1 === dc2 was true (BUG!) // dc2.connectorConfig.connector was 'public' instead of 'user' ``` **Solution:** Include `connector` in the cache key: ```typescript const id = `${location}-${serviceId}-${connector ?? ''}`; ``` **Impact:** This bug affects any application using multiple Data Connect connectors with the same location and serviceId. Operations intended for one connector would be incorrectly routed to another, causing "operation not found" errors. **Tests:** Added 5 new tests to verify cache key behavior with different connector configurations. --- src/data-connect/data-connect.ts | 2 +- test/unit/data-connect/data-connect.spec.ts | 109 ++++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/data-connect/data-connect.ts b/src/data-connect/data-connect.ts index c48e3d1c78..8926cbbdb8 100644 --- a/src/data-connect/data-connect.ts +++ b/src/data-connect/data-connect.ts @@ -36,7 +36,7 @@ export class DataConnectService { } getDataConnect(connectorConfig: ConnectorConfig): DataConnect { - const id = `${connectorConfig.location}-${connectorConfig.serviceId}`; + const id = `${connectorConfig.location}-${connectorConfig.serviceId}-${connectorConfig.connector ?? ''}`; const dc = this.dataConnectInstances.get(id); if (typeof dc !== 'undefined') { return dc; diff --git a/test/unit/data-connect/data-connect.spec.ts b/test/unit/data-connect/data-connect.spec.ts index 992d1c3861..b6a3f7559a 100644 --- a/test/unit/data-connect/data-connect.spec.ts +++ b/test/unit/data-connect/data-connect.spec.ts @@ -224,3 +224,112 @@ describe('DataConnect', () => { }); }); }); + +describe('getDataConnect() caching', () => { + const mockOptions = { + credential: new mocks.MockCredential(), + projectId: 'test-project', + }; + let mockApp: FirebaseApp; + let getAppStub: sinon.SinonStub; + + beforeEach(() => { + mockApp = mocks.appWithOptions(mockOptions); + getAppStub = sinon.stub(appIndex, 'getApp').returns(mockApp); + }); + + afterEach(() => { + getAppStub.restore(); + return mockApp.delete(); + }); + + describe('should cache DataConnect instances correctly', () => { + it('should return the same instance for identical connector configs', () => { + const config: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + connector: 'my-connector', + }; + + const dc1 = getDataConnect(config); + const dc2 = getDataConnect(config); + + expect(dc1).to.equal(dc2); + }); + + it('should return different instances for different connectors with same location and serviceId', () => { + const config1: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + connector: 'connector-a', + }; + const config2: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + connector: 'connector-b', + }; + + const dc1 = getDataConnect(config1); + const dc2 = getDataConnect(config2); + + expect(dc1).to.not.equal(dc2); + expect(dc1.connectorConfig.connector).to.equal('connector-a'); + expect(dc2.connectorConfig.connector).to.equal('connector-b'); + }); + + it('should return different instances for different locations', () => { + const config1: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + connector: 'my-connector', + }; + const config2: ConnectorConfig = { + location: 'us-east1', + serviceId: 'my-service', + connector: 'my-connector', + }; + + const dc1 = getDataConnect(config1); + const dc2 = getDataConnect(config2); + + expect(dc1).to.not.equal(dc2); + }); + + it('should return different instances for different serviceIds', () => { + const config1: ConnectorConfig = { + location: 'us-west2', + serviceId: 'service-a', + connector: 'my-connector', + }; + const config2: ConnectorConfig = { + location: 'us-west2', + serviceId: 'service-b', + connector: 'my-connector', + }; + + const dc1 = getDataConnect(config1); + const dc2 = getDataConnect(config2); + + expect(dc1).to.not.equal(dc2); + }); + + it('should handle connector being undefined vs defined', () => { + const configWithConnector: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + connector: 'my-connector', + }; + const configWithoutConnector: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + }; + + const dc1 = getDataConnect(configWithConnector); + const dc2 = getDataConnect(configWithoutConnector); + + expect(dc1).to.not.equal(dc2); + expect(dc1.connectorConfig.connector).to.equal('my-connector'); + expect(dc2.connectorConfig.connector).to.be.undefined; + }); + }); +}); From 69b1d21290733bf7dd36369a85de19d7bf46e459 Mon Sep 17 00:00:00 2001 From: Takuya Igei Date: Mon, 12 Jan 2026 00:13:15 +0900 Subject: [PATCH 2/3] refactor: Use JSON.stringify for cache key to prevent collisions Address code review feedback from gemini-code-assist: - Use JSON.stringify instead of hyphen separator for cache key to prevent potential collisions when location/serviceId contain hyphens - Add test case for ambiguous key collision scenario --- src/data-connect/data-connect.ts | 2 +- test/unit/data-connect/data-connect.spec.ts | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/data-connect/data-connect.ts b/src/data-connect/data-connect.ts index 8926cbbdb8..e8162b852a 100644 --- a/src/data-connect/data-connect.ts +++ b/src/data-connect/data-connect.ts @@ -36,7 +36,7 @@ export class DataConnectService { } getDataConnect(connectorConfig: ConnectorConfig): DataConnect { - const id = `${connectorConfig.location}-${connectorConfig.serviceId}-${connectorConfig.connector ?? ''}`; + const id = JSON.stringify([connectorConfig.location, connectorConfig.serviceId, connectorConfig.connector ?? '']); const dc = this.dataConnectInstances.get(id); if (typeof dc !== 'undefined') { return dc; diff --git a/test/unit/data-connect/data-connect.spec.ts b/test/unit/data-connect/data-connect.spec.ts index b6a3f7559a..5dfe87031a 100644 --- a/test/unit/data-connect/data-connect.spec.ts +++ b/test/unit/data-connect/data-connect.spec.ts @@ -331,5 +331,21 @@ describe('getDataConnect() caching', () => { expect(dc1.connectorConfig.connector).to.equal('my-connector'); expect(dc2.connectorConfig.connector).to.be.undefined; }); + + it('should not have cache collisions with ambiguous keys', () => { + const config1: ConnectorConfig = { + location: 'us-west2-a', + serviceId: 'b', + }; + const config2: ConnectorConfig = { + location: 'us-west2', + serviceId: 'a-b', + }; + + const dc1 = getDataConnect(config1); + const dc2 = getDataConnect(config2); + + expect(dc1).to.not.equal(dc2); + }); }); }); From eb3d944c1ca953c6a9d434abe0f3458451c652d3 Mon Sep 17 00:00:00 2001 From: Takuya Igei Date: Tue, 13 Jan 2026 14:18:18 +0900 Subject: [PATCH 3/3] refactor(data-connect): Address review feedback for cache key generation - Sort ConnectorConfig keys before JSON.stringify for key ordering consistency - Keep connector undefined instead of empty string (align with Client JS SDK) - Update tests: use deep.equal, add empty string vs undefined test, add key ordering test --- src/data-connect/data-connect.ts | 8 ++- test/unit/data-connect/data-connect.spec.ts | 61 ++++++++++++++++++--- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/src/data-connect/data-connect.ts b/src/data-connect/data-connect.ts index e8162b852a..a1feb3036b 100644 --- a/src/data-connect/data-connect.ts +++ b/src/data-connect/data-connect.ts @@ -36,7 +36,13 @@ export class DataConnectService { } getDataConnect(connectorConfig: ConnectorConfig): DataConnect { - const id = JSON.stringify([connectorConfig.location, connectorConfig.serviceId, connectorConfig.connector ?? '']); + const orderedConfig = Object.keys(connectorConfig) + .sort() + .reduce((obj, key) => { + obj[key] = connectorConfig[key as keyof ConnectorConfig]; + return obj; + }, {} as Record); + const id = JSON.stringify(orderedConfig); const dc = this.dataConnectInstances.get(id); if (typeof dc !== 'undefined') { return dc; diff --git a/test/unit/data-connect/data-connect.spec.ts b/test/unit/data-connect/data-connect.spec.ts index 5dfe87031a..7cad7ce0a1 100644 --- a/test/unit/data-connect/data-connect.spec.ts +++ b/test/unit/data-connect/data-connect.spec.ts @@ -225,7 +225,7 @@ describe('DataConnect', () => { }); }); -describe('getDataConnect() caching', () => { +describe('getDataConnect()', () => { const mockOptions = { credential: new mocks.MockCredential(), projectId: 'test-project', @@ -254,7 +254,7 @@ describe('getDataConnect() caching', () => { const dc1 = getDataConnect(config); const dc2 = getDataConnect(config); - expect(dc1).to.equal(dc2); + expect(dc1).to.deep.equal(dc2); }); it('should return different instances for different connectors with same location and serviceId', () => { @@ -272,7 +272,7 @@ describe('getDataConnect() caching', () => { const dc1 = getDataConnect(config1); const dc2 = getDataConnect(config2); - expect(dc1).to.not.equal(dc2); + expect(dc1).to.not.deep.equal(dc2); expect(dc1.connectorConfig.connector).to.equal('connector-a'); expect(dc2.connectorConfig.connector).to.equal('connector-b'); }); @@ -292,7 +292,7 @@ describe('getDataConnect() caching', () => { const dc1 = getDataConnect(config1); const dc2 = getDataConnect(config2); - expect(dc1).to.not.equal(dc2); + expect(dc1).to.not.deep.equal(dc2); }); it('should return different instances for different serviceIds', () => { @@ -310,10 +310,10 @@ describe('getDataConnect() caching', () => { const dc1 = getDataConnect(config1); const dc2 = getDataConnect(config2); - expect(dc1).to.not.equal(dc2); + expect(dc1).to.not.deep.equal(dc2); }); - it('should handle connector being undefined vs defined', () => { + it('should consider configs with connector undefined and defined as different', () => { const configWithConnector: ConnectorConfig = { location: 'us-west2', serviceId: 'my-service', @@ -327,11 +327,30 @@ describe('getDataConnect() caching', () => { const dc1 = getDataConnect(configWithConnector); const dc2 = getDataConnect(configWithoutConnector); - expect(dc1).to.not.equal(dc2); + expect(dc1).to.not.deep.equal(dc2); expect(dc1.connectorConfig.connector).to.equal('my-connector'); expect(dc2.connectorConfig.connector).to.be.undefined; }); + it('should consider configs with connector empty string and undefined as different', () => { + const configWithEmptyConnector: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + connector: '', + }; + const configWithoutConnector: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + }; + + const dc1 = getDataConnect(configWithEmptyConnector); + const dc2 = getDataConnect(configWithoutConnector); + + expect(dc1).to.not.deep.equal(dc2); + expect(dc1.connectorConfig.connector).to.equal(''); + expect(dc2.connectorConfig.connector).to.be.undefined; + }); + it('should not have cache collisions with ambiguous keys', () => { const config1: ConnectorConfig = { location: 'us-west2-a', @@ -345,7 +364,33 @@ describe('getDataConnect() caching', () => { const dc1 = getDataConnect(config1); const dc2 = getDataConnect(config2); - expect(dc1).to.not.equal(dc2); + expect(dc1).to.not.deep.equal(dc2); + }); + + it('should return the same instance regardless of ConnectorConfig key ordering', () => { + // Define configs with different key orderings + const config1: ConnectorConfig = { + location: 'us-west2', + serviceId: 'my-service', + connector: 'my-connector', + }; + const config2: ConnectorConfig = { + serviceId: 'my-service', + connector: 'my-connector', + location: 'us-west2', + }; + const config3: ConnectorConfig = { + connector: 'my-connector', + location: 'us-west2', + serviceId: 'my-service', + }; + + const dc1 = getDataConnect(config1); + const dc2 = getDataConnect(config2); + const dc3 = getDataConnect(config3); + + expect(dc1).to.deep.equal(dc2); + expect(dc2).to.deep.equal(dc3); }); }); });