-
Notifications
You must be signed in to change notification settings - Fork 64
feat(index-check): add index check functionality before query #309
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
feat(index-check): add index check functionality before query #309
Conversation
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.
Great job so far! Thanks a lot!
I've requested a few small changes that should make things more correct for newest MongoDB versions and one QoL that should simplify your code a bit.
Thanks a lot for contributing!
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
c6b758e
to
3933710
Compare
I was looking at the failing test, and it seems to be failing with the latest changes. In main the test passes fine, so it's likely a regression we introduced with this PR related to the capability calculation of the server. |
Thank you. I will roll back the code to try to locate the cause of the test failure, but the test cannot even run normally on my mac. It always reports the error "mongodb cluster startup failure". Do you know how to solve it? |
Seems that the issue is not related to this PR, but with an upgrade in the SDK that changes the default values for the MCP capabilities. The PR to fix the issue is here: #322. Whenever this is merged I'll run the checks again in your PR. Sorry for the delay, |
Wow, I see. Thank you very much. Once the merge of pr# 322 is completed, I will do the check I mentioned. |
Not sure! For running a mongodb cluster we use https://www.npmjs.com/package/mongodb-runner?activeTab=readme, which we maintain. Can you try to run the cluster manually using npx?
Edit: Before I forget, @Crushdada would you mind signing the contributor agreement if you haven't done it? Sorry for the bureaucracy we are still setting up everything for external contributors: https://www.mongodb.com/legal/contributor-agreement @himanshusinghs , can you review again the PR? You are requesting changes, can you verify if it's ok to merge? |
Done! I just submitted a contributor agreement. |
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.
Sorry for the delay here! Thank you for iterating on the feedback. It looks good to me 🚀
Merging! Thanks @Crushdada for the hard work and the patience to go through the feedback with us! |
Thanks @kmruiz @himanshusinghs for the merge and the thorough review! 🌟 Appreciate the guidance on the changes : ) |
Summary
This PR adds a new
indexCheck
configuration option that enforces MongoDB queries to use indexes, preventing collection scans (COLLSCAN) operations.Changes
indexCheck
helper functions insrc/helpers/indexCheck.ts
.smithery/smithery.yaml
Features
Testing
Related Issues
Resolves #287
Documentation