feat(index-check): add index check functionality before query< 10000 /bdi> #309
Conversation
| * @param explainResult The result of the explain query | ||
| * @returns true if an index is used, false if it's a full collection scan | ||
| */ | ||
| export function usesIndex(explainResult: Document): boolean { |
There was a problem hiding this comment.
There will be situations where this doesn't work in MongoDB 8.0+, as we have a few more stages that are like an index scan. Can you add them? These are:
"EXPRESS_IXSCAN"
"EXPRESS_CLUSTERED_IXSCAN"
"EXPRESS_UPDATE"
"EXPRESS_DELETE"
Also, not related to 8.0, but there is a stage called IDHACK that is also like an index scan.
src/helpers/indexCheck.ts
Outdated
| const explainResult = await explainCallback(); | ||
|
|
||
| if (!usesIndex(explainResult)) { | ||
| throw new Error(getIndexCheckErrorMessage(database, collection, operation)); |
There was a problem hiding this comment.
| throw new Error(getIndexCheckErrorMessage(database, collection, operation)); | |
| throw new MongoDBError(ErrorCodes.ForbiddenCollscan, getIndexCheckErrorMessage(database, collection, operation)); |
Here it's better to use MongoDBError, this way you can rethrow it without taking a look at the message, and it's also what we use as a facade with the MCP Client, so it should be safe to throw anytime!
You'll need to add the error code in src/errors.ts
|
@kmruiz :) Thank you for the detailed code review and suggestions! I have completed the following changes according to your feedback: ✅ Added Please review again, and feel free to provide any additional suggestions. Thank you again for your guidance ! |
There was a problem hiding this comment.
Everything looks good @Crushdada ! I'm going to run the workflows and if everything is green and you don't mind, I'll merge the PR.
Thanks a lot for contributing! Really appreciate it!
|
@Crushdada I see that there are a few issues with the formatting and the usage of You can run Thanks! |
|
@kmruiz Thanks for catching that! Can I use "string | undefined" to solve the "any" type of problem? |
|
Sure! ideally if you are working on an object with an unknown shape, ideally you should define a type and cast it so we know what is the basic shape for the object. For primitives, you can use the type and undefined. However we are not really strict here, if it's clear the purpose and the type, it will be fine. |
There was a problem hiding this comment.
Thank you for this @Crushdada 🚀 Added small comments. It would be nice to add a few integration tests for the added config change as well?
| // Check if delete operation uses an index if enabled | ||
| if (this.config.indexCheck) { | ||
| await checkIndexUsage(provider, database, collection, "deleteMany", async () => { | ||
| return provider.mongoClient.db(database).command({ |
There was a problem hiding this comment.
The provider instance here provides interfaces (runCommand and runCommandWithCheck) for command execution. Please prefer those over accessing methods directly on MongoClient. The same applies to other instances of the MongoClient.db().command() usages.
I believe what you need here is runCommandWithCheck
There was a problem hiding this comment.
Thanks for pointing out this issue, I have revised it. But I encountered a problem that confused me that the "code health" check failed. Should we increase the timeout at tests/integration/tools/mongodb/mongodbHelpers.ts:94 ?
Co-authored-by: Himanshu Singh <himanshu.singhs@outlook.in>
Summary
This PR adds a new
indexCheckconfiguration option that enforces MongoDB queries to use indexes, preventing collection scans (COLLSCAN) operations.Changes
indexCheckhelper functions insrc/helpers/indexCheck.ts.smithery/smithery.yamlFeatures
Testing
Related Issues
Resolves #287
Documentation