[MCP] Added read_records tool implementation #2893
Conversation
RubenCerna2079
requested review from
aaronburtle,
anushakolan,
souvikghosh04,
akashkumar58,
neeraj-sharma2592,
sourabh1007,
vadeveka,
Alekhya-Polavarapu and
rusamant
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 implements the read_records tool for the MCP (Model Context Protocol), enabling reading of entity records with parameters like select, filter, first, orderby, and after for pagination. The implementation includes proper authorization checks and handles both REST and MCP endpoint responses differently.
- Added a new
ReadRecordsToolclass that implements the MCP read_records functionality - Modified response formatting to support both REST (nextLink) and MCP (after) pagination approaches
- Changed
GenerateOrderByListsmethod visibility from private to public for reuse
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs |
Complete implementation of the read_records tool with argument parsing, authorization, and query execution |
src/Core/Resolvers/SqlResponseHelpers.cs |
Enhanced response formatting to support both REST nextLink and MCP after pagination patterns |
src/Core/Parsers/RequestParser.cs |
Changed GenerateOrderByLists method from private to public for reuse |
src/Core/Parsers/FilterParser.cs |
Removed blank line formatting change |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
| }, | ||
| ""filter"": { | ||
| ""type"": ""string"", | ||
| ""description"": ""A filter expression string to restrict results. Optional."" |
There was a problem hiding this comment.
Let's update this to be more expressive:
A case-insensitive OData-like expression that defines a query predicate. Supports logical grouping with parentheses and the operators eq, ne, gt, ge, lt, le, and, or, not. Examples:
year ge 1990,date lt 2025-01-01T00:00:00Z,(title eq 'Foundation') and (available ne false).
| ""properties"": { | ||
| ""entity"": { | ||
| ""type"": ""string"", | ||
| ""description"": ""The entity name to read from. Required."" |
There was a problem hiding this comment.
Let's update this to be more expressive:
The name of the entity to read, as provided by the
describe_entitiestool. Required.
| }, | ||
| ""select"": { | ||
| ""type"": ""string"", | ||
| ""description"": ""A CSV of field names to include in the response. If not provided, all fields are returned. Optional."" |
There was a problem hiding this comment.
Let's update this to be more expressive:
A comma-separated list of field names to include in the response. If omitted, all fields are returned. Optional.
| return new Tool | ||
| { | ||
| Name = "read_records", | ||
| Description = "Reads the records from the specified entity.", |
There was a problem hiding this comment.
Let's update this to be more expressive:
Retrieves records from a given entity.
| }, | ||
| ""first"": { | ||
| ""type"": ""integer"", | ||
| ""description"": ""The maximum number of records to return in this page. Optional."" |
There was a problem hiding this comment.
Let's update this to be more expressive:
The maximum number of records to return in the current page. Optional.
| } | ||
| } | ||
|
|
||
| error = "You do not have permission to read records for this entity."; |
There was a problem hiding this comment.
You know the entity name so include it in the error message. Remember, error messages are intended to inform the caller with as much information as possible without leaking security so that the caller can correct the problem.
entityName
|
|
||
| string output = JsonSerializer.Serialize(normalized, new JsonSerializerOptions { WriteIndented = true }); | ||
|
|
||
| logger?.LogInformation("ReadRecordsTool success for entity {Entity}.", entityName); |
There was a problem hiding this comment.
Logs are nice, but where is the OTEL wrapper? This is important.
mcp_read_total_count, mcp_read_active_count, etc.
We want to ensure the developer experience across all 3 endpoints is the same.
| { | ||
| Dictionary<string, object?> errorObj = new() | ||
| { | ||
| ["status"] = "error", |
There was a problem hiding this comment.
Curious. Why are we not setting Success = true and Success = false?
| // Create a 'nextLink' object if the request comes from REST endpoint. | ||
| else | ||
| { | ||
| string basePaginationUri = SqlPaginationUtil.ConstructBaseUriForPagination(httpContext, runtimeConfig.Runtime?.BaseRoute); |
There was a problem hiding this comment.
I cannot tell by looking, you might just dismiss this comment, but does this line obey this?
{
"runtime": {
"rest": {
"next-link-relative": true // default is false
}
}
}
| { | ||
| public class ReadRecordsTool : IMcpTool | ||
| { | ||
| // private readonly IMetadataProviderFactory _metadataProviderFactory; |
There was a problem hiding this comment.
Please remove these lines since they are no longer used.
| { | ||
| ILogger<ReadRecordsTool>? logger = serviceProvider.GetService<ILogger<ReadRecordsTool>>(); | ||
|
|
||
| if (arguments == null) |
There was a problem hiding this comment.
nit: Maybe we can move this inside try as well.
| fieldsReturnedForFind = authResolver.GetAllowedExposedColumns(context.EntityName, effectiveRole!, context.OperationType); | ||
| } | ||
|
|
||
| context.UpdateReturnFields(fieldsReturnedForFind); |
There was a problem hiding this comment.
nit: we can add a comment on what this call is doing.
|
|
||
| // Normalize response | ||
| string rawPayloadJson = ExtractResultJson(actionResult); | ||
| using JsonDocument result = JsonDocument.Parse(rawPayloadJson); |
There was a problem hiding this comment.
nit: Can we declare this using in the imports section, start of the document?
| } | ||
| } | ||
|
|
||
| private static bool TryResolveAuthorizedRole( |
There was a problem hiding this comment.
Add the function summary.
| error = "You do not have permission to read records for this entity."; | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add the function summary.
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add the function summary
| public FilterClause GetFilterClause(string filterQueryString, string resourcePath, ODataUriResolver? customResolver = null) | ||
| { | ||
| if (_model is null) | ||
| { |
There was a problem hiding this comment.
nit: Is this new line introduced on purpose, if not we can revert it.
There was a problem hiding this comment.
I am not following, I deleted the line that shouldn't be there. I did not introduce a new line.
github-project-automation
bot
moved this from Todo
to Review In Progress
in Data API builder
|
nit: Fix the description, |
Why make this change?
read_records#2833One of the main DML tools that will be used is
read_records, which will allow the MCP to read the records of an entity based on the needs of the user.What is this change?
read_recordstool:SqlQueryEnginein order to create and run the query and receive the resultsGenerateOrderByListsfunction inside theRequestParser.csfile was changed from private to public in order to allow theread_recordstool to also use it to generate the proper context for the query.SqlResponseHelper.csfile were changed to check if the query request comes from theread_recordstool. This is done in order to output the correct information, right now the REST requests can also return anextLinkobject which gives the API link necessary to get values in the case that not all of them were shown. We want to do something similar with theread_recordstool, however we only want to return theafterobject which is the parameter that allows the query to know the exact place where it left from the previous query. This gets rid of unecessary information that can be found in thenextLinkobject.Exceptions thrown when:
orderbyparameter are empty or null. (Note:orderbyis an optional value as a whole, but the individual values it contains need to exist)Errors:
How was this tested?
Sample Request(s)
{ Entity: Book }{ Select: title,publisher_id }{ First: 3 }{ Orderby: ["publisher_id asc", "title desc"] }