diff --git a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs index 00875d2596..e40be63bbd 100644 --- a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs +++ b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs @@ -88,8 +88,7 @@ public class AggregateRecordsTool : IMcpTool ""orderby"": { ""type"": ""string"", ""enum"": [""asc"", ""desc""], - ""description"": ""Sort grouped results by the aggregated value. Requires groupby."", - ""default"": ""desc"" + ""description"": ""Sort direction for grouped results by the aggregated value. Only applies when groupby is provided; ignored otherwise."" }, ""having"": { ""type"": ""object"", @@ -394,23 +393,10 @@ public async Task ExecuteAsync( // Parse filter string? filter = root.TryGetProperty("filter", out JsonElement filterElement) ? filterElement.GetString() : null; - // Parse orderby + // Parse orderby (validation deferred until after groupby is known; + // if groupby is absent, orderby is silently ignored per #3279) bool userProvidedOrderby = root.TryGetProperty("orderby", out JsonElement orderbyElement) && !string.IsNullOrWhiteSpace(orderbyElement.GetString()); string orderby = "desc"; - if (userProvidedOrderby) - { - string normalizedOrderby = (orderbyElement.GetString() ?? string.Empty).Trim().ToLowerInvariant(); - if (normalizedOrderby != "asc" && normalizedOrderby != "desc") - { - return McpResponseBuilder.BuildErrorResult( - toolName, - "InvalidArguments", - $"Argument 'orderby' must be either 'asc' or 'desc' when provided. Got: '{orderbyElement.GetString()}'.", - logger); - } - - orderby = normalizedOrderby; - } // Parse first int? first = null; @@ -443,12 +429,28 @@ public async Task ExecuteAsync( // Validate groupby-dependent parameters CallToolResult? dependencyError = ValidateGroupByDependencies( - groupby.Count, userProvidedOrderby, first, after, toolName, logger); + groupby.Count, ref userProvidedOrderby, first, after, toolName, logger); if (dependencyError != null) { return dependencyError; } + // Validate orderby value only when groupby is present (orderby is ignored otherwise) + if (userProvidedOrderby) + { + string normalizedOrderby = (orderbyElement.GetString() ?? string.Empty).Trim().ToLowerInvariant(); + if (normalizedOrderby != "asc" && normalizedOrderby != "desc") + { + return McpResponseBuilder.BuildErrorResult( + toolName, + "InvalidArguments", + $"Argument 'orderby' must be either 'asc' or 'desc' when provided. Got: '{orderbyElement.GetString()}'.", + logger); + } + + orderby = normalizedOrderby; + } + // Parse having clause Dictionary? havingOperators = null; List? havingInValues = null; @@ -481,12 +483,13 @@ public async Task ExecuteAsync( } /// - /// Validates that parameters requiring groupby (orderby, first, after) are only used when groupby is present. + /// Validates that parameters requiring groupby (first, after) are only used when groupby is present. /// Also validates that 'after' requires 'first'. + /// Note: 'orderby' without groupby is silently ignored rather than rejected (see #3279). /// - private static CallToolResult? ValidateGroupByDependencies( + internal static CallToolResult? ValidateGroupByDependencies( int groupbyCount, - bool userProvidedOrderby, + ref bool userProvidedOrderby, int? first, string? after, string toolName, @@ -494,11 +497,10 @@ public async Task ExecuteAsync( { if (groupbyCount == 0) { - if (userProvidedOrderby) - { - return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", - "The 'orderby' parameter requires 'groupby' to be specified. Sorting applies to grouped aggregation results.", logger); - } + // Silently ignore orderby when groupby is not provided. + // LLMs may send orderby due to schema defaults; this is harmless + // since ordering is meaningless without grouping. + userProvidedOrderby = false; if (first.HasValue) { diff --git a/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs b/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs index 6401e17e22..bb3a1e5dba 100644 --- a/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs +++ b/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs @@ -22,7 +22,14 @@ public static IEndpointRouteBuilder MapDabMcp( RuntimeConfigProvider runtimeConfigProvider, [StringSyntax("Route")] string pattern = "") { - if (!TryGetMcpOptions(runtimeConfigProvider, out McpRuntimeOptions? mcpOptions) || mcpOptions == null || !mcpOptions.Enabled) + if (!runtimeConfigProvider.TryGetConfig(out RuntimeConfig? runtimeConfig)) + { + return endpoints; + } + + McpRuntimeOptions mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions(); + + if (!mcpOptions.Enabled) { return endpoints; } @@ -34,24 +41,5 @@ public static IEndpointRouteBuilder MapDabMcp( return endpoints; } - - /// - /// Gets MCP options from the runtime configuration - /// - /// Runtime config provider - /// MCP options - /// True if MCP options were found, false otherwise - private static bool TryGetMcpOptions(RuntimeConfigProvider runtimeConfigProvider, out McpRuntimeOptions? mcpOptions) - { - mcpOptions = null; - - if (!runtimeConfigProvider.TryGetConfig(out RuntimeConfig? runtimeConfig)) - { - return false; - } - - mcpOptions = runtimeConfig?.Runtime?.Mcp; - return mcpOptions != null; - } } } diff --git a/src/Cli/ConfigGenerator.cs b/src/Cli/ConfigGenerator.cs index 17f176791d..a1c212ed8c 100644 --- a/src/Cli/ConfigGenerator.cs +++ b/src/Cli/ConfigGenerator.cs @@ -2780,7 +2780,7 @@ public static bool IsConfigValid(ValidateOptions options, FileSystemRuntimeConfi { if (runtimeConfigProvider.TryGetConfig(out RuntimeConfig? config) && config is not null) { - bool mcpEnabled = config.Runtime?.Mcp?.Enabled == true; + bool mcpEnabled = config.IsMcpEnabled; if (mcpEnabled) { foreach (KeyValuePair entity in config.Entities) diff --git a/src/Core/Services/RestService.cs b/src/Core/Services/RestService.cs index 2bfab7e05f..5014d942bb 100644 --- a/src/Core/Services/RestService.cs +++ b/src/Core/Services/RestService.cs @@ -402,7 +402,8 @@ private bool TryGetStoredProcedureRESTVerbs(string entityName, [NotNullWhen(true /// does not match the configured REST path or the global REST endpoint is disabled. public string GetRouteAfterPathBase(string route) { - string configuredRestPathBase = _runtimeConfigProvider.GetConfig().RestPath; + RuntimeConfig runtimeConfig = _runtimeConfigProvider.GetConfig(); + string configuredRestPathBase = runtimeConfig.RestPath; // Strip the leading '/' from the REST path provided in the runtime configuration // because the input argument 'route' has no starting '/'. @@ -410,7 +411,8 @@ public string GetRouteAfterPathBase(string route) // forward slash '/'. configuredRestPathBase = configuredRestPathBase.Substring(1); - if (route.Equals(_runtimeConfigProvider.GetConfig().McpPath.Substring(1))) + if (route.Equals(runtimeConfig.McpPath.Substring(1)) + && !runtimeConfig.IsMcpEnabled) { throw new DataApiBuilderException( message: $"Route {route} was not found.", diff --git a/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs b/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs index eefa1f08f4..f706048107 100644 --- a/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs +++ b/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs @@ -191,6 +191,94 @@ public void TestBackwardCompatibilityDeserializationWithoutDescriptionField() Assert.IsNull(deserializedConfig.Runtime.Mcp.Description, "Description should be null when not present in JSON"); } + /// + /// When the mcp and runtime config blocks are omitted entirely, IsMcpEnabled should default to true. + /// + [TestMethod] + public void TestIsMcpEnabledDefaultsTrueWhenMcpAndRuntimeBlocksAbsent() + { + // Arrange - config with no runtime/mcp block + string configJson = @"{ + ""$schema"": ""test-schema"", + ""data-source"": { + ""database-type"": ""mssql"", + ""connection-string"": ""Server=test;Database=test;"" + }, + ""entities"": {} + }"; + + // Act + bool parseSuccess = RuntimeConfigLoader.TryParseConfig(configJson, out RuntimeConfig config); + + // Assert + Assert.IsTrue(parseSuccess); + Assert.IsNull(config.Runtime, "Runtime should be null when not specified"); + Assert.IsNull(config.Runtime?.Mcp, "Mcp should be null when not specified"); + Assert.IsTrue(config.IsMcpEnabled, "IsMcpEnabled should default to true when mcp block is absent"); + Assert.AreEqual(McpRuntimeOptions.DEFAULT_PATH, config.McpPath, "McpPath should return default when mcp block is absent"); + } + + /// + /// When the mcp config block is present but enabled is not specified, + /// IsMcpEnabled should default to true. + /// + [TestMethod] + public void TestIsMcpEnabledDefaultsTrueWhenEnabledNotSpecified() + { + // Arrange - config with mcp block but no enabled field + string configJson = @"{ + ""$schema"": ""test-schema"", + ""data-source"": { + ""database-type"": ""mssql"", + ""connection-string"": ""Server=test;Database=test;"" + }, + ""runtime"": { + ""mcp"": { + ""path"": ""/mcp"" + } + }, + ""entities"": {} + }"; + + // Act + bool parseSuccess = RuntimeConfigLoader.TryParseConfig(configJson, out RuntimeConfig config); + + // Assert + Assert.IsTrue(parseSuccess); + Assert.IsNotNull(config.Runtime?.Mcp); + Assert.IsTrue(config.IsMcpEnabled, "IsMcpEnabled should default to true when enabled is not specified"); + } + + /// + /// When mcp.enabled is explicitly set to false, IsMcpEnabled should return false. + /// + [TestMethod] + public void TestIsMcpEnabledReturnsFalseWhenExplicitlyDisabled() + { + // Arrange + string configJson = @"{ + ""$schema"": ""test-schema"", + ""data-source"": { + ""database-type"": ""mssql"", + ""connection-string"": ""Server=test;Database=test;"" + }, + ""runtime"": { + ""mcp"": { + ""enabled"": false + } + }, + ""entities"": {} + }"; + + // Act + bool parseSuccess = RuntimeConfigLoader.TryParseConfig(configJson, out RuntimeConfig config); + + // Assert + Assert.IsTrue(parseSuccess); + Assert.IsNotNull(config.Runtime?.Mcp); + Assert.IsFalse(config.IsMcpEnabled, "IsMcpEnabled should be false when explicitly disabled"); + } + /// /// Creates a minimal RuntimeConfig with the specified MCP options for testing. /// diff --git a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs index 2e94a7f3e1..cb7514d45e 100644 --- a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs +++ b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs @@ -171,8 +171,6 @@ public async Task AggregateRecords_InvalidFieldFunctionCombination_ReturnsInvali #region Input Validation Tests - GroupBy Dependencies [DataTestMethod] - [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"orderby\": \"desc\"}", "groupby", - DisplayName = "Orderby without groupby")] [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"having\": {\"gt\": 5}}", "groupby", DisplayName = "Having without groupby")] [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"groupby\": [\"title\"], \"orderby\": \"ascending\"}", "'asc' or 'desc'", @@ -190,6 +188,83 @@ public async Task AggregateRecords_GroupByDependencyViolation_ReturnsInvalidArgu #endregion + #region Input Validation Tests - Orderby Without Groupby (Issue #3279) + + /// + /// Verifies that orderby without groupby is silently ignored rather than rejected. + /// This is the core fix for https://github.com/Azure/data-api-builder/issues/3279. + /// The tool should pass input validation and only fail at metadata resolution (no real DB). + /// + [DataTestMethod] + [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"orderby\": \"desc\"}", + DisplayName = "count(*) with orderby desc, no groupby")] + [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"orderby\": \"asc\"}", + DisplayName = "count(*) with orderby asc, no groupby")] + [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"orderby\": \"ascending\"}", + DisplayName = "Invalid orderby value without groupby is silently ignored")] + [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"orderby\": \"garbage\"}", + DisplayName = "Nonsense orderby value without groupby is silently ignored")] + public async Task AggregateRecords_OrderbyWithoutGroupby_PassesValidation(string json) + { + IServiceProvider sp = CreateDefaultServiceProvider(); + + CallToolResult result = await ExecuteToolAsync(sp, json); + + // The tool may fail at metadata resolution (no real DB), but must NOT fail with InvalidArguments. + // If the tool succeeds, that's also acceptable — the test is focused on input validation. + if (result.IsError != true) + { + return; + } + + JsonElement content = ParseContent(result); + string errorType = content.GetProperty("error").GetProperty("type").GetString()!; + Assert.AreNotEqual("InvalidArguments", errorType, + $"orderby without groupby must not be rejected as InvalidArguments. Got error type: {errorType}"); + } + + /// + /// Verifies that simple count(*) without orderby or groupby passes validation. + /// + [TestMethod] + public async Task AggregateRecords_SimpleCountStar_PassesValidation() + { + IServiceProvider sp = CreateDefaultServiceProvider(); + + CallToolResult result = await ExecuteToolAsync(sp, + "{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\"}"); + + // If the tool succeeds, that's acceptable — the test is focused on input validation. + if (result.IsError != true) + { + return; + } + + JsonElement content = ParseContent(result); + string errorType = content.GetProperty("error").GetProperty("type").GetString()!; + Assert.AreNotEqual("InvalidArguments", errorType, + "Simple count(*) must pass input validation."); + } + + /// + /// Verifies that the orderby schema property has no default value (fix for #3279). + /// + [TestMethod] + public void GetToolMetadata_OrderbySchemaHasNoDefault() + { + AggregateRecordsTool tool = new(); + Tool metadata = tool.GetToolMetadata(); + + JsonElement properties = metadata.InputSchema.GetProperty("properties"); + JsonElement orderbyProp = properties.GetProperty("orderby"); + + Assert.IsFalse(orderbyProp.TryGetProperty("default", out _), + "The 'orderby' schema property must not have a default value. " + + "A default causes LLMs to always send orderby, which previously forced groupby to be required."); + } + + #endregion + #region Input Validation Tests - Having Clause [DataTestMethod] diff --git a/src/Service.Tests/UnitTests/RestServiceUnitTests.cs b/src/Service.Tests/UnitTests/RestServiceUnitTests.cs index ac7b3667e6..88a34b903d 100644 --- a/src/Service.Tests/UnitTests/RestServiceUnitTests.cs +++ b/src/Service.Tests/UnitTests/RestServiceUnitTests.cs @@ -167,6 +167,46 @@ public void SinglePathMatchingTest() #endregion + #region MCP Path Guard Tests + + /// + /// When MCP is explicitly disabled and the route matches the MCP path (default or custom), + /// GetRouteAfterPathBase should throw GlobalMcpEndpointDisabled. + /// + [DataTestMethod] + [DataRow("/mcp", "mcp", DisplayName = "MCP disabled with default path")] + [DataRow("/custom-mcp", "custom-mcp", DisplayName = "MCP disabled with custom path")] + public void McpPathThrowsWhenMcpDisabled(string mcpPath, string route) + { + InitializeTestWithMcpConfig("/api", new McpRuntimeOptions(Enabled: false, Path: mcpPath)); + DataApiBuilderException ex = Assert.ThrowsException( + () => _restService.GetRouteAfterPathBase(route)); + Assert.AreEqual(HttpStatusCode.NotFound, ex.StatusCode); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.GlobalMcpEndpointDisabled, ex.SubStatusCode); + } + + /// + /// When MCP is enabled (explicitly or by default when config is absent), + /// the MCP path route should NOT throw GlobalMcpEndpointDisabled. + /// It falls through to the normal path-base check. + /// + [DataTestMethod] + [DataRow(true, DisplayName = "MCP explicitly enabled")] + [DataRow(null, DisplayName = "MCP config absent (defaults to enabled)")] + public void McpPathDoesNotThrowGlobalMcpEndpointDisabledWhenMcpEnabled(bool? mcpEnabled) + { + McpRuntimeOptions mcpOptions = mcpEnabled.HasValue ? new McpRuntimeOptions(Enabled: mcpEnabled.Value) : null; + InitializeTestWithMcpConfig("/api", mcpOptions); + // "mcp" doesn't start with "api", so it should throw BadRequest (invalid path), + // NOT GlobalMcpEndpointDisabled. + DataApiBuilderException ex = Assert.ThrowsException( + () => _restService.GetRouteAfterPathBase("mcp")); + Assert.AreEqual(HttpStatusCode.BadRequest, ex.StatusCode); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.BadRequest, ex.SubStatusCode); + } + + #endregion + #region Helper Functions /// @@ -215,7 +255,7 @@ public static void InitializeTestWithMultipleEntityPaths(string restRoutePrefix, /// /// Core helper to initialize REST Service with specified entity path mappings. /// - private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictionary entityPaths) + private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictionary entityPaths, McpRuntimeOptions mcpOptions = null, bool useMcpParam = false) { RuntimeConfig mockConfig = new( Schema: "", @@ -223,7 +263,7 @@ private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictio Runtime: new( Rest: new(Path: restRoutePrefix), GraphQL: new(), - Mcp: new(), + Mcp: useMcpParam ? mcpOptions : (mcpOptions ?? new()), Host: new(null, null) ), Entities: new(new Dictionary()) @@ -306,6 +346,16 @@ private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictio provider, requestValidator); } + + /// + /// Initializes RestService with a specific MCP configuration for testing MCP path guard behavior. + /// + /// REST path prefix (e.g., "/api"). + /// MCP options, or null to simulate absent mcp config block. + private static void InitializeTestWithMcpConfig(string restRoutePrefix, McpRuntimeOptions mcpOptions) + { + InitializeTestWithEntityPaths(restRoutePrefix, new Dictionary(), mcpOptions, useMcpParam: true); + } #endregion } }