-
Notifications
You must be signed in to change notification settings - Fork 281
[MCP] Added the update-record tool implementation. #2888
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
base: main
Are you sure you want to change the base?
Conversation
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
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.
Pull Request Overview
This PR implements the update-record
MCP tool to enable updating database records via the Model Context Protocol (MCP). It includes validation, authorization, and proper error handling for various edge cases.
- Adds a complete
UpdateRecordTool
implementation with comprehensive validation and error handling - Improves SQL predicate construction to use mapped column fields instead of exposed field names
- Provides testing documentation for MCP Inspector usage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
src/Azure.DataApiBuilder.Mcp/BuiltInTools/UpdateRecordTool.cs |
New MCP tool implementation for updating database records with full validation and authorization |
src/Core/Resolvers/Sql Query Structures/SqlUpdateQueryStructure.cs |
Enhanced predicate construction to use backing column names for safer SQL generation |
docs/Testing/mcp-inspector-testing.md |
Testing guide for using MCP Inspector with local development setup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
{ | ||
if (kv.Value is null || (kv.Value is string str && string.IsNullOrWhiteSpace(str))) | ||
{ | ||
throw new ArgumentException("Keys are required to update an entity"); |
Copilot
AI
Oct 1, 2025
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.
Error message is not specific enough. Should indicate which key has the null/empty value, e.g., 'Key value for '{kv.Key}' cannot be null or empty.'
throw new ArgumentException("Keys are required to update an entity"); | |
throw new ArgumentException($"Key value for '{kv.Key}' cannot be null or empty."); |
Copilot uses AI. Check for mistakes.
if (kvp.Value is null) | ||
{ | ||
return BuildErrorResult("InvalidArguments", $"Primary key value for '{kvp.Key}' cannot be null.", logger); | ||
} | ||
|
Copilot
AI
Oct 1, 2025
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.
Duplicate validation logic. The null check for key values is performed twice - once in TryParseArguments (lines 286-292) and again here. Consider consolidating this validation to avoid duplication.
if (kvp.Value is null) | |
{ | |
return BuildErrorResult("InvalidArguments", $"Primary key value for '{kvp.Key}' cannot be null.", logger); | |
} |
Copilot uses AI. Check for mistakes.
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.
this suggestion from copilot is valid and should be addressed
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
|
||
### 4. **How to use the tool** | ||
- Set the transport type "Streamable HTTP". | ||
- Set the URL "http://localhost:5000/mcp" and hit connect. |
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.
Mention as pre-requisite that dab mcp-server should be running before hitting connect here.
set NODE_TLS_REJECT_UNAUTHORIZED=0 | ||
|
||
### 3. ** Open the inspector with pre-filled token.** | ||
http://localhost:6274/?MCP_PROXY_AUTH_TOKEN=<token> |
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.
what is this token used for?
Steps to run and test MCP tools using the https://www.npmjs.com/package/@modelcontextprotocol/inspector. | ||
|
||
### 1. **Install MCP Inspector** | ||
npx @modelcontextprotocol/inspector |
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.
is nodejs a pre-requisite to be installed?
{ | ||
Name = "update_record", | ||
Description = "Updates an existing record in the specified entity. Requires 'keys' to locate the record and 'fields' to specify new values.", | ||
InputSchema = JsonSerializer.Deserialize<JsonElement>( |
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.
#2829 - please update all the references of update-entity
in the issue to update_record
predicate = new( | ||
new PredicateOperand( | ||
new Column(tableSchema: DatabaseObject.SchemaName, tableName: DatabaseObject.Name, param.Key)), | ||
new Column(tableSchema: DatabaseObject.SchemaName, tableName: DatabaseObject.Name, backingColumn)), |
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.
this seems to have been a bug here. Good catch!
return BuildErrorResult( | ||
"UnexpectedError", | ||
ex.Message ?? "An unexpected error occurred during the update operation.", | ||
logger: null); |
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.
why pass a null to the logger here? why not pass the logger
obtained on line 85
return BuildErrorResult("PermissionDenied", "Permission denied: unable to resolve a valid role context for update operation.", logger); | ||
} | ||
|
||
if (!TryResolveAuthorizedRole(httpContext, authResolver, entityName, out string? effectiveRole, out string authError)) |
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.
What is effectiveRole
being used for here?
return BuildErrorResult("EntityNotFound", $"Entity '{entityName}' is not defined in the configuration.", logger); | ||
} | ||
|
||
// 4) Authorization after we have a known entity |
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 need to ensure the ClientRoleHeaderAuthenticationHeaderMiddleware is invoked
data-api-builder/src/Core/AuthenticationHelpers/ClientRoleHeaderAuthenticationMiddleware.cs
Line 26 in a86996d
public class ClientRoleHeaderAuthenticationMiddleware |
before Authorization resolver kicks in here.
You should perform some testing using the simulator to check what happens for authenticated
roles. anonymous
role is by default.
|
||
if (string.IsNullOrWhiteSpace(roleHeader)) | ||
{ | ||
error = "Client role header is missing or empty."; |
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.
If the ClientRoleHeaderAuthenticationMiddleware
isn't invoked, the roleheader here will be empty. Please test that out.
return true; | ||
} | ||
|
||
private static bool TryResolveAuthorizedRole( |
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.
private static bool TryResolveAuthorizedRole( | |
private static bool TryResolveAuthorizedRoleHasPermission( |
context.PrimaryKeyValuePairs[kvp.Key] = kvp.Value; | ||
} | ||
|
||
if (context.DatabaseObject.SourceType is EntitySourceType.Table) |
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.
Why is this validation restricted to entity type of Table?
insertPayloadRoot: upsertPayloadRoot, | ||
operationType: EntityActionOperation.UpdateIncremental); | ||
|
||
context.UpdateReturnFields(keys.Keys.Concat(fields.Keys).Distinct().ToList()); |
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.
I thought we decided to return the entire record in the response to align with what we have in REST. In REST, do we only return the keys?
{ | ||
return BuildErrorResult( | ||
"InvalidArguments", | ||
"No entity found with the given key.", |
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.
"No entity found with the given key.", | |
"No record found with the given key.", |
valueArray.ValueKind == JsonValueKind.Array && | ||
valueArray.GetArrayLength() > 0) | ||
{ | ||
JsonElement firstItem = valueArray[0]; |
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.
This is assuming there will only be one element in the Value Array.
Dictionary<string, object?> normalized = new() | ||
{ | ||
["status"] = "success", | ||
["result"] = filteredResult // only requested values |
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 are no longer sending back only the requested values. The comment should be updated.
IActionResult? mutationResult = null; | ||
try | ||
{ | ||
mutationResult = await mutationEngine.ExecuteAsync(context).ConfigureAwait(false); |
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.
This ExecuteAsync is specifically creating the response as expected in a Rest Request. We should understand it is an MCP response and craft it as per the MCP endpoint needs instead of deserializing and serializing again.
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 should use the SqlResponseHelper
to create a response that MCP endpoint needs to avoid the duplicate serialization and deserialization.
{ | ||
Content = new List<ContentBlock> | ||
{ | ||
new TextContentBlock { Type = "text", Text = output } |
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.
Should the tool call result be of Type text
or json
?
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.
Some comments can be addressed as a followup - e.g. testing with authenticated, using SqlResponseHelper to create the McpResponse.
But some need to be addressed now - e.g. what are the expected output fields? only keys or all the fields?
IServiceProvider serviceProvider, | ||
CancellationToken cancellationToken = default) | ||
{ | ||
ILogger<UpdateRecordTool>? logger = serviceProvider.GetService<ILogger<UpdateRecordTool>>(); |
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.
How do we ensure the configuration check to enable the tool is propagated to whether we expose the tool or not?
Why make this change?
This PR,
update-record
MCP tool inAzure.DataApiBuilder.Mcp
to support updating records via MCP using keys and field payloads with full validation and authorization.SqlUpdateQueryStructure.cs
to support mapped column fields.What is this change?
docs/Testing/mcp-inspector-testing.md
with step-by-step instructions for running MCP Inspector locally, bypassing TLS, and connecting to MCP endpoints.UpdateRecordTool.cs
under BuiltInTools, enabling record updates via MCP with full validation, authorization, and mutation engine integration.SqlUpdateQueryStructure.cs
to construct predicates using parameterized values for safer and more flexible SQL generation.Exceptions are thrown when:
Error:
How was this tested?
These scenarios were manually tested using MCP Inspector, as automated tests are not yet implemented.
Valid Cases
Failure Cases
Sample Request(s)
{ "id": "00000000-0000-0000-0000-000000000001" }
{ "title": "New Title", "order": 7 }