From c5c91e9fe4ac5b79e8d8707a0a050d4fcf0f6452 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 28 Jul 2025 13:09:07 +0100 Subject: [PATCH 01/33] feat: update connectionString appName param - [MCP-68] --- src/common/session.ts | 9 +- src/helpers/connectionOptions.ts | 45 +++++++-- src/helpers/deviceId.ts | 48 ++++++++++ src/telemetry/telemetry.ts | 37 +------- src/tools/atlas/connect/connectCluster.ts | 12 ++- 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 | 73 ++++----------- 10 files changed, 261 insertions(+), 108 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 dfae6ec9..d5b18093 100644 --- a/src/common/session.ts +++ b/src/common/session.ts @@ -98,10 +98,15 @@ export class Session extends EventEmitter<{ } async connectToMongoDB(connectionString: string, connectOptions: ConnectOptions): Promise { - connectionString = setAppNameParamIfMissing({ + // Use the extended appName format with deviceId and clientName + connectionString = await setAppNameParamIfMissing({ connectionString, - defaultAppName: `${packageInfo.mcpServerName} ${packageInfo.version}`, + components: { + appName: `${packageInfo.mcpServerName} ${packageInfo.version}`, + clientName: this.agentRunner?.name || "unknown", + }, }); + this.serviceProvider = await NodeDriverServiceProvider.connect(connectionString, { productDocsLink: "https://github.com/mongodb-js/mongodb-mcp-server/", productName: "MongoDB MCP", 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 eb759edc..c1917974 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -5,8 +5,7 @@ import logger, { 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 = () => 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,26 +49,7 @@ export class Telemetry { if (!this.isTelemetryEnabled()) { return; } - this.setupPromise = Promise.all([ - getDeviceId({ - getMachineId: () => this.getRawMachineId(), - onError: (reason, error) => { - switch (reason) { - case "resolutionError": - logger.debug(LogId.telemetryDeviceIdFailure, "telemetry", String(error)); - break; - case "timeout": - logger.debug(LogId.telemetryDeviceIdTimeout, "telemetry", "Device ID retrieval timed out"); - 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; @@ -87,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/src/tools/atlas/connect/connectCluster.ts b/src/tools/atlas/connect/connectCluster.ts index e83c3040..aa32064b 100644 --- a/src/tools/atlas/connect/connectCluster.ts +++ b/src/tools/atlas/connect/connectCluster.ts @@ -6,6 +6,8 @@ import { generateSecurePassword } from "../../../helpers/generatePassword.js"; import logger, { LogId } from "../../../common/logger.js"; import { inspectCluster } from "../../../common/atlas/cluster.js"; import { ensureCurrentIpInAccessList } from "../../../common/atlas/accessListUtils.js"; +import { setAppNameParamIfMissing } from "../../../helpers/connectionOptions.js"; +import { packageInfo } from "../../../common/packageInfo.js"; const EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours @@ -120,7 +122,15 @@ export class ConnectClusterTool extends AtlasToolBase { cn.username = username; cn.password = password; cn.searchParams.set("authSource", "admin"); - return cn.toString(); + + const connectionStringWithAuth = cn.toString(); + return await setAppNameParamIfMissing({ + connectionString: connectionStringWithAuth, + components: { + appName: `${packageInfo.mcpServerName} ${packageInfo.version}`, + clientName: this.session.agentRunner?.name || "unknown", + }, + }); } private async connectToCluster(projectId: string, clusterName: string, connectionString: string): Promise { diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index d3a944f1..3846f387 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -1,15 +1,12 @@ -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"; 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 telemetry = Telemetry.create( new Session({ @@ -23,7 +20,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 d8be8e5a..c50313c6 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 1c7b511b..77d5a3a1 100644 --- a/tests/unit/common/session.test.ts +++ b/tests/unit/common/session.test.ts @@ -4,6 +4,10 @@ import { Session } from "../../../src/common/session.js"; import { config } from "../../../src/common/config.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", () => { @@ -50,11 +54,39 @@ 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"); } }); } + + 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); + expect(session.serviceProvider).toBeDefined(); + + const connectMock = MockNodeDriverServiceProvider.connect; + expect(connectMock).toHaveBeenCalledOnce(); + 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"); + }); }); }); 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 be1aeb9c..b6a13a8c 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -1,12 +1,10 @@ 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"; import { afterEach, beforeEach, describe, it, vi, expect } from "vitest"; -import logger, { LogId } from "../../src/common/logger.js"; -import { createHmac } from "crypto"; import type { MockedFunction } from "vitest"; // Mock the ApiClient to avoid real API calls @@ -17,10 +15,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>; @@ -129,7 +129,6 @@ describe("Telemetry", () => { telemetry = Telemetry.create(session, config, { eventCache: mockEventCache as unknown as EventCache, - getRawMachineId: () => Promise.resolve(machineId), }); config.telemetry = "enabled"; @@ -204,27 +203,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); @@ -232,15 +227,14 @@ 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 () => { - const loggerSpy = vi.spyOn(logger, "debug"); - - telemetry = Telemetry.create(session, config, { - getRawMachineId: () => Promise.reject(new Error("Failed to get device ID")), - }); + it("should handle device ID resolution failure", async () => { + // The deviceId utility is already mocked to return "test-device-id" + // We can't easily test the failure case without complex mocking + // So we'll just verify that the deviceId is set correctly + telemetry = Telemetry.create(session, config); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -248,40 +242,7 @@ describe("Telemetry", () => { await telemetry.setupPromise; expect(telemetry["isBufferingEvents"]).toBe(false); - expect(telemetry.getCommonProperties().device_id).toBe("unknown"); - - expect(loggerSpy).toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, - "telemetry", - "Error: Failed to get device ID" - ); - }); - - it("should timeout if machine ID resolution takes too long", async () => { - const loggerSpy = vi.spyOn(logger, "debug"); - - telemetry = Telemetry.create(session, config, { getRawMachineId: () => new Promise(() => {}) }); - - expect(telemetry["isBufferingEvents"]).toBe(true); - expect(telemetry.getCommonProperties().device_id).toBe(undefined); - - vi.advanceTimersByTime(DEVICE_ID_TIMEOUT / 2); - - // Make sure the timeout doesn't happen prematurely. - expect(telemetry["isBufferingEvents"]).toBe(true); - expect(telemetry.getCommonProperties().device_id).toBe(undefined); - - vi.advanceTimersByTime(DEVICE_ID_TIMEOUT); - - await telemetry.setupPromise; - - expect(telemetry.getCommonProperties().device_id).toBe("unknown"); - expect(telemetry["isBufferingEvents"]).toBe(false); - expect(loggerSpy).toHaveBeenCalledWith( - LogId.telemetryDeviceIdTimeout, - "telemetry", - "Device ID retrieval timed out" - ); + expect(telemetry.getCommonProperties().device_id).toBe("test-device-id"); }); }); }); From 026b91af30fb1d634b88e272ce6cb999a3fb6d56 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 28 Jul 2025 17:13:24 +0100 Subject: [PATCH 02/33] add removed test --- tests/unit/common/session.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/unit/common/session.test.ts b/tests/unit/common/session.test.ts index 30e6b0fb..7d2113f7 100644 --- a/tests/unit/common/session.test.ts +++ b/tests/unit/common/session.test.ts @@ -88,5 +88,17 @@ describe("Session", () => { // 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"); + }); }); }); From 680e1e19641046b5a15f0586aa0106cbd6d790e7 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 28 Jul 2025 17:29:20 +0100 Subject: [PATCH 03/33] update tests --- tests/unit/helpers/deviceId.test.ts | 173 ++++++++++++++++++++++++++++ tests/unit/telemetry.test.ts | 29 ++++- 2 files changed, 197 insertions(+), 5 deletions(-) create mode 100644 tests/unit/helpers/deviceId.test.ts diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts new file mode 100644 index 00000000..800f7a76 --- /dev/null +++ b/tests/unit/helpers/deviceId.test.ts @@ -0,0 +1,173 @@ +/* eslint-disable @typescript-eslint/no-unsafe-assignment, @typescript-eslint/unbound-method */ +import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; +import { getDeviceIdForConnection } from "../../../src/helpers/deviceId.js"; +import { getDeviceId } from "@mongodb-js/device-id"; +import nodeMachineId from "node-machine-id"; +import logger, { LogId } from "../../../src/common/logger.js"; + +// Mock the dependencies +vi.mock("@mongodb-js/device-id"); +vi.mock("node-machine-id"); +vi.mock("../../../src/common/logger.js"); + +const MockGetDeviceId = vi.mocked(getDeviceId); +const MockNodeMachineId = vi.mocked(nodeMachineId); +const MockLogger = vi.mocked(logger); + +describe("Device ID Helper", () => { + beforeEach(() => { + vi.clearAllMocks(); + MockLogger.debug = vi.fn(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe("getDeviceIdForConnection", () => { + it("should successfully retrieve device ID", async () => { + const mockDeviceId = "test-device-id-123"; + const mockMachineId = "machine-id-456"; + + MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); + MockGetDeviceId.mockResolvedValue(mockDeviceId); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe(mockDeviceId); + expect(MockGetDeviceId).toHaveBeenCalledWith({ + getMachineId: expect.any(Function), + onError: expect.any(Function), + abortSignal: expect.any(AbortSignal), + }); + + // Verify the getMachineId function works + const callArgs = MockGetDeviceId.mock.calls[0]?.[0]; + if (callArgs?.getMachineId) { + const getMachineIdFn = callArgs.getMachineId; + expect(await getMachineIdFn()).toBe(mockMachineId); + } + }); + + it("should return 'unknown' when getDeviceId throws an error", async () => { + MockNodeMachineId.machineId.mockResolvedValue("machine-id"); + MockGetDeviceId.mockRejectedValue(new Error("Device ID resolution failed")); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe("unknown"); + expect(MockLogger.debug).toHaveBeenCalledWith( + LogId.telemetryDeviceIdFailure, + "deviceId", + "Failed to get device ID: Error: Device ID resolution failed" + ); + }); + + it("should handle resolution error callback", async () => { + const mockMachineId = "machine-id"; + MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); + MockGetDeviceId.mockImplementation((options) => { + // Simulate a resolution error + if (options.onError) { + options.onError("resolutionError", new Error("Resolution failed")); + } + return Promise.resolve("device-id"); + }); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe("device-id"); + expect(MockLogger.debug).toHaveBeenCalledWith( + LogId.telemetryDeviceIdFailure, + "deviceId", + "Error: Resolution failed" + ); + }); + + it("should handle timeout error callback", async () => { + const mockMachineId = "machine-id"; + MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); + MockGetDeviceId.mockImplementation((options) => { + // Simulate a timeout error + if (options.onError) { + options.onError("timeout", new Error("Timeout")); + } + return Promise.resolve("device-id"); + }); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe("device-id"); + expect(MockLogger.debug).toHaveBeenCalledWith( + LogId.telemetryDeviceIdTimeout, + "deviceId", + "Device ID retrieval timed out" + ); + }); + + it("should handle abort error callback without logging", async () => { + const mockMachineId = "machine-id"; + MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); + MockGetDeviceId.mockImplementation((options) => { + // Simulate an abort error + if (options.onError) { + options.onError("abort", new Error("Aborted")); + } + return Promise.resolve("device-id"); + }); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe("device-id"); + // Should not log abort errors + expect(MockLogger.debug).not.toHaveBeenCalledWith( + LogId.telemetryDeviceIdFailure, + "deviceId", + expect.stringContaining("Aborted") + ); + }); + + it("should handle machine ID generation failure", async () => { + MockNodeMachineId.machineId.mockImplementation(() => { + throw new Error("Machine ID generation failed"); + }); + // Also mock getDeviceId to throw to ensure we get the fallback + MockGetDeviceId.mockRejectedValue(new Error("Device ID failed")); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe("unknown"); + expect(MockLogger.debug).toHaveBeenCalledWith( + LogId.telemetryDeviceIdFailure, + "deviceId", + "Failed to get device ID: Error: Device ID failed" + ); + }); + + it("should use AbortController signal", async () => { + MockNodeMachineId.machineId.mockResolvedValue("machine-id"); + MockGetDeviceId.mockResolvedValue("device-id"); + + await getDeviceIdForConnection(); + + const callArgs = MockGetDeviceId.mock.calls[0]?.[0]; + if (callArgs) { + expect(callArgs.abortSignal).toBeInstanceOf(AbortSignal); + } + }); + + it("should handle non-Error exceptions", async () => { + MockNodeMachineId.machineId.mockResolvedValue("machine-id"); + MockGetDeviceId.mockRejectedValue("String error"); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe("unknown"); + expect(MockLogger.debug).toHaveBeenCalledWith( + LogId.telemetryDeviceIdFailure, + "deviceId", + "Failed to get device ID: String error" + ); + }); + }); +}); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index b6a13a8c..e0589cf7 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -230,10 +230,11 @@ describe("Telemetry", () => { expect(telemetry.getCommonProperties().device_id).toBe("test-device-id"); }); - it("should handle device ID resolution failure", async () => { - // The deviceId utility is already mocked to return "test-device-id" - // We can't easily test the failure case without complex mocking - // So we'll just verify that the deviceId is set correctly + it("should handle device ID resolution failure gracefully", async () => { + // Mock the deviceId utility to return "unknown" for this test + const { getDeviceIdForConnection } = await import("../../src/helpers/deviceId.js"); + vi.mocked(getDeviceIdForConnection).mockResolvedValueOnce("unknown"); + telemetry = Telemetry.create(session, config); expect(telemetry["isBufferingEvents"]).toBe(true); @@ -242,7 +243,25 @@ describe("Telemetry", () => { await telemetry.setupPromise; expect(telemetry["isBufferingEvents"]).toBe(false); - expect(telemetry.getCommonProperties().device_id).toBe("test-device-id"); + // Should use "unknown" as fallback when device ID resolution fails + expect(telemetry.getCommonProperties().device_id).toBe("unknown"); + }); + + it("should handle device ID timeout gracefully", async () => { + // Mock the deviceId utility to return "unknown" for this test + const { getDeviceIdForConnection } = await import("../../src/helpers/deviceId.js"); + vi.mocked(getDeviceIdForConnection).mockResolvedValueOnce("unknown"); + + telemetry = Telemetry.create(session, config); + + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); + + await telemetry.setupPromise; + + expect(telemetry["isBufferingEvents"]).toBe(false); + // Should use "unknown" as fallback when device ID times out + expect(telemetry.getCommonProperties().device_id).toBe("unknown"); }); }); }); From eca40e10a4f7d899654557dd3232ed3a858a9760 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Thu, 31 Jul 2025 13:51:47 +0100 Subject: [PATCH 04/33] add timeout test --- tests/unit/helpers/deviceId.test.ts | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 800f7a76..c9144005 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/no-unsafe-assignment, @typescript-eslint/unbound-method */ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; -import { getDeviceIdForConnection } from "../../../src/helpers/deviceId.js"; +import { getDeviceIdForConnection, DEVICE_ID_TIMEOUT } from "../../../src/helpers/deviceId.js"; import { getDeviceId } from "@mongodb-js/device-id"; import nodeMachineId from "node-machine-id"; import logger, { LogId } from "../../../src/common/logger.js"; @@ -105,6 +105,31 @@ describe("Device ID Helper", () => { ); }); + it("should handle timeout with timer advancement", async () => { + vi.useFakeTimers(); + + const mockMachineId = "machine-id"; + MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); + MockGetDeviceId.mockImplementation((options) => { + vi.advanceTimersByTime(DEVICE_ID_TIMEOUT / 2); + if (options.onError) { + options.onError("timeout", new Error("Timeout")); + } + return Promise.resolve("device-id"); + }); + + const result = await getDeviceIdForConnection(); + + expect(result).toBe("device-id"); + expect(MockLogger.debug).toHaveBeenCalledWith( + LogId.telemetryDeviceIdTimeout, + "deviceId", + "Device ID retrieval timed out" + ); + + vi.useRealTimers(); + }); + it("should handle abort error callback without logging", async () => { const mockMachineId = "machine-id"; MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); From 524d965f77379d4afaaca105c86714bcb63033c8 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Thu, 31 Jul 2025 14:04:26 +0100 Subject: [PATCH 05/33] fix --- src/common/session.ts | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/common/session.ts b/src/common/session.ts index 051db2ee..93920b8e 100644 --- a/src/common/session.ts +++ b/src/common/session.ts @@ -111,22 +111,6 @@ export class Session extends EventEmitter { }, }); -<<<<<<< HEAD - this.serviceProvider = await NodeDriverServiceProvider.connect(connectionString, { - productDocsLink: "https://github.com/mongodb-js/mongodb-mcp-server/", - productName: "MongoDB MCP", - readConcern: { - level: connectOptions.readConcern, - }, - readPreference: connectOptions.readPreference, - writeConcern: { - w: connectOptions.writeConcern, - }, - timeoutMS: connectOptions.timeoutMS, - proxy: { useEnvironmentVariableProxies: true }, - applyProxyToOIDC: true, - }); -======= try { this.serviceProvider = await NodeDriverServiceProvider.connect(connectionString, { productDocsLink: "https://github.com/mongodb-js/mongodb-mcp-server/", @@ -151,6 +135,5 @@ export class Session extends EventEmitter { } this.emit("connect"); ->>>>>>> origin/main } } From 72f1ab810ba864cb490025db468c77112c415c13 Mon Sep 17 00:00:00 2001 From: Bianca Lisle <40155621+blva@users.noreply.github.com> Date: Thu, 31 Jul 2025 14:19:33 +0100 Subject: [PATCH 06/33] Update src/helpers/deviceId.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/helpers/deviceId.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 043acb8a..88a3d3b6 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -5,20 +5,16 @@ 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 + * Retrieves the device ID for telemetry purposes. + * The device ID is generated using the machine ID and additional logic to handle errors. * - * @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 + * @returns Promise that resolves to the device ID string + * If an error occurs during retrieval, the function returns "unknown". * * @example * ```typescript - * const result = await setExtendedAppNameParam({ - * connectionString: "mongodb://localhost:27017", - * components: { appName: "MyApp", clientName: "Cursor" } - * }); - * // Result: "mongodb://localhost:27017/?appName=MyApp--deviceId--Cursor" + * const deviceId = await getDeviceIdForConnection(); + * console.log(deviceId); // Outputs the device ID or "unknown" in case of failure * ``` */ export async function getDeviceIdForConnection(): Promise { From c3f09285453d47557def785b7336835bf959e671 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Thu, 31 Jul 2025 14:38:31 +0100 Subject: [PATCH 07/33] add buffering update back --- src/telemetry/telemetry.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index c1917974..0264eb8c 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -60,6 +60,7 @@ export class Telemetry { } public async close(): Promise { + this.isBufferingEvents = false; await this.emitEvents(this.eventCache.getEvents()); } From f8de87740a1f379f72ad0e25250240e1eb59e6ee Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Thu, 31 Jul 2025 14:39:33 +0100 Subject: [PATCH 08/33] squashed commits --- src/helpers/connectionOptions.ts | 13 ++----------- src/helpers/deviceId.ts | 4 +++- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/helpers/connectionOptions.ts b/src/helpers/connectionOptions.ts index 7f8f7856..0d7abf3e 100644 --- a/src/helpers/connectionOptions.ts +++ b/src/helpers/connectionOptions.ts @@ -30,17 +30,8 @@ export async function setAppNameParamIfMissing({ 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"; - } + const deviceId = components.deviceId || (await getDeviceIdForConnection()); + const clientName = components.clientName || "unknown"; // Build the extended appName format: appName--deviceId--clientName const extendedAppName = `${components.appName}--${deviceId}--${clientName}`; diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 88a3d3b6..c3239cad 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -18,6 +18,8 @@ export const DEVICE_ID_TIMEOUT = 3000; * ``` */ export async function getDeviceIdForConnection(): Promise { + const controller = new AbortController(); + try { const deviceId = await getDeviceId({ getMachineId: () => nodeMachineId.machineId(true), @@ -34,7 +36,7 @@ export async function getDeviceIdForConnection(): Promise { break; } }, - abortSignal: new AbortController().signal, + abortSignal: controller.signal, }); return deviceId; } catch (error) { From 0c620b28cfd1a2f4f1ba813e269d8175143683f0 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 11:07:37 +0100 Subject: [PATCH 09/33] move appName setting to connection manager --- src/common/connectionManager.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/common/connectionManager.ts b/src/common/connectionManager.ts index 6c2cb277..b8d9e237 100644 --- a/src/common/connectionManager.ts +++ b/src/common/connectionManager.ts @@ -84,9 +84,12 @@ export class ConnectionManager extends EventEmitter { let serviceProvider: NodeDriverServiceProvider; try { settings = { ...settings }; - settings.connectionString = setAppNameParamIfMissing({ + settings.connectionString = await setAppNameParamIfMissing({ connectionString: settings.connectionString, - defaultAppName: `${packageInfo.mcpServerName} ${packageInfo.version}`, + components: { + appName: `${packageInfo.mcpServerName} ${packageInfo.version}`, + // clientName: this.agentRunner?.name || "unknown", //TODO: MCP-68 - get client name from session + }, }); serviceProvider = await NodeDriverServiceProvider.connect(settings.connectionString, { From d148376dbaca1ced9fe4ce8a3e7d113955534e17 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 12:18:38 +0100 Subject: [PATCH 10/33] chore: rename agentRunner to mcpClient --- src/common/connectionManager.ts | 4 +++- src/common/logger.ts | 1 + src/common/session.ts | 28 +++++++++++++++++++--------- src/helpers/deviceId.ts | 6 +++--- src/server.ts | 4 ++-- src/telemetry/telemetry.ts | 4 ++-- tests/unit/common/session.test.ts | 2 +- 7 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/common/connectionManager.ts b/src/common/connectionManager.ts index b8d9e237..114fa533 100644 --- a/src/common/connectionManager.ts +++ b/src/common/connectionManager.ts @@ -88,7 +88,9 @@ export class ConnectionManager extends EventEmitter { connectionString: settings.connectionString, components: { appName: `${packageInfo.mcpServerName} ${packageInfo.version}`, - // clientName: this.agentRunner?.name || "unknown", //TODO: MCP-68 - get client name from session + // deviceId: deviceId, //TODO: MCP-68 - get deviceId from session + // clientName: this.clientInfo?.name || "unknown", + clientName: "unknown", //TODO: MCP-68 - get client name from session }, }); diff --git a/src/common/logger.ts b/src/common/logger.ts index 0add105c..f377e80a 100644 --- a/src/common/logger.ts +++ b/src/common/logger.ts @@ -14,6 +14,7 @@ export const LogId = { serverClosed: mongoLogId(1_000_004), serverCloseFailure: mongoLogId(1_000_005), serverDuplicateLoggers: mongoLogId(1_000_006), + serverMcpClientSet: mongoLogId(1_000_007), atlasCheckCredentials: mongoLogId(1_001_001), atlasDeleteDatabaseUserFailure: mongoLogId(1_001_002), diff --git a/src/common/session.ts b/src/common/session.ts index 444a747b..c8d46fc6 100644 --- a/src/common/session.ts +++ b/src/common/session.ts @@ -34,9 +34,10 @@ export class Session extends EventEmitter { readonly exportsManager: ExportsManager; readonly connectionManager: ConnectionManager; readonly apiClient: ApiClient; - agentRunner?: { - name: string; - version: string; + mcpClient?: { + name?: string; + version?: string; + title?: string; }; public logger: CompositeLogger; @@ -69,13 +70,22 @@ export class Session extends EventEmitter { this.connectionManager.on("connection-errored", (error) => this.emit("connection-error", error.errorReason)); } - setAgentRunner(agentRunner: Implementation | undefined): void { - if (agentRunner?.name && agentRunner?.version) { - this.agentRunner = { - name: agentRunner.name, - version: agentRunner.version, - }; + setMcpClient(mcpClient: Implementation | undefined): void { + if (!mcpClient) { + this.mcpClient = undefined; + this.logger.debug({ + id: LogId.serverMcpClientSet, + context: "session", + message: "MCP client info not found", + }); + return; } + + this.mcpClient = { + name: mcpClient.name || "unknown", + version: mcpClient.version || "unknown", + title: mcpClient.title || "unknown", + }; } async disconnect(): Promise { diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index c3239cad..9ecf1dcd 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -1,6 +1,6 @@ import { getDeviceId } from "@mongodb-js/device-id"; import nodeMachineId from "node-machine-id"; -import logger, { LogId } from "../common/logger.js"; +import { LogId, CompositeLogger } from "../common/logger.js"; export const DEVICE_ID_TIMEOUT = 3000; @@ -13,11 +13,11 @@ export const DEVICE_ID_TIMEOUT = 3000; * * @example * ```typescript - * const deviceId = await getDeviceIdForConnection(); + * const deviceId = await getDeviceIdForConnection(logger); * console.log(deviceId); // Outputs the device ID or "unknown" in case of failure * ``` */ -export async function getDeviceIdForConnection(): Promise { +export async function getDeviceIdForConnection(logger: CompositeLogger): Promise { const controller = new AbortController(); try { diff --git a/src/server.ts b/src/server.ts index bf41b26d..2bb60593 100644 --- a/src/server.ts +++ b/src/server.ts @@ -97,12 +97,12 @@ export class Server { }); this.mcpServer.server.oninitialized = (): void => { - this.session.setAgentRunner(this.mcpServer.server.getClientVersion()); + this.session.setMcpClient(this.mcpServer.server.getClientVersion()); this.session.logger.info({ id: LogId.serverInitialized, context: "server", - message: `Server started with transport ${transport.constructor.name} and agent runner ${this.session.agentRunner?.name}`, + message: `Server started with transport ${transport.constructor.name} and agent runner ${this.session.mcpClient?.name}`, }); this.emitServerEvent("start", Date.now() - this.startTime); diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index ec8c4961..9a1d6ef8 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -98,8 +98,8 @@ export class Telemetry { return { ...this.commonProperties, transport: this.userConfig.transport, - mcp_client_version: this.session.agentRunner?.version, - mcp_client_name: this.session.agentRunner?.name, + mcp_client_version: this.session.mcpClient?.version, + mcp_client_name: this.session.mcpClient?.name, session_id: this.session.sessionId, config_atlas_auth: this.session.apiClient.hasCredentials() ? "true" : "false", config_connection_string: this.userConfig.connectionString ? "true" : "false", diff --git a/tests/unit/common/session.test.ts b/tests/unit/common/session.test.ts index a54f9168..b837c7af 100644 --- a/tests/unit/common/session.test.ts +++ b/tests/unit/common/session.test.ts @@ -85,7 +85,7 @@ describe("Session", () => { }); it("should include client name when agent runner is set", async () => { - session.setAgentRunner({ name: "test-client", version: "1.0.0" }); + session.setMcpClient({ name: "test-client", version: "1.0.0" }); await session.connectToMongoDB("mongodb://localhost:27017", config.connectOptions); expect(session.serviceProvider).toBeDefined(); From 13a034938cc9ddc908ac95667a9cb81a3ac626bd Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 16:45:29 +0100 Subject: [PATCH 11/33] refactor and address some comments --- src/common/connectionManager.ts | 23 +- src/common/session.ts | 12 +- src/helpers/connectionOptions.ts | 6 +- src/helpers/deviceId.ts | 186 ++++++++++++--- src/index.ts | 5 + src/telemetry/telemetry.ts | 17 +- tests/integration/helpers.ts | 6 + tests/integration/telemetry.test.ts | 11 +- tests/unit/common/session.test.ts | 13 +- tests/unit/helpers/connectionOptions.test.ts | 13 +- tests/unit/helpers/deviceId.test.ts | 235 +++++++++++-------- tests/unit/telemetry.test.ts | 34 ++- 12 files changed, 392 insertions(+), 169 deletions(-) diff --git a/src/common/connectionManager.ts b/src/common/connectionManager.ts index 114fa533..8e418d43 100644 --- a/src/common/connectionManager.ts +++ b/src/common/connectionManager.ts @@ -6,6 +6,8 @@ import { packageInfo } from "./packageInfo.js"; import ConnectionString from "mongodb-connection-string-url"; import { MongoClientOptions } from "mongodb"; import { ErrorCodes, MongoDBError } from "./errors.js"; +import { DeviceIdService } from "../helpers/deviceId.js"; +import { AppNameComponents } from "../helpers/connectionOptions.js"; export interface AtlasClusterConnectionInfo { username: string; @@ -67,11 +69,19 @@ export interface ConnectionManagerEvents { export class ConnectionManager extends EventEmitter { private state: AnyConnectionState; + private deviceId: DeviceIdService; + private clientName: string; constructor() { super(); this.state = { tag: "disconnected" }; + this.deviceId = DeviceIdService.getInstance(); + this.clientName = "unknown"; + } + + setClientName(clientName: string): void { + this.clientName = clientName; } async connect(settings: ConnectionSettings): Promise { @@ -84,14 +94,15 @@ export class ConnectionManager extends EventEmitter { let serviceProvider: NodeDriverServiceProvider; try { settings = { ...settings }; + const appNameComponents: AppNameComponents = { + appName: `${packageInfo.mcpServerName} ${packageInfo.version}`, + deviceId: this.deviceId.getDeviceId(), + clientName: this.clientName, + }; + settings.connectionString = await setAppNameParamIfMissing({ connectionString: settings.connectionString, - components: { - appName: `${packageInfo.mcpServerName} ${packageInfo.version}`, - // deviceId: deviceId, //TODO: MCP-68 - get deviceId from session - // clientName: this.clientInfo?.name || "unknown", - clientName: "unknown", //TODO: MCP-68 - get client name from session - }, + components: appNameComponents, }); serviceProvider = await NodeDriverServiceProvider.connect(settings.connectionString, { diff --git a/src/common/session.ts b/src/common/session.ts index c8d46fc6..b13c4a7e 100644 --- a/src/common/session.ts +++ b/src/common/session.ts @@ -72,20 +72,22 @@ export class Session extends EventEmitter { setMcpClient(mcpClient: Implementation | undefined): void { if (!mcpClient) { - this.mcpClient = undefined; + this.connectionManager.setClientName("unknown"); this.logger.debug({ id: LogId.serverMcpClientSet, context: "session", message: "MCP client info not found", }); - return; } this.mcpClient = { - name: mcpClient.name || "unknown", - version: mcpClient.version || "unknown", - title: mcpClient.title || "unknown", + name: mcpClient?.name || "unknown", + version: mcpClient?.version || "unknown", + title: mcpClient?.title || "unknown", }; + + // Set the client name on the connection manager for appName generation + this.connectionManager.setClientName(this.mcpClient.name || "unknown"); } async disconnect(): Promise { diff --git a/src/helpers/connectionOptions.ts b/src/helpers/connectionOptions.ts index 0d7abf3e..4c2bba2b 100644 --- a/src/helpers/connectionOptions.ts +++ b/src/helpers/connectionOptions.ts @@ -1,10 +1,9 @@ import { MongoClientOptions } from "mongodb"; import ConnectionString from "mongodb-connection-string-url"; -import { getDeviceIdForConnection } from "./deviceId.js"; export interface AppNameComponents { appName: string; - deviceId?: string; + deviceId?: Promise; clientName?: string; } @@ -30,7 +29,8 @@ export async function setAppNameParamIfMissing({ return connectionStringUrl.toString(); } - const deviceId = components.deviceId || (await getDeviceIdForConnection()); + const deviceId = components.deviceId ? await components.deviceId : "unknown"; + const clientName = components.clientName || "unknown"; // Build the extended appName format: appName--deviceId--clientName diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 9ecf1dcd..d1d8801b 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -1,46 +1,156 @@ import { getDeviceId } from "@mongodb-js/device-id"; -import nodeMachineId from "node-machine-id"; -import { LogId, CompositeLogger } from "../common/logger.js"; +import { LogId, LoggerBase } from "../common/logger.js"; export const DEVICE_ID_TIMEOUT = 3000; /** - * Retrieves the device ID for telemetry purposes. - * The device ID is generated using the machine ID and additional logic to handle errors. - * - * @returns Promise that resolves to the device ID string - * If an error occurs during retrieval, the function returns "unknown". - * - * @example - * ```typescript - * const deviceId = await getDeviceIdForConnection(logger); - * console.log(deviceId); // Outputs the device ID or "unknown" in case of failure - * ``` + * Singleton class for managing device ID retrieval and caching. + * Starts device ID calculation early and is shared across all services. */ -export async function getDeviceIdForConnection(logger: CompositeLogger): Promise { - const controller = new AbortController(); - - 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: controller.signal, - }); - return deviceId; - } catch (error) { - logger.debug(LogId.telemetryDeviceIdFailure, "deviceId", `Failed to get device ID: ${String(error)}`); - return "unknown"; +export class DeviceIdService { + private static instance: DeviceIdService | undefined = undefined; + private deviceId: string | undefined = undefined; + private deviceIdPromise: Promise | undefined = undefined; + private abortController: AbortController | undefined = undefined; + private logger: LoggerBase; + private getMachineId: () => Promise; + + private constructor(logger: LoggerBase, getMachineId: () => Promise) { + this.logger = logger; + this.getMachineId = getMachineId; + // Start device ID calculation immediately + this.startDeviceIdCalculation(); + } + + /** + * Initializes the DeviceIdService singleton with a logger. + * A separated init method is used to use a single instance of the logger. + * @param logger - The logger instance to use + * @returns The DeviceIdService instance + */ + public static init(logger: LoggerBase, getMachineId: () => Promise): DeviceIdService { + if (DeviceIdService.instance) { + return DeviceIdService.instance; + } + DeviceIdService.instance = new DeviceIdService(logger, getMachineId); + return DeviceIdService.instance; + } + + /** + * Gets the singleton instance of DeviceIdService. + * @returns The DeviceIdService instance + */ + public static getInstance(): DeviceIdService { + if (!DeviceIdService.instance) { + throw Error("DeviceIdService not initialized"); + } + return DeviceIdService.instance; + } + + /** + * Resets the singleton instance (mainly for testing). + */ + static resetInstance(): void { + DeviceIdService.instance = undefined; + } + + /** + * Starts the device ID calculation process. + * This method is called automatically in the constructor. + */ + private startDeviceIdCalculation(): void { + if (this.deviceIdPromise) { + return; + } + + this.abortController = new AbortController(); + this.deviceIdPromise = this.calculateDeviceId(); + } + + /** + * Gets the device ID, waiting for the calculation to complete if necessary. + * @returns Promise that resolves to the device ID string + */ + public async getDeviceId(): Promise { + // Return cached value if available + if (this.deviceId !== undefined) { + return this.deviceId; + } + + // If calculation is already in progress, wait for it + if (this.deviceIdPromise) { + return this.deviceIdPromise; + } + + // If somehow we don't have a promise, raise an error + throw new Error("Failed to get device ID"); + } + /** + * Aborts any ongoing device ID calculation. + */ + abortCalculation(): void { + if (this.abortController) { + this.abortController.abort(); + this.abortController = undefined; + } + this.deviceIdPromise = undefined; + } + + /** + * Internal method that performs the actual device ID calculation. + */ + private async calculateDeviceId(): Promise { + if (!this.abortController) { + throw new Error("Device ID calculation not started"); + } + + try { + const deviceId = await getDeviceId({ + getMachineId: this.getMachineId, + onError: (reason, error) => { + switch (reason) { + case "resolutionError": + this.logger.debug({ + id: LogId.telemetryDeviceIdFailure, + context: "deviceId", + message: `Device ID resolution error: ${String(error)}`, + }); + break; + case "timeout": + this.logger.debug({ + id: LogId.telemetryDeviceIdTimeout, + context: "deviceId", + message: "Device ID retrieval timed out", + }); + break; + case "abort": + // No need to log in the case of aborts + break; + } + }, + abortSignal: this.abortController.signal, + }); + + // Cache the result + this.deviceId = deviceId; + return deviceId; + } catch (error) { + // Check if this was an abort error + if (error instanceof Error && error.name === "AbortError") { + throw error; // Re-throw abort errors + } + + this.logger.debug({ + id: LogId.telemetryDeviceIdFailure, + context: "deviceId", + message: `Failed to get device ID: ${String(error)}`, + }); + + // Cache the fallback value + this.deviceId = "unknown"; + return "unknown"; + } finally { + this.abortController = undefined; + } } } diff --git a/src/index.ts b/src/index.ts index f391a9a7..339b83eb 100644 --- a/src/index.ts +++ b/src/index.ts @@ -42,6 +42,8 @@ import { packageInfo } from "./common/packageInfo.js"; import { StdioRunner } from "./transports/stdio.js"; import { StreamableHttpRunner } from "./transports/streamableHttp.js"; import { systemCA } from "@mongodb-js/devtools-proxy-support"; +import { DeviceIdService } from "./helpers/deviceId.js"; +import nodeMachineId from "node-machine-id"; async function main(): Promise { systemCA().catch(() => undefined); // load system CA asynchronously as in mongosh @@ -50,6 +52,7 @@ async function main(): Promise { assertVersionMode(); const transportRunner = config.transport === "stdio" ? new StdioRunner(config) : new StreamableHttpRunner(config); + const deviceId = DeviceIdService.init(transportRunner.logger, () => nodeMachineId.machineId(true)); const shutdown = (): void => { transportRunner.logger.info({ @@ -61,6 +64,7 @@ async function main(): Promise { transportRunner .close() .then(() => { + deviceId.abortCalculation(); transportRunner.logger.info({ id: LogId.serverClosed, context: "server", @@ -69,6 +73,7 @@ async function main(): Promise { process.exit(0); }) .catch((error: unknown) => { + deviceId.abortCalculation(); transportRunner.logger.error({ id: LogId.serverCloseFailure, context: "server", diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 9a1d6ef8..229fc7f5 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -5,8 +5,8 @@ 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 { getDeviceIdForConnection } from "../helpers/deviceId.js"; import { detectContainerEnv } from "../helpers/container.js"; +import { DeviceIdService } from "../helpers/deviceId.js"; type EventResult = { success: boolean; @@ -18,14 +18,16 @@ export class Telemetry { /** Resolves when the setup is complete or a timeout occurs */ public setupPromise: Promise<[string, boolean]> | undefined; private eventCache: EventCache; + private deviceId: DeviceIdService; private constructor( private readonly session: Session, private readonly userConfig: UserConfig, private readonly commonProperties: CommonProperties, - { eventCache }: { eventCache: EventCache } + { eventCache, deviceId }: { eventCache: EventCache; deviceId: DeviceIdService } ) { this.eventCache = eventCache; + this.deviceId = deviceId; } static create( @@ -34,12 +36,14 @@ export class Telemetry { { commonProperties = { ...MACHINE_METADATA }, eventCache = EventCache.getInstance(), + deviceId = DeviceIdService.getInstance(), }: { eventCache?: EventCache; + deviceId?: DeviceIdService; commonProperties?: CommonProperties; } = {} ): Telemetry { - const instance = new Telemetry(session, userConfig, commonProperties, { eventCache }); + const instance = new Telemetry(session, userConfig, commonProperties, { eventCache, deviceId }); void instance.setup(); return instance; @@ -49,10 +53,11 @@ export class Telemetry { if (!this.isTelemetryEnabled()) { return; } - this.setupPromise = Promise.all([getDeviceIdForConnection(), detectContainerEnv()]); - const [deviceId, containerEnv] = await this.setupPromise; - this.commonProperties.device_id = deviceId; + this.setupPromise = Promise.all([this.deviceId.getDeviceId(), detectContainerEnv()]); + const [deviceIdValue, containerEnv] = await this.setupPromise; + + this.commonProperties.device_id = deviceIdValue; this.commonProperties.is_container_env = containerEnv; this.isBufferingEvents = false; diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index 738cbdfd..f80d2e72 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -11,6 +11,8 @@ import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest import { ConnectionManager } from "../../src/common/connectionManager.js"; import { CompositeLogger } from "../../src/common/logger.js"; import { ExportsManager } from "../../src/common/exportsManager.js"; +import { DeviceIdService } from "../../src/helpers/deviceId.js"; +import nodeMachineId from "node-machine-id"; interface ParameterInfo { name: string; @@ -58,6 +60,10 @@ export function setupIntegrationTest(getUserConfig: () => UserConfig): Integrati const logger = new CompositeLogger(); const exportsManager = ExportsManager.init(userConfig, logger); + + // Initialize DeviceIdService for tests + DeviceIdService.init(logger, () => nodeMachineId.machineId(true)); + const connectionManager = new ConnectionManager(); const session = new Session({ diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index 7a196fe0..ad886e72 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -1,17 +1,22 @@ import { Telemetry } from "../../src/telemetry/telemetry.js"; import { Session } from "../../src/common/session.js"; import { config } from "../../src/common/config.js"; -import { getDeviceIdForConnection } from "../../src/helpers/deviceId.js"; +import { DeviceIdService } 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"; +import nodeMachineId from "node-machine-id"; describe("Telemetry", () => { it("should resolve the actual device ID", async () => { - const actualDeviceId = await getDeviceIdForConnection(); - const logger = new CompositeLogger(); + + // Initialize DeviceIdService like the main application does + DeviceIdService.init(logger, () => nodeMachineId.machineId(true)); + + const actualDeviceId = await DeviceIdService.getInstance().getDeviceId(); + const telemetry = Telemetry.create( new Session({ apiBaseUrl: "", diff --git a/tests/unit/common/session.test.ts b/tests/unit/common/session.test.ts index b837c7af..69f6a497 100644 --- a/tests/unit/common/session.test.ts +++ b/tests/unit/common/session.test.ts @@ -8,7 +8,14 @@ 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"), + DeviceIdService: { + init: vi.fn(), + getInstance: vi.fn().mockReturnValue({ + getDeviceId: vi.fn().mockResolvedValue("test-device-id"), + }), + getDeviceId: vi.fn().mockResolvedValue("test-device-id"), + resetInstance: vi.fn(), + }, })); const MockNodeDriverServiceProvider = vi.mocked(NodeDriverServiceProvider); @@ -87,7 +94,7 @@ describe("Session", () => { it("should include client name when agent runner is set", async () => { session.setMcpClient({ name: "test-client", version: "1.0.0" }); - await session.connectToMongoDB("mongodb://localhost:27017", config.connectOptions); + await session.connectToMongoDB({ connectionString: "mongodb://localhost:27017" }); expect(session.serviceProvider).toBeDefined(); const connectMock = MockNodeDriverServiceProvider.connect; @@ -99,7 +106,7 @@ describe("Session", () => { }); it("should use 'unknown' for client name when agent runner is not set", async () => { - await session.connectToMongoDB("mongodb://localhost:27017", config.connectOptions); + await session.connectToMongoDB({ connectionString: "mongodb://localhost:27017" }); expect(session.serviceProvider).toBeDefined(); const connectMock = MockNodeDriverServiceProvider.connect; diff --git a/tests/unit/helpers/connectionOptions.test.ts b/tests/unit/helpers/connectionOptions.test.ts index 2e2fee2d..f3c2a971 100644 --- a/tests/unit/helpers/connectionOptions.test.ts +++ b/tests/unit/helpers/connectionOptions.test.ts @@ -1,9 +1,14 @@ import { describe, expect, it, vi } from "vitest"; import { setAppNameParamIfMissing } from "../../../src/helpers/connectionOptions.js"; +import { DeviceIdService } from "../../../src/helpers/deviceId.js"; // Mock the deviceId utility vi.mock("../../../src/helpers/deviceId.js", () => ({ - getDeviceIdForConnection: vi.fn().mockResolvedValue("test-device-id"), + DeviceIdService: { + getInstance: vi.fn().mockReturnValue({ + getDeviceId: vi.fn().mockResolvedValue("test-device-id"), + }), + }, })); describe("Connection Options", () => { @@ -15,6 +20,7 @@ describe("Connection Options", () => { components: { appName: "TestApp", clientName: "TestClient", + deviceId: DeviceIdService.getInstance().getDeviceId(), }, }); @@ -42,7 +48,7 @@ describe("Connection Options", () => { connectionString, components: { appName: "TestApp", - deviceId: "custom-device-id", + deviceId: Promise.resolve("custom-device-id"), clientName: "TestClient", }, }); @@ -56,6 +62,7 @@ describe("Connection Options", () => { connectionString, components: { appName: "TestApp", + deviceId: DeviceIdService.getInstance().getDeviceId(), }, }); @@ -69,6 +76,7 @@ describe("Connection Options", () => { components: { appName: "TestApp", clientName: "TestClient", + deviceId: DeviceIdService.getInstance().getDeviceId(), }, }); @@ -82,6 +90,7 @@ describe("Connection Options", () => { components: { appName: "TestApp", clientName: "TestClient", + deviceId: DeviceIdService.getInstance().getDeviceId(), }, }); diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index c9144005..5c284795 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -1,179 +1,220 @@ -/* eslint-disable @typescript-eslint/no-unsafe-assignment, @typescript-eslint/unbound-method */ +/* eslint-disable @typescript-eslint/no-unsafe-assignment */ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; -import { getDeviceIdForConnection, DEVICE_ID_TIMEOUT } from "../../../src/helpers/deviceId.js"; +import { DeviceIdService } from "../../../src/helpers/deviceId.js"; import { getDeviceId } from "@mongodb-js/device-id"; -import nodeMachineId from "node-machine-id"; -import logger, { LogId } from "../../../src/common/logger.js"; +import { CompositeLogger } from "../../../src/common/logger.js"; // Mock the dependencies vi.mock("@mongodb-js/device-id"); vi.mock("node-machine-id"); -vi.mock("../../../src/common/logger.js"); const MockGetDeviceId = vi.mocked(getDeviceId); -const MockNodeMachineId = vi.mocked(nodeMachineId); -const MockLogger = vi.mocked(logger); describe("Device ID Helper", () => { + let testLogger: CompositeLogger; + let mockGetMachineId: () => Promise; + beforeEach(() => { vi.clearAllMocks(); - MockLogger.debug = vi.fn(); + testLogger = new CompositeLogger(); + + // Create a mock getMachineId function + mockGetMachineId = vi.fn().mockResolvedValue("test-machine-id"); + + // Reset singleton between tests + DeviceIdService.resetInstance(); }); afterEach(() => { vi.restoreAllMocks(); + DeviceIdService.resetInstance(); }); - describe("getDeviceIdForConnection", () => { + describe("DeviceIdService Singleton", () => { + it("should return the same instance for multiple getInstance calls", () => { + // Initialize first + DeviceIdService.init(testLogger, mockGetMachineId); + + const instance1 = DeviceIdService.getInstance(); + const instance2 = DeviceIdService.getInstance(); + + expect(instance1).toBe(instance2); + }); + + it("should throw error when getInstance is called before init", () => { + expect(() => DeviceIdService.getInstance()).toThrow("DeviceIdService not initialized"); + }); + it("should successfully retrieve device ID", async () => { const mockDeviceId = "test-device-id-123"; - const mockMachineId = "machine-id-456"; - - MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); MockGetDeviceId.mockResolvedValue(mockDeviceId); - const result = await getDeviceIdForConnection(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); expect(result).toBe(mockDeviceId); expect(MockGetDeviceId).toHaveBeenCalledWith({ - getMachineId: expect.any(Function), + getMachineId: mockGetMachineId, onError: expect.any(Function), abortSignal: expect.any(AbortSignal), }); + }); - // Verify the getMachineId function works - const callArgs = MockGetDeviceId.mock.calls[0]?.[0]; - if (callArgs?.getMachineId) { - const getMachineIdFn = callArgs.getMachineId; - expect(await getMachineIdFn()).toBe(mockMachineId); - } + it("should cache device ID after first retrieval", async () => { + const mockDeviceId = "test-device-id-123"; + MockGetDeviceId.mockResolvedValue(mockDeviceId); + + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + + // First call should trigger calculation + const result1 = await deviceId.getDeviceId(); + expect(result1).toBe(mockDeviceId); + expect(MockGetDeviceId).toHaveBeenCalledTimes(1); + + // Second call should use cached value + const result2 = await deviceId.getDeviceId(); + expect(result2).toBe(mockDeviceId); + expect(MockGetDeviceId).toHaveBeenCalledTimes(1); // Still only called once + }); + + it("should return cached device ID without triggering calculation", async () => { + const mockDeviceId = "test-device-id-123"; + MockGetDeviceId.mockResolvedValue(mockDeviceId); + + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + + // First call to populate cache + await deviceId.getDeviceId(); + + // Get cached value without triggering calculation + const cachedValue = await deviceId.getDeviceId(); + expect(cachedValue).toBe(mockDeviceId); + }); + + it("should allow aborting calculation", async () => { + MockGetDeviceId.mockImplementation((options) => { + // Simulate a long-running operation that can be aborted + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => resolve("device-id"), 1000); + options.abortSignal?.addEventListener("abort", () => { + clearTimeout(timeout); + const abortError = new Error("Aborted"); + abortError.name = "AbortError"; + reject(abortError); + }); + }); + }); + + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + + // Start calculation + const promise = deviceId.getDeviceId(); + + // Abort immediately + deviceId.abortCalculation(); + + // Should reject with AbortError + await expect(promise).rejects.toThrow("Aborted"); }); it("should return 'unknown' when getDeviceId throws an error", async () => { - MockNodeMachineId.machineId.mockResolvedValue("machine-id"); MockGetDeviceId.mockRejectedValue(new Error("Device ID resolution failed")); - const result = await getDeviceIdForConnection(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); expect(result).toBe("unknown"); - expect(MockLogger.debug).toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, - "deviceId", - "Failed to get device ID: Error: Device ID resolution failed" - ); }); it("should handle resolution error callback", async () => { - const mockMachineId = "machine-id"; - MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); MockGetDeviceId.mockImplementation((options) => { - // Simulate a resolution error if (options.onError) { options.onError("resolutionError", new Error("Resolution failed")); } return Promise.resolve("device-id"); }); - const result = await getDeviceIdForConnection(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); expect(result).toBe("device-id"); - expect(MockLogger.debug).toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, - "deviceId", - "Error: Resolution failed" - ); }); it("should handle timeout error callback", async () => { - const mockMachineId = "machine-id"; - MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); MockGetDeviceId.mockImplementation((options) => { - // Simulate a timeout error if (options.onError) { options.onError("timeout", new Error("Timeout")); } return Promise.resolve("device-id"); }); - const result = await getDeviceIdForConnection(); - - expect(result).toBe("device-id"); - expect(MockLogger.debug).toHaveBeenCalledWith( - LogId.telemetryDeviceIdTimeout, - "deviceId", - "Device ID retrieval timed out" - ); - }); - - it("should handle timeout with timer advancement", async () => { - vi.useFakeTimers(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); - const mockMachineId = "machine-id"; - MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); - MockGetDeviceId.mockImplementation((options) => { - vi.advanceTimersByTime(DEVICE_ID_TIMEOUT / 2); - if (options.onError) { - options.onError("timeout", new Error("Timeout")); - } - return Promise.resolve("device-id"); - }); - - const result = await getDeviceIdForConnection(); + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); expect(result).toBe("device-id"); - expect(MockLogger.debug).toHaveBeenCalledWith( - LogId.telemetryDeviceIdTimeout, - "deviceId", - "Device ID retrieval timed out" - ); - - vi.useRealTimers(); }); it("should handle abort error callback without logging", async () => { - const mockMachineId = "machine-id"; - MockNodeMachineId.machineId.mockResolvedValue(mockMachineId); MockGetDeviceId.mockImplementation((options) => { - // Simulate an abort error if (options.onError) { options.onError("abort", new Error("Aborted")); } return Promise.resolve("device-id"); }); - const result = await getDeviceIdForConnection(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); expect(result).toBe("device-id"); - // Should not log abort errors - expect(MockLogger.debug).not.toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, - "deviceId", - expect.stringContaining("Aborted") - ); }); it("should handle machine ID generation failure", async () => { - MockNodeMachineId.machineId.mockImplementation(() => { - throw new Error("Machine ID generation failed"); - }); - // Also mock getDeviceId to throw to ensure we get the fallback + // Mock getMachineId to throw an error + mockGetMachineId = vi.fn().mockRejectedValue(new Error("Machine ID generation failed")); + MockGetDeviceId.mockRejectedValue(new Error("Device ID failed")); - const result = await getDeviceIdForConnection(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); expect(result).toBe("unknown"); - expect(MockLogger.debug).toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, - "deviceId", - "Failed to get device ID: Error: Device ID failed" - ); }); it("should use AbortController signal", async () => { - MockNodeMachineId.machineId.mockResolvedValue("machine-id"); MockGetDeviceId.mockResolvedValue("device-id"); - await getDeviceIdForConnection(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + await deviceId.getDeviceId(); const callArgs = MockGetDeviceId.mock.calls[0]?.[0]; if (callArgs) { @@ -182,17 +223,15 @@ describe("Device ID Helper", () => { }); it("should handle non-Error exceptions", async () => { - MockNodeMachineId.machineId.mockResolvedValue("machine-id"); MockGetDeviceId.mockRejectedValue("String error"); - const result = await getDeviceIdForConnection(); + // Initialize after mocking + DeviceIdService.init(testLogger, mockGetMachineId); + + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); expect(result).toBe("unknown"); - expect(MockLogger.debug).toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, - "deviceId", - "Failed to get device ID: String error" - ); }); }); }); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 0b4d97df..4148f090 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -7,6 +7,7 @@ import { config } from "../../src/common/config.js"; import { afterEach, beforeEach, describe, it, vi, expect } from "vitest"; import { NullLogger } from "../../src/common/logger.js"; import type { MockedFunction } from "vitest"; +import { DeviceIdService } from "../../src/helpers/deviceId.js"; // Mock the ApiClient to avoid real API calls vi.mock("../../src/common/atlas/apiClient.js"); @@ -18,7 +19,11 @@ const MockEventCache = vi.mocked(EventCache); // Mock the deviceId utility vi.mock("../../src/helpers/deviceId.js", () => ({ - getDeviceIdForConnection: vi.fn().mockResolvedValue("test-device-id"), + DeviceIdService: { + init: vi.fn(), + getInstance: vi.fn(), + resetInstance: vi.fn(), + }, })); describe("Telemetry", () => { @@ -119,11 +124,27 @@ describe("Telemetry", () => { mockEventCache.appendEvents = vi.fn().mockResolvedValue(undefined); MockEventCache.getInstance = vi.fn().mockReturnValue(mockEventCache as unknown as EventCache); + // Setup mocked DeviceId + const mockDeviceId = { + getDeviceId: vi.fn().mockResolvedValue("test-device-id"), + abortCalculation: vi.fn().mockResolvedValue(undefined), + deviceId: undefined, + deviceIdPromise: undefined, + abortController: undefined, + logger: new NullLogger(), + getMachineId: vi.fn(), + startDeviceIdCalculation: vi.fn(), + calculateDeviceId: vi.fn(), + } as unknown as DeviceIdService; + DeviceIdService.getInstance = vi.fn().mockReturnValue(mockDeviceId); + DeviceIdService.init = vi.fn().mockReturnValue(mockDeviceId); + // Create a simplified session with our mocked API client session = { apiClient: mockApiClient as unknown as ApiClient, sessionId: "test-session-id", agentRunner: { name: "test-agent", version: "1.0.0" } as const, + mcpClient: { name: "test-agent", version: "1.0.0" }, close: vi.fn().mockResolvedValue(undefined), setAgentRunner: vi.fn().mockResolvedValue(undefined), logger: new NullLogger(), @@ -131,6 +152,7 @@ describe("Telemetry", () => { telemetry = Telemetry.create(session, config, { eventCache: mockEventCache as unknown as EventCache, + deviceId: mockDeviceId, }); config.telemetry = "enabled"; @@ -234,8 +256,9 @@ describe("Telemetry", () => { it("should handle device ID resolution failure gracefully", async () => { // Mock the deviceId utility to return "unknown" for this test - const { getDeviceIdForConnection } = await import("../../src/helpers/deviceId.js"); - vi.mocked(getDeviceIdForConnection).mockResolvedValueOnce("unknown"); + const { DeviceIdService } = await import("../../src/helpers/deviceId.js"); + const mockDeviceId = vi.mocked(DeviceIdService.getInstance()); + mockDeviceId.getDeviceId.mockResolvedValueOnce("unknown"); telemetry = Telemetry.create(session, config); @@ -251,8 +274,9 @@ describe("Telemetry", () => { it("should handle device ID timeout gracefully", async () => { // Mock the deviceId utility to return "unknown" for this test - const { getDeviceIdForConnection } = await import("../../src/helpers/deviceId.js"); - vi.mocked(getDeviceIdForConnection).mockResolvedValueOnce("unknown"); + const { DeviceIdService } = await import("../../src/helpers/deviceId.js"); + const mockDeviceId = vi.mocked(DeviceIdService.getInstance()); + mockDeviceId.getDeviceId.mockResolvedValueOnce("unknown"); telemetry = Telemetry.create(session, config); From bb97041d0557406458f5a5d8e6368c5fcfd892c1 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 17:26:20 +0100 Subject: [PATCH 12/33] keep getMachineId under deviceId --- src/helpers/deviceId.ts | 13 +++++--- src/index.ts | 3 +- tests/integration/helpers.ts | 19 +++++------ tests/integration/telemetry.test.ts | 4 +-- .../tools/mongodb/connect/connect.test.ts | 7 +--- tests/unit/helpers/deviceId.test.ts | 33 +++++++------------ 6 files changed, 33 insertions(+), 46 deletions(-) diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index d1d8801b..3396b058 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -1,4 +1,5 @@ import { getDeviceId } from "@mongodb-js/device-id"; +import nodeMachineId from "node-machine-id"; import { LogId, LoggerBase } from "../common/logger.js"; export const DEVICE_ID_TIMEOUT = 3000; @@ -15,9 +16,9 @@ export class DeviceIdService { private logger: LoggerBase; private getMachineId: () => Promise; - private constructor(logger: LoggerBase, getMachineId: () => Promise) { + private constructor(logger: LoggerBase) { this.logger = logger; - this.getMachineId = getMachineId; + this.getMachineId = (): Promise => nodeMachineId.machineId(true); // Start device ID calculation immediately this.startDeviceIdCalculation(); } @@ -28,11 +29,11 @@ export class DeviceIdService { * @param logger - The logger instance to use * @returns The DeviceIdService instance */ - public static init(logger: LoggerBase, getMachineId: () => Promise): DeviceIdService { + public static init(logger: LoggerBase): DeviceIdService { if (DeviceIdService.instance) { return DeviceIdService.instance; } - DeviceIdService.instance = new DeviceIdService(logger, getMachineId); + DeviceIdService.instance = new DeviceIdService(logger); return DeviceIdService.instance; } @@ -51,6 +52,10 @@ export class DeviceIdService { * Resets the singleton instance (mainly for testing). */ static resetInstance(): void { + // abort any ongoing calculation + if (DeviceIdService.instance?.abortController) { + DeviceIdService.instance.abortController.abort(); + } DeviceIdService.instance = undefined; } diff --git a/src/index.ts b/src/index.ts index 339b83eb..4be18433 100644 --- a/src/index.ts +++ b/src/index.ts @@ -43,7 +43,6 @@ import { StdioRunner } from "./transports/stdio.js"; import { StreamableHttpRunner } from "./transports/streamableHttp.js"; import { systemCA } from "@mongodb-js/devtools-proxy-support"; import { DeviceIdService } from "./helpers/deviceId.js"; -import nodeMachineId from "node-machine-id"; async function main(): Promise { systemCA().catch(() => undefined); // load system CA asynchronously as in mongosh @@ -52,7 +51,7 @@ async function main(): Promise { assertVersionMode(); const transportRunner = config.transport === "stdio" ? new StdioRunner(config) : new StreamableHttpRunner(config); - const deviceId = DeviceIdService.init(transportRunner.logger, () => nodeMachineId.machineId(true)); + const deviceId = DeviceIdService.init(transportRunner.logger); const shutdown = (): void => { transportRunner.logger.info({ diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index f80d2e72..fd13eccd 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -1,18 +1,17 @@ +import { CompositeLogger } from "../../src/common/logger.js"; +import { ExportsManager } from "../../src/common/exportsManager.js"; +import { Session } from "../../src/common/session.js"; +import { Server } from "../../src/server.js"; +import { Telemetry } from "../../src/telemetry/telemetry.js"; +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "./inMemoryTransport.js"; -import { Server } from "../../src/server.js"; import { UserConfig } from "../../src/common/config.js"; +import { DeviceIdService } from "../../src/helpers/deviceId.js"; import { McpError, ResourceUpdatedNotificationSchema } from "@modelcontextprotocol/sdk/types.js"; -import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { Session } from "../../src/common/session.js"; -import { Telemetry } from "../../src/telemetry/telemetry.js"; import { config } from "../../src/common/config.js"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { ConnectionManager } from "../../src/common/connectionManager.js"; -import { CompositeLogger } from "../../src/common/logger.js"; -import { ExportsManager } from "../../src/common/exportsManager.js"; -import { DeviceIdService } from "../../src/helpers/deviceId.js"; -import nodeMachineId from "node-machine-id"; interface ParameterInfo { name: string; @@ -41,6 +40,7 @@ export function setupIntegrationTest(getUserConfig: () => UserConfig): Integrati const userConfig = getUserConfig(); const clientTransport = new InMemoryTransport(); const serverTransport = new InMemoryTransport(); + const logger = new CompositeLogger(); await serverTransport.start(); await clientTransport.start(); @@ -58,11 +58,10 @@ export function setupIntegrationTest(getUserConfig: () => UserConfig): Integrati } ); - const logger = new CompositeLogger(); const exportsManager = ExportsManager.init(userConfig, logger); // Initialize DeviceIdService for tests - DeviceIdService.init(logger, () => nodeMachineId.machineId(true)); + DeviceIdService.init(logger); const connectionManager = new ConnectionManager(); diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index ad886e72..8cee767b 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -6,15 +6,13 @@ 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"; -import nodeMachineId from "node-machine-id"; describe("Telemetry", () => { it("should resolve the actual device ID", async () => { const logger = new CompositeLogger(); // Initialize DeviceIdService like the main application does - DeviceIdService.init(logger, () => nodeMachineId.machineId(true)); - + DeviceIdService.init(logger); const actualDeviceId = await DeviceIdService.getInstance().getDeviceId(); const telemetry = Telemetry.create( diff --git a/tests/integration/tools/mongodb/connect/connect.test.ts b/tests/integration/tools/mongodb/connect/connect.test.ts index 2ce8bf1d..7dd275d3 100644 --- a/tests/integration/tools/mongodb/connect/connect.test.ts +++ b/tests/integration/tools/mongodb/connect/connect.test.ts @@ -7,12 +7,7 @@ import { } from "../../../helpers.js"; import { config } from "../../../../../src/common/config.js"; import { defaultTestConfig, setupIntegrationTest } from "../../../helpers.js"; -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"), -})); +import { beforeEach, describe, expect, it } from "vitest"; describeWithMongoDB( "SwitchConnection tool", diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 5c284795..aa4acb59 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -7,7 +7,6 @@ import { CompositeLogger } from "../../../src/common/logger.js"; // Mock the dependencies vi.mock("@mongodb-js/device-id"); vi.mock("node-machine-id"); - const MockGetDeviceId = vi.mocked(getDeviceId); describe("Device ID Helper", () => { @@ -18,9 +17,6 @@ describe("Device ID Helper", () => { vi.clearAllMocks(); testLogger = new CompositeLogger(); - // Create a mock getMachineId function - mockGetMachineId = vi.fn().mockResolvedValue("test-machine-id"); - // Reset singleton between tests DeviceIdService.resetInstance(); }); @@ -33,7 +29,7 @@ describe("Device ID Helper", () => { describe("DeviceIdService Singleton", () => { it("should return the same instance for multiple getInstance calls", () => { // Initialize first - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const instance1 = DeviceIdService.getInstance(); const instance2 = DeviceIdService.getInstance(); @@ -50,17 +46,12 @@ describe("Device ID Helper", () => { MockGetDeviceId.mockResolvedValue(mockDeviceId); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); const result = await deviceId.getDeviceId(); expect(result).toBe(mockDeviceId); - expect(MockGetDeviceId).toHaveBeenCalledWith({ - getMachineId: mockGetMachineId, - onError: expect.any(Function), - abortSignal: expect.any(AbortSignal), - }); }); it("should cache device ID after first retrieval", async () => { @@ -68,7 +59,7 @@ describe("Device ID Helper", () => { MockGetDeviceId.mockResolvedValue(mockDeviceId); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); @@ -88,7 +79,7 @@ describe("Device ID Helper", () => { MockGetDeviceId.mockResolvedValue(mockDeviceId); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); @@ -115,7 +106,7 @@ describe("Device ID Helper", () => { }); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); @@ -133,7 +124,7 @@ describe("Device ID Helper", () => { MockGetDeviceId.mockRejectedValue(new Error("Device ID resolution failed")); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); const result = await deviceId.getDeviceId(); @@ -150,7 +141,7 @@ describe("Device ID Helper", () => { }); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); const result = await deviceId.getDeviceId(); @@ -167,7 +158,7 @@ describe("Device ID Helper", () => { }); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); const result = await deviceId.getDeviceId(); @@ -184,7 +175,7 @@ describe("Device ID Helper", () => { }); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); const result = await deviceId.getDeviceId(); @@ -199,7 +190,7 @@ describe("Device ID Helper", () => { MockGetDeviceId.mockRejectedValue(new Error("Device ID failed")); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); const result = await deviceId.getDeviceId(); @@ -211,7 +202,7 @@ describe("Device ID Helper", () => { MockGetDeviceId.mockResolvedValue("device-id"); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); await deviceId.getDeviceId(); @@ -226,7 +217,7 @@ describe("Device ID Helper", () => { MockGetDeviceId.mockRejectedValue("String error"); // Initialize after mocking - DeviceIdService.init(testLogger, mockGetMachineId); + DeviceIdService.init(testLogger); const deviceId = DeviceIdService.getInstance(); const result = await deviceId.getDeviceId(); From 5fb028463a2712995b83c85bb998253fd565a009 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 17:33:52 +0100 Subject: [PATCH 13/33] chore: lint and test fix --- tests/integration/tools/atlas/atlasHelpers.ts | 2 +- tests/integration/transports/stdio.test.ts | 3 +++ tests/integration/transports/streamableHttp.test.ts | 2 ++ tests/unit/helpers/deviceId.test.ts | 4 ---- tests/unit/resources/common/debug.test.ts | 2 ++ 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/integration/tools/atlas/atlasHelpers.ts b/tests/integration/tools/atlas/atlasHelpers.ts index 622a99dc..3e0fd57b 100644 --- a/tests/integration/tools/atlas/atlasHelpers.ts +++ b/tests/integration/tools/atlas/atlasHelpers.ts @@ -15,7 +15,7 @@ export function describeWithAtlas(name: string, fn: IntegrationTestFunction): Su })); describe(name, () => { - fn(integration); + fn(clea); }); }; diff --git a/tests/integration/transports/stdio.test.ts b/tests/integration/transports/stdio.test.ts index aaa61d63..f972ebc0 100644 --- a/tests/integration/transports/stdio.test.ts +++ b/tests/integration/transports/stdio.test.ts @@ -2,6 +2,8 @@ import { describe, expect, it, beforeAll, afterAll } from "vitest"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { describeWithMongoDB } from "../tools/mongodb/mongodbHelpers.js"; +import { DeviceIdService } from "../../../src/helpers/deviceId.js"; +import { CompositeLogger } from "../../../src/common/logger.js"; describeWithMongoDB("StdioRunner", (integration) => { describe("client connects successfully", () => { @@ -20,6 +22,7 @@ describeWithMongoDB("StdioRunner", (integration) => { name: "test", version: "0.0.0", }); + DeviceIdService.init(new CompositeLogger()); await client.connect(transport); }); diff --git a/tests/integration/transports/streamableHttp.test.ts b/tests/integration/transports/streamableHttp.test.ts index d5b6e0be..bbee0457 100644 --- a/tests/integration/transports/streamableHttp.test.ts +++ b/tests/integration/transports/streamableHttp.test.ts @@ -3,6 +3,7 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { describe, expect, it, beforeAll, afterAll } from "vitest"; import { config } from "../../../src/common/config.js"; +import { DeviceIdService } from "../../../src/helpers/deviceId.js"; describe("StreamableHttpRunner", () => { let runner: StreamableHttpRunner; @@ -15,6 +16,7 @@ describe("StreamableHttpRunner", () => { config.telemetry = "disabled"; config.loggers = ["stderr"]; runner = new StreamableHttpRunner(config); + DeviceIdService.init(runner.logger); await runner.start(); }); diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index aa4acb59..365905ec 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -1,4 +1,3 @@ -/* eslint-disable @typescript-eslint/no-unsafe-assignment */ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; import { DeviceIdService } from "../../../src/helpers/deviceId.js"; import { getDeviceId } from "@mongodb-js/device-id"; @@ -184,9 +183,6 @@ describe("Device ID Helper", () => { }); it("should handle machine ID generation failure", async () => { - // Mock getMachineId to throw an error - mockGetMachineId = vi.fn().mockRejectedValue(new Error("Machine ID generation failed")); - MockGetDeviceId.mockRejectedValue(new Error("Device ID failed")); // Initialize after mocking diff --git a/tests/unit/resources/common/debug.test.ts b/tests/unit/resources/common/debug.test.ts index 8e798827..e1a4953d 100644 --- a/tests/unit/resources/common/debug.test.ts +++ b/tests/unit/resources/common/debug.test.ts @@ -6,6 +6,7 @@ import { config } from "../../../../src/common/config.js"; import { CompositeLogger } from "../../../../src/common/logger.js"; import { ConnectionManager } from "../../../../src/common/connectionManager.js"; import { ExportsManager } from "../../../../src/common/exportsManager.js"; +import { DeviceIdService } from "../../../../src/helpers/deviceId.js"; describe("debug resource", () => { const logger = new CompositeLogger(); @@ -21,6 +22,7 @@ describe("debug resource", () => { beforeEach(() => { debugResource = new DebugResource(session, config, telemetry); + DeviceIdService.init(logger); }); it("should be connected when a connected event happens", () => { From a6908508631425fa8650edbdab38bf6a34975866 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 17:47:43 +0100 Subject: [PATCH 14/33] fix typo --- tests/integration/tools/atlas/atlasHelpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/tools/atlas/atlasHelpers.ts b/tests/integration/tools/atlas/atlasHelpers.ts index 3e0fd57b..622a99dc 100644 --- a/tests/integration/tools/atlas/atlasHelpers.ts +++ b/tests/integration/tools/atlas/atlasHelpers.ts @@ -15,7 +15,7 @@ export function describeWithAtlas(name: string, fn: IntegrationTestFunction): Su })); describe(name, () => { - fn(clea); + fn(integration); }); }; From a2e1091ea411d4f15f100564b3996520a9c3c969 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 17:53:35 +0100 Subject: [PATCH 15/33] fix test --- tests/unit/helpers/deviceId.test.ts | 1 - tests/unit/resources/common/debug.test.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 365905ec..15437d7e 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -10,7 +10,6 @@ const MockGetDeviceId = vi.mocked(getDeviceId); describe("Device ID Helper", () => { let testLogger: CompositeLogger; - let mockGetMachineId: () => Promise; beforeEach(() => { vi.clearAllMocks(); diff --git a/tests/unit/resources/common/debug.test.ts b/tests/unit/resources/common/debug.test.ts index e1a4953d..11f365f8 100644 --- a/tests/unit/resources/common/debug.test.ts +++ b/tests/unit/resources/common/debug.test.ts @@ -10,6 +10,7 @@ import { DeviceIdService } from "../../../../src/helpers/deviceId.js"; describe("debug resource", () => { const logger = new CompositeLogger(); + DeviceIdService.init(logger); const session = new Session({ apiBaseUrl: "", logger, @@ -22,7 +23,6 @@ describe("debug resource", () => { beforeEach(() => { debugResource = new DebugResource(session, config, telemetry); - DeviceIdService.init(logger); }); it("should be connected when a connected event happens", () => { From f964e12db4261bc81b52cbb29d759f43868eeca8 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 18:04:30 +0100 Subject: [PATCH 16/33] fix: update appName and set it to unknown if not available --- src/helpers/connectionOptions.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/helpers/connectionOptions.ts b/src/helpers/connectionOptions.ts index 4c2bba2b..b7f0de46 100644 --- a/src/helpers/connectionOptions.ts +++ b/src/helpers/connectionOptions.ts @@ -29,12 +29,12 @@ export async function setAppNameParamIfMissing({ return connectionStringUrl.toString(); } + const appName = components.appName || "unknown"; const deviceId = components.deviceId ? await components.deviceId : "unknown"; - const clientName = components.clientName || "unknown"; // Build the extended appName format: appName--deviceId--clientName - const extendedAppName = `${components.appName}--${deviceId}--${clientName}`; + const extendedAppName = `${appName}--${deviceId}--${clientName}`; searchParams.set("appName", extendedAppName); From 6e922e6c0c6f30d4965578cb240fa15269812179 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 18:33:30 +0100 Subject: [PATCH 17/33] fix: fix test --- tests/unit/helpers/connectionOptions.test.ts | 5 ++--- tests/unit/helpers/deviceId.test.ts | 18 +----------------- 2 files changed, 3 insertions(+), 20 deletions(-) diff --git a/tests/unit/helpers/connectionOptions.test.ts b/tests/unit/helpers/connectionOptions.test.ts index f3c2a971..be771563 100644 --- a/tests/unit/helpers/connectionOptions.test.ts +++ b/tests/unit/helpers/connectionOptions.test.ts @@ -69,18 +69,17 @@ describe("Connection Options", () => { expect(result).toContain("appName=TestApp--test-device-id--unknown"); }); - it("should use deviceId utility when deviceId is not provided", async () => { + it("should use deviceId as unknown when deviceId is not provided", async () => { const connectionString = "mongodb://localhost:27017"; const result = await setAppNameParamIfMissing({ connectionString, components: { appName: "TestApp", clientName: "TestClient", - deviceId: DeviceIdService.getInstance().getDeviceId(), }, }); - expect(result).toContain("appName=TestApp--test-device-id--TestClient"); + expect(result).toContain("appName=TestApp--unknown--TestClient"); }); it("should preserve other query parameters", async () => { diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 15437d7e..3f996fa7 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -72,23 +72,6 @@ describe("Device ID Helper", () => { expect(MockGetDeviceId).toHaveBeenCalledTimes(1); // Still only called once }); - it("should return cached device ID without triggering calculation", async () => { - const mockDeviceId = "test-device-id-123"; - MockGetDeviceId.mockResolvedValue(mockDeviceId); - - // Initialize after mocking - DeviceIdService.init(testLogger); - - const deviceId = DeviceIdService.getInstance(); - - // First call to populate cache - await deviceId.getDeviceId(); - - // Get cached value without triggering calculation - const cachedValue = await deviceId.getDeviceId(); - expect(cachedValue).toBe(mockDeviceId); - }); - it("should allow aborting calculation", async () => { MockGetDeviceId.mockImplementation((options) => { // Simulate a long-running operation that can be aborted @@ -145,6 +128,7 @@ describe("Device ID Helper", () => { const result = await deviceId.getDeviceId(); expect(result).toBe("device-id"); + expect(MockGetDeviceId).toHaveBeenCalledTimes(1); }); it("should handle timeout error callback", async () => { From 4ba3a16de6ea964ec519f123de023c6e037ba3b4 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 18:53:32 +0100 Subject: [PATCH 18/33] decouple error handling into it's own method --- src/helpers/deviceId.ts | 47 ++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 3396b058..9a6e7235 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -113,25 +113,7 @@ export class DeviceIdService { const deviceId = await getDeviceId({ getMachineId: this.getMachineId, onError: (reason, error) => { - switch (reason) { - case "resolutionError": - this.logger.debug({ - id: LogId.telemetryDeviceIdFailure, - context: "deviceId", - message: `Device ID resolution error: ${String(error)}`, - }); - break; - case "timeout": - this.logger.debug({ - id: LogId.telemetryDeviceIdTimeout, - context: "deviceId", - message: "Device ID retrieval timed out", - }); - break; - case "abort": - // No need to log in the case of aborts - break; - } + this.handleDeviceIdError(reason, error); }, abortSignal: this.abortController.signal, }); @@ -158,4 +140,31 @@ export class DeviceIdService { this.abortController = undefined; } } + + /** + * Handles device ID error. + * @param reason - The reason for the error + * @param error - The error object + */ + private handleDeviceIdError(reason: string, error: Error): void { + switch (reason) { + case "resolutionError": + this.logger.debug({ + id: LogId.telemetryDeviceIdFailure, + context: "deviceId", + message: `Device ID resolution error: ${String(error)}`, + }); + break; + case "timeout": + this.logger.debug({ + id: LogId.telemetryDeviceIdTimeout, + context: "deviceId", + message: "Device ID retrieval timed out", + }); + break; + case "abort": + // No need to log in the case of aborts + break; + } + } } From 78d430a8ef00f865a45bdc4a210a113af0dd84e8 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Mon, 18 Aug 2025 23:33:20 +0100 Subject: [PATCH 19/33] more linting --- src/common/logger.ts | 4 +- src/helpers/deviceId.ts | 58 +++---- src/index.ts | 4 +- tests/unit/helpers/deviceId.test.ts | 252 +++++++++++----------------- 4 files changed, 135 insertions(+), 183 deletions(-) diff --git a/src/common/logger.ts b/src/common/logger.ts index f377e80a..d836924b 100644 --- a/src/common/logger.ts +++ b/src/common/logger.ts @@ -31,8 +31,8 @@ export const LogId = { telemetryEmitStart: mongoLogId(1_002_003), telemetryEmitSuccess: mongoLogId(1_002_004), telemetryMetadataError: mongoLogId(1_002_005), - telemetryDeviceIdFailure: mongoLogId(1_002_006), - telemetryDeviceIdTimeout: mongoLogId(1_002_007), + deviceIdResolutionError: mongoLogId(1_002_006), + deviceIdTimeout: mongoLogId(1_002_007), toolExecute: mongoLogId(1_003_001), toolExecuteFailure: mongoLogId(1_003_002), diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 9a6e7235..8e6c4077 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -14,10 +14,12 @@ export class DeviceIdService { private deviceIdPromise: Promise | undefined = undefined; private abortController: AbortController | undefined = undefined; private logger: LoggerBase; - private getMachineId: () => Promise; + private readonly getMachineId: () => Promise; + private timeout: number; - private constructor(logger: LoggerBase) { + private constructor(logger: LoggerBase, timeout: number) { this.logger = logger; + this.timeout = timeout; this.getMachineId = (): Promise => nodeMachineId.machineId(true); // Start device ID calculation immediately this.startDeviceIdCalculation(); @@ -29,36 +31,33 @@ export class DeviceIdService { * @param logger - The logger instance to use * @returns The DeviceIdService instance */ - public static init(logger: LoggerBase): DeviceIdService { + public static init(logger: LoggerBase, timeout?: number): DeviceIdService { if (DeviceIdService.instance) { return DeviceIdService.instance; } - DeviceIdService.instance = new DeviceIdService(logger); + DeviceIdService.instance = new DeviceIdService(logger, timeout ?? DEVICE_ID_TIMEOUT); return DeviceIdService.instance; } + /** + * Checks if the DeviceIdService is initialized. + * @returns True if the DeviceIdService is initialized, false otherwise + */ + public static isInitialized(): boolean { + return DeviceIdService.instance !== undefined; + } + /** * Gets the singleton instance of DeviceIdService. * @returns The DeviceIdService instance */ public static getInstance(): DeviceIdService { if (!DeviceIdService.instance) { - throw Error("DeviceIdService not initialized"); + throw new Error("DeviceIdService not initialized"); } return DeviceIdService.instance; } - /** - * Resets the singleton instance (mainly for testing). - */ - static resetInstance(): void { - // abort any ongoing calculation - if (DeviceIdService.instance?.abortController) { - DeviceIdService.instance.abortController.abort(); - } - DeviceIdService.instance = undefined; - } - /** * Starts the device ID calculation process. * This method is called automatically in the constructor. @@ -77,28 +76,27 @@ export class DeviceIdService { * @returns Promise that resolves to the device ID string */ public async getDeviceId(): Promise { - // Return cached value if available if (this.deviceId !== undefined) { return this.deviceId; } - // If calculation is already in progress, wait for it - if (this.deviceIdPromise) { - return this.deviceIdPromise; + if (!this.deviceIdPromise) { + throw new Error("DeviceIdService calculation not started"); } - // If somehow we don't have a promise, raise an error - throw new Error("Failed to get device ID"); + return this.deviceIdPromise; } /** * Aborts any ongoing device ID calculation. */ - abortCalculation(): void { + public close(): void { if (this.abortController) { this.abortController.abort(); this.abortController = undefined; } + this.deviceId = undefined; this.deviceIdPromise = undefined; + DeviceIdService.instance = undefined; } /** @@ -113,8 +111,9 @@ export class DeviceIdService { const deviceId = await getDeviceId({ getMachineId: this.getMachineId, onError: (reason, error) => { - this.handleDeviceIdError(reason, error); + this.handleDeviceIdError(reason, String(error)); }, + timeout: this.timeout, abortSignal: this.abortController.signal, }); @@ -128,7 +127,7 @@ export class DeviceIdService { } this.logger.debug({ - id: LogId.telemetryDeviceIdFailure, + id: LogId.deviceIdResolutionError, context: "deviceId", message: `Failed to get device ID: ${String(error)}`, }); @@ -146,20 +145,21 @@ export class DeviceIdService { * @param reason - The reason for the error * @param error - The error object */ - private handleDeviceIdError(reason: string, error: Error): void { + private handleDeviceIdError(reason: string, error: string): void { switch (reason) { case "resolutionError": this.logger.debug({ - id: LogId.telemetryDeviceIdFailure, + id: LogId.deviceIdResolutionError, context: "deviceId", - message: `Device ID resolution error: ${String(error)}`, + message: `Resolution error: ${String(error)}`, }); break; case "timeout": this.logger.debug({ - id: LogId.telemetryDeviceIdTimeout, + id: LogId.deviceIdTimeout, context: "deviceId", message: "Device ID retrieval timed out", + noRedaction: true, }); break; case "abort": diff --git a/src/index.ts b/src/index.ts index 4be18433..02ae3f1f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -63,7 +63,7 @@ async function main(): Promise { transportRunner .close() .then(() => { - deviceId.abortCalculation(); + deviceId.close(); transportRunner.logger.info({ id: LogId.serverClosed, context: "server", @@ -72,7 +72,7 @@ async function main(): Promise { process.exit(0); }) .catch((error: unknown) => { - deviceId.abortCalculation(); + deviceId.close(); transportRunner.logger.error({ id: LogId.serverCloseFailure, context: "server", diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 3f996fa7..a07459bb 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -8,200 +8,152 @@ vi.mock("@mongodb-js/device-id"); vi.mock("node-machine-id"); const MockGetDeviceId = vi.mocked(getDeviceId); -describe("Device ID Helper", () => { +describe("deviceId", () => { let testLogger: CompositeLogger; beforeEach(() => { vi.clearAllMocks(); testLogger = new CompositeLogger(); - - // Reset singleton between tests - DeviceIdService.resetInstance(); }); afterEach(() => { vi.restoreAllMocks(); - DeviceIdService.resetInstance(); + if (DeviceIdService.isInitialized()) { + DeviceIdService.getInstance().close(); + } }); - describe("DeviceIdService Singleton", () => { - it("should return the same instance for multiple getInstance calls", () => { - // Initialize first - DeviceIdService.init(testLogger); - - const instance1 = DeviceIdService.getInstance(); - const instance2 = DeviceIdService.getInstance(); - - expect(instance1).toBe(instance2); - }); - - it("should throw error when getInstance is called before init", () => { - expect(() => DeviceIdService.getInstance()).toThrow("DeviceIdService not initialized"); - }); + it("should return the same instance for multiple getInstance calls", () => { + // Initialize first + DeviceIdService.init(testLogger); - it("should successfully retrieve device ID", async () => { - const mockDeviceId = "test-device-id-123"; - MockGetDeviceId.mockResolvedValue(mockDeviceId); + const instance1 = DeviceIdService.getInstance(); + const instance2 = DeviceIdService.getInstance(); - // Initialize after mocking - DeviceIdService.init(testLogger); + expect(instance1).toBe(instance2); + }); - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + it("should correctly report initialization status", () => { + expect(DeviceIdService.isInitialized()).toBe(false); + + DeviceIdService.init(testLogger); + expect(DeviceIdService.isInitialized()).toBe(true); + + DeviceIdService.getInstance().close(); + expect(DeviceIdService.isInitialized()).toBe(false); + }); - expect(result).toBe(mockDeviceId); - }); + it("should throw error when getInstance is called before init", () => { + expect(() => DeviceIdService.getInstance()).toThrow("DeviceIdService not initialized"); + }); - it("should cache device ID after first retrieval", async () => { - const mockDeviceId = "test-device-id-123"; - MockGetDeviceId.mockResolvedValue(mockDeviceId); + it("should successfully retrieve device ID", async () => { + const mockDeviceId = "test-device-id-123"; + MockGetDeviceId.mockResolvedValue(mockDeviceId); - // Initialize after mocking - DeviceIdService.init(testLogger); + // Initialize after mocking + DeviceIdService.init(testLogger); - const deviceId = DeviceIdService.getInstance(); + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); - // First call should trigger calculation - const result1 = await deviceId.getDeviceId(); - expect(result1).toBe(mockDeviceId); - expect(MockGetDeviceId).toHaveBeenCalledTimes(1); + expect(result).toBe(mockDeviceId); + }); - // Second call should use cached value - const result2 = await deviceId.getDeviceId(); - expect(result2).toBe(mockDeviceId); - expect(MockGetDeviceId).toHaveBeenCalledTimes(1); // Still only called once - }); + it("should cache device ID after first retrieval", async () => { + const mockDeviceId = "test-device-id-123"; + MockGetDeviceId.mockResolvedValue(mockDeviceId); + + // Initialize after mocking + const deviceId = DeviceIdService.init(testLogger); + // First call should trigger calculation + const result1 = await deviceId.getDeviceId(); + expect(result1).toBe(mockDeviceId); + expect(MockGetDeviceId).toHaveBeenCalledTimes(1); + + // Second call should use cached value + const result2 = await deviceId.getDeviceId(); + expect(result2).toBe(mockDeviceId); + expect(MockGetDeviceId).toHaveBeenCalledTimes(1); // Still only called once + }); - it("should allow aborting calculation", async () => { - MockGetDeviceId.mockImplementation((options) => { - // Simulate a long-running operation that can be aborted - return new Promise((resolve, reject) => { - const timeout = setTimeout(() => resolve("device-id"), 1000); - options.abortSignal?.addEventListener("abort", () => { - clearTimeout(timeout); - const abortError = new Error("Aborted"); - abortError.name = "AbortError"; - reject(abortError); - }); + it("should allow aborting calculation", async () => { + MockGetDeviceId.mockImplementation((options) => { + // Simulate a long-running operation that can be aborted + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => resolve("device-id"), 1000); + options.abortSignal?.addEventListener("abort", () => { + clearTimeout(timeout); + const abortError = new Error("Aborted"); + abortError.name = "AbortError"; + reject(abortError); }); }); - - // Initialize after mocking - DeviceIdService.init(testLogger); - - const deviceId = DeviceIdService.getInstance(); - - // Start calculation - const promise = deviceId.getDeviceId(); - - // Abort immediately - deviceId.abortCalculation(); - - // Should reject with AbortError - await expect(promise).rejects.toThrow("Aborted"); }); - it("should return 'unknown' when getDeviceId throws an error", async () => { - MockGetDeviceId.mockRejectedValue(new Error("Device ID resolution failed")); + // Initialize after mocking + DeviceIdService.init(testLogger); - // Initialize after mocking - DeviceIdService.init(testLogger); + const deviceId = DeviceIdService.getInstance(); - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + // Start calculation + const promise = deviceId.getDeviceId(); - expect(result).toBe("unknown"); - }); + // Abort immediately + deviceId.close(); - it("should handle resolution error callback", async () => { - MockGetDeviceId.mockImplementation((options) => { - if (options.onError) { - options.onError("resolutionError", new Error("Resolution failed")); - } - return Promise.resolve("device-id"); - }); - - // Initialize after mocking - DeviceIdService.init(testLogger); - - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); - - expect(result).toBe("device-id"); - expect(MockGetDeviceId).toHaveBeenCalledTimes(1); - }); - - it("should handle timeout error callback", async () => { - MockGetDeviceId.mockImplementation((options) => { - if (options.onError) { - options.onError("timeout", new Error("Timeout")); - } - return Promise.resolve("device-id"); - }); - - // Initialize after mocking - DeviceIdService.init(testLogger); - - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); - - expect(result).toBe("device-id"); - }); + // Should reject with AbortError + await expect(promise).rejects.toThrow("Aborted"); + }); - it("should handle abort error callback without logging", async () => { - MockGetDeviceId.mockImplementation((options) => { - if (options.onError) { - options.onError("abort", new Error("Aborted")); - } - return Promise.resolve("device-id"); - }); + it("should return 'unknown' when getDeviceId throws an error", async () => { + MockGetDeviceId.mockRejectedValue(new Error("Device ID resolution failed")); - // Initialize after mocking - DeviceIdService.init(testLogger); + // Initialize after mocking + DeviceIdService.init(testLogger); - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); - expect(result).toBe("device-id"); - }); + expect(result).toBe("unknown"); + }); - it("should handle machine ID generation failure", async () => { - MockGetDeviceId.mockRejectedValue(new Error("Device ID failed")); + it("should handle machine ID generation failure", async () => { + MockGetDeviceId.mockRejectedValue(new Error("Device ID failed")); - // Initialize after mocking - DeviceIdService.init(testLogger); + // Initialize after mocking + DeviceIdService.init(testLogger); - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); - expect(result).toBe("unknown"); - }); + expect(result).toBe("unknown"); + }); - it("should use AbortController signal", async () => { - MockGetDeviceId.mockResolvedValue("device-id"); + it("should use AbortController signal", async () => { + MockGetDeviceId.mockResolvedValue("device-id"); - // Initialize after mocking - DeviceIdService.init(testLogger); + // Initialize after mocking + DeviceIdService.init(testLogger); - const deviceId = DeviceIdService.getInstance(); - await deviceId.getDeviceId(); + const deviceId = DeviceIdService.getInstance(); + await deviceId.getDeviceId(); - const callArgs = MockGetDeviceId.mock.calls[0]?.[0]; - if (callArgs) { - expect(callArgs.abortSignal).toBeInstanceOf(AbortSignal); - } - }); + const callArgs = MockGetDeviceId.mock.calls[0]?.[0]; + if (callArgs) { + expect(callArgs.abortSignal).toBeInstanceOf(AbortSignal); + } + }); - it("should handle non-Error exceptions", async () => { - MockGetDeviceId.mockRejectedValue("String error"); + it("should handle non-Error exceptions", async () => { + MockGetDeviceId.mockRejectedValue("String error"); - // Initialize after mocking - DeviceIdService.init(testLogger); + // Initialize after mocking + DeviceIdService.init(testLogger); - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + const deviceId = DeviceIdService.getInstance(); + const result = await deviceId.getDeviceId(); - expect(result).toBe("unknown"); - }); + expect(result).toBe("unknown"); }); }); From ae1d6d0df28bf6113167b5c69afe29ba0b76905c Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 19 Aug 2025 07:58:59 +0100 Subject: [PATCH 20/33] reformat --- src/tools/atlas/connect/connectCluster.ts | 190 +++++++++++++++------- tests/unit/helpers/deviceId.test.ts | 4 +- 2 files changed, 130 insertions(+), 64 deletions(-) diff --git a/src/tools/atlas/connect/connectCluster.ts b/src/tools/atlas/connect/connectCluster.ts index 1653c3f6..01a73048 100644 --- a/src/tools/atlas/connect/connectCluster.ts +++ b/src/tools/atlas/connect/connectCluster.ts @@ -8,7 +8,12 @@ import { inspectCluster } from "../../../common/atlas/cluster.js"; import { ensureCurrentIpInAccessList } from "../../../common/atlas/accessListUtils.js"; import { AtlasClusterConnectionInfo } from "../../../common/connectionManager.js"; -const EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours +// Connection configuration constants +const USER_EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours +const CONNECTION_RETRY_ATTEMPTS = 600; // 5 minutes (600 * 500ms = 5 minutes) +const CONNECTION_RETRY_DELAY_MS = 500; // 500ms between retries +const CONNECTION_CHECK_ATTEMPTS = 60; // 30 seconds (60 * 500ms = 30 seconds) +const CONNECTION_CHECK_DELAY_MS = 500; // 500ms between connection state checks function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); @@ -23,6 +28,41 @@ export class ConnectClusterTool extends AtlasToolBase { clusterName: z.string().describe("Atlas cluster name"), }; + private determineReadOnlyRole(): boolean { + if (this.config.readOnly) return true; + + const disabledTools = this.config.disabledTools || []; + const hasWriteAccess = !disabledTools.includes("create") && + !disabledTools.includes("update") && + !disabledTools.includes("delete"); + const hasReadAccess = !disabledTools.includes("read") && + !disabledTools.includes("metadata"); + + return !hasWriteAccess && hasReadAccess; + } + + private isConnectedToOtherCluster(projectId: string, clusterName: string): boolean { + return this.session.isConnectedToMongoDB && + (!this.session.connectedAtlasCluster || + this.session.connectedAtlasCluster.projectId !== projectId || + this.session.connectedAtlasCluster.clusterName !== clusterName); + } + + private getConnectionState(): "connected" | "connecting" | "disconnected" | "errored" { + const state = this.session.connectionManager.currentConnectionState; + switch (state.tag) { + case "connected": return "connected"; + case "connecting": return "connecting"; + case "disconnected": return "disconnected"; + case "errored": return "errored"; + } + } + + private getErrorReason(): string | undefined { + const state = this.session.connectionManager.currentConnectionState; + return state.tag === "errored" ? state.errorReason : undefined; + } + private queryConnection( projectId: string, clusterName: string @@ -34,52 +74,40 @@ export class ConnectClusterTool extends AtlasToolBase { return "disconnected"; } - const currentConectionState = this.session.connectionManager.currentConnectionState; - if ( - this.session.connectedAtlasCluster.projectId !== projectId || - this.session.connectedAtlasCluster.clusterName !== clusterName - ) { + if (this.isConnectedToOtherCluster(projectId, clusterName)) { return "connected-to-other-cluster"; } - switch (currentConectionState.tag) { + const connectionState = this.getConnectionState(); + switch (connectionState) { case "connecting": case "disconnected": // we might still be calling Atlas APIs and not attempted yet to connect to MongoDB, but we are still "connecting" return "connecting"; case "connected": return "connected"; case "errored": + const errorReason = this.getErrorReason(); this.session.logger.debug({ id: LogId.atlasConnectFailure, context: "atlas-connect-cluster", - message: `error querying cluster: ${currentConectionState.errorReason}`, + message: `error querying cluster: ${errorReason || "unknown error"}`, }); return "unknown"; } } - private async prepareClusterConnection( + private async createDatabaseUser( projectId: string, - clusterName: string - ): Promise<{ connectionString: string; atlas: AtlasClusterConnectionInfo }> { - const cluster = await inspectCluster(this.session.apiClient, projectId, clusterName); - - if (!cluster.connectionString) { - throw new Error("Connection string not available"); - } - + clusterName: string, + readOnly: boolean + ): Promise<{ + username: string; + password: string; + expiryDate: Date; + }> { const username = `mcpUser${Math.floor(Math.random() * 100000)}`; const password = await generateSecurePassword(); - - const expiryDate = new Date(Date.now() + EXPIRY_MS); - - const readOnly = - this.config.readOnly || - (this.config.disabledTools?.includes("create") && - this.config.disabledTools?.includes("update") && - this.config.disabledTools?.includes("delete") && - !this.config.disabledTools?.includes("read") && - !this.config.disabledTools?.includes("metadata")); + const expiryDate = new Date(Date.now() + USER_EXPIRY_MS); const roleName = readOnly ? "readAnyDatabase" : "readWriteAnyDatabase"; @@ -109,6 +137,38 @@ export class ConnectClusterTool extends AtlasToolBase { }, }); + return { username, password, expiryDate }; + } + + private buildConnectionString( + clusterConnectionString: string, + username: string, + password: string + ): string { + const cn = new URL(clusterConnectionString); + cn.username = username; + cn.password = password; + cn.searchParams.set("authSource", "admin"); + return cn.toString(); + } + + private async prepareClusterConnection( + projectId: string, + clusterName: string + ): Promise<{ connectionString: string; atlas: AtlasClusterConnectionInfo }> { + const cluster = await inspectCluster(this.session.apiClient, projectId, clusterName); + + if (!cluster.connectionString) { + throw new Error("Connection string not available"); + } + + const readOnly = this.determineReadOnlyRole(); + const { username, password, expiryDate } = await this.createDatabaseUser( + projectId, + clusterName, + readOnly + ); + const connectedAtlasCluster = { username, projectId, @@ -116,12 +176,13 @@ export class ConnectClusterTool extends AtlasToolBase { expiryDate, }; - const cn = new URL(cluster.connectionString); - cn.username = username; - cn.password = password; - cn.searchParams.set("authSource", "admin"); + const connectionString = this.buildConnectionString( + cluster.connectionString, + username, + password + ); - return { connectionString: cn.toString(), atlas: connectedAtlasCluster }; + return { connectionString, atlas: connectedAtlasCluster }; } private async connectToCluster(connectionString: string, atlas: AtlasClusterConnectionInfo): Promise { @@ -135,7 +196,7 @@ export class ConnectClusterTool extends AtlasToolBase { }); // try to connect for about 5 minutes - for (let i = 0; i < 600; i++) { + for (let i = 0; i < CONNECTION_RETRY_ATTEMPTS; i++) { try { lastError = undefined; @@ -152,7 +213,7 @@ export class ConnectClusterTool extends AtlasToolBase { message: `error connecting to cluster: ${error.message}`, }); - await sleep(500); // wait for 500ms before retrying + await sleep(CONNECTION_RETRY_DELAY_MS); // wait for 500ms before retrying } if ( @@ -165,30 +226,7 @@ export class ConnectClusterTool extends AtlasToolBase { } if (lastError) { - if ( - this.session.connectedAtlasCluster?.projectId === atlas.projectId && - this.session.connectedAtlasCluster?.clusterName === atlas.clusterName && - this.session.connectedAtlasCluster?.username - ) { - void this.session.apiClient - .deleteDatabaseUser({ - params: { - path: { - groupId: this.session.connectedAtlasCluster.projectId, - username: this.session.connectedAtlasCluster.username, - databaseName: "admin", - }, - }, - }) - .catch((err: unknown) => { - const error = err instanceof Error ? err : new Error(String(err)); - this.session.logger.debug({ - id: LogId.atlasConnectFailure, - context: "atlas-connect-cluster", - message: `error deleting database user: ${error.message}`, - }); - }); - } + await this.cleanupDatabaseUserOnFailure(atlas); throw lastError; } @@ -200,9 +238,35 @@ export class ConnectClusterTool extends AtlasToolBase { }); } + private async cleanupDatabaseUserOnFailure(atlas: AtlasClusterConnectionInfo): Promise { + const currentCluster = this.session.connectedAtlasCluster; + if (currentCluster?.projectId === atlas.projectId && + currentCluster?.clusterName === atlas.clusterName && + currentCluster?.username) { + try { + await this.session.apiClient.deleteDatabaseUser({ + params: { + path: { + groupId: currentCluster.projectId, + username: currentCluster.username, + databaseName: "admin", + }, + }, + }); + } catch (err: unknown) { + const error = err instanceof Error ? err : new Error(String(err)); + this.session.logger.debug({ + id: LogId.atlasConnectFailure, + context: "atlas-connect-cluster", + message: `error deleting database user: ${error.message}`, + }); + } + } + } + protected async execute({ projectId, clusterName }: ToolArgs): Promise { await ensureCurrentIpInAccessList(this.session.apiClient, projectId); - for (let i = 0; i < 60; i++) { + for (let i = 0; i < CONNECTION_CHECK_ATTEMPTS; i++) { const state = this.queryConnection(projectId, clusterName); switch (state) { case "connected": { @@ -226,19 +290,21 @@ export class ConnectClusterTool extends AtlasToolBase { const { connectionString, atlas } = await this.prepareClusterConnection(projectId, clusterName); // try to connect for about 5 minutes asynchronously - void this.connectToCluster(connectionString, atlas).catch((err: unknown) => { + try { + await this.connectToCluster(connectionString, atlas); + } catch (err: unknown) { const error = err instanceof Error ? err : new Error(String(err)); this.session.logger.error({ id: LogId.atlasConnectFailure, context: "atlas-connect-cluster", message: `error connecting to cluster: ${error.message}`, }); - }); + } break; } } - await sleep(500); + await sleep(CONNECTION_CHECK_DELAY_MS); } return { diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index a07459bb..1ebdd6fa 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -35,10 +35,10 @@ describe("deviceId", () => { it("should correctly report initialization status", () => { expect(DeviceIdService.isInitialized()).toBe(false); - + DeviceIdService.init(testLogger); expect(DeviceIdService.isInitialized()).toBe(true); - + DeviceIdService.getInstance().close(); expect(DeviceIdService.isInitialized()).toBe(false); }); From 68a462e739d7fd417dc65a097f9e187aeed0308f Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 19 Aug 2025 09:17:31 +0100 Subject: [PATCH 21/33] reformat and add integration test --- src/tools/atlas/connect/connectCluster.ts | 56 ++++----- tests/integration/common/deviceId.test.ts | 138 ++++++++++++++++++++++ 2 files changed, 163 insertions(+), 31 deletions(-) create mode 100644 tests/integration/common/deviceId.test.ts diff --git a/src/tools/atlas/connect/connectCluster.ts b/src/tools/atlas/connect/connectCluster.ts index 01a73048..1c14b85e 100644 --- a/src/tools/atlas/connect/connectCluster.ts +++ b/src/tools/atlas/connect/connectCluster.ts @@ -30,31 +30,35 @@ export class ConnectClusterTool extends AtlasToolBase { private determineReadOnlyRole(): boolean { if (this.config.readOnly) return true; - + const disabledTools = this.config.disabledTools || []; - const hasWriteAccess = !disabledTools.includes("create") && - !disabledTools.includes("update") && - !disabledTools.includes("delete"); - const hasReadAccess = !disabledTools.includes("read") && - !disabledTools.includes("metadata"); - + const hasWriteAccess = + !disabledTools.includes("create") && !disabledTools.includes("update") && !disabledTools.includes("delete"); + const hasReadAccess = !disabledTools.includes("read") && !disabledTools.includes("metadata"); + return !hasWriteAccess && hasReadAccess; } private isConnectedToOtherCluster(projectId: string, clusterName: string): boolean { - return this.session.isConnectedToMongoDB && - (!this.session.connectedAtlasCluster || + return ( + this.session.isConnectedToMongoDB && + (!this.session.connectedAtlasCluster || this.session.connectedAtlasCluster.projectId !== projectId || - this.session.connectedAtlasCluster.clusterName !== clusterName); + this.session.connectedAtlasCluster.clusterName !== clusterName) + ); } private getConnectionState(): "connected" | "connecting" | "disconnected" | "errored" { const state = this.session.connectionManager.currentConnectionState; switch (state.tag) { - case "connected": return "connected"; - case "connecting": return "connecting"; - case "disconnected": return "disconnected"; - case "errored": return "errored"; + case "connected": + return "connected"; + case "connecting": + return "connecting"; + case "disconnected": + return "disconnected"; + case "errored": + return "errored"; } } @@ -140,11 +144,7 @@ export class ConnectClusterTool extends AtlasToolBase { return { username, password, expiryDate }; } - private buildConnectionString( - clusterConnectionString: string, - username: string, - password: string - ): string { + private buildConnectionString(clusterConnectionString: string, username: string, password: string): string { const cn = new URL(clusterConnectionString); cn.username = username; cn.password = password; @@ -163,11 +163,7 @@ export class ConnectClusterTool extends AtlasToolBase { } const readOnly = this.determineReadOnlyRole(); - const { username, password, expiryDate } = await this.createDatabaseUser( - projectId, - clusterName, - readOnly - ); + const { username, password, expiryDate } = await this.createDatabaseUser(projectId, clusterName, readOnly); const connectedAtlasCluster = { username, @@ -176,11 +172,7 @@ export class ConnectClusterTool extends AtlasToolBase { expiryDate, }; - const connectionString = this.buildConnectionString( - cluster.connectionString, - username, - password - ); + const connectionString = this.buildConnectionString(cluster.connectionString, username, password); return { connectionString, atlas: connectedAtlasCluster }; } @@ -240,9 +232,11 @@ export class ConnectClusterTool extends AtlasToolBase { private async cleanupDatabaseUserOnFailure(atlas: AtlasClusterConnectionInfo): Promise { const currentCluster = this.session.connectedAtlasCluster; - if (currentCluster?.projectId === atlas.projectId && + if ( + currentCluster?.projectId === atlas.projectId && currentCluster?.clusterName === atlas.clusterName && - currentCluster?.username) { + currentCluster?.username + ) { try { await this.session.apiClient.deleteDatabaseUser({ params: { diff --git a/tests/integration/common/deviceId.test.ts b/tests/integration/common/deviceId.test.ts new file mode 100644 index 00000000..ec5b61b0 --- /dev/null +++ b/tests/integration/common/deviceId.test.ts @@ -0,0 +1,138 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; +import { DeviceIdService } from "../../../src/helpers/deviceId.js"; +import { CompositeLogger } from "../../../src/common/logger.js"; +import nodeMachineId from "node-machine-id"; + +describe("Device ID", () => { + let testLogger: CompositeLogger; + + beforeEach(() => { + testLogger = new CompositeLogger(); + testLogger.debug = vi.fn(); + }); + + afterEach(() => { + if (DeviceIdService.isInitialized()) { + DeviceIdService.getInstance().close(); + } + }); + + describe("when resolving device ID", () => { + it("should successfully resolve device ID in real environment", async () => { + const deviceId = DeviceIdService.init(testLogger); + const result = await deviceId.getDeviceId(); + + expect(result).not.toBe("unknown"); + expect(result).toBeTruthy(); + expect(typeof result).toBe("string"); + expect(result.length).toBeGreaterThan(0); + }); + + it("should cache device ID after first resolution", async () => { + // spy on machineId + const machineIdSpy = vi.spyOn(nodeMachineId, "machineId"); + const deviceId = DeviceIdService.init(testLogger); + + // First call + const result1 = await deviceId.getDeviceId(); + expect(result1).not.toBe("unknown"); + + // Second call should be cached + const result2 = await deviceId.getDeviceId(); + expect(result2).toBe(result1); + // check that machineId was called only once + expect(machineIdSpy).toHaveBeenCalledOnce(); + }); + + it("should handle concurrent device ID requests correctly", async () => { + const deviceId = DeviceIdService.init(testLogger); + + const promises = Array.from({ length: 5 }, () => deviceId.getDeviceId()); + + // All should resolve to the same value + const results = await Promise.all(promises); + const firstResult = results[0]; + expect(firstResult).not.toBe("unknown"); + + // All results should be identical + results.forEach((result) => { + expect(result).toBe(firstResult); + }); + }); + }); + + describe("when resolving device ID fails", () => { + const originalMachineId: typeof nodeMachineId.machineId = nodeMachineId.machineId; + + beforeEach(() => { + // mock the machineId function to throw an abort error + nodeMachineId.machineId = vi.fn(); + }); + + afterEach(() => { + // Restore original implementation + nodeMachineId.machineId = originalMachineId; + }); + + it("should handle resolution errors gracefully", async () => { + // mock the machineId function to throw a resolution error + nodeMachineId.machineId = vi.fn().mockImplementation(() => { + return new Promise((resolve, reject) => { + reject(new Error("Machine ID failed")); + }); + }); + const deviceId = DeviceIdService.init(testLogger); + const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceIdService); + + const result = await deviceId.getDeviceId(); + + expect(result).toBe("unknown"); + expect(handleDeviceIdErrorSpy).toHaveBeenCalledWith( + "resolutionError", + expect.stringContaining("Machine ID failed") + ); + }); + + it("should handle abort signal scenarios gracefully", async () => { + // slow down the machineId function to give time to send abort signal + nodeMachineId.machineId = vi.fn().mockImplementation(() => { + return new Promise((resolve) => { + setTimeout(() => resolve("delayed-id"), 1000); + }); + }); + + const deviceId = DeviceIdService.init(testLogger); + const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceIdService); + + deviceId.close(); + + // expect the deviceId service to throw an error + await expect(deviceId.getDeviceId()).rejects.toThrow(Error); + // test that the private function handleDeviceIdError was called with reason "abort" + expect(handleDeviceIdErrorSpy).toHaveBeenCalledWith( + "abort", + expect.stringContaining("Aborted by abort signal") + ); + + // check that the deviceId service is not initialized anymore + expect(() => DeviceIdService.getInstance()).toThrow(Error); + }); + + it("should handle timeout scenarios gracefully", async () => { + nodeMachineId.machineId = vi.fn().mockImplementation(() => { + return new Promise((resolve) => { + setTimeout(() => resolve("delayed-id"), 200); + }); + }); + + // override the timeout to 100ms + const deviceId = DeviceIdService.init(testLogger, 100); + const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceIdService); + + const result = await deviceId.getDeviceId(); + + expect(result).toBe("unknown"); + expect(handleDeviceIdErrorSpy).toHaveBeenCalledWith("timeout", expect.stringContaining("Timeout")); + }, 5000); + }); +}); From 720be15cddb47092a002225e0454797fe9c1055b Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 19 Aug 2025 09:24:02 +0100 Subject: [PATCH 22/33] Revert "reformat" This reverts commit ae1d6d0df28bf6113167b5c69afe29ba0b76905c. --- src/tools/atlas/connect/connectCluster.ts | 184 ++++++++-------------- 1 file changed, 62 insertions(+), 122 deletions(-) diff --git a/src/tools/atlas/connect/connectCluster.ts b/src/tools/atlas/connect/connectCluster.ts index 1c14b85e..1653c3f6 100644 --- a/src/tools/atlas/connect/connectCluster.ts +++ b/src/tools/atlas/connect/connectCluster.ts @@ -8,12 +8,7 @@ import { inspectCluster } from "../../../common/atlas/cluster.js"; import { ensureCurrentIpInAccessList } from "../../../common/atlas/accessListUtils.js"; import { AtlasClusterConnectionInfo } from "../../../common/connectionManager.js"; -// Connection configuration constants -const USER_EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours -const CONNECTION_RETRY_ATTEMPTS = 600; // 5 minutes (600 * 500ms = 5 minutes) -const CONNECTION_RETRY_DELAY_MS = 500; // 500ms between retries -const CONNECTION_CHECK_ATTEMPTS = 60; // 30 seconds (60 * 500ms = 30 seconds) -const CONNECTION_CHECK_DELAY_MS = 500; // 500ms between connection state checks +const EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); @@ -28,45 +23,6 @@ export class ConnectClusterTool extends AtlasToolBase { clusterName: z.string().describe("Atlas cluster name"), }; - private determineReadOnlyRole(): boolean { - if (this.config.readOnly) return true; - - const disabledTools = this.config.disabledTools || []; - const hasWriteAccess = - !disabledTools.includes("create") && !disabledTools.includes("update") && !disabledTools.includes("delete"); - const hasReadAccess = !disabledTools.includes("read") && !disabledTools.includes("metadata"); - - return !hasWriteAccess && hasReadAccess; - } - - private isConnectedToOtherCluster(projectId: string, clusterName: string): boolean { - return ( - this.session.isConnectedToMongoDB && - (!this.session.connectedAtlasCluster || - this.session.connectedAtlasCluster.projectId !== projectId || - this.session.connectedAtlasCluster.clusterName !== clusterName) - ); - } - - private getConnectionState(): "connected" | "connecting" | "disconnected" | "errored" { - const state = this.session.connectionManager.currentConnectionState; - switch (state.tag) { - case "connected": - return "connected"; - case "connecting": - return "connecting"; - case "disconnected": - return "disconnected"; - case "errored": - return "errored"; - } - } - - private getErrorReason(): string | undefined { - const state = this.session.connectionManager.currentConnectionState; - return state.tag === "errored" ? state.errorReason : undefined; - } - private queryConnection( projectId: string, clusterName: string @@ -78,40 +34,52 @@ export class ConnectClusterTool extends AtlasToolBase { return "disconnected"; } - if (this.isConnectedToOtherCluster(projectId, clusterName)) { + const currentConectionState = this.session.connectionManager.currentConnectionState; + if ( + this.session.connectedAtlasCluster.projectId !== projectId || + this.session.connectedAtlasCluster.clusterName !== clusterName + ) { return "connected-to-other-cluster"; } - const connectionState = this.getConnectionState(); - switch (connectionState) { + switch (currentConectionState.tag) { case "connecting": case "disconnected": // we might still be calling Atlas APIs and not attempted yet to connect to MongoDB, but we are still "connecting" return "connecting"; case "connected": return "connected"; case "errored": - const errorReason = this.getErrorReason(); this.session.logger.debug({ id: LogId.atlasConnectFailure, context: "atlas-connect-cluster", - message: `error querying cluster: ${errorReason || "unknown error"}`, + message: `error querying cluster: ${currentConectionState.errorReason}`, }); return "unknown"; } } - private async createDatabaseUser( + private async prepareClusterConnection( projectId: string, - clusterName: string, - readOnly: boolean - ): Promise<{ - username: string; - password: string; - expiryDate: Date; - }> { + clusterName: string + ): Promise<{ connectionString: string; atlas: AtlasClusterConnectionInfo }> { + const cluster = await inspectCluster(this.session.apiClient, projectId, clusterName); + + if (!cluster.connectionString) { + throw new Error("Connection string not available"); + } + const username = `mcpUser${Math.floor(Math.random() * 100000)}`; const password = await generateSecurePassword(); - const expiryDate = new Date(Date.now() + USER_EXPIRY_MS); + + const expiryDate = new Date(Date.now() + EXPIRY_MS); + + const readOnly = + this.config.readOnly || + (this.config.disabledTools?.includes("create") && + this.config.disabledTools?.includes("update") && + this.config.disabledTools?.includes("delete") && + !this.config.disabledTools?.includes("read") && + !this.config.disabledTools?.includes("metadata")); const roleName = readOnly ? "readAnyDatabase" : "readWriteAnyDatabase"; @@ -141,30 +109,6 @@ export class ConnectClusterTool extends AtlasToolBase { }, }); - return { username, password, expiryDate }; - } - - private buildConnectionString(clusterConnectionString: string, username: string, password: string): string { - const cn = new URL(clusterConnectionString); - cn.username = username; - cn.password = password; - cn.searchParams.set("authSource", "admin"); - return cn.toString(); - } - - private async prepareClusterConnection( - projectId: string, - clusterName: string - ): Promise<{ connectionString: string; atlas: AtlasClusterConnectionInfo }> { - const cluster = await inspectCluster(this.session.apiClient, projectId, clusterName); - - if (!cluster.connectionString) { - throw new Error("Connection string not available"); - } - - const readOnly = this.determineReadOnlyRole(); - const { username, password, expiryDate } = await this.createDatabaseUser(projectId, clusterName, readOnly); - const connectedAtlasCluster = { username, projectId, @@ -172,9 +116,12 @@ export class ConnectClusterTool extends AtlasToolBase { expiryDate, }; - const connectionString = this.buildConnectionString(cluster.connectionString, username, password); + const cn = new URL(cluster.connectionString); + cn.username = username; + cn.password = password; + cn.searchParams.set("authSource", "admin"); - return { connectionString, atlas: connectedAtlasCluster }; + return { connectionString: cn.toString(), atlas: connectedAtlasCluster }; } private async connectToCluster(connectionString: string, atlas: AtlasClusterConnectionInfo): Promise { @@ -188,7 +135,7 @@ export class ConnectClusterTool extends AtlasToolBase { }); // try to connect for about 5 minutes - for (let i = 0; i < CONNECTION_RETRY_ATTEMPTS; i++) { + for (let i = 0; i < 600; i++) { try { lastError = undefined; @@ -205,7 +152,7 @@ export class ConnectClusterTool extends AtlasToolBase { message: `error connecting to cluster: ${error.message}`, }); - await sleep(CONNECTION_RETRY_DELAY_MS); // wait for 500ms before retrying + await sleep(500); // wait for 500ms before retrying } if ( @@ -218,7 +165,30 @@ export class ConnectClusterTool extends AtlasToolBase { } if (lastError) { - await this.cleanupDatabaseUserOnFailure(atlas); + if ( + this.session.connectedAtlasCluster?.projectId === atlas.projectId && + this.session.connectedAtlasCluster?.clusterName === atlas.clusterName && + this.session.connectedAtlasCluster?.username + ) { + void this.session.apiClient + .deleteDatabaseUser({ + params: { + path: { + groupId: this.session.connectedAtlasCluster.projectId, + username: this.session.connectedAtlasCluster.username, + databaseName: "admin", + }, + }, + }) + .catch((err: unknown) => { + const error = err instanceof Error ? err : new Error(String(err)); + this.session.logger.debug({ + id: LogId.atlasConnectFailure, + context: "atlas-connect-cluster", + message: `error deleting database user: ${error.message}`, + }); + }); + } throw lastError; } @@ -230,37 +200,9 @@ export class ConnectClusterTool extends AtlasToolBase { }); } - private async cleanupDatabaseUserOnFailure(atlas: AtlasClusterConnectionInfo): Promise { - const currentCluster = this.session.connectedAtlasCluster; - if ( - currentCluster?.projectId === atlas.projectId && - currentCluster?.clusterName === atlas.clusterName && - currentCluster?.username - ) { - try { - await this.session.apiClient.deleteDatabaseUser({ - params: { - path: { - groupId: currentCluster.projectId, - username: currentCluster.username, - databaseName: "admin", - }, - }, - }); - } catch (err: unknown) { - const error = err instanceof Error ? err : new Error(String(err)); - this.session.logger.debug({ - id: LogId.atlasConnectFailure, - context: "atlas-connect-cluster", - message: `error deleting database user: ${error.message}`, - }); - } - } - } - protected async execute({ projectId, clusterName }: ToolArgs): Promise { await ensureCurrentIpInAccessList(this.session.apiClient, projectId); - for (let i = 0; i < CONNECTION_CHECK_ATTEMPTS; i++) { + for (let i = 0; i < 60; i++) { const state = this.queryConnection(projectId, clusterName); switch (state) { case "connected": { @@ -284,21 +226,19 @@ export class ConnectClusterTool extends AtlasToolBase { const { connectionString, atlas } = await this.prepareClusterConnection(projectId, clusterName); // try to connect for about 5 minutes asynchronously - try { - await this.connectToCluster(connectionString, atlas); - } catch (err: unknown) { + void this.connectToCluster(connectionString, atlas).catch((err: unknown) => { const error = err instanceof Error ? err : new Error(String(err)); this.session.logger.error({ id: LogId.atlasConnectFailure, context: "atlas-connect-cluster", message: `error connecting to cluster: ${error.message}`, }); - } + }); break; } } - await sleep(CONNECTION_CHECK_DELAY_MS); + await sleep(500); } return { From 565ca2b4728dbe4742a42f2363c1159ac04318ea Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 19 Aug 2025 12:21:28 +0100 Subject: [PATCH 23/33] decouple config validation from connection --- src/helpers/connectionOptions.ts | 15 ++++++++++++++ src/server.ts | 34 ++++++++++++++++++++++++-------- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/helpers/connectionOptions.ts b/src/helpers/connectionOptions.ts index b7f0de46..885533b6 100644 --- a/src/helpers/connectionOptions.ts +++ b/src/helpers/connectionOptions.ts @@ -40,3 +40,18 @@ export async function setAppNameParamIfMissing({ return connectionStringUrl.toString(); } + +/** + * Validates the connection string + * @param connectionString - The connection string to validate + * @param looseValidation - Whether to allow loose validation + * @returns void + * @throws Error if the connection string is invalid + */ +export async function validateConnectionString(connectionString: string, looseValidation: boolean): Promise { + try { + new ConnectionString(connectionString, { looseValidation }); + } catch (error) { + throw new Error(`Invalid connection string with error: ${error}`); + } +} diff --git a/src/server.ts b/src/server.ts index 2bb60593..23916ab8 100644 --- a/src/server.ts +++ b/src/server.ts @@ -9,6 +9,7 @@ import { Telemetry } from "./telemetry/telemetry.js"; import { UserConfig } from "./common/config.js"; import { type ServerEvent } from "./telemetry/types.js"; import { type ServerCommand } from "./telemetry/types.js"; +import ConnectionString from "mongodb-connection-string-url"; import { CallToolRequestSchema, CallToolResult, @@ -17,6 +18,7 @@ import { } from "@modelcontextprotocol/sdk/types.js"; import assert from "assert"; import { ToolBase } from "./tools/tool.js"; +import { validateConnectionString } from "./helpers/connectionOptions.js"; export interface ServerOptions { session: Session; @@ -106,6 +108,9 @@ export class Server { }); this.emitServerEvent("start", Date.now() - this.startTime); + + // Placed here to start the connection to the config connection string as soon as the server is initialized. + this.connectToConfigConnectionString(); }; this.mcpServer.server.onclose = (): void => { @@ -188,20 +193,17 @@ export class Server { } private async validateConfig(): Promise { + // Validate connection string if (this.userConfig.connectionString) { try { - await this.session.connectToMongoDB({ - connectionString: this.userConfig.connectionString, - }); + await validateConnectionString(this.userConfig.connectionString, false); } catch (error) { - console.error( - "Failed to connect to MongoDB instance using the connection string from the config: ", - error - ); - throw new Error("Failed to connect to MongoDB instance using the connection string from the config"); + console.error("Connection string validation failed with error: ", error); + throw new Error("Connection string validation failed with error: " + error); } } + // Validate API client credentials if (this.userConfig.apiClientId && this.userConfig.apiClientSecret) { try { await this.session.apiClient.validateAccessToken(); @@ -219,4 +221,20 @@ export class Server { } } } + + private async connectToConfigConnectionString(): Promise { + if (this.userConfig.connectionString) { + try { + await this.session.connectToMongoDB({ + connectionString: this.userConfig.connectionString, + }); + } catch (error) { + console.error( + "Failed to connect to MongoDB instance using the connection string from the config: ", + error + ); + throw new Error("Failed to connect to MongoDB instance using the connection string from the config"); + } + } + } } From 69609b4dab606c7244bc1012428ce45aed28105a Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 19 Aug 2025 12:35:53 +0100 Subject: [PATCH 24/33] lint --- src/helpers/connectionOptions.ts | 6 ++++-- src/server.ts | 13 +++++++------ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/helpers/connectionOptions.ts b/src/helpers/connectionOptions.ts index 885533b6..009996ee 100644 --- a/src/helpers/connectionOptions.ts +++ b/src/helpers/connectionOptions.ts @@ -48,10 +48,12 @@ export async function setAppNameParamIfMissing({ * @returns void * @throws Error if the connection string is invalid */ -export async function validateConnectionString(connectionString: string, looseValidation: boolean): Promise { +export function validateConnectionString(connectionString: string, looseValidation: boolean): void { try { new ConnectionString(connectionString, { looseValidation }); } catch (error) { - throw new Error(`Invalid connection string with error: ${error}`); + throw new Error( + `Invalid connection string with error: ${error instanceof Error ? error.message : String(error)}` + ); } } diff --git a/src/server.ts b/src/server.ts index 23916ab8..5121a858 100644 --- a/src/server.ts +++ b/src/server.ts @@ -9,7 +9,6 @@ import { Telemetry } from "./telemetry/telemetry.js"; import { UserConfig } from "./common/config.js"; import { type ServerEvent } from "./telemetry/types.js"; import { type ServerCommand } from "./telemetry/types.js"; -import ConnectionString from "mongodb-connection-string-url"; import { CallToolRequestSchema, CallToolResult, @@ -100,6 +99,8 @@ export class Server { this.mcpServer.server.oninitialized = (): void => { this.session.setMcpClient(this.mcpServer.server.getClientVersion()); + // Placed here to start the connection to the config connection string as soon as the server is initialized. + void this.connectToConfigConnectionString(); this.session.logger.info({ id: LogId.serverInitialized, @@ -108,9 +109,6 @@ export class Server { }); this.emitServerEvent("start", Date.now() - this.startTime); - - // Placed here to start the connection to the config connection string as soon as the server is initialized. - this.connectToConfigConnectionString(); }; this.mcpServer.server.onclose = (): void => { @@ -196,10 +194,13 @@ export class Server { // Validate connection string if (this.userConfig.connectionString) { try { - await validateConnectionString(this.userConfig.connectionString, false); + validateConnectionString(this.userConfig.connectionString, false); } catch (error) { console.error("Connection string validation failed with error: ", error); - throw new Error("Connection string validation failed with error: " + error); + throw new Error( + "Connection string validation failed with error: " + + (error instanceof Error ? error.message : String(error)) + ); } } From 658cdc8b6ad4a58c3660cdd6343c084165e1e512 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Wed, 20 Aug 2025 21:33:39 +0100 Subject: [PATCH 25/33] new device id --- src/helpers/deviceId.ts | 83 +++++++++++------------------------------ 1 file changed, 22 insertions(+), 61 deletions(-) diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 8e6c4077..fb677e37 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -4,10 +4,6 @@ import { LogId, LoggerBase } from "../common/logger.js"; export const DEVICE_ID_TIMEOUT = 3000; -/** - * Singleton class for managing device ID retrieval and caching. - * Starts device ID calculation early and is shared across all services. - */ export class DeviceIdService { private static instance: DeviceIdService | undefined = undefined; private deviceId: string | undefined = undefined; @@ -21,55 +17,37 @@ export class DeviceIdService { this.logger = logger; this.timeout = timeout; this.getMachineId = (): Promise => nodeMachineId.machineId(true); - // Start device ID calculation immediately - this.startDeviceIdCalculation(); } - /** - * Initializes the DeviceIdService singleton with a logger. - * A separated init method is used to use a single instance of the logger. - * @param logger - The logger instance to use - * @returns The DeviceIdService instance - */ - public static init(logger: LoggerBase, timeout?: number): DeviceIdService { - if (DeviceIdService.instance) { - return DeviceIdService.instance; - } - DeviceIdService.instance = new DeviceIdService(logger, timeout ?? DEVICE_ID_TIMEOUT); - return DeviceIdService.instance; - } + static create( + logger: LoggerBase, + { timeout = DEVICE_ID_TIMEOUT }: { timeout?: number } = {}, + ): DeviceIdService { + const instance = new DeviceIdService(logger, timeout); - /** - * Checks if the DeviceIdService is initialized. - * @returns True if the DeviceIdService is initialized, false otherwise - */ - public static isInitialized(): boolean { - return DeviceIdService.instance !== undefined; + void instance.setup(); + return instance; } - /** - * Gets the singleton instance of DeviceIdService. - * @returns The DeviceIdService instance - */ - public static getInstance(): DeviceIdService { - if (!DeviceIdService.instance) { - throw new Error("DeviceIdService not initialized"); - } - return DeviceIdService.instance; + private async setup(): Promise { + this.abortController = new AbortController(); + + this.deviceIdPromise = this.calculateDeviceId(); + const deviceId = await this.deviceIdPromise; + this.deviceId = deviceId; } - /** - * Starts the device ID calculation process. - * This method is called automatically in the constructor. - */ - private startDeviceIdCalculation(): void { - if (this.deviceIdPromise) { - return; + + public close(): void { + if (this.abortController) { + this.abortController.abort(); + this.abortController = undefined; } - - this.abortController = new AbortController(); - this.deviceIdPromise = this.calculateDeviceId(); + this.deviceId = undefined; + this.deviceIdPromise = undefined; + DeviceIdService.instance = undefined; } + /** * Gets the device ID, waiting for the calculation to complete if necessary. @@ -86,18 +64,6 @@ export class DeviceIdService { return this.deviceIdPromise; } - /** - * Aborts any ongoing device ID calculation. - */ - public close(): void { - if (this.abortController) { - this.abortController.abort(); - this.abortController = undefined; - } - this.deviceId = undefined; - this.deviceIdPromise = undefined; - DeviceIdService.instance = undefined; - } /** * Internal method that performs the actual device ID calculation. @@ -140,11 +106,6 @@ export class DeviceIdService { } } - /** - * Handles device ID error. - * @param reason - The reason for the error - * @param error - The error object - */ private handleDeviceIdError(reason: string, error: string): void { switch (reason) { case "resolutionError": From dae4f0649d592ec37277c19334b113e1609e45d2 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Wed, 20 Aug 2025 23:35:53 +0100 Subject: [PATCH 26/33] simplify device id --- src/common/connectionManager.ts | 10 +- src/helpers/deviceId.ts | 42 ++--- src/index.ts | 5 +- src/telemetry/telemetry.ts | 12 +- tests/integration/common/deviceId.test.ts | 61 ++------ tests/integration/helpers.ts | 5 +- tests/integration/telemetry.test.ts | 10 +- tests/integration/transports/stdio.test.ts | 3 +- .../transports/streamableHttp.test.ts | 3 +- tests/unit/common/session.test.ts | 18 +-- tests/unit/helpers/connectionOptions.test.ts | 31 ++-- tests/unit/helpers/deviceId.test.ts | 148 +++++++++--------- tests/unit/resources/common/debug.test.ts | 3 +- tests/unit/telemetry.test.ts | 50 ++---- 14 files changed, 179 insertions(+), 222 deletions(-) diff --git a/src/common/connectionManager.ts b/src/common/connectionManager.ts index da7ab246..a0e2a13e 100644 --- a/src/common/connectionManager.ts +++ b/src/common/connectionManager.ts @@ -6,7 +6,7 @@ import { packageInfo } from "./packageInfo.js"; import ConnectionString from "mongodb-connection-string-url"; import { MongoClientOptions } from "mongodb"; import { ErrorCodes, MongoDBError } from "./errors.js"; -import { DeviceIdService } from "../helpers/deviceId.js"; +import { DeviceId } from "../helpers/deviceId.js"; import { AppNameComponents } from "../helpers/connectionOptions.js"; import { CompositeLogger, LogId } from "./logger.js"; import { ConnectionInfo, generateConnectionInfoFromCliArgs } from "@mongosh/arg-parser"; @@ -71,7 +71,7 @@ export interface ConnectionManagerEvents { export class ConnectionManager extends EventEmitter { private state: AnyConnectionState; - private deviceId: DeviceIdService; + private deviceId: DeviceId; private clientName: string; private bus: EventEmitter; @@ -88,8 +88,8 @@ export class ConnectionManager extends EventEmitter { this.bus.on("mongodb-oidc-plugin:auth-failed", this.onOidcAuthFailed.bind(this)); this.bus.on("mongodb-oidc-plugin:auth-succeeded", this.onOidcAuthSucceeded.bind(this)); - - this.deviceId = DeviceIdService.getInstance(); + + this.deviceId = DeviceId.create(this.logger); this.clientName = "unknown"; } @@ -111,7 +111,7 @@ export class ConnectionManager extends EventEmitter { settings = { ...settings }; const appNameComponents: AppNameComponents = { appName: `${packageInfo.mcpServerName} ${packageInfo.version}`, - deviceId: this.deviceId.getDeviceId(), + deviceId: this.deviceId.get(), clientName: this.clientName, }; diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index fb677e37..0e1c1d9c 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -4,62 +4,68 @@ import { LogId, LoggerBase } from "../common/logger.js"; export const DEVICE_ID_TIMEOUT = 3000; -export class DeviceIdService { - private static instance: DeviceIdService | undefined = undefined; +export class DeviceId { private deviceId: string | undefined = undefined; private deviceIdPromise: Promise | undefined = undefined; private abortController: AbortController | undefined = undefined; private logger: LoggerBase; private readonly getMachineId: () => Promise; private timeout: number; + private static instance: DeviceId | undefined = undefined; - private constructor(logger: LoggerBase, timeout: number) { + private constructor(logger: LoggerBase, timeout: number = DEVICE_ID_TIMEOUT) { this.logger = logger; this.timeout = timeout; this.getMachineId = (): Promise => nodeMachineId.machineId(true); } - static create( - logger: LoggerBase, - { timeout = DEVICE_ID_TIMEOUT }: { timeout?: number } = {}, - ): DeviceIdService { - const instance = new DeviceIdService(logger, timeout); + public static create(logger: LoggerBase, timeout?: number): DeviceId { + if (this.instance) { + return this.instance; + } + const instance = new DeviceId(logger, timeout ?? DEVICE_ID_TIMEOUT); void instance.setup(); + + this.instance = instance; + return instance; } + /** + * Sets up the device ID calculation promise and abort controller. + */ private async setup(): Promise { this.abortController = new AbortController(); - this.deviceIdPromise = this.calculateDeviceId(); - const deviceId = await this.deviceIdPromise; - this.deviceId = deviceId; + // start the promise upon setup + void this.deviceIdPromise; } - - public close(): void { + /** + * Closes the device ID calculation promise and abort controller. + */ + public close(): void { if (this.abortController) { this.abortController.abort(); this.abortController = undefined; } this.deviceId = undefined; this.deviceIdPromise = undefined; - DeviceIdService.instance = undefined; + DeviceId.instance = undefined; } - /** * Gets the device ID, waiting for the calculation to complete if necessary. * @returns Promise that resolves to the device ID string */ - public async getDeviceId(): Promise { + public async get(): Promise { if (this.deviceId !== undefined) { return this.deviceId; } if (!this.deviceIdPromise) { - throw new Error("DeviceIdService calculation not started"); + this.deviceIdPromise = this.calculateDeviceId(); } return this.deviceIdPromise; @@ -124,7 +130,7 @@ export class DeviceIdService { }); break; case "abort": - // No need to log in the case of aborts + // No need to log in the case of 'abort' errors break; } } diff --git a/src/index.ts b/src/index.ts index 02ae3f1f..d4947c8c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -42,7 +42,7 @@ import { packageInfo } from "./common/packageInfo.js"; import { StdioRunner } from "./transports/stdio.js"; import { StreamableHttpRunner } from "./transports/streamableHttp.js"; import { systemCA } from "@mongodb-js/devtools-proxy-support"; -import { DeviceIdService } from "./helpers/deviceId.js"; +import { DeviceId } from "./helpers/deviceId.js"; async function main(): Promise { systemCA().catch(() => undefined); // load system CA asynchronously as in mongosh @@ -51,8 +51,7 @@ async function main(): Promise { assertVersionMode(); const transportRunner = config.transport === "stdio" ? new StdioRunner(config) : new StreamableHttpRunner(config); - const deviceId = DeviceIdService.init(transportRunner.logger); - + const deviceId = DeviceId.create(transportRunner.logger); const shutdown = (): void => { transportRunner.logger.info({ id: LogId.serverCloseRequested, diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 229fc7f5..3b311f14 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -6,7 +6,7 @@ import { ApiClient } from "../common/atlas/apiClient.js"; import { MACHINE_METADATA } from "./constants.js"; import { EventCache } from "./eventCache.js"; import { detectContainerEnv } from "../helpers/container.js"; -import { DeviceIdService } from "../helpers/deviceId.js"; +import { DeviceId } from "../helpers/deviceId.js"; type EventResult = { success: boolean; @@ -18,13 +18,13 @@ export class Telemetry { /** Resolves when the setup is complete or a timeout occurs */ public setupPromise: Promise<[string, boolean]> | undefined; private eventCache: EventCache; - private deviceId: DeviceIdService; + private deviceId: DeviceId; private constructor( private readonly session: Session, private readonly userConfig: UserConfig, private readonly commonProperties: CommonProperties, - { eventCache, deviceId }: { eventCache: EventCache; deviceId: DeviceIdService } + { eventCache, deviceId }: { eventCache: EventCache; deviceId: DeviceId } ) { this.eventCache = eventCache; this.deviceId = deviceId; @@ -36,10 +36,10 @@ export class Telemetry { { commonProperties = { ...MACHINE_METADATA }, eventCache = EventCache.getInstance(), - deviceId = DeviceIdService.getInstance(), + deviceId = DeviceId.create(session.logger), }: { eventCache?: EventCache; - deviceId?: DeviceIdService; + deviceId?: DeviceId; commonProperties?: CommonProperties; } = {} ): Telemetry { @@ -54,7 +54,7 @@ export class Telemetry { return; } - this.setupPromise = Promise.all([this.deviceId.getDeviceId(), detectContainerEnv()]); + this.setupPromise = Promise.all([this.deviceId.get(), detectContainerEnv()]); const [deviceIdValue, containerEnv] = await this.setupPromise; this.commonProperties.device_id = deviceIdValue; diff --git a/tests/integration/common/deviceId.test.ts b/tests/integration/common/deviceId.test.ts index ec5b61b0..5335c37b 100644 --- a/tests/integration/common/deviceId.test.ts +++ b/tests/integration/common/deviceId.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; -import { DeviceIdService } from "../../../src/helpers/deviceId.js"; +import { DeviceId } from "../../../src/helpers/deviceId.js"; import { CompositeLogger } from "../../../src/common/logger.js"; import nodeMachineId from "node-machine-id"; @@ -12,15 +12,13 @@ describe("Device ID", () => { }); afterEach(() => { - if (DeviceIdService.isInitialized()) { - DeviceIdService.getInstance().close(); - } + DeviceId.create(testLogger).close(); }); describe("when resolving device ID", () => { it("should successfully resolve device ID in real environment", async () => { - const deviceId = DeviceIdService.init(testLogger); - const result = await deviceId.getDeviceId(); + const deviceId = DeviceId.create(testLogger); + const result = await deviceId.get(); expect(result).not.toBe("unknown"); expect(result).toBeTruthy(); @@ -31,23 +29,23 @@ describe("Device ID", () => { it("should cache device ID after first resolution", async () => { // spy on machineId const machineIdSpy = vi.spyOn(nodeMachineId, "machineId"); - const deviceId = DeviceIdService.init(testLogger); + const deviceId = DeviceId.create(testLogger); // First call - const result1 = await deviceId.getDeviceId(); + const result1 = await deviceId.get(); expect(result1).not.toBe("unknown"); // Second call should be cached - const result2 = await deviceId.getDeviceId(); + const result2 = await deviceId.get(); expect(result2).toBe(result1); // check that machineId was called only once expect(machineIdSpy).toHaveBeenCalledOnce(); }); it("should handle concurrent device ID requests correctly", async () => { - const deviceId = DeviceIdService.init(testLogger); + const deviceId = DeviceId.create(testLogger); - const promises = Array.from({ length: 5 }, () => deviceId.getDeviceId()); + const promises = Array.from({ length: 5 }, () => deviceId.get()); // All should resolve to the same value const results = await Promise.all(promises); @@ -81,10 +79,10 @@ describe("Device ID", () => { reject(new Error("Machine ID failed")); }); }); - const deviceId = DeviceIdService.init(testLogger); - const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceIdService); + const deviceId = DeviceId.create(testLogger); + const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceId); - const result = await deviceId.getDeviceId(); + const result = await deviceId.get(); expect(result).toBe("unknown"); expect(handleDeviceIdErrorSpy).toHaveBeenCalledWith( @@ -101,38 +99,13 @@ describe("Device ID", () => { }); }); - const deviceId = DeviceIdService.init(testLogger); - const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceIdService); + const deviceId = DeviceId.create(testLogger, 100); // Short timeout + const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceId); - deviceId.close(); - - // expect the deviceId service to throw an error - await expect(deviceId.getDeviceId()).rejects.toThrow(Error); - // test that the private function handleDeviceIdError was called with reason "abort" - expect(handleDeviceIdErrorSpy).toHaveBeenCalledWith( - "abort", - expect.stringContaining("Aborted by abort signal") - ); - - // check that the deviceId service is not initialized anymore - expect(() => DeviceIdService.getInstance()).toThrow(Error); - }); - - it("should handle timeout scenarios gracefully", async () => { - nodeMachineId.machineId = vi.fn().mockImplementation(() => { - return new Promise((resolve) => { - setTimeout(() => resolve("delayed-id"), 200); - }); - }); - - // override the timeout to 100ms - const deviceId = DeviceIdService.init(testLogger, 100); - const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceIdService); - - const result = await deviceId.getDeviceId(); + const result = await deviceId.get(); expect(result).toBe("unknown"); - expect(handleDeviceIdErrorSpy).toHaveBeenCalledWith("timeout", expect.stringContaining("Timeout")); - }, 5000); + expect(handleDeviceIdErrorSpy).toHaveBeenCalledWith("timeout", expect.any(String)); + }); }); }); diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index 866f8d1b..db1d49e1 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -7,7 +7,7 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "./inMemoryTransport.js"; import { UserConfig, DriverOptions } from "../../src/common/config.js"; -import { DeviceIdService } from "../../src/helpers/deviceId.js"; +import { DeviceId } from "../../src/helpers/deviceId.js"; import { McpError, ResourceUpdatedNotificationSchema } from "@modelcontextprotocol/sdk/types.js"; import { config, driverOptions } from "../../src/common/config.js"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; @@ -69,9 +69,6 @@ export function setupIntegrationTest( const exportsManager = ExportsManager.init(userConfig, logger); - // Initialize DeviceIdService for tests - DeviceIdService.init(logger); - const connectionManager = new ConnectionManager(userConfig, driverOptions, logger); const session = new Session({ diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index 5661c9bd..a9dfe290 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -1,7 +1,7 @@ import { Telemetry } from "../../src/telemetry/telemetry.js"; import { Session } from "../../src/common/session.js"; import { config, driverOptions } from "../../src/common/config.js"; -import { DeviceIdService } from "../../src/helpers/deviceId.js"; +import { DeviceId } 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"; @@ -11,9 +11,8 @@ describe("Telemetry", () => { it("should resolve the actual device ID", async () => { const logger = new CompositeLogger(); - // Initialize DeviceIdService like the main application does - DeviceIdService.init(logger); - const actualDeviceId = await DeviceIdService.getInstance().getDeviceId(); + const deviceId = DeviceId.create(logger); + const actualDeviceId = await deviceId.get(); const telemetry = Telemetry.create( new Session({ @@ -22,7 +21,8 @@ describe("Telemetry", () => { exportsManager: ExportsManager.init(config, logger), connectionManager: new ConnectionManager(config, driverOptions, logger), }), - config + config, + { deviceId } ); expect(telemetry.getCommonProperties().device_id).toBe(undefined); diff --git a/tests/integration/transports/stdio.test.ts b/tests/integration/transports/stdio.test.ts index f972ebc0..fc0cb423 100644 --- a/tests/integration/transports/stdio.test.ts +++ b/tests/integration/transports/stdio.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, beforeAll, afterAll } from "vitest"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { describeWithMongoDB } from "../tools/mongodb/mongodbHelpers.js"; -import { DeviceIdService } from "../../../src/helpers/deviceId.js"; +import { DeviceId } from "../../../src/helpers/deviceId.js"; import { CompositeLogger } from "../../../src/common/logger.js"; describeWithMongoDB("StdioRunner", (integration) => { @@ -22,7 +22,6 @@ describeWithMongoDB("StdioRunner", (integration) => { name: "test", version: "0.0.0", }); - DeviceIdService.init(new CompositeLogger()); await client.connect(transport); }); diff --git a/tests/integration/transports/streamableHttp.test.ts b/tests/integration/transports/streamableHttp.test.ts index bbee0457..26e7ba6b 100644 --- a/tests/integration/transports/streamableHttp.test.ts +++ b/tests/integration/transports/streamableHttp.test.ts @@ -3,7 +3,7 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { describe, expect, it, beforeAll, afterAll } from "vitest"; import { config } from "../../../src/common/config.js"; -import { DeviceIdService } from "../../../src/helpers/deviceId.js"; +import { DeviceId } from "../../../src/helpers/deviceId.js"; describe("StreamableHttpRunner", () => { let runner: StreamableHttpRunner; @@ -16,7 +16,6 @@ describe("StreamableHttpRunner", () => { config.telemetry = "disabled"; config.loggers = ["stderr"]; runner = new StreamableHttpRunner(config); - DeviceIdService.init(runner.logger); await runner.start(); }); diff --git a/tests/unit/common/session.test.ts b/tests/unit/common/session.test.ts index 57d51373..66127337 100644 --- a/tests/unit/common/session.test.ts +++ b/tests/unit/common/session.test.ts @@ -1,29 +1,25 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, Mocked, vi } from "vitest"; import { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver"; import { Session } from "../../../src/common/session.js"; import { config, driverOptions } from "../../../src/common/config.js"; import { CompositeLogger } from "../../../src/common/logger.js"; import { ConnectionManager } from "../../../src/common/connectionManager.js"; import { ExportsManager } from "../../../src/common/exportsManager.js"; +import { DeviceId } from "../../../src/helpers/deviceId.js"; vi.mock("@mongosh/service-provider-node-driver"); -vi.mock("../../../src/helpers/deviceId.js", () => ({ - DeviceIdService: { - init: vi.fn(), - getInstance: vi.fn().mockReturnValue({ - getDeviceId: vi.fn().mockResolvedValue("test-device-id"), - }), - getDeviceId: vi.fn().mockResolvedValue("test-device-id"), - resetInstance: vi.fn(), - }, -})); const MockNodeDriverServiceProvider = vi.mocked(NodeDriverServiceProvider); describe("Session", () => { let session: Session; + let mockDeviceId: Mocked; + beforeEach(() => { const logger = new CompositeLogger(); + mockDeviceId = vi.mocked(DeviceId.create(logger)); + mockDeviceId.get = vi.fn().mockResolvedValue("test-device-id"); + session = new Session({ apiClientId: "test-client-id", apiBaseUrl: "https://api.test.com", diff --git a/tests/unit/helpers/connectionOptions.test.ts b/tests/unit/helpers/connectionOptions.test.ts index be771563..eac04070 100644 --- a/tests/unit/helpers/connectionOptions.test.ts +++ b/tests/unit/helpers/connectionOptions.test.ts @@ -1,17 +1,20 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, Mocked, vi } from "vitest"; import { setAppNameParamIfMissing } from "../../../src/helpers/connectionOptions.js"; -import { DeviceIdService } from "../../../src/helpers/deviceId.js"; - -// Mock the deviceId utility -vi.mock("../../../src/helpers/deviceId.js", () => ({ - DeviceIdService: { - getInstance: vi.fn().mockReturnValue({ - getDeviceId: vi.fn().mockResolvedValue("test-device-id"), - }), - }, -})); +import { DeviceId } from "../../../src/helpers/deviceId.js"; +import { CompositeLogger } from "../../../src/common/logger.js"; describe("Connection Options", () => { + let testLogger: CompositeLogger; + let mockDeviceId: Mocked; + + beforeEach(() => { + testLogger = new CompositeLogger(); + testLogger.debug = vi.fn(); + + mockDeviceId = vi.mocked(DeviceId.create(testLogger)); + mockDeviceId.get = vi.fn().mockResolvedValue("test-device-id"); + }); + describe("setAppNameParamIfMissing", () => { it("should set extended appName when no appName is present", async () => { const connectionString = "mongodb://localhost:27017"; @@ -20,7 +23,7 @@ describe("Connection Options", () => { components: { appName: "TestApp", clientName: "TestClient", - deviceId: DeviceIdService.getInstance().getDeviceId(), + deviceId: mockDeviceId.get(), }, }); @@ -62,7 +65,7 @@ describe("Connection Options", () => { connectionString, components: { appName: "TestApp", - deviceId: DeviceIdService.getInstance().getDeviceId(), + deviceId: mockDeviceId.get(), }, }); @@ -89,7 +92,7 @@ describe("Connection Options", () => { components: { appName: "TestApp", clientName: "TestClient", - deviceId: DeviceIdService.getInstance().getDeviceId(), + deviceId: mockDeviceId.get(), }, }); diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 1ebdd6fa..9a1d135a 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; -import { DeviceIdService } from "../../../src/helpers/deviceId.js"; +import { DeviceId } from "../../../src/helpers/deviceId.js"; import { getDeviceId } from "@mongodb-js/device-id"; import { CompositeLogger } from "../../../src/common/logger.js"; @@ -18,44 +18,21 @@ describe("deviceId", () => { afterEach(() => { vi.restoreAllMocks(); - if (DeviceIdService.isInitialized()) { - DeviceIdService.getInstance().close(); - } }); - it("should return the same instance for multiple getInstance calls", () => { - // Initialize first - DeviceIdService.init(testLogger); - - const instance1 = DeviceIdService.getInstance(); - const instance2 = DeviceIdService.getInstance(); + it("should create not separate instances", () => { + const instance1 = DeviceId.create(testLogger); + const instance2 = DeviceId.create(testLogger); expect(instance1).toBe(instance2); }); - it("should correctly report initialization status", () => { - expect(DeviceIdService.isInitialized()).toBe(false); - - DeviceIdService.init(testLogger); - expect(DeviceIdService.isInitialized()).toBe(true); - - DeviceIdService.getInstance().close(); - expect(DeviceIdService.isInitialized()).toBe(false); - }); - - it("should throw error when getInstance is called before init", () => { - expect(() => DeviceIdService.getInstance()).toThrow("DeviceIdService not initialized"); - }); - it("should successfully retrieve device ID", async () => { const mockDeviceId = "test-device-id-123"; MockGetDeviceId.mockResolvedValue(mockDeviceId); - // Initialize after mocking - DeviceIdService.init(testLogger); - - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + const deviceId = DeviceId.create(testLogger); + const result = await deviceId.get(); expect(result).toBe(mockDeviceId); }); @@ -64,15 +41,15 @@ describe("deviceId", () => { const mockDeviceId = "test-device-id-123"; MockGetDeviceId.mockResolvedValue(mockDeviceId); - // Initialize after mocking - const deviceId = DeviceIdService.init(testLogger); + const deviceId = DeviceId.create(testLogger); + // First call should trigger calculation - const result1 = await deviceId.getDeviceId(); + const result1 = await deviceId.get(); expect(result1).toBe(mockDeviceId); expect(MockGetDeviceId).toHaveBeenCalledTimes(1); // Second call should use cached value - const result2 = await deviceId.getDeviceId(); + const result2 = await deviceId.get(); expect(result2).toBe(mockDeviceId); expect(MockGetDeviceId).toHaveBeenCalledTimes(1); // Still only called once }); @@ -91,69 +68,96 @@ describe("deviceId", () => { }); }); - // Initialize after mocking - DeviceIdService.init(testLogger); - - const deviceId = DeviceIdService.getInstance(); + const deviceId = DeviceId.create(testLogger); // Start calculation - const promise = deviceId.getDeviceId(); + const promise = deviceId.get(); - // Abort immediately + // Abort the calculation deviceId.close(); - // Should reject with AbortError - await expect(promise).rejects.toThrow("Aborted"); + // Should reject with AbortError - handle the unhandled rejection + try { + await promise; + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect((error as Error).name).toBe("AbortError"); + } }); - it("should return 'unknown' when getDeviceId throws an error", async () => { - MockGetDeviceId.mockRejectedValue(new Error("Device ID resolution failed")); + it("should handle resolution errors gracefully", async () => { + MockGetDeviceId.mockRejectedValue(new Error("Resolution failed")); - // Initialize after mocking - DeviceIdService.init(testLogger); - - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + const deviceId = DeviceId.create(testLogger); + const result = await deviceId.get(); expect(result).toBe("unknown"); }); - it("should handle machine ID generation failure", async () => { - MockGetDeviceId.mockRejectedValue(new Error("Device ID failed")); - - // Initialize after mocking - DeviceIdService.init(testLogger); + it("should handle timeout errors gracefully", async () => { + MockGetDeviceId.mockImplementation((options) => { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + const timeoutError = new Error("Timeout"); + timeoutError.name = "TimeoutError"; + reject(timeoutError); + }, 100); + options.abortSignal?.addEventListener("abort", () => { + clearTimeout(timeout); + }); + }); + }); - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + const deviceId = DeviceId.create(testLogger, 50); // Short timeout + const result = await deviceId.get(); expect(result).toBe("unknown"); }); - it("should use AbortController signal", async () => { - MockGetDeviceId.mockResolvedValue("device-id"); + it("should use custom timeout", async () => { + const mockDeviceId = "test-device-id-123"; + MockGetDeviceId.mockResolvedValue(mockDeviceId); - // Initialize after mocking - DeviceIdService.init(testLogger); + const deviceId = DeviceId.create(testLogger, 5000); + const result = await deviceId.get(); - const deviceId = DeviceIdService.getInstance(); - await deviceId.getDeviceId(); + expect(result).toBe(mockDeviceId); + expect(MockGetDeviceId).toHaveBeenCalledWith( + expect.objectContaining({ + timeout: 5000, + }) + ); + }); - const callArgs = MockGetDeviceId.mock.calls[0]?.[0]; - if (callArgs) { - expect(callArgs.abortSignal).toBeInstanceOf(AbortSignal); - } + it("should use default timeout when not specified", async () => { + const mockDeviceId = "test-device-id-123"; + MockGetDeviceId.mockResolvedValue(mockDeviceId); + + const deviceId = DeviceId.create(testLogger); + const result = await deviceId.get(); + + expect(result).toBe(mockDeviceId); + expect(MockGetDeviceId).toHaveBeenCalledWith( + expect.objectContaining({ + timeout: 3000, // DEVICE_ID_TIMEOUT + }) + ); }); - it("should handle non-Error exceptions", async () => { - MockGetDeviceId.mockRejectedValue("String error"); + it("should handle multiple close calls gracefully", () => { + const deviceId = DeviceId.create(testLogger); - // Initialize after mocking - DeviceIdService.init(testLogger); + // First close should work + expect(() => deviceId.close()).not.toThrow(); - const deviceId = DeviceIdService.getInstance(); - const result = await deviceId.getDeviceId(); + // Second close should also work without error + expect(() => deviceId.close()).not.toThrow(); + }); - expect(result).toBe("unknown"); + it("should not throw error when get is called after close", async () => { + const deviceId = DeviceId.create(testLogger); + deviceId.close(); + + await expect(deviceId.get()).rejects.toThrow("Device ID calculation not started"); }); }); diff --git a/tests/unit/resources/common/debug.test.ts b/tests/unit/resources/common/debug.test.ts index 52894709..46d2c0bf 100644 --- a/tests/unit/resources/common/debug.test.ts +++ b/tests/unit/resources/common/debug.test.ts @@ -6,11 +6,10 @@ import { config, driverOptions } from "../../../../src/common/config.js"; import { CompositeLogger } from "../../../../src/common/logger.js"; import { ConnectionManager } from "../../../../src/common/connectionManager.js"; import { ExportsManager } from "../../../../src/common/exportsManager.js"; -import { DeviceIdService } from "../../../../src/helpers/deviceId.js"; +import { DeviceId } from "../../../../src/helpers/deviceId.js"; describe("debug resource", () => { const logger = new CompositeLogger(); - DeviceIdService.init(logger); const session = new Session({ apiBaseUrl: "", logger, diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 4148f090..3baddb53 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -7,7 +7,7 @@ import { config } from "../../src/common/config.js"; import { afterEach, beforeEach, describe, it, vi, expect } from "vitest"; import { NullLogger } from "../../src/common/logger.js"; import type { MockedFunction } from "vitest"; -import { DeviceIdService } from "../../src/helpers/deviceId.js"; +import { DeviceId } from "../../src/helpers/deviceId.js"; // Mock the ApiClient to avoid real API calls vi.mock("../../src/common/atlas/apiClient.js"); @@ -17,15 +17,6 @@ const MockApiClient = vi.mocked(ApiClient); vi.mock("../../src/telemetry/eventCache.js"); const MockEventCache = vi.mocked(EventCache); -// Mock the deviceId utility -vi.mock("../../src/helpers/deviceId.js", () => ({ - DeviceIdService: { - init: vi.fn(), - getInstance: vi.fn(), - resetInstance: vi.fn(), - }, -})); - describe("Telemetry", () => { let mockApiClient: { sendEvents: MockedFunction<(events: BaseEvent[]) => Promise>; @@ -124,20 +115,9 @@ describe("Telemetry", () => { mockEventCache.appendEvents = vi.fn().mockResolvedValue(undefined); MockEventCache.getInstance = vi.fn().mockReturnValue(mockEventCache as unknown as EventCache); - // Setup mocked DeviceId const mockDeviceId = { - getDeviceId: vi.fn().mockResolvedValue("test-device-id"), - abortCalculation: vi.fn().mockResolvedValue(undefined), - deviceId: undefined, - deviceIdPromise: undefined, - abortController: undefined, - logger: new NullLogger(), - getMachineId: vi.fn(), - startDeviceIdCalculation: vi.fn(), - calculateDeviceId: vi.fn(), - } as unknown as DeviceIdService; - DeviceIdService.getInstance = vi.fn().mockReturnValue(mockDeviceId); - DeviceIdService.init = vi.fn().mockReturnValue(mockDeviceId); + get: vi.fn().mockResolvedValue("test-device-id"), + } as unknown as DeviceId; // Create a simplified session with our mocked API client session = { @@ -243,7 +223,11 @@ describe("Telemetry", () => { }); it("should successfully resolve the device ID", async () => { - telemetry = Telemetry.create(session, config); + const mockDeviceId = { + get: vi.fn().mockResolvedValue("test-device-id"), + } as unknown as DeviceId; + + telemetry = Telemetry.create(session, config, { deviceId: mockDeviceId }); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -255,12 +239,11 @@ describe("Telemetry", () => { }); it("should handle device ID resolution failure gracefully", async () => { - // Mock the deviceId utility to return "unknown" for this test - const { DeviceIdService } = await import("../../src/helpers/deviceId.js"); - const mockDeviceId = vi.mocked(DeviceIdService.getInstance()); - mockDeviceId.getDeviceId.mockResolvedValueOnce("unknown"); + const mockDeviceId = { + get: vi.fn().mockResolvedValue("unknown"), + } as unknown as DeviceId; - telemetry = Telemetry.create(session, config); + telemetry = Telemetry.create(session, config, { deviceId: mockDeviceId }); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -273,12 +256,11 @@ describe("Telemetry", () => { }); it("should handle device ID timeout gracefully", async () => { - // Mock the deviceId utility to return "unknown" for this test - const { DeviceIdService } = await import("../../src/helpers/deviceId.js"); - const mockDeviceId = vi.mocked(DeviceIdService.getInstance()); - mockDeviceId.getDeviceId.mockResolvedValueOnce("unknown"); + const mockDeviceId = { + get: vi.fn().mockResolvedValue("unknown"), + } as unknown as DeviceId; - telemetry = Telemetry.create(session, config); + telemetry = Telemetry.create(session, config, { deviceId: mockDeviceId }); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); From 858ce1e65477001633f786ae91bbc5fd8d8ab9e9 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Wed, 20 Aug 2025 23:57:49 +0100 Subject: [PATCH 27/33] fix linting --- src/helpers/deviceId.ts | 4 ++-- tests/integration/helpers.ts | 1 - tests/integration/transports/stdio.test.ts | 2 -- tests/integration/transports/streamableHttp.test.ts | 1 - tests/unit/resources/common/debug.test.ts | 1 - 5 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 0e1c1d9c..1680b06f 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -25,7 +25,7 @@ export class DeviceId { } const instance = new DeviceId(logger, timeout ?? DEVICE_ID_TIMEOUT); - void instance.setup(); + instance.setup(); this.instance = instance; @@ -35,7 +35,7 @@ export class DeviceId { /** * Sets up the device ID calculation promise and abort controller. */ - private async setup(): Promise { + private setup(): void { this.abortController = new AbortController(); this.deviceIdPromise = this.calculateDeviceId(); // start the promise upon setup diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index db1d49e1..824ad9b1 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -7,7 +7,6 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { InMemoryTransport } from "./inMemoryTransport.js"; import { UserConfig, DriverOptions } from "../../src/common/config.js"; -import { DeviceId } from "../../src/helpers/deviceId.js"; import { McpError, ResourceUpdatedNotificationSchema } from "@modelcontextprotocol/sdk/types.js"; import { config, driverOptions } from "../../src/common/config.js"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; diff --git a/tests/integration/transports/stdio.test.ts b/tests/integration/transports/stdio.test.ts index fc0cb423..aaa61d63 100644 --- a/tests/integration/transports/stdio.test.ts +++ b/tests/integration/transports/stdio.test.ts @@ -2,8 +2,6 @@ import { describe, expect, it, beforeAll, afterAll } from "vitest"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { describeWithMongoDB } from "../tools/mongodb/mongodbHelpers.js"; -import { DeviceId } from "../../../src/helpers/deviceId.js"; -import { CompositeLogger } from "../../../src/common/logger.js"; describeWithMongoDB("StdioRunner", (integration) => { describe("client connects successfully", () => { diff --git a/tests/integration/transports/streamableHttp.test.ts b/tests/integration/transports/streamableHttp.test.ts index 26e7ba6b..d5b6e0be 100644 --- a/tests/integration/transports/streamableHttp.test.ts +++ b/tests/integration/transports/streamableHttp.test.ts @@ -3,7 +3,6 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { describe, expect, it, beforeAll, afterAll } from "vitest"; import { config } from "../../../src/common/config.js"; -import { DeviceId } from "../../../src/helpers/deviceId.js"; describe("StreamableHttpRunner", () => { let runner: StreamableHttpRunner; diff --git a/tests/unit/resources/common/debug.test.ts b/tests/unit/resources/common/debug.test.ts index 46d2c0bf..4f51e381 100644 --- a/tests/unit/resources/common/debug.test.ts +++ b/tests/unit/resources/common/debug.test.ts @@ -6,7 +6,6 @@ import { config, driverOptions } from "../../../../src/common/config.js"; import { CompositeLogger } from "../../../../src/common/logger.js"; import { ConnectionManager } from "../../../../src/common/connectionManager.js"; import { ExportsManager } from "../../../../src/common/exportsManager.js"; -import { DeviceId } from "../../../../src/helpers/deviceId.js"; describe("debug resource", () => { const logger = new CompositeLogger(); From 0bc1a9ff61c4f799c0cca0fa5bc086e7ac638b00 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Thu, 21 Aug 2025 09:03:25 +0100 Subject: [PATCH 28/33] update test --- tests/unit/helpers/deviceId.test.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 9a1d135a..98818c52 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -17,6 +17,8 @@ describe("deviceId", () => { }); afterEach(() => { + DeviceId.create(testLogger).close(); + vi.restoreAllMocks(); }); @@ -76,13 +78,8 @@ describe("deviceId", () => { // Abort the calculation deviceId.close(); - // Should reject with AbortError - handle the unhandled rejection - try { - await promise; - } catch (error) { - expect(error).toBeInstanceOf(Error); - expect((error as Error).name).toBe("AbortError"); - } + // Should reject with AbortError + await expect(promise).rejects.toThrow("Aborted"); }); it("should handle resolution errors gracefully", async () => { From b0972bd17fd60d5f5c2dc234789015167e5f55fd Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 22 Aug 2025 12:25:09 +0100 Subject: [PATCH 29/33] address comment: inject deviceId --- src/common/connectionManager.ts | 3 +- src/helpers/deviceId.ts | 66 +++++++------------- src/index.ts | 4 -- src/telemetry/telemetry.ts | 3 +- src/transports/base.ts | 7 ++- tests/integration/common/deviceId.test.ts | 13 ++-- tests/integration/helpers.ts | 6 +- tests/integration/telemetry.test.ts | 4 +- tests/unit/common/session.test.ts | 8 ++- tests/unit/helpers/connectionOptions.test.ts | 15 +++-- tests/unit/helpers/deviceId.test.ts | 54 ++++------------ tests/unit/resources/common/debug.test.ts | 6 +- tests/unit/telemetry.test.ts | 9 ++- 13 files changed, 75 insertions(+), 123 deletions(-) diff --git a/src/common/connectionManager.ts b/src/common/connectionManager.ts index a0e2a13e..68504e24 100644 --- a/src/common/connectionManager.ts +++ b/src/common/connectionManager.ts @@ -79,6 +79,7 @@ export class ConnectionManager extends EventEmitter { private userConfig: UserConfig, private driverOptions: DriverOptions, private logger: CompositeLogger, + deviceId: DeviceId, bus?: EventEmitter ) { super(); @@ -89,7 +90,7 @@ export class ConnectionManager extends EventEmitter { this.bus.on("mongodb-oidc-plugin:auth-failed", this.onOidcAuthFailed.bind(this)); this.bus.on("mongodb-oidc-plugin:auth-succeeded", this.onOidcAuthSucceeded.bind(this)); - this.deviceId = DeviceId.create(this.logger); + this.deviceId = deviceId; this.clientName = "unknown"; } diff --git a/src/helpers/deviceId.ts b/src/helpers/deviceId.ts index 1680b06f..246b0bd1 100644 --- a/src/helpers/deviceId.ts +++ b/src/helpers/deviceId.ts @@ -21,7 +21,7 @@ export class DeviceId { public static create(logger: LoggerBase, timeout?: number): DeviceId { if (this.instance) { - return this.instance; + throw new Error("DeviceId instance already exists, use get() to retrieve the device ID"); } const instance = new DeviceId(logger, timeout ?? DEVICE_ID_TIMEOUT); @@ -32,14 +32,8 @@ export class DeviceId { return instance; } - /** - * Sets up the device ID calculation promise and abort controller. - */ private setup(): void { - this.abortController = new AbortController(); this.deviceIdPromise = this.calculateDeviceId(); - // start the promise upon setup - void this.deviceIdPromise; } /** @@ -50,6 +44,7 @@ export class DeviceId { this.abortController.abort(); this.abortController = undefined; } + this.deviceId = undefined; this.deviceIdPromise = undefined; DeviceId.instance = undefined; @@ -59,16 +54,16 @@ export class DeviceId { * Gets the device ID, waiting for the calculation to complete if necessary. * @returns Promise that resolves to the device ID string */ - public async get(): Promise { - if (this.deviceId !== undefined) { - return this.deviceId; + public get(): Promise { + if (this.deviceId) { + return Promise.resolve(this.deviceId); } - if (!this.deviceIdPromise) { - this.deviceIdPromise = this.calculateDeviceId(); + if (this.deviceIdPromise) { + return this.deviceIdPromise; } - return this.deviceIdPromise; + return this.calculateDeviceId(); } /** @@ -76,43 +71,24 @@ export class DeviceId { */ private async calculateDeviceId(): Promise { if (!this.abortController) { - throw new Error("Device ID calculation not started"); + this.abortController = new AbortController(); } - try { - const deviceId = await getDeviceId({ - getMachineId: this.getMachineId, - onError: (reason, error) => { - this.handleDeviceIdError(reason, String(error)); - }, - timeout: this.timeout, - abortSignal: this.abortController.signal, - }); - - // Cache the result - this.deviceId = deviceId; - return deviceId; - } catch (error) { - // Check if this was an abort error - if (error instanceof Error && error.name === "AbortError") { - throw error; // Re-throw abort errors - } - - this.logger.debug({ - id: LogId.deviceIdResolutionError, - context: "deviceId", - message: `Failed to get device ID: ${String(error)}`, - }); - - // Cache the fallback value - this.deviceId = "unknown"; - return "unknown"; - } finally { - this.abortController = undefined; - } + this.deviceIdPromise = getDeviceId({ + getMachineId: this.getMachineId, + onError: (reason, error) => { + this.handleDeviceIdError(reason, String(error)); + }, + timeout: this.timeout, + abortSignal: this.abortController.signal, + }); + + return this.deviceIdPromise; } private handleDeviceIdError(reason: string, error: string): void { + this.deviceIdPromise = Promise.resolve("unknown"); + switch (reason) { case "resolutionError": this.logger.debug({ diff --git a/src/index.ts b/src/index.ts index d4947c8c..29f525dc 100644 --- a/src/index.ts +++ b/src/index.ts @@ -42,7 +42,6 @@ import { packageInfo } from "./common/packageInfo.js"; import { StdioRunner } from "./transports/stdio.js"; import { StreamableHttpRunner } from "./transports/streamableHttp.js"; import { systemCA } from "@mongodb-js/devtools-proxy-support"; -import { DeviceId } from "./helpers/deviceId.js"; async function main(): Promise { systemCA().catch(() => undefined); // load system CA asynchronously as in mongosh @@ -51,7 +50,6 @@ async function main(): Promise { assertVersionMode(); const transportRunner = config.transport === "stdio" ? new StdioRunner(config) : new StreamableHttpRunner(config); - const deviceId = DeviceId.create(transportRunner.logger); const shutdown = (): void => { transportRunner.logger.info({ id: LogId.serverCloseRequested, @@ -62,7 +60,6 @@ async function main(): Promise { transportRunner .close() .then(() => { - deviceId.close(); transportRunner.logger.info({ id: LogId.serverClosed, context: "server", @@ -71,7 +68,6 @@ async function main(): Promise { process.exit(0); }) .catch((error: unknown) => { - deviceId.close(); transportRunner.logger.error({ id: LogId.serverCloseFailure, context: "server", diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 3b311f14..af75fead 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -33,13 +33,12 @@ export class Telemetry { static create( session: Session, userConfig: UserConfig, + deviceId: DeviceId, { commonProperties = { ...MACHINE_METADATA }, eventCache = EventCache.getInstance(), - deviceId = DeviceId.create(session.logger), }: { eventCache?: EventCache; - deviceId?: DeviceId; commonProperties?: CommonProperties; } = {} ): Telemetry { diff --git a/src/transports/base.ts b/src/transports/base.ts index b2ca3e1a..f91566c7 100644 --- a/src/transports/base.ts +++ b/src/transports/base.ts @@ -7,9 +7,11 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { CompositeLogger, ConsoleLogger, DiskLogger, LoggerBase, McpLogger } from "../common/logger.js"; import { ExportsManager } from "../common/exportsManager.js"; import { ConnectionManager } from "../common/connectionManager.js"; +import { DeviceId } from "../helpers/deviceId.js"; export abstract class TransportRunnerBase { public logger: LoggerBase; + public deviceId: DeviceId; protected constructor(protected readonly userConfig: UserConfig) { const loggers: LoggerBase[] = []; @@ -28,6 +30,7 @@ export abstract class TransportRunnerBase { } this.logger = new CompositeLogger(...loggers); + this.deviceId = DeviceId.create(this.logger); } protected setupServer(userConfig: UserConfig): Server { @@ -43,7 +46,7 @@ export abstract class TransportRunnerBase { const logger = new CompositeLogger(...loggers); const exportsManager = ExportsManager.init(userConfig, logger); - const connectionManager = new ConnectionManager(userConfig, driverOptions, logger); + const connectionManager = new ConnectionManager(userConfig, driverOptions, logger, this.deviceId); const session = new Session({ apiBaseUrl: userConfig.apiBaseUrl, @@ -54,7 +57,7 @@ export abstract class TransportRunnerBase { connectionManager, }); - const telemetry = Telemetry.create(session, userConfig); + const telemetry = Telemetry.create(session, userConfig, this.deviceId); return new Server({ mcpServer, diff --git a/tests/integration/common/deviceId.test.ts b/tests/integration/common/deviceId.test.ts index 5335c37b..296d3440 100644 --- a/tests/integration/common/deviceId.test.ts +++ b/tests/integration/common/deviceId.test.ts @@ -5,6 +5,7 @@ import nodeMachineId from "node-machine-id"; describe("Device ID", () => { let testLogger: CompositeLogger; + let deviceId: DeviceId; beforeEach(() => { testLogger = new CompositeLogger(); @@ -12,12 +13,12 @@ describe("Device ID", () => { }); afterEach(() => { - DeviceId.create(testLogger).close(); + deviceId?.close(); }); describe("when resolving device ID", () => { it("should successfully resolve device ID in real environment", async () => { - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); const result = await deviceId.get(); expect(result).not.toBe("unknown"); @@ -29,7 +30,7 @@ describe("Device ID", () => { it("should cache device ID after first resolution", async () => { // spy on machineId const machineIdSpy = vi.spyOn(nodeMachineId, "machineId"); - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); // First call const result1 = await deviceId.get(); @@ -43,7 +44,7 @@ describe("Device ID", () => { }); it("should handle concurrent device ID requests correctly", async () => { - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); const promises = Array.from({ length: 5 }, () => deviceId.get()); @@ -79,7 +80,7 @@ describe("Device ID", () => { reject(new Error("Machine ID failed")); }); }); - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceId); const result = await deviceId.get(); @@ -99,7 +100,7 @@ describe("Device ID", () => { }); }); - const deviceId = DeviceId.create(testLogger, 100); // Short timeout + deviceId = DeviceId.create(testLogger, 100); // Short timeout const handleDeviceIdErrorSpy = vi.spyOn(deviceId, "handleDeviceIdError" as keyof DeviceId); const result = await deviceId.get(); diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index 824ad9b1..2699bcaa 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -11,6 +11,7 @@ import { McpError, ResourceUpdatedNotificationSchema } from "@modelcontextprotoc import { config, driverOptions } from "../../src/common/config.js"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { ConnectionManager, ConnectionState } from "../../src/common/connectionManager.js"; +import { DeviceId } from "../../src/helpers/deviceId.js"; interface ParameterInfo { name: string; @@ -68,7 +69,8 @@ export function setupIntegrationTest( const exportsManager = ExportsManager.init(userConfig, logger); - const connectionManager = new ConnectionManager(userConfig, driverOptions, logger); + const deviceId = DeviceId.create(logger); + const connectionManager = new ConnectionManager(userConfig, driverOptions, logger, deviceId); const session = new Session({ apiBaseUrl: userConfig.apiBaseUrl, @@ -87,7 +89,7 @@ export function setupIntegrationTest( userConfig.telemetry = "disabled"; - const telemetry = Telemetry.create(session, userConfig); + const telemetry = Telemetry.create(session, userConfig, deviceId); mcpServer = new Server({ session, diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index a9dfe290..cc51ed8b 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -19,10 +19,10 @@ describe("Telemetry", () => { apiBaseUrl: "", logger, exportsManager: ExportsManager.init(config, logger), - connectionManager: new ConnectionManager(config, driverOptions, logger), + connectionManager: new ConnectionManager(config, driverOptions, logger, deviceId), }), config, - { deviceId } + deviceId ); expect(telemetry.getCommonProperties().device_id).toBe(undefined); diff --git a/tests/unit/common/session.test.ts b/tests/unit/common/session.test.ts index 66127337..2720c941 100644 --- a/tests/unit/common/session.test.ts +++ b/tests/unit/common/session.test.ts @@ -10,6 +10,7 @@ import { DeviceId } from "../../../src/helpers/deviceId.js"; vi.mock("@mongosh/service-provider-node-driver"); const MockNodeDriverServiceProvider = vi.mocked(NodeDriverServiceProvider); +const MockDeviceId = vi.mocked(DeviceId.create(new CompositeLogger())); describe("Session", () => { let session: Session; @@ -17,18 +18,19 @@ describe("Session", () => { beforeEach(() => { const logger = new CompositeLogger(); - mockDeviceId = vi.mocked(DeviceId.create(logger)); - mockDeviceId.get = vi.fn().mockResolvedValue("test-device-id"); + + mockDeviceId = MockDeviceId; session = new Session({ apiClientId: "test-client-id", apiBaseUrl: "https://api.test.com", logger, exportsManager: ExportsManager.init(config, logger), - connectionManager: new ConnectionManager(config, driverOptions, logger), + connectionManager: new ConnectionManager(config, driverOptions, logger, mockDeviceId), }); MockNodeDriverServiceProvider.connect = vi.fn().mockResolvedValue({} as unknown as NodeDriverServiceProvider); + MockDeviceId.get = vi.fn().mockResolvedValue("test-device-id"); }); describe("connectToMongoDB", () => { diff --git a/tests/unit/helpers/connectionOptions.test.ts b/tests/unit/helpers/connectionOptions.test.ts index eac04070..5ee3d29f 100644 --- a/tests/unit/helpers/connectionOptions.test.ts +++ b/tests/unit/helpers/connectionOptions.test.ts @@ -1,18 +1,17 @@ -import { beforeEach, describe, expect, it, Mocked, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { setAppNameParamIfMissing } from "../../../src/helpers/connectionOptions.js"; import { DeviceId } from "../../../src/helpers/deviceId.js"; import { CompositeLogger } from "../../../src/common/logger.js"; +const MockDeviceId = vi.mocked(DeviceId.create(new CompositeLogger())); + describe("Connection Options", () => { let testLogger: CompositeLogger; - let mockDeviceId: Mocked; beforeEach(() => { testLogger = new CompositeLogger(); testLogger.debug = vi.fn(); - - mockDeviceId = vi.mocked(DeviceId.create(testLogger)); - mockDeviceId.get = vi.fn().mockResolvedValue("test-device-id"); + MockDeviceId.get = vi.fn().mockResolvedValue("test-device-id"); }); describe("setAppNameParamIfMissing", () => { @@ -23,7 +22,7 @@ describe("Connection Options", () => { components: { appName: "TestApp", clientName: "TestClient", - deviceId: mockDeviceId.get(), + deviceId: MockDeviceId.get(), }, }); @@ -65,7 +64,7 @@ describe("Connection Options", () => { connectionString, components: { appName: "TestApp", - deviceId: mockDeviceId.get(), + deviceId: MockDeviceId.get(), }, }); @@ -92,7 +91,7 @@ describe("Connection Options", () => { components: { appName: "TestApp", clientName: "TestClient", - deviceId: mockDeviceId.get(), + deviceId: MockDeviceId.get(), }, }); diff --git a/tests/unit/helpers/deviceId.test.ts b/tests/unit/helpers/deviceId.test.ts index 98818c52..68fd54e0 100644 --- a/tests/unit/helpers/deviceId.test.ts +++ b/tests/unit/helpers/deviceId.test.ts @@ -10,6 +10,7 @@ const MockGetDeviceId = vi.mocked(getDeviceId); describe("deviceId", () => { let testLogger: CompositeLogger; + let deviceId: DeviceId; beforeEach(() => { vi.clearAllMocks(); @@ -17,23 +18,22 @@ describe("deviceId", () => { }); afterEach(() => { - DeviceId.create(testLogger).close(); - vi.restoreAllMocks(); + deviceId.close(); }); - it("should create not separate instances", () => { - const instance1 = DeviceId.create(testLogger); - const instance2 = DeviceId.create(testLogger); + it("should fail to create separate instances", () => { + deviceId = DeviceId.create(testLogger); - expect(instance1).toBe(instance2); + // try to create a new device id and see it raises an error + expect(() => DeviceId.create(testLogger)).toThrow("DeviceId instance already exists"); }); it("should successfully retrieve device ID", async () => { const mockDeviceId = "test-device-id-123"; MockGetDeviceId.mockResolvedValue(mockDeviceId); - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); const result = await deviceId.get(); expect(result).toBe(mockDeviceId); @@ -43,7 +43,7 @@ describe("deviceId", () => { const mockDeviceId = "test-device-id-123"; MockGetDeviceId.mockResolvedValue(mockDeviceId); - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); // First call should trigger calculation const result1 = await deviceId.get(); @@ -82,35 +82,6 @@ describe("deviceId", () => { await expect(promise).rejects.toThrow("Aborted"); }); - it("should handle resolution errors gracefully", async () => { - MockGetDeviceId.mockRejectedValue(new Error("Resolution failed")); - - const deviceId = DeviceId.create(testLogger); - const result = await deviceId.get(); - - expect(result).toBe("unknown"); - }); - - it("should handle timeout errors gracefully", async () => { - MockGetDeviceId.mockImplementation((options) => { - return new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - const timeoutError = new Error("Timeout"); - timeoutError.name = "TimeoutError"; - reject(timeoutError); - }, 100); - options.abortSignal?.addEventListener("abort", () => { - clearTimeout(timeout); - }); - }); - }); - - const deviceId = DeviceId.create(testLogger, 50); // Short timeout - const result = await deviceId.get(); - - expect(result).toBe("unknown"); - }); - it("should use custom timeout", async () => { const mockDeviceId = "test-device-id-123"; MockGetDeviceId.mockResolvedValue(mockDeviceId); @@ -130,7 +101,7 @@ describe("deviceId", () => { const mockDeviceId = "test-device-id-123"; MockGetDeviceId.mockResolvedValue(mockDeviceId); - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); const result = await deviceId.get(); expect(result).toBe(mockDeviceId); @@ -142,7 +113,7 @@ describe("deviceId", () => { }); it("should handle multiple close calls gracefully", () => { - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); // First close should work expect(() => deviceId.close()).not.toThrow(); @@ -152,9 +123,10 @@ describe("deviceId", () => { }); it("should not throw error when get is called after close", async () => { - const deviceId = DeviceId.create(testLogger); + deviceId = DeviceId.create(testLogger); deviceId.close(); - await expect(deviceId.get()).rejects.toThrow("Device ID calculation not started"); + // undefined should be returned + expect(await deviceId.get()).toBeUndefined(); }); }); diff --git a/tests/unit/resources/common/debug.test.ts b/tests/unit/resources/common/debug.test.ts index 4f51e381..0292a726 100644 --- a/tests/unit/resources/common/debug.test.ts +++ b/tests/unit/resources/common/debug.test.ts @@ -6,16 +6,18 @@ import { config, driverOptions } from "../../../../src/common/config.js"; import { CompositeLogger } from "../../../../src/common/logger.js"; import { ConnectionManager } from "../../../../src/common/connectionManager.js"; import { ExportsManager } from "../../../../src/common/exportsManager.js"; +import { DeviceId } from "../../../../src/helpers/deviceId.js"; describe("debug resource", () => { const logger = new CompositeLogger(); + const deviceId = DeviceId.create(logger); const session = new Session({ apiBaseUrl: "", logger, exportsManager: ExportsManager.init(config, logger), - connectionManager: new ConnectionManager(config, driverOptions, logger), + connectionManager: new ConnectionManager(config, driverOptions, logger, deviceId), }); - const telemetry = Telemetry.create(session, { ...config, telemetry: "disabled" }); + const telemetry = Telemetry.create(session, { ...config, telemetry: "disabled" }, deviceId); let debugResource: DebugResource = new DebugResource(session, config, telemetry); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 3baddb53..e1a159d0 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -130,9 +130,8 @@ describe("Telemetry", () => { logger: new NullLogger(), } as unknown as Session; - telemetry = Telemetry.create(session, config, { + telemetry = Telemetry.create(session, config, mockDeviceId, { eventCache: mockEventCache as unknown as EventCache, - deviceId: mockDeviceId, }); config.telemetry = "enabled"; @@ -227,7 +226,7 @@ describe("Telemetry", () => { get: vi.fn().mockResolvedValue("test-device-id"), } as unknown as DeviceId; - telemetry = Telemetry.create(session, config, { deviceId: mockDeviceId }); + telemetry = Telemetry.create(session, config, mockDeviceId); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -243,7 +242,7 @@ describe("Telemetry", () => { get: vi.fn().mockResolvedValue("unknown"), } as unknown as DeviceId; - telemetry = Telemetry.create(session, config, { deviceId: mockDeviceId }); + telemetry = Telemetry.create(session, config, mockDeviceId); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -260,7 +259,7 @@ describe("Telemetry", () => { get: vi.fn().mockResolvedValue("unknown"), } as unknown as DeviceId; - telemetry = Telemetry.create(session, config, { deviceId: mockDeviceId }); + telemetry = Telemetry.create(session, config, mockDeviceId); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); From e570562f8be5b3c3990bb6f1cf54ce7096552768 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 22 Aug 2025 12:28:52 +0100 Subject: [PATCH 30/33] chore: update transport to close deviceId last --- src/transports/base.ts | 10 +++++++++- src/transports/stdio.ts | 2 +- src/transports/streamableHttp.ts | 2 +- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/transports/base.ts b/src/transports/base.ts index f91566c7..50213334 100644 --- a/src/transports/base.ts +++ b/src/transports/base.ts @@ -69,5 +69,13 @@ export abstract class TransportRunnerBase { abstract start(): Promise; - abstract close(): Promise; + abstract closeTransport(): Promise; + + async close(): Promise { + try { + await this.closeTransport(); + } finally { + this.deviceId.close(); + } + } } diff --git a/src/transports/stdio.ts b/src/transports/stdio.ts index 81141b5f..f74022d2 100644 --- a/src/transports/stdio.ts +++ b/src/transports/stdio.ts @@ -74,7 +74,7 @@ export class StdioRunner extends TransportRunnerBase { } } - async close(): Promise { + async closeTransport(): Promise { await this.server?.close(); } } diff --git a/src/transports/streamableHttp.ts b/src/transports/streamableHttp.ts index e6e93ba7..c78638e1 100644 --- a/src/transports/streamableHttp.ts +++ b/src/transports/streamableHttp.ts @@ -147,7 +147,7 @@ export class StreamableHttpRunner extends TransportRunnerBase { }); } - async close(): Promise { + async closeTransport(): Promise { await Promise.all([ this.sessionStore.closeAllSessions(), new Promise((resolve, reject) => { From 0ed8d230cd027cf50523e13d68a467b3f0c13583 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 22 Aug 2025 12:34:04 +0100 Subject: [PATCH 31/33] chore: add deviceId close to integration --- tests/integration/helpers.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index 2699bcaa..2fc5c93d 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -42,6 +42,7 @@ export function setupIntegrationTest( ): IntegrationTest { let mcpClient: Client | undefined; let mcpServer: Server | undefined; + let deviceId: DeviceId | undefined; beforeAll(async () => { const userConfig = getUserConfig(); @@ -69,7 +70,7 @@ export function setupIntegrationTest( const exportsManager = ExportsManager.init(userConfig, logger); - const deviceId = DeviceId.create(logger); + deviceId = DeviceId.create(logger); const connectionManager = new ConnectionManager(userConfig, driverOptions, logger, deviceId); const session = new Session({ @@ -117,6 +118,9 @@ export function setupIntegrationTest( await mcpServer?.close(); mcpServer = undefined; + + await deviceId.close(); + deviceId = undefined; }); const getMcpClient = (): Client => { From abf285ad3961baf64afd24c14336cb62a7f037db Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 22 Aug 2025 12:37:45 +0100 Subject: [PATCH 32/33] fix check --- tests/integration/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index 2fc5c93d..b2ffaeb9 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -119,7 +119,7 @@ export function setupIntegrationTest( await mcpServer?.close(); mcpServer = undefined; - await deviceId.close(); + deviceId?.close(); deviceId = undefined; }); From e7727eea155b5d74f09b5746aee409c209430429 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 22 Aug 2025 12:39:31 +0100 Subject: [PATCH 33/33] fix check --- tests/accuracy/export.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/accuracy/export.test.ts b/tests/accuracy/export.test.ts index 6ece170a..5b262417 100644 --- a/tests/accuracy/export.test.ts +++ b/tests/accuracy/export.test.ts @@ -8,7 +8,6 @@ describeAccuracyTests([ { toolName: "export", parameters: { - exportTitle: Matcher.string(), database: "mflix", collection: "movies", exportTitle: Matcher.string(), @@ -28,7 +27,6 @@ describeAccuracyTests([ { toolName: "export", parameters: { - exportTitle: Matcher.string(), database: "mflix", collection: "movies", exportTitle: Matcher.string(), @@ -52,7 +50,6 @@ describeAccuracyTests([ { toolName: "export", parameters: { - exportTitle: Matcher.string(), database: "mflix", collection: "movies", exportTitle: Matcher.string(), @@ -81,7 +78,6 @@ describeAccuracyTests([ { toolName: "export", parameters: { - exportTitle: Matcher.string(), database: "mflix", collection: "movies", exportTitle: Matcher.string(),