-
Notifications
You must be signed in to change notification settings - Fork 30
Update connection string app name if not present #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
a2fd0cf
d53ba26
52c8c0c
6b2ad7a
4da79ad
8cab5a1
88bad25
aea9657
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { MongoClientOptions } from "mongodb"; | ||
import ConnectionString from "mongodb-connection-string-url"; | ||
import { isAtlas } from "mongodb-build-info"; | ||
|
||
export function setAppNameParamIfMissing({ | ||
connectionString, | ||
defaultAppName, | ||
telemetryAnonymousId, | ||
}: { | ||
connectionString: string; | ||
defaultAppName?: string; | ||
telemetryAnonymousId?: string; | ||
}): string { | ||
const connectionStringUrl = new ConnectionString(connectionString); | ||
|
||
const searchParams = connectionStringUrl.typedSearchParams<MongoClientOptions>(); | ||
|
||
if (!searchParams.has("appName") && defaultAppName !== undefined) { | ||
const appName = isAtlas(connectionString) | ||
? `${defaultAppName}${telemetryAnonymousId ? `-${telemetryAnonymousId}` : ""}` | ||
: defaultAppName; | ||
|
||
searchParams.set("appName", appName); | ||
} | ||
|
||
return connectionStringUrl.toString(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
declare module "mongodb-connection-string-url" { | ||
import { URL } from "whatwg-url"; | ||
import { redactConnectionString, ConnectionStringRedactionOptions } from "./redact"; | ||
export { redactConnectionString, ConnectionStringRedactionOptions }; | ||
declare class CaseInsensitiveMap<K extends string = string> extends Map<K, string> { | ||
delete(name: K): boolean; | ||
get(name: K): string | undefined; | ||
has(name: K): boolean; | ||
set(name: K, value: any): this; | ||
_normalizeKey(name: any): K; | ||
} | ||
declare abstract class URLWithoutHost extends URL { | ||
abstract get host(): never; | ||
abstract set host(value: never); | ||
abstract get hostname(): never; | ||
abstract set hostname(value: never); | ||
abstract get port(): never; | ||
abstract set port(value: never); | ||
abstract get href(): string; | ||
abstract set href(value: string); | ||
} | ||
export interface ConnectionStringParsingOptions { | ||
looseValidation?: boolean; | ||
} | ||
export declare class ConnectionString extends URLWithoutHost { | ||
_hosts: string[]; | ||
constructor(uri: string, options?: ConnectionStringParsingOptions); | ||
get host(): never; | ||
set host(_ignored: never); | ||
get hostname(): never; | ||
set hostname(_ignored: never); | ||
get port(): never; | ||
set port(_ignored: never); | ||
get href(): string; | ||
set href(_ignored: string); | ||
get isSRV(): boolean; | ||
get hosts(): string[]; | ||
set hosts(list: string[]); | ||
toString(): string; | ||
clone(): ConnectionString; | ||
redact(options?: ConnectionStringRedactionOptions): ConnectionString; | ||
typedSearchParams<T extends {}>(): { | ||
append(name: keyof T & string, value: any): void; | ||
delete(name: keyof T & string): void; | ||
get(name: keyof T & string): string | null; | ||
getAll(name: keyof T & string): string[]; | ||
has(name: keyof T & string): boolean; | ||
set(name: keyof T & string, value: any): void; | ||
keys(): IterableIterator<keyof T & string>; | ||
values(): IterableIterator<string>; | ||
entries(): IterableIterator<[keyof T & string, string]>; | ||
_normalizeKey(name: keyof T & string): string; | ||
[Symbol.iterator](): IterableIterator<[keyof T & string, string]>; | ||
sort(): void; | ||
forEach<THIS_ARG = void>( | ||
callback: (this: THIS_ARG, value: string, name: string, searchParams: any) => void, | ||
thisArg?: THIS_ARG | undefined | ||
): void; | ||
readonly [Symbol.toStringTag]: "URLSearchParams"; | ||
}; | ||
} | ||
export declare class CommaAndColonSeparatedRecord< | ||
K extends {} = Record<string, unknown>, | ||
> extends CaseInsensitiveMap<keyof K & string> { | ||
constructor(from?: string | null); | ||
toString(): string; | ||
} | ||
export default ConnectionString; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,6 +7,10 @@ | |||||
import { jest } from "@jest/globals"; | ||||||
import logger, { LogId } from "../../src/logger.js"; | ||||||
import { createHmac } from "crypto"; | ||||||
import { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver"; | ||||||
|
||||||
jest.mock("@mongosh/service-provider-node-driver"); | ||||||
const MockNodeDriverServiceProvider = NodeDriverServiceProvider as jest.MockedClass<typeof NodeDriverServiceProvider>; | ||||||
|
||||||
// Mock the ApiClient to avoid real API calls | ||||||
jest.mock("../../src/common/atlas/apiClient.js"); | ||||||
|
@@ -315,4 +319,96 @@ | |||||
}); | ||||||
}); | ||||||
}); | ||||||
|
||||||
describe("connectToMongoDB", () => { | ||||||
beforeEach(() => { | ||||||
session = new Session({ | ||||||
apiClientId: "test-client-id", | ||||||
apiBaseUrl: "https://api.test.com", | ||||||
}); | ||||||
|
||||||
MockNodeDriverServiceProvider.connect = jest.fn(() => | ||||||
Promise.resolve({} as unknown as NodeDriverServiceProvider) | ||||||
); | ||||||
}); | ||||||
|
||||||
const testCases: { | ||||||
connectionString: string; | ||||||
isAtlas: boolean; | ||||||
expectAppName: boolean; | ||||||
expectTelemetryId: boolean; | ||||||
name: string; | ||||||
disableTelemetry?: boolean; | ||||||
}[] = [ | ||||||
{ | ||||||
connectionString: "mongodb://localhost:27017", | ||||||
isAtlas: false, | ||||||
expectAppName: true, | ||||||
expectTelemetryId: false, | ||||||
name: "local db", | ||||||
}, | ||||||
{ | ||||||
connectionString: "mongodb+srv://test.mongodb.net/test?retryWrites=true&w=majority", | ||||||
isAtlas: true, | ||||||
expectAppName: true, | ||||||
expectTelemetryId: true, | ||||||
name: "atlas db", | ||||||
}, | ||||||
{ | ||||||
connectionString: "mongodb://localhost:27017?appName=CustomAppName", | ||||||
isAtlas: false, | ||||||
expectAppName: false, | ||||||
expectTelemetryId: false, | ||||||
name: "local db with custom appName", | ||||||
}, | ||||||
{ | ||||||
connectionString: | ||||||
"mongodb+srv://test.mongodb.net/test?retryWrites=true&w=majority&appName=CustomAppName", | ||||||
isAtlas: true, | ||||||
expectAppName: false, | ||||||
expectTelemetryId: false, | ||||||
name: "atlas db with custom appName", | ||||||
}, | ||||||
|
||||||
{ | ||||||
connectionString: "mongodb+srv://test.mongodb.net/test?retryWrites=true&w=majority", | ||||||
isAtlas: true, | ||||||
expectAppName: true, | ||||||
expectTelemetryId: false, | ||||||
name: "atlas db with telemetry disabled", | ||||||
disableTelemetry: true, | ||||||
}, | ||||||
]; | ||||||
|
||||||
for (const testCase of testCases) { | ||||||
it(`should update connection string for ${testCase.name}`, async () => { | ||||||
if (testCase.disableTelemetry) { | ||||||
config.telemetry = "disabled"; | ||||||
telemetry = Telemetry.create(session, config, { | ||||||
getRawMachineId: () => Promise.resolve(machineId), | ||||||
}); | ||||||
} | ||||||
await session.connectToMongoDB(testCase.connectionString, config.connectOptions, telemetry); | ||||||
expect(session.serviceProvider).toBeDefined(); | ||||||
expect(MockNodeDriverServiceProvider.connect).toHaveBeenCalledOnce(); | ||||||
const connectionString = MockNodeDriverServiceProvider.connect.mock.calls[0][0]; | ||||||
if (testCase.expectAppName) { | ||||||
expect(connectionString).toContain("appName=MongoDB+MCP+Server"); | ||||||
} else { | ||||||
expect(connectionString).not.toContain("appName=MongoDB+MCP+Server"); | ||||||
} | ||||||
|
||||||
if (testCase.disableTelemetry) { | ||||||
expect(connectionString).not.toMatch(/appName=[^-]*-[^&]*/); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] Consider extracting the regex pattern into a named constant or helper function to improve maintainability and clarity, in case the connection string format changes in the future.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
} else { | ||||||
const telemetryId = await telemetry.deviceIdPromise; | ||||||
if (testCase.isAtlas && testCase.expectTelemetryId) { | ||||||
expect(connectionString).toContain(telemetryId); | ||||||
} else { | ||||||
expect(connectionString).not.toContain(telemetryId); | ||||||
} | ||||||
} | ||||||
}); | ||||||
} | ||||||
}); | ||||||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does mean we're delaying connecting to a cluster for the sake of telemetry 😢 could we not append anonymous ID yet, especially since we use the segment anonymous ID in all other places at the moment so this won't match it anyhow