Skip to content

Update connection string app name if not present #199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 6, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
"bson": "^6.10.3",
"lru-cache": "^11.1.0",
"mongodb": "^6.15.0",
"mongodb-build-info": "^1.7.2",
"mongodb-connection-string-url": "^3.0.2",
"mongodb-log-writer": "^2.4.1",
"mongodb-redact": "^1.1.6",
"mongodb-schema": "^12.6.2",
Expand Down
2 changes: 1 addition & 1 deletion src/common/atlas/apiClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { AccessToken, ClientCredentials } from "simple-oauth2";
import { ApiClientError } from "./apiClientError.js";
import { paths, operations } from "./openapi.js";
import { CommonProperties, TelemetryEvent } from "../../telemetry/types.js";
import { packageInfo } from "../../packageInfo.js";
import { packageInfo } from "../../helpers/packageInfo.js";

const ATLAS_API_VERSION = "2025-03-12";

Expand Down
27 changes: 27 additions & 0 deletions src/helpers/connectionOptions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { MongoClientOptions } from "mongodb";
import ConnectionString from "mongodb-connection-string-url";
import { isAtlas } from "mongodb-build-info";

export function setAppNameParamIfMissing({
connectionString,
defaultAppName,
telemetryAnonymousId,
}: {
connectionString: string;
defaultAppName?: string;
telemetryAnonymousId?: string;
}): string {
const connectionStringUrl = new ConnectionString(connectionString);

const searchParams = connectionStringUrl.typedSearchParams<MongoClientOptions>();

if (!searchParams.has("appName") && defaultAppName !== undefined) {
const appName = isAtlas(connectionString)
? `${defaultAppName}${telemetryAnonymousId ? `-${telemetryAnonymousId}` : ""}`
: defaultAppName;

searchParams.set("appName", appName);
}

return connectionStringUrl.toString();
}
File renamed without changes.
2 changes: 1 addition & 1 deletion src/packageInfo.ts → src/helpers/packageInfo.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import packageJson from "../package.json" with { type: "json" };
import packageJson from "../../package.json" with { type: "json" };

