-
Notifications
You must be signed in to change notification settings - Fork 301
[MCP] describe_entities tool fixes and refactoring #2900
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
Conversation
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 renames the describe-entities tool to describe_entities and enhances its functionality to provide comprehensive entity metadata including descriptions, fields for stored procedures, and permissions.
- Renamed tool from
describe-entitiestodescribe_entitiesacross configuration and implementation - Enhanced entity response to include descriptions, fields for stored procedures, and permissions
- Added proper documentation and error handling to the tool implementation
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Config/ObjectModel/DmlToolsConfig.cs | Updated comments to reflect the new tool name |
| src/Config/Converters/DmlToolsConfigConverter.cs | Updated JSON property name and validation logic |
| src/Cli/ConfigGenerator.cs | Updated logging message to use new tool name |
| src/Cli/Commands/ConfigureOptions.cs | Updated CLI option name for the tool |
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs | Major refactoring with enhanced functionality and documentation |
| schemas/dab.draft.schema.json | Updated JSON schema property name and description |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs
Outdated
Show resolved
Hide resolved
src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs
Outdated
Show resolved
Hide resolved
This reverts commit eeac569.
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. I will test this with Claude to see if there are any other errors.
…://github.com/Azure/data-api-builder into dev/anushakolan/mcp/add-field-description-new
…scription-new' into Usr/sogh/describe-entities
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
RubenCerna2079
left a comment
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!
The changes have been made
| { | ||
| ["entities"] = finalEntityList, | ||
| ["count"] = finalEntityList.Count, | ||
| ["mode"] = nameOnly ? "basic" : "full" |
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 mode in the response was not expected as per the spec. Why do we need to add it? Without looking at code, I didnt realize basic meant nameOnly. We should remove this from the response unless we confirm value from adding this.
| ["mode"] = nameOnly ? "basic" : "full" | ||
| }; | ||
|
|
||
| if (entityFilter != null && entityFilter.Count > 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.
Why is the entityFilter needed in the response? The spec doesnt ask for it
| logger?.LogInformation( | ||
| "DescribeEntitiesTool returned {EntityCount} entities in {Mode} mode.", | ||
| finalEntityList.Count, | ||
| nameOnly ? "basic" : "full"); |
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.
Are the terms basic and full defined anywhere in the spec? We should log nameOnly or not instead of these terms because they are not self explanatory.
| { | ||
| Content = [new TextContentBlock { Type = "text", Text = $"Error: {ex.Message}" }] | ||
| }); | ||
| foreach (EntityAction action in permission.Actions) |
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 limit the actions that are added to the permissions HashSet to be only those actions of the role in which this request was made.
| { | ||
| result.Add(new | ||
| { | ||
| name = field.Name, |
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 should be sending back the alias not the name. Onlyu if alias is absent, should it send back the name.
| { | ||
| name = param.Name, | ||
| required = param.Default == null, // required if no default | ||
| @default = param.Default, |
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 the expected response key @default? why not simply default?
… describe-entities MCP tool (#2956) ## Why make this change? - Addresses follow ups to PR #2900 The `describe_entities` tool response format needed improvements to better align with MCP specifications and provide more accurate, user-scoped information. Key issues included non-specification compliant response fields, overly broad permission reporting across all roles, and inconsistent entity/field naming conventions that didn't prioritize user-friendly aliases. ## What is this change? - **Removed non-spec fields from response**: Eliminated `mode` and `filter` fields that were not part of the MCP specification - **Scoped permissions to current user's role**: Modified permissions logic to only return permissions available to the requesting user's role instead of all permissions across all roles - **Implemented entity alias support**: Updated entity name resolution to prefer GraphQL singular names (aliases) over configuration names, falling back to entity name only when alias is absent - **Fixed parameter metadata format**: Changed parameter default value key from `@default` to `default` in JSON response - **Enhanced field name resolution**: Updated field metadata to use field aliases when available, falling back to field names when aliases are absent - **Added proper authorization context**: Integrated HTTP context and authorization resolver to determine current user's role for permission filtering ## How was this tested? - [x] Manual Tests ## Sample Request(s) ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities" }, "id": 1 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "nameOnly": true } }, "id": 2 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "entities": ["Book", "Publisher"] } }, "id": 1 } ```
… describe-entities MCP tool (#2956) ## Why make this change? - Addresses follow ups to PR #2900 The `describe_entities` tool response format needed improvements to better align with MCP specifications and provide more accurate, user-scoped information. Key issues included non-specification compliant response fields, overly broad permission reporting across all roles, and inconsistent entity/field naming conventions that didn't prioritize user-friendly aliases. ## What is this change? - **Removed non-spec fields from response**: Eliminated `mode` and `filter` fields that were not part of the MCP specification - **Scoped permissions to current user's role**: Modified permissions logic to only return permissions available to the requesting user's role instead of all permissions across all roles - **Implemented entity alias support**: Updated entity name resolution to prefer GraphQL singular names (aliases) over configuration names, falling back to entity name only when alias is absent - **Fixed parameter metadata format**: Changed parameter default value key from `@default` to `default` in JSON response - **Enhanced field name resolution**: Updated field metadata to use field aliases when available, falling back to field names when aliases are absent - **Added proper authorization context**: Integrated HTTP context and authorization resolver to determine current user's role for permission filtering ## How was this tested? - [x] Manual Tests ## Sample Request(s) ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities" }, "id": 1 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "nameOnly": true } }, "id": 2 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "entities": ["Book", "Publisher"] } }, "id": 1 } ```
… describe-entities MCP tool (#2956) ## Why make this change? - Addresses follow ups to PR #2900 The `describe_entities` tool response format needed improvements to better align with MCP specifications and provide more accurate, user-scoped information. Key issues included non-specification compliant response fields, overly broad permission reporting across all roles, and inconsistent entity/field naming conventions that didn't prioritize user-friendly aliases. ## What is this change? - **Removed non-spec fields from response**: Eliminated `mode` and `filter` fields that were not part of the MCP specification - **Scoped permissions to current user's role**: Modified permissions logic to only return permissions available to the requesting user's role instead of all permissions across all roles - **Implemented entity alias support**: Updated entity name resolution to prefer GraphQL singular names (aliases) over configuration names, falling back to entity name only when alias is absent - **Fixed parameter metadata format**: Changed parameter default value key from `@default` to `default` in JSON response - **Enhanced field name resolution**: Updated field metadata to use field aliases when available, falling back to field names when aliases are absent - **Added proper authorization context**: Integrated HTTP context and authorization resolver to determine current user's role for permission filtering ## How was this tested? - [x] Manual Tests ## Sample Request(s) ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities" }, "id": 1 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "nameOnly": true } }, "id": 2 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "entities": ["Book", "Publisher"] } }, "id": 1 } ```
… describe-entities MCP tool (#2956) ## Why make this change? - Addresses follow ups to PR #2900 The `describe_entities` tool response format needed improvements to better align with MCP specifications and provide more accurate, user-scoped information. Key issues included non-specification compliant response fields, overly broad permission reporting across all roles, and inconsistent entity/field naming conventions that didn't prioritize user-friendly aliases. ## What is this change? - **Removed non-spec fields from response**: Eliminated `mode` and `filter` fields that were not part of the MCP specification - **Scoped permissions to current user's role**: Modified permissions logic to only return permissions available to the requesting user's role instead of all permissions across all roles - **Implemented entity alias support**: Updated entity name resolution to prefer GraphQL singular names (aliases) over configuration names, falling back to entity name only when alias is absent - **Fixed parameter metadata format**: Changed parameter default value key from `@default` to `default` in JSON response - **Enhanced field name resolution**: Updated field metadata to use field aliases when available, falling back to field names when aliases are absent - **Added proper authorization context**: Integrated HTTP context and authorization resolver to determine current user's role for permission filtering ## How was this tested? - [x] Manual Tests ## Sample Request(s) ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities" }, "id": 1 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "nameOnly": true } }, "id": 2 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "entities": ["Book", "Publisher"] } }, "id": 1 } ```
… describe-entities MCP tool (#2956) ## Why make this change? - Addresses follow ups to PR #2900 The `describe_entities` tool response format needed improvements to better align with MCP specifications and provide more accurate, user-scoped information. Key issues included non-specification compliant response fields, overly broad permission reporting across all roles, and inconsistent entity/field naming conventions that didn't prioritize user-friendly aliases. ## What is this change? - **Removed non-spec fields from response**: Eliminated `mode` and `filter` fields that were not part of the MCP specification - **Scoped permissions to current user's role**: Modified permissions logic to only return permissions available to the requesting user's role instead of all permissions across all roles - **Implemented entity alias support**: Updated entity name resolution to prefer GraphQL singular names (aliases) over configuration names, falling back to entity name only when alias is absent - **Fixed parameter metadata format**: Changed parameter default value key from `@default` to `default` in JSON response - **Enhanced field name resolution**: Updated field metadata to use field aliases when available, falling back to field names when aliases are absent - **Added proper authorization context**: Integrated HTTP context and authorization resolver to determine current user's role for permission filtering ## How was this tested? - [x] Manual Tests ## Sample Request(s) ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities" }, "id": 1 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "nameOnly": true } }, "id": 2 } ``` ``` POST http://localhost:5000/mcp { "jsonrpc": "2.0", "method": "tools/call", "params": { "name": "describe_entities", "arguments": { "entities": ["Book", "Publisher"] } }, "id": 1 } ```
Why make this change?
Closes on
describe_entities#2827Added fixes and refactored the describe_entities tool to support entity metadata discovery for AI agents and LLM clients before performing CRUD operations.
What is this change?
How was this tested?
Functional testing using Insomnia client by running DAB in localhost and local SQL DB database
Expected (In-progress) format of entity description in response-
Sample Request(s)