From 4591c262eee0c302e30442f6f03c8b52f07c2f41 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 28 Jul 2025 13:09:07 +0100 Subject: [PATCH] feat: update connectionString appName param - [MCP-68] --- src/common/session.ts | 2 +- src/helpers/connectionOptions.ts | 45 +++++++-- src/helpers/deviceId.ts | 48 ++++++++++ src/telemetry/telemetry.ts | 46 +-------- tests/integration/telemetry.test.ts | 11 +-- .../tools/mongodb/connect/connect.test.ts | 7 +- tests/unit/common/session.test.ts | 34 ++++++- tests/unit/helpers/connectionOptions.test.ts | 93 +++++++++++++++++++ tests/unit/telemetry.test.ts | 26 +++--- 9 files changed, 238 insertions(+), 74 deletions(-) create mode 100644 src/helpers/deviceId.ts create mode 100644 tests/unit/helpers/connectionOptions.test.ts diff --git a/src/common/session.ts b/src/common/session.ts index 444a747b..0e28ecdb 100644 --- a/src/common/session.ts +++ b/src/common/session.ts @@ -147,4 +147,4 @@ export class Session extends EventEmitter { get connectedAtlasCluster(): AtlasClusterConnectionInfo | undefined { return this.connectionManager.currentConnectionState.connectedAtlasCluster; } -} +} \ No newline at end of file diff --git a/src/helpers/connectionOptions.ts b/src/helpers/connectionOptions.ts index 10b1ecc8..7f8f7856 100644 --- a/src/helpers/connectionOptions.ts +++ b/src/helpers/connectionOptions.ts @@ -1,20 +1,51 @@ import { MongoClientOptions } from "mongodb"; import ConnectionString from "mongodb-connection-string-url"; +import { getDeviceIdForConnection } from "./deviceId.js"; -export function setAppNameParamIfMissing({ +export interface AppNameComponents { + appName: string; + deviceId?: string; + clientName?: string; +} + +/** + * Sets the appName parameter with the extended format: appName--deviceId--clientName + * Only sets the appName if it's not already present in the connection string + * @param connectionString - The connection string to modify + * @param components - The components to build the appName from + * @returns The modified connection string + */ +export async function setAppNameParamIfMissing({ connectionString, - defaultAppName, + components, }: { connectionString: string; - defaultAppName?: string; -}): string { + components: AppNameComponents; +}): Promise { const connectionStringUrl = new ConnectionString(connectionString); - const searchParams = connectionStringUrl.typedSearchParams(); - if (!searchParams.has("appName") && defaultAppName !== undefined) { - searchParams.set("appName", defaultAppName); + // Only set appName if it's not already present + if (searchParams.has("appName")) { + return connectionStringUrl.toString(); + } + + // Get deviceId if not provided + let deviceId = components.deviceId; + if (!deviceId) { + deviceId = await getDeviceIdForConnection(); } + // Get clientName if not provided + let clientName = components.clientName; + if (!clientName) { + clientName = "unknown"; + } + + // Build the extended appName format: appName--deviceId--clientName + const extendedAppName = `${components.appName}--${deviceId}--${clientName}`; + + searchParams.set("appName", extendedAppName); + return connectionStringUrl.toString(); } diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts new file mode 100644 index 00000000..043acb8a --- /dev/null +++ b/src/helpers/deviceId.ts @@ -0,0 +1,48 @@ +import { getDeviceId } from "@mongodb-js/device-id"; +import nodeMachineId from "node-machine-id"; +import logger, { LogId } from "../common/logger.js"; + +export const DEVICE_ID_TIMEOUT = 3000; + +/** + * Sets the appName parameter with the extended format: appName--deviceId--clientName + * Only sets the appName if it's not already present in the connection string + * + * @param connectionString - The connection string to modify + * @param components - The components to build the appName from + * @returns Promise that resolves to the modified connection string + * + * @example + * ```typescript + * const result = await setExtendedAppNameParam({ + * connectionString: "mongodb://localhost:27017", + * components: { appName: "MyApp", clientName: "Cursor" } + * }); + * // Result: "mongodb://localhost:27017/?appName=MyApp--deviceId--Cursor" + * ``` + */ +export async function getDeviceIdForConnection(): Promise { + try { + const deviceId = await getDeviceId({ + getMachineId: () => nodeMachineId.machineId(true), + onError: (reason, error) => { + switch (reason) { + case "resolutionError": + logger.debug(LogId.telemetryDeviceIdFailure, "deviceId", String(error)); + break; + case "timeout": + logger.debug(LogId.telemetryDeviceIdTimeout, "deviceId", "Device ID retrieval timed out"); + break; + case "abort": + // No need to log in the case of aborts + break; + } + }, + abortSignal: new AbortController().signal, + }); + return deviceId; + } catch (error) { + logger.debug(LogId.telemetryDeviceIdFailure, "deviceId", `Failed to get device ID: ${String(error)}`); + return "unknown"; + } +} diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 1d08f2e1..5ada536a 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -5,8 +5,7 @@ import { LogId } from "../common/logger.js"; import { ApiClient } from "../common/atlas/apiClient.js"; import { MACHINE_METADATA } from "./constants.js"; import { EventCache } from "./eventCache.js"; -import nodeMachineId from "node-machine-id"; -import { getDeviceId } from "@mongodb-js/device-id"; +import { getDeviceIdForConnection } from "../helpers/deviceId.js"; import { detectContainerEnv } from "../helpers/container.js"; type EventResult = { @@ -14,24 +13,19 @@ type EventResult = { error?: Error; }; -export const DEVICE_ID_TIMEOUT = 3000; - export class Telemetry { private isBufferingEvents: boolean = true; /** Resolves when the setup is complete or a timeout occurs */ public setupPromise: Promise<[string, boolean]> | undefined; - private deviceIdAbortController = new AbortController(); private eventCache: EventCache; - private getRawMachineId: () => Promise; private constructor( private readonly session: Session, private readonly userConfig: UserConfig, private readonly commonProperties: CommonProperties, - { eventCache, getRawMachineId }: { eventCache: EventCache; getRawMachineId: () => Promise } + { eventCache }: { eventCache: EventCache } ) { this.eventCache = eventCache; - this.getRawMachineId = getRawMachineId; } static create( @@ -40,14 +34,12 @@ export class Telemetry { { commonProperties = { ...MACHINE_METADATA }, eventCache = EventCache.getInstance(), - getRawMachineId = (): Promise => nodeMachineId.machineId(true), }: { eventCache?: EventCache; - getRawMachineId?: () => Promise; commonProperties?: CommonProperties; } = {} ): Telemetry { - const instance = new Telemetry(session, userConfig, commonProperties, { eventCache, getRawMachineId }); + const instance = new Telemetry(session, userConfig, commonProperties, { eventCache }); void instance.setup(); return instance; @@ -57,35 +49,7 @@ export class Telemetry { if (!this.isTelemetryEnabled()) { return; } - this.setupPromise = Promise.all([ - getDeviceId({ - getMachineId: () => this.getRawMachineId(), - onError: (reason, error) => { - switch (reason) { - case "resolutionError": - this.session.logger.debug({ - id: LogId.telemetryDeviceIdFailure, - context: "telemetry", - message: String(error), - }); - break; - case "timeout": - this.session.logger.debug({ - id: LogId.telemetryDeviceIdTimeout, - context: "telemetry", - message: "Device ID retrieval timed out", - noRedaction: true, - }); - break; - case "abort": - // No need to log in the case of aborts - break; - } - }, - abortSignal: this.deviceIdAbortController.signal, - }), - detectContainerEnv(), - ]); + this.setupPromise = Promise.all([getDeviceIdForConnection(), detectContainerEnv()]); const [deviceId, containerEnv] = await this.setupPromise; @@ -96,8 +60,6 @@ export class Telemetry { } public async close(): Promise { - this.deviceIdAbortController.abort(); - this.isBufferingEvents = false; await this.emitEvents(this.eventCache.getEvents()); } diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index 95bc79c2..7a196fe0 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -1,18 +1,15 @@ -import { createHmac } from "crypto"; import { Telemetry } from "../../src/telemetry/telemetry.js"; import { Session } from "../../src/common/session.js"; import { config } from "../../src/common/config.js"; -import nodeMachineId from "node-machine-id"; +import { getDeviceIdForConnection } from "../../src/helpers/deviceId.js"; import { describe, expect, it } from "vitest"; import { CompositeLogger } from "../../src/common/logger.js"; import { ConnectionManager } from "../../src/common/connectionManager.js"; import { ExportsManager } from "../../src/common/exportsManager.js"; describe("Telemetry", () => { - it("should resolve the actual machine ID", async () => { - const actualId: string = await nodeMachineId.machineId(true); - - const actualHashedId = createHmac("sha256", actualId.toUpperCase()).update("atlascli").digest("hex"); + it("should resolve the actual device ID", async () => { + const actualDeviceId = await getDeviceIdForConnection(); const logger = new CompositeLogger(); const telemetry = Telemetry.create( @@ -30,7 +27,7 @@ describe("Telemetry", () => { await telemetry.setupPromise; - expect(telemetry.getCommonProperties().device_id).toBe(actualHashedId); + expect(telemetry.getCommonProperties().device_id).toBe(actualDeviceId); expect(telemetry["isBufferingEvents"]).toBe(false); }); }); diff --git a/tests/integration/tools/mongodb/connect/connect.test.ts b/tests/integration/tools/mongodb/connect/connect.test.ts index 7dd275d3..2ce8bf1d 100644 --- a/tests/integration/tools/mongodb/connect/connect.test.ts +++ b/tests/integration/tools/mongodb/connect/connect.test.ts @@ -7,7 +7,12 @@ import { } from "../../../helpers.js"; import { config } from "../../../../../src/common/config.js"; import { defaultTestConfig, setupIntegrationTest } from "../../../helpers.js"; -import { beforeEach, describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// Mock the deviceId utility for consistent testing +vi.mock("../../../../../src/helpers/deviceId.js", () => ({ + getDeviceIdForConnection: vi.fn().mockResolvedValue("test-device-id"), +})); describeWithMongoDB( "SwitchConnection tool", diff --git a/tests/unit/common/session.test.ts b/tests/unit/common/session.test.ts index add1cac5..6774e796 100644 --- a/tests/unit/common/session.test.ts +++ b/tests/unit/common/session.test.ts @@ -7,6 +7,10 @@ import { ConnectionManager } from "../../../src/common/connectionManager.js"; import { ExportsManager } from "../../../src/common/exportsManager.js"; vi.mock("@mongosh/service-provider-node-driver"); +vi.mock("../../../src/helpers/deviceId.js", () => ({ + getDeviceIdForConnection: vi.fn().mockResolvedValue("test-device-id"), +})); + const MockNodeDriverServiceProvider = vi.mocked(NodeDriverServiceProvider); describe("Session", () => { @@ -59,23 +63,51 @@ describe("Session", () => { expect(connectMock).toHaveBeenCalledOnce(); const connectionString = connectMock.mock.calls[0]?.[0]; if (testCase.expectAppName) { - expect(connectionString).toContain("appName=MongoDB+MCP+Server"); + // Check for the extended appName format: appName--deviceId--clientName + expect(connectionString).toContain("appName=MongoDB+MCP+Server+"); + expect(connectionString).toContain("--test-device-id--"); } else { expect(connectionString).not.toContain("appName=MongoDB+MCP+Server"); } }); } +<<<<<<< HEAD it("should configure the proxy to use environment variables", async () => { await session.connectToMongoDB({ connectionString: "mongodb://localhost" }); +======= + it("should include client name when agent runner is set", async () => { + session.setAgentRunner({ name: "test-client", version: "1.0.0" }); + + await session.connectToMongoDB("mongodb://localhost:27017", config.connectOptions); +>>>>>>> c5c91e9 (feat: update connectionString appName param - [MCP-68]) expect(session.serviceProvider).toBeDefined(); const connectMock = MockNodeDriverServiceProvider.connect; expect(connectMock).toHaveBeenCalledOnce(); +<<<<<<< HEAD const connectionConfig = connectMock.mock.calls[0]?.[1]; expect(connectionConfig?.proxy).toEqual({ useEnvironmentVariableProxies: true }); expect(connectionConfig?.applyProxyToOIDC).toEqual(true); +======= + const connectionString = connectMock.mock.calls[0]?.[0]; + + // Should include the client name in the appName + expect(connectionString).toContain("--test-device-id--test-client"); + }); + + it("should use 'unknown' for client name when agent runner is not set", async () => { + await session.connectToMongoDB("mongodb://localhost:27017", config.connectOptions); + expect(session.serviceProvider).toBeDefined(); + + const connectMock = MockNodeDriverServiceProvider.connect; + expect(connectMock).toHaveBeenCalledOnce(); + const connectionString = connectMock.mock.calls[0]?.[0]; + + // Should use 'unknown' for client name when agent runner is not set + expect(connectionString).toContain("--test-device-id--unknown"); +>>>>>>> c5c91e9 (feat: update connectionString appName param - [MCP-68]) }); }); }); diff --git a/tests/unit/helpers/connectionOptions.test.ts b/tests/unit/helpers/connectionOptions.test.ts new file mode 100644 index 00000000..2e2fee2d --- /dev/null +++ b/tests/unit/helpers/connectionOptions.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it, vi } from "vitest"; +import { setAppNameParamIfMissing } from "../../../src/helpers/connectionOptions.js"; + +// Mock the deviceId utility +vi.mock("../../../src/helpers/deviceId.js", () => ({ + getDeviceIdForConnection: vi.fn().mockResolvedValue("test-device-id"), +})); + +describe("Connection Options", () => { + describe("setAppNameParamIfMissing", () => { + it("should set extended appName when no appName is present", async () => { + const connectionString = "mongodb://localhost:27017"; + const result = await setAppNameParamIfMissing({ + connectionString, + components: { + appName: "TestApp", + clientName: "TestClient", + }, + }); + + expect(result).toContain("appName=TestApp--test-device-id--TestClient"); + }); + + it("should not modify connection string when appName is already present", async () => { + const connectionString = "mongodb://localhost:27017?appName=ExistingApp"; + const result = await setAppNameParamIfMissing({ + connectionString, + components: { + appName: "TestApp", + clientName: "TestClient", + }, + }); + + // The ConnectionString library normalizes URLs, so we need to check the content rather than exact equality + expect(result).toContain("appName=ExistingApp"); + expect(result).not.toContain("TestApp--test-device-id--TestClient"); + }); + + it("should use provided deviceId when available", async () => { + const connectionString = "mongodb://localhost:27017"; + const result = await setAppNameParamIfMissing({ + connectionString, + components: { + appName: "TestApp", + deviceId: "custom-device-id", + clientName: "TestClient", + }, + }); + + expect(result).toContain("appName=TestApp--custom-device-id--TestClient"); + }); + + it("should use 'unknown' for clientName when not provided", async () => { + const connectionString = "mongodb://localhost:27017"; + const result = await setAppNameParamIfMissing({ + connectionString, + components: { + appName: "TestApp", + }, + }); + + expect(result).toContain("appName=TestApp--test-device-id--unknown"); + }); + + it("should use deviceId utility when deviceId is not provided", async () => { + const connectionString = "mongodb://localhost:27017"; + const result = await setAppNameParamIfMissing({ + connectionString, + components: { + appName: "TestApp", + clientName: "TestClient", + }, + }); + + expect(result).toContain("appName=TestApp--test-device-id--TestClient"); + }); + + it("should preserve other query parameters", async () => { + const connectionString = "mongodb://localhost:27017?retryWrites=true&w=majority"; + const result = await setAppNameParamIfMissing({ + connectionString, + components: { + appName: "TestApp", + clientName: "TestClient", + }, + }); + + expect(result).toContain("retryWrites=true"); + expect(result).toContain("w=majority"); + expect(result).toContain("appName=TestApp--test-device-id--TestClient"); + }); + }); +}); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 6bc3ec45..2938728d 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -1,6 +1,6 @@ import { ApiClient } from "../../src/common/atlas/apiClient.js"; import { Session } from "../../src/common/session.js"; -import { DEVICE_ID_TIMEOUT, Telemetry } from "../../src/telemetry/telemetry.js"; +import { Telemetry } from "../../src/telemetry/telemetry.js"; import { BaseEvent, TelemetryResult } from "../../src/telemetry/types.js"; import { EventCache } from "../../src/telemetry/eventCache.js"; import { config } from "../../src/common/config.js"; @@ -17,10 +17,12 @@ const MockApiClient = vi.mocked(ApiClient); vi.mock("../../src/telemetry/eventCache.js"); const MockEventCache = vi.mocked(EventCache); -describe("Telemetry", () => { - const machineId = "test-machine-id"; - const hashedMachineId = createHmac("sha256", machineId.toUpperCase()).update("atlascli").digest("hex"); +// Mock the deviceId utility +vi.mock("../../src/helpers/deviceId.js", () => ({ + getDeviceIdForConnection: vi.fn().mockResolvedValue("test-device-id"), +})); +describe("Telemetry", () => { let mockApiClient: { sendEvents: MockedFunction<(events: BaseEvent[]) => Promise>; hasCredentials: MockedFunction<() => boolean>; @@ -130,7 +132,6 @@ describe("Telemetry", () => { telemetry = Telemetry.create(session, config, { eventCache: mockEventCache as unknown as EventCache, - getRawMachineId: () => Promise.resolve(machineId), }); config.telemetry = "enabled"; @@ -205,27 +206,23 @@ describe("Telemetry", () => { session_id: "test-session-id", config_atlas_auth: "true", config_connection_string: expect.any(String) as unknown as string, - device_id: hashedMachineId, + device_id: "test-device-id", }; expect(commonProps).toMatchObject(expectedProps); }); - describe("machine ID resolution", () => { + describe("device ID resolution", () => { beforeEach(() => { vi.clearAllMocks(); - vi.useFakeTimers(); }); afterEach(() => { vi.clearAllMocks(); - vi.useRealTimers(); }); - it("should successfully resolve the machine ID", async () => { - telemetry = Telemetry.create(session, config, { - getRawMachineId: () => Promise.resolve(machineId), - }); + it("should successfully resolve the device ID", async () => { + telemetry = Telemetry.create(session, config); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -233,7 +230,7 @@ describe("Telemetry", () => { await telemetry.setupPromise; expect(telemetry["isBufferingEvents"]).toBe(false); - expect(telemetry.getCommonProperties().device_id).toBe(hashedMachineId); + expect(telemetry.getCommonProperties().device_id).toBe("test-device-id"); }); it("should handle machine ID resolution failure", async () => { @@ -284,7 +281,6 @@ describe("Telemetry", () => { message: "Device ID retrieval timed out", noRedaction: true, }); - }); }); });