-
Notifications
You must be signed in to change notification settings - Fork 44
feat: Add drop-search-index tool #243
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: Add drop-search-index tool #243
Conversation
|
||
protected async execute({ database, collection, indexName }: ToolArgs<typeof this.argsShape>): Promise<CallToolResult> { | ||
const provider = await this.ensureConnected(); | ||
const result = await provider.dropSearchIndex(database, collection, indexName); |
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.
nit: Should the tool be a bit smarter and check whether the index exists first? If it doesn't, maybe we notify the LLM? I guess the more context LLM has, the better.
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
already checks and returns an error if the index doesn't exist
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.
Please also look at Github actions checkstyle suggestions
"Deletes a text or vector search index from the database."; | ||
protected argsShape = { | ||
...DbOperationArgs, | ||
indexName: z.string().describe("The name of the search or vector index to delete"), |
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.
We'll need to repeat this often, so let's register a
export const SearchIndexOperationArgs = {
database: z.string().describe("Database name"),
collection: z.string().describe("Collection name"),
searchIndexName: z.string().describe("Search or VectorSearch Index Name");
};
in mongodbTool.ts and import it here and use it on L16 and anywhere else we need.
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.
Done, see 17f9022
return { | ||
content: [ | ||
{ | ||
text: `"Successfully dropped index "${indexName}" from database "${database}"`, |
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.
nit: looks like we don't need double quotes
`"Successfully - before
`Successfully - after
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.
Done, see 17f9022
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.
lgtm!
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.
lgtm
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.
LGTM
Add drop-search-index tool
Tested manually with VS code + Copilot and Claude 3.5
Unit/Integration tests will be added in a separate PR