export const packageInfo = {
version: packageJson.version,
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
import { config } from "./config.js";
import { Session } from "./session.js";
import { Server } from "./server.js";
import { packageInfo } from "./packageInfo.js";
import { packageInfo } from "./helpers/packageInfo.js";
import { Telemetry } from "./telemetry/telemetry.js";

try {
Expand Down
6 changes: 5 additions & 1 deletion src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,11 @@ export class Server {

if (this.userConfig.connectionString) {
try {
await this.session.connectToMongoDB(this.userConfig.connectionString, this.userConfig.connectOptions);
await this.session.connectToMongoDB(
this.userConfig.connectionString,
this.userConfig.connectOptions,
this.telemetry
);
} catch (error) {
console.error(
"Failed to connect to MongoDB instance using the connection string from the config: ",
Expand Down
14 changes: 13 additions & 1 deletion src/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import { Implementation } from "@modelcontextprotocol/sdk/types.js";
import logger, { LogId } from "./logger.js";
import EventEmitter from "events";
import { ConnectOptions } from "./config.js";
import { setAppNameParamIfMissing } from "./helpers/connectionOptions.js";
import { packageInfo } from "./helpers/packageInfo.js";
import { Telemetry } from "./telemetry/telemetry.js";

export interface SessionOptions {
apiBaseUrl: string;
Expand Down Expand Up @@ -97,7 +100,16 @@ export class Session extends EventEmitter<{
this.emit("close");
}

async connectToMongoDB(connectionString: string, connectOptions: ConnectOptions): Promise<void> {
async connectToMongoDB(
connectionString: string,
connectOptions: ConnectOptions,
telemetry: Telemetry
): Promise<void> {
connectionString = setAppNameParamIfMissing({
connectionString,
defaultAppName: `${packageInfo.mcpServerName} ${packageInfo.version}`,
telemetryAnonymousId: await telemetry.deviceIdPromise,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does mean we're delaying connecting to a cluster for the sake of telemetry 😢 could we not append anonymous ID yet, especially since we use the segment anonymous ID in all other places at the moment so this won't match it anyhow

});
const provider = await NodeDriverServiceProvider.connect(connectionString, {
productDocsLink: "https://docs.mongodb.com/todo-mcp",
productName: "MongoDB MCP",
Expand Down
2 changes: 1 addition & 1 deletion src/telemetry/constants.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { packageInfo } from "../packageInfo.js";
import { packageInfo } from "../helpers/packageInfo.js";
import { type CommonStaticProperties } from "./types.js";

/**
Expand Down
2 changes: 1 addition & 1 deletion src/telemetry/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { MACHINE_METADATA } from "./constants.js";
import { EventCache } from "./eventCache.js";
import { createHmac } from "crypto";
import nodeMachineId from "node-machine-id";
import { DeferredPromise } from "../deferred-promise.js";
import { DeferredPromise } from "../helpers/deferred-promise.js";

type EventResult = {
success: boolean;
Expand Down
2 changes: 1 addition & 1 deletion src/tools/atlas/metadata/connectCluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class ConnectClusterTool extends AtlasToolBase {

for (let i = 0; i < 20; i++) {
try {
await this.session.connectToMongoDB(connectionString, this.config.connectOptions);
await this.session.connectToMongoDB(connectionString, this.config.connectOptions, this.telemetry);
lastError = undefined;
break;
} catch (err: unknown) {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/mongodb/mongodbTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export abstract class MongoDBToolBase extends ToolBase {
}

protected connectToMongoDB(connectionString: string): Promise<void> {
return this.session.connectToMongoDB(connectionString, this.config.connectOptions);
return this.session.connectToMongoDB(connectionString, this.config.connectOptions, this.telemetry);
}

protected resolveTelemetryMetadata(
Expand Down
69 changes: 69 additions & 0 deletions src/types/mongodb-connection-string-url.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
declare module "mongodb-connection-string-url" {
import { URL } from "whatwg-url";
import { redactConnectionString, ConnectionStringRedactionOptions } from "./redact";
export { redactConnectionString, ConnectionStringRedactionOptions };
declare class CaseInsensitiveMap<K extends string = string> extends Map<K, string> {
delete(name: K): boolean;
get(name: K): string | undefined;
has(name: K): boolean;
set(name: K, value: any): this;

Check failure on line 9 in src/types/mongodb-connection-string-url.d.ts

View workflow job for this annotation

GitHub Actions / check-style

Unexpected any. Specify a different type
_normalizeKey(name: any): K;

Check failure on line 10 in src/types/mongodb-connection-string-url.d.ts

View workflow job for this annotation

GitHub Actions / check-style

Unexpected any. Specify a different type
}
declare abstract class URLWithoutHost extends URL {
abstract get host(): never;
abstract set host(value: never);
abstract get hostname(): never;
abstract set hostname(value: never);
abstract get port(): never;
abstract set port(value: never);
abstract get href(): string;
abstract set href(value: string);
}
export interface ConnectionStringParsingOptions {
looseValidation?: boolean;
}
export declare class ConnectionString extends URLWithoutHost {
_hosts: string[];
constructor(uri: string, options?: ConnectionStringParsingOptions);
get host(): never;
set host(_ignored: never);
get hostname(): never;
set hostname(_ignored: never);
get port(): never;
set port(_ignored: never);
get href(): string;
set href(_ignored: string);
get isSRV(): boolean;
get hosts(): string[];
set hosts(list: string[]);
toString(): string;
clone(): ConnectionString;
redact(options?: ConnectionStringRedactionOptions): ConnectionString;
typedSearchParams<T extends {}>(): {
append(name: keyof T & string, value: any): void;
delete(name: keyof T & string): void;
get(name: keyof T & string): string | null;
getAll(name: keyof T & string): string[];
has(name: keyof T & string): boolean;
set(name: keyof T & string, value: any): void;
keys(): IterableIterator<keyof T & string>;
values(): IterableIterator<string>;
entries(): IterableIterator<[keyof T & string, string]>;
_normalizeKey(name: keyof T & string): string;
[Symbol.iterator](): IterableIterator<[keyof T & string, string]>;
sort(): void;
forEach<THIS_ARG = void>(
callback: (this: THIS_ARG, value: string, name: string, searchParams: any) => void,
thisArg?: THIS_ARG | undefined
): void;
readonly [Symbol.toStringTag]: "URLSearchParams";
};
}
export declare class CommaAndColonSeparatedRecord<
K extends {} = Record<string, unknown>,
> extends CaseInsensitiveMap<keyof K & string> {
constructor(from?: string | null);
toString(): string;
}
export default ConnectionString;
}
2 changes: 1 addition & 1 deletion tests/unit/deferred-promise.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { DeferredPromise } from "../../src/deferred-promise.js";
import { DeferredPromise } from "../../src/helpers/deferred-promise.js";
import { jest } from "@jest/globals";

describe("DeferredPromise", () => {
Expand Down
96 changes: 96 additions & 0 deletions tests/unit/telemetry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
import { jest } from "@jest/globals";
import logger, { LogId } from "../../src/logger.js";
import { createHmac } from "crypto";
import { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver";

jest.mock("@mongosh/service-provider-node-driver");
const MockNodeDriverServiceProvider = NodeDriverServiceProvider as jest.MockedClass<typeof NodeDriverServiceProvider>;

// Mock the ApiClient to avoid real API calls
jest.mock("../../src/common/atlas/apiClient.js");
Expand Down Expand Up @@ -315,4 +319,96 @@
});
});
});

describe("connectToMongoDB", () => {
beforeEach(() => {
session = new Session({
apiClientId: "test-client-id",
apiBaseUrl: "https://api.test.com",
});

MockNodeDriverServiceProvider.connect = jest.fn(() =>
Promise.resolve({} as unknown as NodeDriverServiceProvider)
);
});

const testCases: {
connectionString: string;
isAtlas: boolean;
expectAppName: boolean;
expectTelemetryId: boolean;
name: string;
disableTelemetry?: boolean;
}[] = [
{
connectionString: "mongodb://localhost:27017",
isAtlas: false,
expectAppName: true,
expectTelemetryId: false,
name: "local db",
},
{
connectionString: "mongodb+srv://test.mongodb.net/test?retryWrites=true&w=majority",
isAtlas: true,
expectAppName: true,
expectTelemetryId: true,
name: "atlas db",
},
{
connectionString: "mongodb://localhost:27017?appName=CustomAppName",
isAtlas: false,
expectAppName: false,
expectTelemetryId: false,
name: "local db with custom appName",
},
{
connectionString:
"mongodb+srv://test.mongodb.net/test?retryWrites=true&w=majority&appName=CustomAppName",
isAtlas: true,
expectAppName: false,
expectTelemetryId: false,
name: "atlas db with custom appName",
},

{
connectionString: "mongodb+srv://test.mongodb.net/test?retryWrites=true&w=majority",
isAtlas: true,
expectAppName: true,
expectTelemetryId: false,
name: "atlas db with telemetry disabled",
disableTelemetry: true,
},
];

for (const testCase of testCases) {
it(`should update connection string for ${testCase.name}`, async () => {
if (testCase.disableTelemetry) {
config.telemetry = "disabled";
telemetry = Telemetry.create(session, config, {
getRawMachineId: () => Promise.resolve(machineId),
});
}
await session.connectToMongoDB(testCase.connectionString, config.connectOptions, telemetry);
expect(session.serviceProvider).toBeDefined();
expect(MockNodeDriverServiceProvider.connect).toHaveBeenCalledOnce();

Check failure on line 393 in tests/unit/telemetry.test.ts

View workflow job for this annotation

GitHub Actions / check-style

Avoid referencing unbound methods which may cause unintentional scoping of `this`
const connectionString = MockNodeDriverServiceProvider.connect.mock.calls[0][0];
if (testCase.expectAppName) {
expect(connectionString).toContain("appName=MongoDB+MCP+Server");
} else {
expect(connectionString).not.toContain("appName=MongoDB+MCP+Server");
}

if (testCase.disableTelemetry) {
expect(connectionString).not.toMatch(/appName=[^-]*-[^&]*/);
Copy link
Preview

Copilot AI May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider extracting the regex pattern into a named constant or helper function to improve maintainability and clarity, in case the connection string format changes in the future.

Suggested change
expect(connectionString).not.toMatch(/appName=[^-]*-[^&]*/);
expect(connectionString).not.toMatch(APP_NAME_REGEX);

Copilot uses AI. Check for mistakes.

} else {
const telemetryId = await telemetry.deviceIdPromise;
if (testCase.isAtlas && testCase.expectTelemetryId) {
expect(connectionString).toContain(telemetryId);
} else {
expect(connectionString).not.toContain(telemetryId);
}
}
});
}
});
});
Loading