-
Notifications
You must be signed in to change notification settings - Fork 96
chore: disable the connect tool #142
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,31 @@ import { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver | |
import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | ||
import { ErrorCodes, MongoDBError } from "../../errors.js"; | ||
import logger, { LogId } from "../../logger.js"; | ||
import { UserConfig } from "../../config.js"; | ||
import { Session } from "../../session.js"; | ||
|
||
export const DbOperationArgs = { | ||
database: z.string().describe("Database name"), | ||
collection: z.string().describe("Collection name"), | ||
}; | ||
|
||
export async function connectToMongoDB(connectionString: string, config: UserConfig, session: Session): Promise<void> { | ||
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. this is slighly confusing to have Have you considered moving this to session given it currently holds instance of 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. in similar realm, a solution could be to have a wrapper class for My thinking is session is the entry to i.e. 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. Not opposed to this, but I'd suggest doing it separately - I'd prefer that this PR is the bare minimum that disables the connect tool, rather than a broader restructuring of the codebase. |
||
const provider = await NodeDriverServiceProvider.connect(connectionString, { | ||
productDocsLink: "https://docs.mongodb.com/todo-mcp", | ||
productName: "MongoDB MCP", | ||
readConcern: { | ||
level: config.connectOptions.readConcern, | ||
}, | ||
readPreference: config.connectOptions.readPreference, | ||
writeConcern: { | ||
w: config.connectOptions.writeConcern, | ||
}, | ||
timeoutMS: config.connectOptions.timeoutMS, | ||
}); | ||
|
||
session.serviceProvider = provider; | ||
} | ||
|
||
export abstract class MongoDBToolBase extends ToolBase { | ||
protected category: ToolCategory = "mongodb"; | ||
|
||
|
@@ -70,20 +89,7 @@ export abstract class MongoDBToolBase extends ToolBase { | |
return super.handleError(error, args); | ||
} | ||
|
||
protected async connectToMongoDB(connectionString: string): Promise<void> { | ||
const provider = await NodeDriverServiceProvider.connect(connectionString, { | ||
productDocsLink: "https://docs.mongodb.com/todo-mcp", | ||
productName: "MongoDB MCP", | ||
readConcern: { | ||
level: this.config.connectOptions.readConcern, | ||
}, | ||
readPreference: this.config.connectOptions.readPreference, | ||
writeConcern: { | ||
w: this.config.connectOptions.writeConcern, | ||
}, | ||
timeoutMS: this.config.connectOptions.timeoutMS, | ||
}); | ||
|
||
this.session.serviceProvider = provider; | ||
protected connectToMongoDB(connectionString: string): Promise<void> { | ||
return connectToMongoDB(connectionString, this.config, this.session); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,9 @@ | ||
import { expectDefined, setupIntegrationTest } from "./helpers.js"; | ||
import { config } from "../../src/config.js"; | ||
import { describeWithMongoDB } from "./tools/mongodb/mongodbHelpers.js"; | ||
|
||
describe("Server integration test", () => { | ||
describe("without atlas", () => { | ||
const integration = setupIntegrationTest(() => ({ | ||
...config, | ||
apiClientId: undefined, | ||
apiClientSecret: undefined, | ||
})); | ||
|
||
describeWithMongoDB("without atlas", (integration) => { | ||
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. It was the way it was to be resilient against providing env vars on CI/terminal 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. nvm I see you've changed describeAtlas 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. You mean the undefining of the apiClientId/Secret - I can bring it back, the important thing here was to provide the connection string as otherwise the server was failing due to neither atlas credentials, nor connection string being configured. |
||
it("should return positive number of tools and have no atlas tools", async () => { | ||
const tools = await integration.mcpClient().listTools(); | ||
expectDefined(tools); | ||
|
@@ -18,6 +13,7 @@ describe("Server integration test", () => { | |
expect(atlasTools.length).toBeLessThanOrEqual(0); | ||
}); | ||
}); | ||
|
||
describe("with atlas", () => { | ||
const integration = setupIntegrationTest(() => ({ | ||
...config, | ||
|
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.
the code was structured like that originally we moved
https://cloud.mongodb.com/
to be part of apiClient to keep everything about the API in one place, any reason to move it back?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.
I think it makes more sense to have the defaults for the config options together rather than disconnected like that. That way we can see that the
baseUrl
option is actually always set and the exact value it is set to.