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

itok01 · 2026-01-11T15:07:09Z

The DataConnectService.getDataConnect() method was caching DataConnect instances using only location and serviceId as the cache key, ignoring the connector field from ConnectorConfig.

This caused incorrect behavior when calling getDataConnect() with different connector configs that share the same location and serviceId:

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!)

Fixed by including connector in the cache key. Added 5 new unit tests for cache key behavior.

Fixes #3054.

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.

gemini-code-assist · 2026-01-11T15:07:22Z

Summary of Changes

Hello @itok01, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical caching issue within the DataConnectService that caused DataConnect instances to be incorrectly reused. By incorporating the connector field into the cache key, the system now accurately distinguishes and caches instances based on their full configuration, preventing unexpected behavior and ensuring data integrity. The change is supported by comprehensive new unit tests.

Highlights

Caching Bug Fix: The DataConnectService.getDataConnect() method was incorrectly caching DataConnect instances by only using location and serviceId as the cache key, leading to unintended reuse of instances when the connector field differed.
Cache Key Enhancement: The cache key generation has been updated to include the connector field from ConnectorConfig, ensuring that each unique combination of location, serviceId, and connector results in a distinct DataConnect instance.
New Unit Tests: Five new unit tests have been added to thoroughly validate the corrected caching behavior, covering scenarios with identical configurations, differing connectors, locations, service IDs, and the presence/absence of the connector field.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request correctly fixes a caching bug in DataConnectService.getDataConnect() by including the connector property in the cache key. The accompanying unit tests are well-written and cover the intended fix. My review includes a suggestion to make the cache key generation more robust to prevent potential key collisions, which is a subtle bug in the current implementation. I've also recommended adding a test case to cover this specific scenario.

src/data-connect/data-connect.ts

test/unit/data-connect/data-connect.spec.ts

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

stephenarosaj

This is a solid PR @itok01! I went ahead and requested some small changes throughout the changed files. Take a look and feel free to reach out with any questions :)

And thanks so much for submitting this!!

stephenarosaj · 2026-01-12T19:59:38Z

src/data-connect/data-connect.ts


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


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

stephenarosaj · 2026-01-12T20:55:10Z

test/unit/data-connect/data-connect.spec.ts

  });
 });
+
+describe('getDataConnect() caching', () => {


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

stephenarosaj · 2026-01-12T21:03:22Z

test/unit/data-connect/data-connect.spec.ts

+      const dc1 = getDataConnect(config);
+      const dc2 = getDataConnect(config);
+
+      expect(dc1).to.equal(dc2);


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

stephenarosaj · 2026-01-12T21:26:30Z

test/unit/data-connect/data-connect.spec.ts

+      const dc1 = getDataConnect(config1);
+      const dc2 = getDataConnect(config2);
+
+      expect(dc1).to.not.equal(dc2);


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

stephenarosaj · 2026-01-12T21:43:41Z

test/unit/data-connect/data-connect.spec.ts

+      expect(dc1).to.not.equal(dc2);
+    });
+
+    it('should handle connector being undefined vs defined', () => {


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

stephenarosaj · 2026-01-12T21:45:09Z

test/unit/data-connect/data-connect.spec.ts

+      expect(dc1).to.not.equal(dc2);
+    });
+
+    it('should handle connector being undefined vs defined', () => {


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

stephenarosaj · 2026-01-12T21:45:59Z

test/unit/data-connect/data-connect.spec.ts

+      expect(dc2.connectorConfig.connector).to.be.undefined;
+    });
+
+    it('should not have cache collisions with ambiguous keys', () => {


this is a good test. could you also add one for different ordering of ConnectorConfig keys?

- 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

itok01 · 2026-01-13T05:25:50Z

Thank you for the thorough review @stephenarosaj! I've addressed all your feedback:

Cache key generation:

Updated to use Object.keys().sort().reduce() before JSON.stringify() to ensure consistent keys regardless of property order
Kept connector as undefined instead of converting to empty string, aligning with the Client JS SDK approach

Test updates:

Renamed describe to 'getDataConnect()'
Changed to .to.deep.equal / .to.not.deep.equal for DataConnect comparisons
Renamed test to be more descriptive
Added test for empty string vs undefined connector
Added test for different ConnectorConfig key orderings

All 165 Data Connect tests pass. Let me know if there's anything else!

gemini-code-assist bot reviewed Jan 11, 2026

View reviewed changes

src/data-connect/data-connect.ts Outdated Show resolved Hide resolved

test/unit/data-connect/data-connect.spec.ts Show resolved Hide resolved

itok01 mentioned this pull request Jan 11, 2026

[Data Connect] getDataConnect() ignores connector field in cache key #3054

Open

stephenarosaj requested changes Jan 12, 2026

View reviewed changes

itok01 requested a review from stephenarosaj January 13, 2026 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

itok01 commented Jan 11, 2026

Uh oh!

gemini-code-assist bot commented Jan 11, 2026

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

stephenarosaj left a comment

Uh oh!

stephenarosaj Jan 12, 2026

Uh oh!

stephenarosaj Jan 12, 2026

Uh oh!

stephenarosaj Jan 12, 2026

Uh oh!

stephenarosaj Jan 12, 2026

Uh oh!

stephenarosaj Jan 12, 2026

Uh oh!

stephenarosaj Jan 12, 2026

Uh oh!

stephenarosaj Jan 12, 2026

Uh oh!

itok01 commented Jan 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Are you sure you want to change the base?

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

Conversation

itok01 commented Jan 11, 2026

Uh oh!

gemini-code-assist bot commented Jan 11, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

stephenarosaj left a comment

Choose a reason for hiding this comment

Uh oh!

stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

stephenarosaj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

itok01 commented Jan 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants