Refactor response for Read and Update MCP tools to use built in utility functions #2984
Conversation
aaronburtle
requested review from
Alekhya-Polavarapu,
Aniruddh25,
RubenCerna2079,
anushakolan,
neeraj-sharma2592,
rusamant,
sourabh1007,
souvikghosh04 and
vadeveka
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ReadRecordsTool and UpdateRecordTool to use centralized utility functions from McpResponseBuilder, improving code consistency and maintainability across all MCP tools.
- Replaces local
BuildErrorResult,BuildSuccessResult, andExtractResultJsonmethods with calls toMcpResponseBuilderutilities - Enhances response format to include
entityandmessagefields, aligning with other MCP tools - Moves result filtering logic inline in
UpdateRecordToolbefore passing to the utility function
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs | Refactored to use McpResponseBuilder utilities; removed duplicate helper methods; improved exception handling to capture exception details |
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/UpdateRecordTool.cs | Refactored to use McpResponseBuilder utilities; moved result filtering logic inline; removed duplicate helper methods while preserving GetJsonValue utility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
Need to ensure we log the kind of tool it is in BuildErrorResult
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
Since there are no automated tests yet, have you tested the refactoring did not introduce any regressions using Mcp Inspector?
Still need to test with the inspector, will add the test results to the PR description when done. |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
github-project-automation
bot
moved this from Todo
to Review In Progress
in Data API builder
souvikghosh04
left a comment
souvikghosh04
left a comment
There was a problem hiding this comment.
Thanks for the changes. Please address the pending comments.
…ithub.com/Azure/data-api-builder into dev/aaronburtle/RefactorCommonMCPResponse
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
github-project-automation
bot
moved this from Review In Progress
to Done
in Data API builder
Why make this change?
Closes #2919
What is this change?
Refactors the
ReadandUpdatebuilt in MCP tools so that they use the commonBuildErrorResultandBuildSuccessResultfunctiosn in the Utils, aligning their usage with the other tools.How was this tested?
Manually tested using MCP Inspector tool, and run against normal test suite.
Sample Request(s)
N/A