Skip to content

fix(data-connect): Include connector in DataConnect cache key #3055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
itok01 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(data-connect): Include connector in DataConnect cache key #3055

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
208b680
fix(data-connect): Include connector in DataConnect cache key
itok01 Jan 11, 2026
69b1d21
refactor: Use JSON.stringify for cache key to prevent collisions
itok01 Jan 11, 2026
eb3d944
refactor(data-connect): Address review feedback for cache key generation
itok01 Jan 13, 2026
358d2db
Merge branch 'main' into fix/getdataconnect-connector-caching
itok01 Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/data-connect/data-connect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class DataConnectService {
}

getDataConnect(connectorConfig: ConnectorConfig): DataConnect {
const id = `${connectorConfig.location}-${connectorConfig.serviceId}`;
const id = JSON.stringify([connectorConfig.location, connectorConfig.serviceId, connectorConfig.connector ?? '']);
Copy link
Contributor

@stephenarosaj stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This successfully makes the cache keys unique per connector. JSON.stringify() is also what we use in Data Connect's Client JS SDK (link), so that's definitely the approach we want.

I feel that instead of using an empty string when the connector field is undefined, we should keep it undefined and just call JSON.stringify(connectorConfig), so that we're aligned with the Client JS SDK. This also makes it so that if a user explicitly sets their connector field to be an empty string, there's no collision with a config that has the field unset.

I also think that in both the Client JS SDK and here, we should be ordering our keys before we stringify. That way, two ConnectorConfig objects defined as { serviceId: "s", location: "l" } and { location: "l", serviceId: "s" } return the same DataConnect instance. Something like:

const orderedConfig = Object.keys(connectorConfig)
	.sort()
	.reduce((obj, key) => {
		obj[key] = connectorConfig[key as keyof ConnectorConfig];
		return obj;
	}, {} as any);
const id = JSON.stringify(orderedConfig);

const dc = this.dataConnectInstances.get(id);
if (typeof dc !== 'undefined') {
return dc;
Expand Down
125 changes: 125 additions & 0 deletions test/unit/data-connect/data-connect.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,3 +224,128 @@ describe('DataConnect', () => {
});
});
});

describe('getDataConnect() caching', () => {
Copy link
Contributor

@stephenarosaj stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename this section just 'getDataConnect()'?

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);
Copy link
Contributor

@stephenarosaj stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably use .to.deep.equal when checking DataConnect instances

});

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);
Copy link
Contributor

@stephenarosaj stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above - .to.not.deep.equal

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', () => {
Copy link
Contributor

@stephenarosaj stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you make this more descriptive? something like "should consider configs with connector undefined and defined as different"

Copy link
Contributor

@stephenarosaj stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add a test here to make sure the difference between empty connector: "" and no defined value for connector is picked up on by the code

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;
});

it('should not have cache collisions with ambiguous keys', () => {
Copy link
Contributor

@stephenarosaj stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good test. could you also add one for different ordering of ConnectorConfig 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);
});
});
});