Skip to content

Commit cc079e1

Browse files
himanshusinghshimanshusinghs
committed
fix: don't use config opts for further connections
This commit adds a test to assert that we don't use the driver options derived from the CLI arguments -> userConfig for further connections (other than the first connection) such as switch-connection tool. The problem earlier was that MCPConnectionManager was accepting a driverOptions object which it was then using in conjunction with userConfig to construct a fresh set of driver options for all the connections established by the MCP server. Now this was particularly problematic because the driver options for the configured connection might not be compatible with the new connection attempts being made in the MCP server. Take for example - MCP server configured to connect with an OIDC enabled MongoDB server and later switch-connection tool using the same driver options to connect to a locally running mongodb server without any auth. To fix that we've removed DriverOptions object from the interface of MCPConnectionManager and instead put it on the connect method. It is the responsibility of the caller to provide a correct ConnectionInfo object. For pre-configured connections server constructs the ConnectionInfo object using user config and passes it down to the MCPConnectionManager.connect and for the rest usage, they simply pass the connection string that they want to connect to.
1 parent 8433b85 commit cc079e1

File tree

14 files changed

+107
-74
lines changed

14 files changed

+107
-74
lines changed

‎src/common/connectionManager.ts‎

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,8 @@ import { EventEmitter } from "events";
22
import type { MongoClientOptions } from "mongodb";
33
import { ConnectionString } from "mongodb-connection-string-url";
44
import { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver";
5-
import { type ConnectionInfo, generateConnectionInfoFromCliArgs } from "@mongosh/arg-parser";
5+
import { type ConnectionInfo } from "@mongosh/arg-parser";
66
import type { DeviceId } from "../helpers/deviceId.js";
7-
import { createDriverOptions, type DriverOptions } from "./config/driverOptions.js";
87
import { type UserConfig } from "./config.js";
98
import { MongoDBError, ErrorCodes } from "./errors.js";
109
import { type LoggerBase, LogId } from "./logger.js";
@@ -18,8 +17,8 @@ export interface AtlasClusterConnectionInfo {
1817
expiryDate: Date;
1918
}
2019

21-
export interface ConnectionSettings {
22-
connectionString: string;
20+
export interface ConnectionSettings extends Omit<ConnectionInfo, "driverOptions"> {
21+
driverOptions?: ConnectionInfo["driverOptions"];
2322
atlas?: AtlasClusterConnectionInfo;
2423
}
2524

@@ -137,7 +136,6 @@ export class MCPConnectionManager extends ConnectionManager {
137136

138137
constructor(
139138
private userConfig: UserConfig,
140-
private driverOptions: DriverOptions,
141139
private logger: LoggerBase,
142140
deviceId: DeviceId,
143141
bus?: EventEmitter
@@ -158,7 +156,6 @@ export class MCPConnectionManager extends ConnectionManager {
158156
}
159157

160158
let serviceProvider: Promise<NodeDriverServiceProvider>;
161-
let connectionInfo: ConnectionInfo;
162159
let connectionStringAuthType: ConnectionStringAuthType = "scram";
163160

164161
try {
@@ -174,11 +171,10 @@ export class MCPConnectionManager extends ConnectionManager {
174171
components: appNameComponents,
175172
});
176173

177-
connectionInfo = generateConnectionInfoFromCliArgs({
178-
...this.userConfig,
179-
...this.driverOptions,
180-
connectionSpecifier: settings.connectionString,
181-
});
174+
const connectionInfo = {
175+
connectionString: settings.connectionString,
176+
driverOptions: settings.driverOptions ?? {},
177+
};
182178

183179
if (connectionInfo.driverOptions.oidc) {
184180
connectionInfo.driverOptions.oidc.allowedFlows ??= ["auth-code"];
@@ -395,7 +391,5 @@ export type ConnectionManagerFactoryFn = (createParams: {
395391
}) => Promise<ConnectionManager>;
396392

397393
export const createMCPConnectionManager: ConnectionManagerFactoryFn = ({ logger, deviceId, userConfig }) => {
398-
const driverOptions = createDriverOptions(userConfig);
399-
400-
return Promise.resolve(new MCPConnectionManager(userConfig, driverOptions, logger, deviceId));
394+
return Promise.resolve(new MCPConnectionManager(userConfig, logger, deviceId));
401395
};

‎src/resources/common/config.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { ReactiveResource } from "../resource.js";
2-
import { createDriverOptions } from "../../common/config/driverOptions.js";
32
import type { UserConfig } from "../../common/config.js";
43
import type { Telemetry } from "../../telemetry/telemetry.js";
54
import type { Session } from "../../lib.js";
5+
import { generateConnectionInfoFromCliArgs } from "@mongosh/arg-parser";
66

77
export class ConfigResource extends ReactiveResource<UserConfig, readonly []> {
88
constructor(session: Session, config: UserConfig, telemetry: Telemetry) {
@@ -32,13 +32,14 @@ export class ConfigResource extends ReactiveResource<UserConfig, readonly []> {
3232
}
3333

3434
toOutput(): string {
35+
const connectionInfo = generateConnectionInfoFromCliArgs(this.current);
3536
const result = {
3637
telemetry: this.current.telemetry,
3738
logPath: this.current.logPath,
38-
connectionString: this.current.connectionString
39+
connectionString: connectionInfo.connectionString
3940
? "set; access to MongoDB tools are currently available to use"
4041
: "not set; before using any MongoDB tool, you need to configure a connection string, alternatively you can setup MongoDB Atlas access, more info at 'https://github.com/mongodb-js/mongodb-mcp-server'.",
41-
connectOptions: createDriverOptions(this.config),
42+
connectOptions: connectionInfo.driverOptions,
4243
atlas:
4344
this.current.apiClientId && this.current.apiClientSecret
4445
? "set; MongoDB Atlas tools are currently available to use"

‎src/server.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { generateConnectionInfoFromCliArgs } from "@mongosh/arg-parser";
12
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
23
import type { Session } from "./common/session.js";
34
import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js";
@@ -327,9 +328,11 @@ export class Server {
327328
context: "server",
328329
message: `Detected a MongoDB connection string in the configuration, trying to connect...`,
329330
});
330-
await this.session.connectToMongoDB({
331-
connectionString: this.userConfig.connectionString,
331+
const connectionInfo = generateConnectionInfoFromCliArgs({
332+
...this.userConfig,
333+
connectionSpecifier: this.userConfig.connectionString,
332334
});
335+
await this.session.connectToMongoDB(connectionInfo);
333336
} catch (error) {
334337
// We don't throw an error here because we want to allow the server to start even if the connection string is invalid.
335338
this.session.logger.error({

‎tests/integration/common/connectionManager.oidc.test.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { describeWithMongoDB, isCommunityServer, getServerVersion } from "../too
77
import { defaultTestConfig, responseAsText, timeout, waitUntil } from "../helpers.js";
88
import type { ConnectionStateConnected, ConnectionStateConnecting } from "../../../src/common/connectionManager.js";
99
import type { UserConfig } from "../../../src/common/config.js";
10-
import { createDriverOptions } from "../../../src/common/config/driverOptions.js";
1110
import path from "path";
1211
import type { OIDCMockProviderConfig } from "@mongodb-js/oidc-mock-provider";
1312
import { OIDCMockProvider } from "@mongodb-js/oidc-mock-provider";
@@ -142,7 +141,6 @@ describe.skipIf(process.platform !== "linux")("ConnectionManager OIDC Tests", as
142141
},
143142
{
144143
getUserConfig: () => oidcConfig,
145-
getDriverOptions: () => createDriverOptions(oidcConfig),
146144
downloadOptions: {
147145
runner: true,
148146
downloadOptions: { enterprise: true, version: mongodbVersion },

‎tests/integration/helpers.ts‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
77
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
88
import { InMemoryTransport } from "./inMemoryTransport.js";
99
import { type UserConfig, config } from "../../src/common/config.js";
10-
import { type DriverOptions, createDriverOptions } from "../../src/common/config/driverOptions.js";
1110
import { McpError, ResourceUpdatedNotificationSchema } from "@modelcontextprotocol/sdk/types.js";
1211
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
1312
import type { ConnectionManager, ConnectionState } from "../../src/common/connectionManager.js";
@@ -48,13 +47,10 @@ export const defaultTestConfig: UserConfig = {
4847
loggers: ["stderr"],
4948
};
5049

51-
export const defaultDriverOptions: DriverOptions = createDriverOptions(defaultTestConfig);
52-
5350
export const DEFAULT_LONG_RUNNING_TEST_WAIT_TIMEOUT_MS = 1_200_000;
5451

5552
export function setupIntegrationTest(
5653
getUserConfig: () => UserConfig,
57-
getDriverOptions: () => DriverOptions,
5854
{
5955
elicitInput,
6056
getClientCapabilities,
@@ -71,7 +67,6 @@ export function setupIntegrationTest(
7167

7268
beforeAll(async () => {
7369
const userConfig = getUserConfig();
74-
const driverOptions = getDriverOptions();
7570
const clientCapabilities = getClientCapabilities?.() ?? (elicitInput ? { elicitation: {} } : {});
7671

7772
const clientTransport = new InMemoryTransport();
@@ -97,7 +92,7 @@ export function setupIntegrationTest(
9792
const exportsManager = ExportsManager.init(userConfig, logger);
9893

9994
deviceId = DeviceId.create(logger);
100-
const connectionManager = new MCPConnectionManager(userConfig, driverOptions, logger, deviceId);
95+
const connectionManager = new MCPConnectionManager(userConfig, logger, deviceId);
10196

10297
const session = new Session({
10398
apiBaseUrl: userConfig.apiBaseUrl,

‎tests/integration/telemetry.test.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { Telemetry } from "../../src/telemetry/telemetry.js";
22
import { Session } from "../../src/common/session.js";
33
import { config } from "../../src/common/config.js";
4-
import { defaultDriverOptions } from "./helpers.js";
54
import { DeviceId } from "../../src/helpers/deviceId.js";
65
import { describe, expect, it } from "vitest";
76
import { CompositeLogger } from "../../src/common/logger.js";
@@ -16,7 +15,7 @@ describe("Telemetry", () => {
1615

1716
const deviceId = DeviceId.create(logger);
1817
const actualDeviceId = await deviceId.get();
19-
const connectionManager = new MCPConnectionManager(config, defaultDriverOptions, logger, deviceId);
18+
const connectionManager = new MCPConnectionManager(config, logger, deviceId);
2019

2120
const telemetry = Telemetry.create(
2221
new Session({

‎tests/integration/tools/atlas-local/atlasLocalHelpers.ts‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { defaultDriverOptions, defaultTestConfig, setupIntegrationTest, type IntegrationTest } from "../../helpers.js";
1+
import { defaultTestConfig, setupIntegrationTest, type IntegrationTest } from "../../helpers.js";
22
import { describe } from "vitest";
33

44
const isMacOSInGitHubActions = process.platform === "darwin" && process.env.GITHUB_ACTIONS === "true";
@@ -11,10 +11,7 @@ export type IntegrationTestFunction = (integration: IntegrationTest) => void;
1111
*/
1212
export function describeWithAtlasLocal(name: string, fn: IntegrationTestFunction): void {
1313
describe.skipIf(isMacOSInGitHubActions)(name, () => {
14-
const integration = setupIntegrationTest(
15-
() => defaultTestConfig,
16-
() => defaultDriverOptions
17-
);
14+
const integration = setupIntegrationTest(() => defaultTestConfig);
1815
fn(integration);
1916
});
2017
}
@@ -25,10 +22,7 @@ export function describeWithAtlasLocal(name: string, fn: IntegrationTestFunction
2522
*/
2623
export function describeWithAtlasLocalDisabled(name: string, fn: IntegrationTestFunction): void {
2724
describe.skipIf(!isMacOSInGitHubActions)(name, () => {
28-
const integration = setupIntegrationTest(
29-
() => defaultTestConfig,
30-
() => defaultDriverOptions
31-
);
25+
const integration = setupIntegrationTest(() => defaultTestConfig);
3226
fn(integration);
3327
});
3428
}

‎tests/integration/tools/atlas/atlasHelpers.ts‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { ObjectId } from "mongodb";
22
import type { ClusterDescription20240805, Group } from "../../../../src/common/atlas/openapi.js";
33
import type { ApiClient } from "../../../../src/common/atlas/apiClient.js";
44
import type { IntegrationTest } from "../../helpers.js";
5-
import { setupIntegrationTest, defaultTestConfig, defaultDriverOptions } from "../../helpers.js";
5+
import { setupIntegrationTest, defaultTestConfig } from "../../helpers.js";
66
import type { SuiteCollector } from "vitest";
77
import { afterAll, beforeAll, describe } from "vitest";
88
import type { Session } from "../../../../src/common/session.js";
@@ -15,15 +15,12 @@ export function describeWithAtlas(name: string, fn: IntegrationTestFunction): vo
1515
? describe.skip
1616
: describe;
1717
describeFn(name, () => {
18-
const integration = setupIntegrationTest(
19-
() => ({
20-
...defaultTestConfig,
21-
apiClientId: process.env.MDB_MCP_API_CLIENT_ID || "test-client",
22-
apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET || "test-secret",
23-
apiBaseUrl: process.env.MDB_MCP_API_BASE_URL ?? "https://cloud-dev.mongodb.com",
24-
}),
25-
() => defaultDriverOptions
26-
);
18+
const integration = setupIntegrationTest(() => ({
19+
...defaultTestConfig,
20+
apiClientId: process.env.MDB_MCP_API_CLIENT_ID || "test-client",
21+
apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET || "test-secret",
22+
apiBaseUrl: process.env.MDB_MCP_API_BASE_URL ?? "https://cloud-dev.mongodb.com",
23+
}));
2724
fn(integration);
2825
});
2926
}

‎tests/integration/tools/atlas/performanceAdvisor.test.ts‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { ObjectId } from "bson";
55
import type { Session } from "../../../../src/common/session.js";
66
import {
77
DEFAULT_LONG_RUNNING_TEST_WAIT_TIMEOUT_MS,
8-
defaultDriverOptions,
98
defaultTestConfig,
109
expectDefined,
1110
getResponseElements,
@@ -132,15 +131,12 @@ describeWithAtlas("performanceAdvisor", (integration) => {
132131
});
133132

134133
describe("mocked atlas-get-performance-advisor", () => {
135-
const integration = setupIntegrationTest(
136-
() => ({
137-
...defaultTestConfig,
138-
apiClientId: process.env.MDB_MCP_API_CLIENT_ID || "test-client",
139-
apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET || "test-secret",
140-
apiBaseUrl: process.env.MDB_MCP_API_BASE_URL ?? "https://cloud-dev.mongodb.com",
141-
}),
142-
() => defaultDriverOptions
143-
);
134+
const integration = setupIntegrationTest(() => ({
135+
...defaultTestConfig,
136+
apiClientId: process.env.MDB_MCP_API_CLIENT_ID || "test-client",
137+
apiClientSecret: process.env.MDB_MCP_API_CLIENT_SECRET || "test-secret",
138+
apiBaseUrl: process.env.MDB_MCP_API_BASE_URL ?? "https://cloud-dev.mongodb.com",
139+
}));
144140

145141
let mockEmitEvents: MockInstance<(events: BaseEvent[]) => void>;
146142
let projectId: string;

‎tests/integration/tools/mongodb/connect/connect.test.ts‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ import {
66
validateToolMetadata,
77
} from "../../../helpers.js";
88
import { defaultTestConfig } from "../../../helpers.js";
9-
import { beforeEach, describe, expect, it } from "vitest";
9+
import { beforeEach, describe, expect, it, type MockInstance, vi } from "vitest";
10+
import { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver";
1011

1112
describeWithMongoDB(
1213
"SwitchConnection tool",
@@ -88,6 +89,72 @@ describeWithMongoDB(
8889
}
8990
);
9091

92+
describeWithMongoDB(
93+
"SwitchConnection tool when server is configured to connect with complex connection",
94+
(integration) => {
95+
let connectFnSpy: MockInstance<typeof NodeDriverServiceProvider.connect>;
96+
beforeEach(async () => {
97+
connectFnSpy = vi.spyOn(NodeDriverServiceProvider, "connect");
98+
await integration.mcpServer().session.connectToMongoDB({
99+
connectionString: integration.connectionString(),
100+
});
101+
});
102+
103+
it("should be able to connect to next connection and not use the connect options of the connection setup during server boot", async () => {
104+
const newConnectionString = `${integration.connectionString()}`;
105+
// Note: The connect function is called with OIDC options for the
106+
// configured string
107+
expect(connectFnSpy).toHaveBeenNthCalledWith(
108+
1,
109+
expect.stringContaining(`${integration.connectionString()}/?directConnection=true`),
110+
expect.objectContaining({
111+
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
112+
oidc: expect.objectContaining({ openBrowser: { command: "not-a-browser" } }),
113+
}),
114+
undefined,
115+
expect.anything()
116+
);
117+
const response = await integration.mcpClient().callTool({
118+
name: "switch-connection",
119+
arguments: {
120+
connectionString: newConnectionString,
121+
},
122+
});
123+
124+
const content = getResponseContent(response.content);
125+
// The connection will still be connected because the --browser
126+
// option only sets the command to be used when opening the browser
127+
// for OIDC handling.
128+
expect(content).toContain("Successfully connected");
129+
130+
// Now that we're connected lets verify the config
131+
// Note: The connect function is called with OIDC options for the
132+
// configured string
133+
expect(connectFnSpy).toHaveBeenNthCalledWith(
134+
2,
135+
expect.stringContaining(`${integration.connectionString()}`),
136+
expect.not.objectContaining({
137+
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
138+
oidc: expect.objectContaining({ openBrowser: { command: "not-a-browser" } }),
139+
}),
140+
undefined,
141+
expect.anything()
142+
);
143+
});
144+
},
145+
{
146+
getUserConfig: (mdbIntegration) => ({
147+
...defaultTestConfig,
148+
// Setting browser in config is the same as passing `--browser` CLI
149+
// argument to the MCP server CLI entry point. We expect that the
150+
// further connection attempts stay detached from the connection
151+
// options passed during server boot, in this case browser.
152+
browser: "not-a-browser",
153+
connectionString: `${mdbIntegration.connectionString()}/?directConnection=true`,
154+
}),
155+
}
156+
);
157+
91158
describeWithMongoDB("Connect tool", (integration) => {
92159
validateToolMetadata(
93160
integration,

0 commit comments

Comments
 (0)