Skip to content

Conversation

@aaronburtle
Copy link
Contributor

Why make this change?

Closes #2932

What is this change?

Add helper class McpMetadataHelper, extend McpArgumentParser, and utilize McpAuthorizationHelper to factor out common code. We now do the initialization of the metadata, the parsing of arguments, and the authorization checks in these shared helper classes.

How was this tested?

Against the normal test suite.

Sample Request(s)

N/A

Copilot finished reviewing on behalf of aaronburtle November 19, 2025 05:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors common code shared among built-in MCP tools by introducing the McpMetadataHelper class and extending McpArgumentParser to centralize metadata resolution, argument parsing, and authorization checks across all MCP tools.

  • Introduced McpMetadataHelper.TryResolveMetadata() to consolidate entity metadata resolution logic
  • Extended McpArgumentParser with TryParseEntity(), TryParseEntityAndData(), and TryParseEntityAndKeys() methods
  • Refactored all tool classes to use the new helper classes, removing duplicated code

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
src/Azure.DataApiBuilder.Mcp/Utils/McpMetadataHelper.cs New helper class for resolving entity metadata, data sources, and database objects
src/Azure.DataApiBuilder.Mcp/Utils/McpArgumentParser.cs Extended with new parsing methods for entity, entity+data, and entity+keys argument combinations
src/Azure.DataApiBuilder.Mcp/BuiltInTools/UpdateRecordTool.cs Refactored to use shared helpers; removed duplicate parsing and authorization code
src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs Refactored to use shared helpers; optimized orderby processing with string.Join
src/Azure.DataApiBuilder.Mcp/BuiltInTools/ExecuteEntityTool.cs Refactored to use McpMetadataHelper for metadata resolution
src/Azure.DataApiBuilder.Mcp/BuiltInTools/DeleteRecordTool.cs Refactored to use shared helpers; simplified tool metadata method with expression body
src/Azure.DataApiBuilder.Mcp/BuiltInTools/CreateRecordTool.cs Refactored to use shared helpers for parsing, metadata resolution, and authorization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@anushakolan
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@anushakolan
Copy link
Contributor

The main issue here is the testing. I see you tested against normal test suite, which I assume are all the tests in Cli.Tests and Service.tests. But we don't have many automated tests for the MCP tools, and this change mainly targets the MCP tools. The best approach here is testing if all the tools are working as expected with MCP Inspector and we did not break anything as part of the refactoring.
Please do this testing and update the description accordingly.

@anushakolan
Copy link
Contributor

Is there any common code in describe_entities tool that can also be refactored, basics like checking permissions or if the tool is enabled etc. If so, we can refactor that as well.

@anushakolan
Copy link
Contributor

Weare having around 4 helper class so far McpAuthorizationHelper, McpResponseBuilder, McpArgumentParser, McpMetadataHelper. Do we need all 4 of them or can this be simplified, we can have a single helper and reuse that single helper.

@aaronburtle
Copy link
Contributor Author

Weare having around 4 helper class so far McpAuthorizationHelper, McpResponseBuilder, McpArgumentParser, McpMetadataHelper. Do we need all 4 of them or can this be simplified, we can have a single helper and reuse that single helper.

I think for now we have good separation of responsibilities with these different helper classes. If we find ourselves adding more and more we can think about reorganizing them but for now I think this makes sense.

@aaronburtle
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create common base code for all the MCP tools

5 participants