feat: add data access tools #14
Conversation
b6be098 to
f042b5f
Compare
| @@ -14,6 +14,7 @@ | |||
| "publishConfig": { | |||
| "access": "public" | |||
| }, | |||
| "type": "module", | |||
There was a problem hiding this comment.
Had to add this since I was seeing complaints that node was first trying to parse it as cjs, but was seeing an esm. I don't have a strong opinion on whether we should go with one or the other.
| const packageMetadata = fs.readFileSync(path.resolve("./package.json"), "utf8"); | ||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
|
|
||
| const packageMetadata = fs.readFileSync(path.join(__dirname, "..", "package.json"), "utf8"); |
There was a problem hiding this comment.
This may have regressed at some point, but path.resolve("./package.json") is looking at dist/config.js rather than in the parent directory. For some reason, Claude is not happy about relative paths, so "../package.json" didn't work either, so I needed the __dirname shenanigans above, since esm doesn't have __dirname built in.
There was a problem hiding this comment.
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (1)
src/tools/mongodb/createIndex.ts:28
- Ensure that the 'indexes' array is not empty before accessing indexes[0]; consider adding a check or handling the scenario where no index is created.
text: `Created the index `${indexes[0]}``,
src/tools/tool.ts
Outdated
|
|
||
| protected constructor(protected state: State) {} | ||
|
|
||
| public register(server: McpServer): void { | ||
| const callback = async (args: z.objectOutputType<Args, ZodTypeAny>): Promise<CallToolResult> => { | ||
| const callback = async (args: z.objectOutputType<Args, ZodNever>): Promise<CallToolResult> => { |
There was a problem hiding this comment.
Using z.ZodNever in the callback signature (and in ToolArgs) might unintentionally restrict valid input data if argsShape is expected to allow non-empty objects. Please verify that this change aligns with the intended usage of the tool arguments.
| const callback = async (args: z.objectOutputType<Args, ZodNever>): Promise<CallToolResult> => { | |
| const callback = async (args: ToolArgs<Args>): Promise<CallToolResult> => { |
Copilot uses AI. Check for mistakes.
|
|
||
| private async connect(connectionString: string): Promise<void> { | ||
| const provider = await NodeDriverServiceProvider.connect(connectionString, { | ||
| productDocsLink: "https://docs.mongodb.com/todo-mcp", |
There was a problem hiding this comment.
The productDocsLink value appears to be a placeholder. It is recommended to update this URL to point to the correct documentation for MongoDB MCP.
| productDocsLink: "https://docs.mongodb.com/todo-mcp", | |
| productDocsLink: "https://docs.mongodb.com/manual/reference/mongodb-mcp/", |
Copilot uses AI. Check for mistakes.
src/tools/tool.ts
Outdated
| export abstract class ToolBase<Args extends ZodRawShape> { | ||
| protected abstract name: string; | ||
|
|
||
| protected abstract description: string; | ||
|
|
||
| protected abstract argsShape: Args; |
There was a problem hiding this comment.
| export abstract class ToolBase<Args extends ZodRawShape> { | |
| protected abstract name: string; | |
| protected abstract description: string; | |
| protected abstract argsShape: Args; | |
| export abstract class ToolBase { | |
| protected abstract name: string; | |
| protected abstract description: string; | |
| protected abstract argsShape: ZodRawShape; | |
| protected abstract execute(args: ToolArgs<typeof this.argsShape>): Promise<CallToolResult>; |
I went on a rabbithole trying to get rid of some redundancy here. Unfortunately I don't think the Args generic is doing anything for us.
I would instead get rid of it and use typeof this.argsShape. This is still not really going to be a strict type as is. For example in find.ts in both cases you can define execute to use ToolArgs<ZodRawShape> and it'd still pass fine so unfortunately TypeScript isn't that strict/smart anyhow.
Same for MongoDBToolBase.
Then we can simply define argsShape inside the class as is without replicating it
There was a problem hiding this comment.
Yeah, I couldn't get typescript to behave exactly how I wanted it to, but figured it's better this way than rewriting the entire server in a proper language 🥲 Anyway, agree with your assessment and have removed the args generic.
There was a problem hiding this comment.
yeah I was also hoping there's a better way but couldn't come up with an alternative either 🥲
Adds the following data-access tools:
Follow-ups: