chore(data-service): use identifyServerName from mongodb-build-info COMPASS-9930
#7507
Conversation
|
I think mongodb-js/devtools-shared#585 (comment) would be another good suggestion to the previous PR, in case you didn't see that |
|
|
||
| /** | ||
| * Indicates whether this connection is to a genuine MongoDB server. | ||
| */ | ||
| isGenuineMongoDB?: boolean; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Pushed a suggestion to revert the isGenuineMongoDB to ConnectionInfo 👍
kraenhansen
force-pushed
the
kh/COMPASS-9930
branch
from
5ef152d to
112c724
Compare
| * @return true if any connected connection is to a non-genuine MongoDB server. | ||
| */ | ||
| export function useHasNonGenuineConnections(): boolean { | ||
| const instancesManager = mongoDBInstancesManagerLocator(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ConnectionOptions, | ||
| DeepPartial<ConnectionOptions> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
| 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; |
There was a problem hiding this comment.
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:
| 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; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
kraenhansen
force-pushed
the
kh/COMPASS-9930
branch
from
112c724 to
5acf06b
Compare
gribnoysup
left a comment
gribnoysup
left a comment
There was a problem hiding this comment.
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
kraenhansen
force-pushed
the
kh/COMPASS-9930
branch
from
ddcb935 to
69d216c
Compare
kraenhansen
force-pushed
the
kh/COMPASS-9930
branch
from
69d216c to
8fa3973
Compare
There was a problem hiding this comment.
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
getGenuineMongoDBto asyncidentifyServerNamefunction - Property rename from
dbTypetoserverNameacross all interfaces and implementations - Package version upgrade to
mongodb-build-infov1.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.
| 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; | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
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.
| 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]); |
There was a problem hiding this comment.
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.
| 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]); |
There was a problem hiding this comment.
@copilot it's already wrapped in useCallback? 🙃
|
Assigned |
|
@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. |
Description
Draft: This PR is awaiting the release of mongodb-js/devtools-shared#585
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes