Added missing stored procedure parameters in describe_entities response.#3425
Added missing stored procedure parameters in describe_entities response.#3425anushakolan wants to merge 1 commit intomainfrom
Conversation
anushakolan
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
aaronburtle,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
There was a problem hiding this comment.
Pull request overview
Updates MCP describe_entities to reliably include complete stored procedure parameter metadata by augmenting/merging config-defined parameters with runtime database metadata, and adds targeted tests to validate the behavior.
Changes:
- Resolve stored procedure parameter metadata from runtime DB metadata when config parameters are missing/partial.
- Merge config + DB parameter metadata, preferring config values for overlapping fields.
- Add MCP unit tests covering metadata-only discovery and config-overrides-DB merging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs | Adds metadata resolution for stored procedure entities and merges DB/config parameter metadata in describe_entities output. |
| src/Service.Tests/Mcp/DescribeEntitiesStoredProcedureParametersTests.cs | Adds unit tests validating parameter discovery from DB metadata and config-overrides behavior. |
| foreach ((string parameterName, ParameterDefinition parameterDefinition) in storedProcedure.StoredProcedureDefinition.Parameters) | ||
| { | ||
| configParameters.TryGetValue(parameterName, out ParameterMetadata? configParameter); | ||
|
|
||
| Dictionary<string, object?> paramInfo = new() | ||
| { | ||
| ["name"] = configParameter?.Name ?? parameterName, | ||
| ["required"] = configParameter?.Required ?? parameterDefinition.Required ?? false, | ||
| ["default"] = configParameter?.Default ?? parameterDefinition.Default, | ||
| ["description"] = configParameter?.Description ?? parameterDefinition.Description ?? string.Empty | ||
| }; | ||
|
|
||
| result.Add(paramInfo); | ||
| } | ||
|
|
||
| // Preserve config-only parameters if metadata is not available for a configured name. | ||
| foreach (ParameterMetadata configParameter in configParameters.Values) | ||
| { | ||
| if (!storedProcedure.StoredProcedureDefinition.Parameters.ContainsKey(configParameter.Name)) | ||
| { | ||
| Dictionary<string, object?> paramInfo = new() | ||
| { | ||
| ["name"] = configParameter.Name, | ||
| ["required"] = configParameter.Required, | ||
| ["default"] = configParameter.Default, | ||
| ["description"] = configParameter.Description ?? string.Empty | ||
| }; | ||
|
|
||
| result.Add(paramInfo); | ||
| } | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
BuildParameterMetadataInfo assumes storedProcedure.StoredProcedureDefinition (and its Parameters) is non-null. DatabaseStoredProcedure.StoredProcedureDefinition is declared nullable-forgiving (null!) and there are code paths/tests that construct DatabaseStoredProcedure without setting it, which would throw here and cause the entity to be omitted from the response. Guard for null (or empty) StoredProcedureDefinition/Parameters and fall back to config parameters when it isn't available.
| foreach ((string parameterName, ParameterDefinition parameterDefinition) in storedProcedure.StoredProcedureDefinition.Parameters) | |
| { | |
| configParameters.TryGetValue(parameterName, out ParameterMetadata? configParameter); | |
| Dictionary<string, object?> paramInfo = new() | |
| { | |
| ["name"] = configParameter?.Name ?? parameterName, | |
| ["required"] = configParameter?.Required ?? parameterDefinition.Required ?? false, | |
| ["default"] = configParameter?.Default ?? parameterDefinition.Default, | |
| ["description"] = configParameter?.Description ?? parameterDefinition.Description ?? string.Empty | |
| }; | |
| result.Add(paramInfo); | |
| } | |
| // Preserve config-only parameters if metadata is not available for a configured name. | |
| foreach (ParameterMetadata configParameter in configParameters.Values) | |
| { | |
| if (!storedProcedure.StoredProcedureDefinition.Parameters.ContainsKey(configParameter.Name)) | |
| { | |
| Dictionary<string, object?> paramInfo = new() | |
| { | |
| ["name"] = configParameter.Name, | |
| ["required"] = configParameter.Required, | |
| ["default"] = configParameter.Default, | |
| ["description"] = configParameter.Description ?? string.Empty | |
| }; | |
| result.Add(paramInfo); | |
| } | |
| } | |
| return result; | |
| IReadOnlyDictionary<string, ParameterDefinition>? storedProcedureParameters = | |
| storedProcedure.StoredProcedureDefinition?.Parameters; | |
| if (storedProcedureParameters is not null && storedProcedureParameters.Count > 0) | |
| { | |
| foreach ((string parameterName, ParameterDefinition parameterDefinition) in storedProcedureParameters) | |
| { | |
| configParameters.TryGetValue(parameterName, out ParameterMetadata? configParameter); | |
| Dictionary<string, object?> paramInfo = new() | |
| { | |
| ["name"] = configParameter?.Name ?? parameterName, | |
| ["required"] = configParameter?.Required ?? parameterDefinition.Required ?? false, | |
| ["default"] = configParameter?.Default ?? parameterDefinition.Default, | |
| ["description"] = configParameter?.Description ?? parameterDefinition.Description ?? string.Empty | |
| }; | |
| result.Add(paramInfo); | |
| } | |
| // Preserve config-only parameters if metadata is not available for a configured name. | |
| foreach (ParameterMetadata configParameter in configParameters.Values) | |
| { | |
| if (!storedProcedureParameters.ContainsKey(configParameter.Name)) | |
| { | |
| Dictionary<string, object?> paramInfo = new() | |
| { | |
| ["name"] = configParameter.Name, | |
| ["required"] = configParameter.Required, | |
| ["default"] = configParameter.Default, | |
| ["description"] = configParameter.Description ?? string.Empty | |
| }; | |
| result.Add(paramInfo); | |
| } | |
| } | |
| return result; | |
| } |
| // Preserve config-only parameters if metadata is not available for a configured name. | ||
| foreach (ParameterMetadata configParameter in configParameters.Values) | ||
| { | ||
| if (!storedProcedure.StoredProcedureDefinition.Parameters.ContainsKey(configParameter.Name)) | ||
| { | ||
| Dictionary<string, object?> paramInfo = new() |
There was a problem hiding this comment.
The config-only parameter preservation check uses Parameters.ContainsKey(configParameter.Name), but StoredProcedureDefinition.Parameters is a default Dictionary<string, ParameterDefinition> (case-sensitive). Since configParameters matching earlier is case-insensitive, a casing difference (e.g., "Id" vs "id") can cause duplicates to be added. Use a case-insensitive key comparer for Parameters or perform the contains check with OrdinalIgnoreCase.
| Dictionary<string, object?> paramInfo = new() | ||
| { | ||
| ["name"] = configParameter?.Name ?? parameterName, | ||
| ["required"] = configParameter?.Required ?? parameterDefinition.Required ?? false, |
There was a problem hiding this comment.
Merging Required prefers configParameter.Required whenever a parameter is present in config, but ParameterMetadata.Required is a non-nullable bool so an omitted "required" in config deserializes to false and will override DB metadata (potentially marking required DB parameters as optional). If the intended behavior is "config overrides only when explicitly set", Required likely needs to become nullable (bool?) or track a user-provided flag and only override when provided.
| ["required"] = configParameter?.Required ?? parameterDefinition.Required ?? false, | |
| ["required"] = parameterDefinition.Required ?? configParameter?.Required ?? false, |
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
Want to know reasoning behind why the bug happened.
| return null; | ||
| } | ||
|
|
||
| IMetadataProviderFactory? metadataProviderFactory = serviceProvider.GetService<IMetadataProviderFactory>(); |
There was a problem hiding this comment.
where is the metadataProviderFactory used?
|
|
||
| if (databaseObject is DatabaseStoredProcedure storedProcedure) | ||
| { | ||
| foreach ((string parameterName, ParameterDefinition parameterDefinition) in storedProcedure.StoredProcedureDefinition.Parameters) |
There was a problem hiding this comment.
What was the reason again to have two different data structures? ParameterDefinitionand ParameterMetadata? why couldnt we have used ParameterDefinition for the config parameters?
Why make this change?
Closes #3400.
describe_entitiescould return stored procedure entities with empty or partialparametersmetadata when parameters were not fully listed in config. This caused MCP clients/agents to miss required inputs and failexecute_entitycalls.Additional discussion: #3400 issue thread.
What is this change?
DescribeEntitiesToolto include stored procedure parameters from runtime DB metadata.required,default,description).How was this tested?
Ran:
Sample Request(s)
MCP request example:
describe_entitieswith full metadata:{"name":"describe_entities","arguments":{}}execute_entityusing discovered parameters:{"name":"execute_entity","arguments":{"entity":"GetBook","parameters":{"id":1}}}