chore: refactor connections to use the new ConnectionManager to isolate long running processes like OIDC connections MCP-81 #423
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now the connection itself and the user flow is managed by the new connection manager. There are tests missing for this class, that will be added in a following commit.
these test should validate that connecting to a cluster is fine and also the connection mechanism inference is working as expected until we support both oidc flows.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the connection handling architecture by introducing a centralized ConnectionManager class that manages all MongoDB connections, replacing the previous approach where connection logic was distributed across tools and sessions. The main goal is to isolate long-running processes like OIDC connections and provide better error handling and debugging capabilities.
Key changes include:
- Introduction of
ConnectionManagerclass with state tracking and event emission - Consolidation of connection logic from both Atlas and MongoDB tools into a single interface
- Enhanced error handling and debugging information through connection state management
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/connectionManager.ts | New centralized connection management class with state tracking and event emission |
| src/common/session.ts | Refactored to use ConnectionManager instead of directly managing ServiceProvider |
| src/tools/mongodb/mongodbTool.ts | Updated to use session's connection state checking methods |
| src/tools/mongodb/connect/connect.ts | Modified to use new connection interface and added connect event listener |
| src/tools/atlas/connect/connectCluster.ts | Updated connection calls to use new ConnectionManager interface |
| src/server.ts | Updated to use new connection interface and moved resource registration earlier |
| src/resources/common/debug.ts | Updated resource naming for MongoDB debugging |
| tests/unit/common/session.test.ts | Updated test calls to match new connection interface |
| tests/integration/transports/stdio.test.ts | Added connection state setup for integration tests |
| tests/integration/tools/mongodb/connect/connect.test.ts | Updated tests with connection state setup and error message expectations |
| tests/integration/helpers.ts | Added ConnectionManager instantiation to test setup |
| tests/integration/common/connectionManager.test.ts | New comprehensive test suite for ConnectionManager functionality |
nirinchev
reviewed
Aug 6, 2025
Also remove the code that forces the reconnection in the stdio transport test as it's not needed anymore.
But keep deleting users outside, as it's out of scope of the connection manager as it's not related to MongoDB itself but to Atlas.
We are not using it, but to be consistent with what we had before we are keeping it.
nirinchev
reviewed
Aug 6, 2025
fmenezes
reviewed
Aug 6, 2025
…ction state We used to support only having it when connected, but it might be useful also when errored
Now it doesn't need to be async because the connection itself attemps to ping, it just needs to check the current connection status and map the exact values
nirinchev
approved these changes
Aug 6, 2025
Pull Request Test Coverage Report for Build 16778685993Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
This PR basically proposes consolidating all connections into a single class, ConnectionManager. After this PR, both Atlas tools and MongoDB tools use the same code to connect to MongoDB, independently of the connection type.
Also, the ConnectionManager gets an upgrade on how to handle errors and debugging information:
To avoid changing the whole architecture and maybe support multitenancy in the future, the ConnectionManager is hidden and isolated in the Session.
This PR is bigger than usual because the refactor touches some root infrastructure, but hopefully the unit and integration tests I've added, along with the manual testing, ensures everything works correctly.
Checklist