chore: add arg-parser and put the config under test MCP-86 #429
Conversation
This commit is biggish because it adds a big chunk of tests that weren't there before. While it would be arguably that testing every flag might be too much, because I'm changing how arguments are parsed, I want to have this as a safeline.
For help we are refering to the README.md, if we want something more fancy we porobably want to have better documentation generation so we don't maintain two files (here and the README.md)
The driver options can not be sent by the agent, so we are going to use the ones specified in the CLI arguments / ENV Vars. This is relevant because new connections should use any inherited configuration like TLS settings or FIPS by default.
This is necessary when connecting for the first time.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the configuration management system to use the @mongosh/arg-parser library for generating MongoDB connection strings. This change aligns with how mongosh handles connection arguments and ensures support for additional authentication mechanisms. The refactoring removes several deprecated flags and types that are no longer necessary with the consolidated approach.
- Uses mongosh argument parser to standardize connection string generation and authentication support
- Consolidates configuration by removing the separate
ConnectOptionsinterface and moving connection options to driver-level configuration - Removes hardcoded connection options from test files and connection calls throughout the codebase
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/config.ts | Major refactor to use mongosh arg-parser with new configuration parsing and driver options setup |
| tests/unit/common/config.test.ts | Comprehensive test suite for the new configuration parsing functionality |
| src/index.ts | Added FIPS mode support, help/version modes, and system CA loading |
| tests/unit/common/session.test.ts | Removed deprecated connectOptions usage in tests |
| tests/integration/tools/mongodb/connect/connect.test.ts | Removed deprecated connectOptions usage in integration tests |
| tests/integration/common/connectionManager.test.ts | Removed deprecated connectOptions usage and import |
| src/tools/mongodb/mongodbTool.ts | Simplified connection call by removing connectOptions spread |
| src/tools/atlas/connect/connectCluster.ts | Simplified connection call by removing connectOptions spread |
| src/server.ts | Simplified connection call by removing connectOptions spread |
| src/resources/common/config.ts | Updated to use new driverOptions export instead of connectOptions |
| src/lib.ts | Removed ConnectOptions from exports |
| src/common/connectionManager.ts | Refactored to use new driverOptions configuration |
| package.json | Added @mongosh/arg-parser dependency |
| // From: https://github.com/mongodb-js/mongosh/blob/main/packages/cli-repl/src/arg-parser.ts | ||
| const OPTIONS = { |
There was a problem hiding this comment.
Would it be an option to import it from the args-parser package instead?
There was a problem hiding this comment.
I don't think so:
- args-parser does not expose it, we would need to update it, which is fine but
- we don't support all arguments (eval for example) so we would still need to filter or do some post processing.
There was a problem hiding this comment.
Yea but I still think its just a pretty long list to copy here instead of something like this?
Omit<Options, "eval" | "or" | "others">
that way you at-least have one source of truth.
There was a problem hiding this comment.
While they are similar, we do support more options (for the legacy options and Atlas credentials) and I feel being explicit on what we do support additionally and what we don't support is better. If we depend on mongosh and we add more mongosh specific flags we would inherit them without validation.
| console.log("For usage information refer to the README.md:"); | ||
| console.log("https://github.com/mongodb-js/mongodb-mcp-server?tab=readme-ov-file#quick-start"); |
There was a problem hiding this comment.
I think these should be console.error or console.warn. We shouldn't be logging to stdio as that is reserved for transport.
There was a problem hiding this comment.
It's reserved once the transport is started, but this is done before the transport is started. This is for developers using --help to understand the arguments in their console, it's not expected to be used in a mcp.json file or similar.
There was a problem hiding this comment.
Agreed, but then you might wanna verify the behaviour of transport on start when you already have pushed some non-json rpc messages.
There was a problem hiding this comment.
These functions early exit, so there wouldn't be a situation where we issue a warning and then start the transport. That's why the signature of the function is void | never.
Pull Request Test Coverage Report for Build 16830919759Details
💛 - Coveralls |
Proposed changes
Use mongosh argument parser to generate the connection string that is going to be used to connect to MongoDB, aligning with how mongosh does it and making sure we support additional authentication mechanisms. It removes a bunch of flags and types not necessary now with the consolidation.
There will be integration tests for each of the supported connection types when playing each of their tasks. Specific support is out of the scope of this PR.
Checklist