chore(data-service): use `identifyServerName` from `mongodb-build-info` COMPASS-9930 #7507

kraenhansen · 2025-10-27T12:25:05Z

Description

Draft: This PR is awaiting the release of mongodb-js/devtools-shared#585

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
If this change updates the UI, screenshots/videos are added and a design review is requested
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

addaleax · 2025-10-27T13:20:28Z

I think mongodb-js/devtools-shared#585 (comment) would be another good suggestion to the previous PR, in case you didn't see that

gribnoysup · 2025-10-27T13:35:13Z

packages/connection-info/src/connection-info.ts

+
+  /**
+   * Indicates whether this connection is to a genuine MongoDB server.
+   */
+  isGenuineMongoDB?: boolean;


I'm unsure about making this change: ConnectionInfo is not the most clear place to hold this information. This type holds info available before the connection gets established and doesn't have any dependencies on server state. The state that depends on the server is stored and accessed from the MongoDBInstance class throughout the application at the moment.

The change you are making right now will start storing this info with the connection on disk, but in a lot of cases the stored value might not even be true anymore between being retrieved from disk and creating the connection (like if someone edited the connection string in between). Now that this check depends on running commands against server and is not derived from connection string anymore, we only know for sure if we connected to a "genuine" mongodb after the connection was established and instance info was pulled.

I can't say I'm strongly opposed to storing it as part of ConnectionInfo as it's more or less a corner case we're trying to deal with, but If we want to move this state to ConnectionInfo, then we need to remove this from MongoDBInstance class completely. Having multiple sources of truth for information like this is an anti-pattern that we shouldn't introduce in the codebase.

I also think that making this value optional is not a good default taking into account that the "missing" value is basically "we don't know", enforcing explicit false checks everyhere in the code would be really hard, and so the chances of someone messing it up and doing !connectionInfo.isGenuineMongoDB somewhere in the code would be very easy.

I'll do another pass on getting this into the MongoDBInstance. I think it was mostly that wanted to run the checks only once and didn't want to run an async potentially costly check in the assistant drawer.

MongoDBInstance model instances stored per connection are specifically there to avoid re-fetching and provide unified access to these values, but there is no easy way to access them in the React rendering lifecycle, just be aware that this is not something we have a clear solution for at the moment (EDIT: and taking into account the outdated tech used there we don't want to rush something into the codebase here). Most of the code first maps model state to redux store and sets up the sync code in activate, then views access it via connect, but assistant drawer is not using a redux store

Pushed a suggestion to revert the isGenuineMongoDB to ConnectionInfo 👍

packages/compass-app-stores/src/provider.tsx

gribnoysup · 2025-10-29T08:43:28Z

packages/compass-app-stores/src/provider.tsx

+ * @return true if any connected connection is to a non-genuine MongoDB server.
+ */
+export function useHasNonGenuineConnections(): boolean {
+  const instancesManager = mongoDBInstancesManagerLocator();


Service locators by design shouldn't be used inside real hooks, this is an abstraction layer and if at any point we change how exactly DI works, there's no guarantees this will continue to work inside a normal hook so that's why this is disallowed (we also generally don't want services to be used directly in render, but we don't have another option here unfortunately). I'm not sure if you have tried to run this code already, but it throws immediately on the first render:

Using service locator function "mongoDBInstancesManagerLocator" outside of the service location lifecycle. Make sure that service locator function is passed as a second argument to the registerCompassPlugin method and is not used directly in a React render method.

I brought it to compiling, but I had to rush out the door and just pushed what I had at the moment 🙈

Sorry about that.

gribnoysup · 2025-10-29T08:52:50Z

packages/compass-connections/src/stores/connections-store-redux.ts

+              ConnectionOptions,
+              DeepPartial<ConnectionOptions>


Nit: unsure why you added explicit generic values here (and in a few other places), merge function can derive the type pretty well itself from provided arguments, and especially because DeepPartial is coming from a diffrerent library than the method itself, this seems like an odd choice for type definition here. Is this needed for some reason?

merge function can derive the type pretty well itself from provided arguments

This is mostly drive-by because I noticed the merge function wouldn't restrict fields of the second argument and I found that weird. I'll revert for now, since I'm no longer proposing changes to this code 👍

gribnoysup · 2025-10-29T09:43:27Z

packages/compass-app-stores/src/provider.tsx

+  const activeNonGenuineConnectionIds = useConnectionIds((conn) => {
+    if (conn.status !== 'connected') {
+      return false;
+    }
+
+    try {
+      const instance = instancesManager.getMongoDBInstanceForConnection(
+        conn.info.id
+      );
+      return instance.genuineMongoDB.isGenuine === false;
+    } catch {
+      // Instance not found or not ready yet
+      return false;
+    }
+  });
+
+  return activeNonGenuineConnectionIds.length > 0;


Just to give a different pespective on how to approach this, doesn't require any action, but something to consider. This code you have relies on the fact that instance model is currently created syncronously before we change connection status to connected, this is a fair and pretty stable assumption, but you also can just remove the connection list from the equation here completely: because instance models are created when connection is established, you can just observe instanceManager events to keep this state around, this will remove the need to check for connection status directly and to have a try-catch block (which should probably just throw if we couldn't get the instance for a connected connection as this is an unexpected state). Something like this:

Suggested change

const activeNonGenuineConnectionIds = useConnectionIds((conn) => {

if (conn.status !== 'connected') {

return false;

}

try {

const instance = instancesManager.getMongoDBInstanceForConnection(

conn.info.id

);

return instance.genuineMongoDB.isGenuine === false;

} catch {

// Instance not found or not ready yet

return false;

}

});

return activeNonGenuineConnectionIds.length > 0;

const hasAnyNonGenuineConnections = useCallback(

function () {

return Array.from(instancesManager.listMongoDBInstances().values()).some(

(instance) => {

return !instance.genuineMongoDB.isGenuine;

}

);

},

[instancesManager]

);

const [hasNonGenuineConnections, setHasNonGenuineConnections] = useState(

hasAnyNonGenuineConnections

);

useEffect(() => {

instancesManager.on('instance-started', () => {

setHasNonGenuineConnections(hasAnyNonGenuineConnections());

});

instancesManager.on('instance-removed', () => {

setHasNonGenuineConnections(hasAnyNonGenuineConnections());

});

}, [hasAnyNonGenuineConnections, instancesManager]);

return hasNonGenuineConnections;

I noticed you're not removing the listeners here, is that on purpose or just to keep the suggestion simple?

(I vaguely remember that being a pattern used elsewhere - or I might be mis-remembering)

Yeah, sorry, this is not a fully finished suggestion, just an example, this should be unsubscribing (inside activate / deactive lifecycle inside the plugins we would indeed do the cleanup in an easier way, you're right, but that's just a hook and so needs to do the whole thing itself)

gribnoysup

I'm guessing the ci is red because we're still strugging with publishing the package, so omitting that part this looks good to me

Copilot

Pull Request Overview

This PR migrates from the deprecated getGenuineMongoDB function to the new identifyServerName function from mongodb-build-info, updating the codebase to use an async approach for server identification. The change also renames the dbType property to serverName throughout the codebase for consistency.

Migration from synchronous getGenuineMongoDB to async identifyServerName function
Property rename from dbType to serverName across all interfaces and implementations
Package version upgrade to mongodb-build-info v1.8.1

Reviewed Changes

Copilot reviewed 20 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/instance-model/lib/model.js	Updates model property from `dbType` to `serverName`
packages/data-service/src/instance-detail-helper.ts	Replaces `getGenuineMongoDB` with async `identifyServerName` implementation
packages/data-service/src/instance-detail-helper.spec.ts	Updates test assertions to use `serverName` property
packages/data-service/src/data-service.spec.ts	Updates test expectations for property name change
packages/data-service/package.json	Upgrades `mongodb-build-info` to v1.8.1
packages/connection-form/package.json	Upgrades `mongodb-build-info` to v1.8.1
packages/compass/src/app/components/compass-assistant-drawer.tsx	Simplifies non-genuine connection detection using new hook
packages/compass/package.json	Upgrades `mongodb-build-info` to v1.8.1
packages/compass-web/src/compass-assistant-drawer.tsx	Simplifies non-genuine connection detection using new hook
packages/compass-web/package.json	Removes unused `mongodb-build-info` dependency
packages/compass-sidebar/src/modules/instance.ts	Updates property reference from `dbType` to `serverName`
packages/compass-sidebar/src/components/multiple-connections/sidebar.spec.tsx	Updates test data property name
packages/compass-e2e-tests/tests/logging.test.ts	Updates test assertion for property name change
packages/compass-e2e-tests/package.json	Upgrades `mongodb-build-info` to v1.8.1
packages/compass-connections/src/stores/connections-store-redux.ts	Updates imports and property references, simplifies genuine connection check
packages/compass-connections/package.json	Upgrades `mongodb-build-info` to v1.8.1
packages/compass-connections-navigation/package.json	Upgrades `mongodb-build-info` to v1.8.1
packages/compass-app-stores/src/provider.tsx	Adds new `useHasNonGenuineConnections` hook
configs/testing-library-compass/src/index.tsx	Updates mock service to use async `identifyServerName`
configs/testing-library-compass/package.json	Adds `mongodb-build-info` dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-31T11:07:48Z

packages/data-service/src/instance-detail-helper.ts

+async function buildGenuineMongoDBInfo(
+  uri: string,
+  client: MongoClient
+): Promise<GenuineMongoDBDetails> {
+  const adminDb = client.db('admin');
+  const serverName = await identifyServerName({
+    connectionString: uri,
+    async adminCommand(doc) {
+      try {
+        const result = await adminDb.command(doc);
+        debug('adminCommand(%O) = %O', doc, result);
+        return result;
+      } catch (err) {
+        debug('adminCommand(%O) failed %O', doc, err);
+        throw err;
+      }
+    },
+  });


The debug function used in the adminCommand callback may not be properly scoped. Consider importing or defining the debug function within this function's scope to ensure it's available.

Copilot · 2025-10-31T11:07:48Z

packages/compass-app-stores/src/provider.tsx

+  const [result, setResult] = useState(hasNonGenuineConnections);
+
+  useEffect(() => {
+    if (instancesManager) {
+      const updateResult = () => {
+        setResult(hasNonGenuineConnections);
+      };
+      instancesManager.on('instance-started', updateResult);
+      instancesManager.on('instance-removed', updateResult);
+      return () => {
+        instancesManager.off('instance-started', updateResult);
+        instancesManager.off('instance-removed', updateResult);
+      };
+    }
+  }, [instancesManager, setResult, hasNonGenuineConnections]);


The hasNonGenuineConnections function is included in the useEffect dependency array, which will cause the effect to re-run every render since it's recreated each time. Consider removing it from the dependencies or memoizing the function with useCallback.

Suggested change

const [result, setResult] = useState(hasNonGenuineConnections);

useEffect(() => {

if (instancesManager) {

const updateResult = () => {

setResult(hasNonGenuineConnections);

};

instancesManager.on('instance-started', updateResult);

instancesManager.on('instance-removed', updateResult);

return () => {

instancesManager.off('instance-started', updateResult);

instancesManager.off('instance-removed', updateResult);

};

}

}, [instancesManager, setResult, hasNonGenuineConnections]);

const [result, setResult] = useState(() => hasNonGenuineConnections());

useEffect(() => {

if (instancesManager) {

const updateResult = () => {

setResult(hasNonGenuineConnections());

};

instancesManager.on('instance-started', updateResult);

instancesManager.on('instance-removed', updateResult);

// Set initial value in case the event hasn't fired yet

updateResult();

return () => {

instancesManager.off('instance-started', updateResult);

instancesManager.off('instance-removed', updateResult);

};

}

}, [instancesManager]);

@copilot it's already wrapped in useCallback? 🙃

codeowners-service-app · 2025-10-31T11:09:58Z

Assigned ivandevp for team compass-developers because lerouxb is out of office.

Copilot · 2025-10-31T11:10:48Z

@kraenhansen I've opened a new pull request, #7525, to work on those changes. Once the pull request is ready, I'll request review from you.

gribnoysup reviewed Oct 27, 2025

View reviewed changes

kraenhansen self-assigned this Oct 28, 2025

kraenhansen force-pushed the kh/COMPASS-9930 branch from 5ef152d to 112c724 Compare October 29, 2025 07:46

kraenhansen commented Oct 29, 2025

View reviewed changes

packages/compass-app-stores/src/provider.tsx Show resolved Hide resolved

gribnoysup reviewed Oct 29, 2025

View reviewed changes

kraenhansen force-pushed the kh/COMPASS-9930 branch from 112c724 to 5acf06b Compare October 29, 2025 18:53

gribnoysup approved these changes Oct 30, 2025

View reviewed changes

kraenhansen force-pushed the kh/COMPASS-9930 branch from ddcb935 to 69d216c Compare October 30, 2025 22:06

kraenhansen added 8 commits October 31, 2025 10:17

Bumped mongodb-build-info dependency

f8c61c8

Rename dbType to serverName

02bda77

Use identifyServerName instead of getGenuineMongoDB

dd4a17f

Handle "unknown" server names as genuine

f89f301

Implement reusable useHasNonGenuineConnections hook

2b11c54

Use useHasNonGenuineConnections hook in assistant drawer

5fe10f1

Use useHasNonGenuineConnections hook in assistant drawer

6ab09ca

Call into mongodb-build-info when mocking the MockDataService

8fa3973

kraenhansen force-pushed the kh/COMPASS-9930 branch from 69d216c to 8fa3973 Compare October 31, 2025 09:18

kraenhansen marked this pull request as ready for review October 31, 2025 11:07

kraenhansen requested a review from a team as a code owner October 31, 2025 11:07

kraenhansen requested review from Copilot and lerouxb October 31, 2025 11:07

Copilot AI reviewed Oct 31, 2025

View reviewed changes

codeowners-service-app bot requested a review from ivandevp October 31, 2025 11:09

Copilot AI mentioned this pull request Oct 31, 2025

Fix useState initialization in useHasNonGenuineConnections hook #7525

Closed

kraenhansen requested a review from gribnoysup October 31, 2025 11:11

gribnoysup approved these changes Oct 31, 2025

View reviewed changes

kraenhansen merged commit 2dbe83b into main Oct 31, 2025
82 of 85 checks passed

kraenhansen deleted the kh/COMPASS-9930 branch October 31, 2025 12:05

-  const activeNonGenuineConnectionIds = useConnectionIds((conn) => {
-    if (conn.status !== 'connected') {
-      return false;
-    }
-    try {
-      const instance = instancesManager.getMongoDBInstanceForConnection(
-        conn.info.id
-      );
-      return instance.genuineMongoDB.isGenuine === false;
-    } catch {
-      // Instance not found or not ready yet
-      return false;
-    }
-  });
-  return activeNonGenuineConnectionIds.length > 0;
+  const hasAnyNonGenuineConnections = useCallback(
+    function () {
+      return Array.from(instancesManager.listMongoDBInstances().values()).some(
+        (instance) => {
+          return !instance.genuineMongoDB.isGenuine;
+        }
+      );
+    },
+    [instancesManager]
+  );
+  const [hasNonGenuineConnections, setHasNonGenuineConnections] = useState(
+    hasAnyNonGenuineConnections
+  );
+  useEffect(() => {
+    instancesManager.on('instance-started', () => {
+      setHasNonGenuineConnections(hasAnyNonGenuineConnections());
+    });
+    instancesManager.on('instance-removed', () => {
+      setHasNonGenuineConnections(hasAnyNonGenuineConnections());
+    });
+  }, [hasAnyNonGenuineConnections, instancesManager]);
+  return hasNonGenuineConnections;

chore(data-service): use identifyServerName from mongodb-build-info COMPASS-9930 #7507

chore(data-service): use identifyServerName from mongodb-build-info COMPASS-9930 #7507

Uh oh!

Conversation

kraenhansen commented Oct 27, 2025

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Uh oh!

addaleax commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gribnoysup Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeowners-service-app bot commented Oct 31, 2025

Uh oh!

Copilot AI commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chore(data-service): use `identifyServerName` from `mongodb-build-info` COMPASS-9930 #7507

chore(data-service): use `identifyServerName` from `mongodb-build-info` COMPASS-9930 #7507

gribnoysup Oct 27, 2025 •

edited

Loading

gribnoysup Oct 29, 2025 •

edited

Loading

kraenhansen Oct 29, 2025 •

edited

Loading