-
Notifications
You must be signed in to change notification settings - Fork 330
Added missing stored procedure parameters in describe_entities response. #3425
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| using System.Text.Json; | ||
| using Azure.DataApiBuilder.Auth; | ||
| using Azure.DataApiBuilder.Config.DatabasePrimitives; | ||
| using Azure.DataApiBuilder.Config.ObjectModel; | ||
| using Azure.DataApiBuilder.Core.Authorization; | ||
| using Azure.DataApiBuilder.Core.Configurations; | ||
|
|
@@ -179,7 +180,7 @@ public Task<CallToolResult> ExecuteAsync( | |
| { | ||
| Dictionary<string, object?> entityInfo = nameOnly | ||
| ? BuildBasicEntityInfo(entityName, entity) | ||
| : BuildFullEntityInfo(entityName, entity, currentUserRole); | ||
| : BuildFullEntityInfo(entityName, entity, currentUserRole, TryResolveDatabaseObject(entityName, entity, runtimeConfig, serviceProvider, logger, cancellationToken)); | ||
|
|
||
| entityList.Add(entityInfo); | ||
| } | ||
|
|
@@ -403,10 +404,11 @@ private static bool ShouldIncludeEntity(string entityName, HashSet<string>? enti | |
| /// <param name="entityName">The name of the entity to include in the dictionary.</param> | ||
| /// <param name="entity">The entity object from which to extract additional information.</param> | ||
| /// <param name="currentUserRole">The role of the current user, used to determine permissions.</param> | ||
| /// <param name="databaseObject">The resolved database object metadata if available.</param> | ||
| /// <returns> | ||
| /// A dictionary containing the entity's name, description, fields, parameters (if applicable), and permissions. | ||
| /// </returns> | ||
| private static Dictionary<string, object?> BuildFullEntityInfo(string entityName, Entity entity, string? currentUserRole) | ||
| private static Dictionary<string, object?> BuildFullEntityInfo(string entityName, Entity entity, string? currentUserRole, DatabaseObject? databaseObject) | ||
| { | ||
| // Use GraphQL singular name as alias if available, otherwise use entity name | ||
| string displayName = !string.IsNullOrWhiteSpace(entity.GraphQL?.Singular) | ||
|
|
@@ -422,14 +424,47 @@ private static bool ShouldIncludeEntity(string entityName, HashSet<string>? enti | |
|
|
||
| if (entity.Source.Type == EntitySourceType.StoredProcedure) | ||
| { | ||
| info["parameters"] = BuildParameterMetadataInfo(entity.Source.Parameters); | ||
| info["parameters"] = BuildParameterMetadataInfo(entity.Source.Parameters, databaseObject); | ||
| } | ||
|
|
||
| info["permissions"] = BuildPermissionsInfo(entity, currentUserRole); | ||
|
|
||
| return info; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tries to resolve the metadata-backed database object for an entity. | ||
| /// </summary> | ||
| private static DatabaseObject? TryResolveDatabaseObject( | ||
| string entityName, | ||
| Entity entity, | ||
| RuntimeConfig runtimeConfig, | ||
| IServiceProvider serviceProvider, | ||
| ILogger? logger, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| if (entity.Source.Type != EntitySourceType.StoredProcedure) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| if (McpMetadataHelper.TryResolveMetadata( | ||
| entityName, | ||
| runtimeConfig, | ||
| serviceProvider, | ||
| out _, | ||
| out DatabaseObject dbObject, | ||
| out _, | ||
| out string error, | ||
| cancellationToken)) | ||
| { | ||
| return dbObject; | ||
| } | ||
|
|
||
| logger?.LogDebug("Could not resolve metadata for stored procedure entity {EntityName}. Falling back to config parameters. Error: {Error}", entityName, error); | ||
| return null; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Builds a list of metadata information objects from the provided collection of fields. | ||
| /// </summary> | ||
|
|
@@ -461,10 +496,66 @@ private static List<object> BuildFieldMetadataInfo(List<FieldMetadata>? fields) | |
| /// <param name="parameters">A list of <see cref="ParameterMetadata"/> objects representing the parameters to process. Can be null.</param> | ||
| /// <returns>A list of dictionaries, each containing the parameter's name, whether it is required, its default | ||
| /// value, and its description. Returns an empty list if <paramref name="parameters"/> is null.</returns> | ||
| private static List<object> BuildParameterMetadataInfo(List<ParameterMetadata>? parameters) | ||
| private static List<object> BuildParameterMetadataInfo(List<ParameterMetadata>? parameters, DatabaseObject? databaseObject) | ||
| { | ||
| List<object> result = new(); | ||
|
|
||
| Dictionary<string, ParameterMetadata> configParameters = new(StringComparer.OrdinalIgnoreCase); | ||
| if (parameters != null) | ||
| { | ||
| foreach (ParameterMetadata parameter in parameters) | ||
| { | ||
| configParameters[parameter.Name] = parameter; | ||
| } | ||
| } | ||
|
|
||
| if (databaseObject is DatabaseStoredProcedure storedProcedure) | ||
| { | ||
| IReadOnlyDictionary<string, ParameterDefinition>? storedProcedureParameters = | ||
| storedProcedure.StoredProcedureDefinition?.Parameters; | ||
|
|
||
| if (storedProcedureParameters is null || storedProcedureParameters.Count == 0) | ||
| { | ||
| // No runtime metadata available for this stored procedure. Fall back to config-only parameters. | ||
| return BuildParameterMetadataInfo(parameters, databaseObject: null); | ||
| } | ||
|
|
||
| foreach ((string parameterName, ParameterDefinition parameterDefinition) in storedProcedureParameters) | ||
| { | ||
| configParameters.TryGetValue(parameterName, out ParameterMetadata? configParameter); | ||
|
|
||
| Dictionary<string, object?> paramInfo = new() | ||
| { | ||
| ["name"] = configParameter?.Name ?? parameterName, | ||
| ["required"] = parameterDefinition.Required ?? configParameter?.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) && | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why cant the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, all There might a validation check to ensure this. |
||
| !storedProcedureParameters.Keys.Any(k => string.Equals(k, configParameter.Name, StringComparison.OrdinalIgnoreCase))) | ||
| { | ||
| Dictionary<string, object?> paramInfo = new() | ||
|
anushakolan marked this conversation as resolved.
|
||
| { | ||
| ["name"] = configParameter.Name, | ||
| ["required"] = configParameter.Required, | ||
| ["default"] = configParameter.Default, | ||
| ["description"] = configParameter.Description ?? string.Empty | ||
| }; | ||
|
|
||
| result.Add(paramInfo); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| if (parameters != null) | ||
| { | ||
| foreach (ParameterMetadata param in parameters) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,204 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text.Json; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Azure.DataApiBuilder.Auth; | ||
| using Azure.DataApiBuilder.Config.DatabasePrimitives; | ||
| using Azure.DataApiBuilder.Config.ObjectModel; | ||
| using Azure.DataApiBuilder.Core.Authorization; | ||
| using Azure.DataApiBuilder.Core.Configurations; | ||
| using Azure.DataApiBuilder.Core.Services; | ||
| using Azure.DataApiBuilder.Core.Services.MetadataProviders; | ||
| using Azure.DataApiBuilder.Mcp.BuiltInTools; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| using ModelContextProtocol.Protocol; | ||
| using Moq; | ||
|
|
||
| namespace Azure.DataApiBuilder.Service.Tests.Mcp | ||
| { | ||
| [TestClass] | ||
| public class DescribeEntitiesStoredProcedureParametersTests | ||
| { | ||
| [TestMethod] | ||
| public async Task DescribeEntities_IncludesStoredProcedureParametersFromMetadata_WhenConfigParametersMissing() | ||
| { | ||
| RuntimeConfig config = CreateRuntimeConfig(CreateStoredProcedureEntity(parameters: null)); | ||
| ServiceCollection services = new(); | ||
| RegisterCommonServices(services, config); | ||
| RegisterMetadataProvider(services, "GetBook", CreateStoredProcedureObject("dbo", "get_book", new Dictionary<string, ParameterDefinition> | ||
| { | ||
| ["id"] = new() { Required = true, Description = "Book id" }, | ||
| ["locale"] = new() { Required = false, Default = "en-US", Description = "Locale" } | ||
| })); | ||
|
|
||
| IServiceProvider serviceProvider = services.BuildServiceProvider(); | ||
| DescribeEntitiesTool tool = new(); | ||
|
|
||
| CallToolResult result = await tool.ExecuteAsync(null, serviceProvider, CancellationToken.None); | ||
|
|
||
| Assert.IsTrue(result.IsError == false || result.IsError == null); | ||
|
|
||
| JsonElement entity = GetSingleEntityFromResult(result); | ||
| JsonElement parameters = entity.GetProperty("parameters"); | ||
| Assert.AreEqual(2, parameters.GetArrayLength(), "Stored procedure parameters should be sourced from metadata when not specified in config."); | ||
|
|
||
| JsonElement idParam = parameters.EnumerateArray().Single(p => p.GetProperty("name").GetString() == "id"); | ||
| Assert.IsTrue(idParam.GetProperty("required").GetBoolean()); | ||
| Assert.AreEqual("Book id", idParam.GetProperty("description").GetString()); | ||
|
|
||
| JsonElement localeParam = parameters.EnumerateArray().Single(p => p.GetProperty("name").GetString() == "locale"); | ||
| Assert.AreEqual("en-US", localeParam.GetProperty("default").GetString()); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task DescribeEntities_ConfigParameterMetadataOverridesDatabaseParameterMetadata() | ||
| { | ||
| List<ParameterMetadata> configuredParameters = new() | ||
| { | ||
| new() { Name = "id", Required = true, Default = "42", Description = "Config description" } | ||
| }; | ||
|
|
||
| RuntimeConfig config = CreateRuntimeConfig(CreateStoredProcedureEntity(parameters: configuredParameters)); | ||
| ServiceCollection services = new(); | ||
| RegisterCommonServices(services, config); | ||
| RegisterMetadataProvider(services, "GetBook", CreateStoredProcedureObject("dbo", "get_book", new Dictionary<string, ParameterDefinition> | ||
| { | ||
| ["id"] = new() { Required = false, Default = "1", Description = "Database description" }, | ||
| ["tenant"] = new() { Required = true, Description = "Tenant from DB" } | ||
| })); | ||
|
|
||
| IServiceProvider serviceProvider = services.BuildServiceProvider(); | ||
| DescribeEntitiesTool tool = new(); | ||
|
|
||
| CallToolResult result = await tool.ExecuteAsync(null, serviceProvider, CancellationToken.None); | ||
|
|
||
| Assert.IsTrue(result.IsError == false || result.IsError == null); | ||
|
|
||
| JsonElement entity = GetSingleEntityFromResult(result); | ||
| JsonElement parameters = entity.GetProperty("parameters"); | ||
| Assert.AreEqual(2, parameters.GetArrayLength()); | ||
|
|
||
| JsonElement idParam = parameters.EnumerateArray().Single(p => p.GetProperty("name").GetString() == "id"); | ||
| Assert.IsFalse(idParam.GetProperty("required").GetBoolean(), "DB required metadata should be preferred when config cannot indicate whether 'required' was explicitly set."); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test is titled DescribeEntities_ConfigParameterMetadataOverridesDatabaseParameterMetadata Assert should be Assert.IsTrue |
||
| Assert.AreEqual("Config description", idParam.GetProperty("description").GetString(), "Config description should override DB metadata."); | ||
| Assert.AreEqual("42", idParam.GetProperty("default").ToString(), "Config default should override DB metadata."); | ||
|
|
||
| JsonElement tenantParam = parameters.EnumerateArray().Single(p => p.GetProperty("name").GetString() == "tenant"); | ||
| Assert.IsTrue(tenantParam.GetProperty("required").GetBoolean()); | ||
| Assert.AreEqual("Tenant from DB", tenantParam.GetProperty("description").GetString()); | ||
| } | ||
|
|
||
| private static RuntimeConfig CreateRuntimeConfig(Entity storedProcedureEntity) | ||
| { | ||
| Dictionary<string, Entity> entities = new() | ||
| { | ||
| ["GetBook"] = storedProcedureEntity | ||
| }; | ||
|
|
||
| return new RuntimeConfig( | ||
| Schema: "test-schema", | ||
| DataSource: new DataSource(DatabaseType: DatabaseType.MSSQL, ConnectionString: "", Options: null), | ||
| Runtime: new( | ||
| Rest: new(), | ||
| GraphQL: new(), | ||
| Mcp: new(Enabled: true, Path: "/mcp", DmlTools: null), | ||
| Host: new(Cors: null, Authentication: null, Mode: HostMode.Development) | ||
| ), | ||
| Entities: new(entities) | ||
| ); | ||
| } | ||
|
|
||
| private static Entity CreateStoredProcedureEntity(List<ParameterMetadata> parameters) | ||
| { | ||
| return new Entity( | ||
| Source: new("get_book", EntitySourceType.StoredProcedure, parameters, null), | ||
| GraphQL: new("GetBook", "GetBook"), | ||
| Rest: new(Enabled: true), | ||
| Fields: null, | ||
| Permissions: new[] | ||
| { | ||
| new EntityPermission( | ||
| Role: "anonymous", | ||
| Actions: new[] | ||
| { | ||
| new EntityAction(Action: EntityActionOperation.Execute, Fields: null, Policy: null) | ||
| }) | ||
| }, | ||
| Relationships: null, | ||
| Mappings: null, | ||
| Mcp: null | ||
| ); | ||
| } | ||
|
|
||
| private static DatabaseStoredProcedure CreateStoredProcedureObject( | ||
| string schema, | ||
| string name, | ||
| Dictionary<string, ParameterDefinition> parameters) | ||
| { | ||
| return new DatabaseStoredProcedure(schema, name) | ||
| { | ||
| SourceType = EntitySourceType.StoredProcedure, | ||
| StoredProcedureDefinition = new StoredProcedureDefinition | ||
| { | ||
| Parameters = parameters | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private static void RegisterCommonServices(ServiceCollection services, RuntimeConfig config) | ||
| { | ||
| RuntimeConfigProvider configProvider = TestHelper.GenerateInMemoryRuntimeConfigProvider(config); | ||
| services.AddSingleton(configProvider); | ||
|
|
||
| Mock<IAuthorizationResolver> mockAuthResolver = new(); | ||
| mockAuthResolver.Setup(x => x.IsValidRoleContext(It.IsAny<HttpContext>())).Returns(true); | ||
| services.AddSingleton(mockAuthResolver.Object); | ||
|
|
||
| Mock<HttpContext> mockHttpContext = new(); | ||
| Mock<HttpRequest> mockRequest = new(); | ||
| mockRequest.Setup(x => x.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns("anonymous"); | ||
| mockHttpContext.Setup(x => x.Request).Returns(mockRequest.Object); | ||
|
|
||
| Mock<IHttpContextAccessor> mockHttpContextAccessor = new(); | ||
| mockHttpContextAccessor.Setup(x => x.HttpContext).Returns(mockHttpContext.Object); | ||
| services.AddSingleton(mockHttpContextAccessor.Object); | ||
|
|
||
| services.AddLogging(); | ||
| } | ||
|
|
||
| private static void RegisterMetadataProvider(ServiceCollection services, string entityName, DatabaseObject dbObject) | ||
| { | ||
| Mock<ISqlMetadataProvider> mockSqlMetadataProvider = new(); | ||
| mockSqlMetadataProvider.Setup(x => x.EntityToDatabaseObject).Returns(new Dictionary<string, DatabaseObject> | ||
| { | ||
| [entityName] = dbObject | ||
| }); | ||
| mockSqlMetadataProvider.Setup(x => x.GetDatabaseType()).Returns(DatabaseType.MSSQL); | ||
|
|
||
| Mock<IMetadataProviderFactory> mockMetadataProviderFactory = new(); | ||
| mockMetadataProviderFactory.Setup(x => x.GetMetadataProvider(It.IsAny<string>())).Returns(mockSqlMetadataProvider.Object); | ||
| services.AddSingleton(mockMetadataProviderFactory.Object); | ||
| } | ||
|
|
||
| private static JsonElement GetSingleEntityFromResult(CallToolResult result) | ||
| { | ||
| Assert.IsNotNull(result.Content); | ||
| Assert.IsTrue(result.Content.Count > 0); | ||
| Assert.IsInstanceOfType(result.Content[0], typeof(TextContentBlock)); | ||
|
|
||
| TextContentBlock firstContent = (TextContentBlock)result.Content[0]; | ||
| JsonElement root = JsonDocument.Parse(firstContent.Text!).RootElement; | ||
| JsonElement entities = root.GetProperty("entities"); | ||
|
|
||
| Assert.AreEqual(1, entities.GetArrayLength()); | ||
| return entities.EnumerateArray().First(); | ||
| } | ||
| } | ||
| } | ||
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.
PR description says -
Merged DB metadata with config metadata so config values override matching fields (required, default, description).However, this is not what is implemented.
With the current implementation, config values will never be added to the result since ALL config parameters should be discoverable in storedProcedureParameters by definition.
But the current check
!storedProcedureParameters.ContainsKey(configParameter.Name)tries to identify the missing ones.Config metadata should only be used to override, which means after reading params from DB metadata, the config values should be used to replace the values from DB.