diff --git a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs index 16edaaebe4..6b28db546e 100644 --- a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs +++ b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs @@ -436,9 +436,20 @@ public async Task ExecuteAsync( // Parse filter string? filter = root.TryGetProperty("filter", out JsonElement filterElement) ? filterElement.GetString() : null; - // 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()); + // Validate the JSON type before reading orderby. Preserve null/blank as omitted; + // direction validation is deferred until groupby is known (see #3279). + bool userProvidedOrderby = root.TryGetProperty("orderby", out JsonElement orderbyElement); + if (userProvidedOrderby) + { + if (orderbyElement.ValueKind != JsonValueKind.String && orderbyElement.ValueKind != JsonValueKind.Null) + { + return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", + $"Argument 'orderby' must be a string ('asc' or 'desc'), for example \"desc\". Got: '{orderbyElement.ValueKind}'.", logger); + } + + userProvidedOrderby = !string.IsNullOrWhiteSpace(orderbyElement.GetString()); + } + string orderby = "desc"; // Parse first diff --git a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs index bd33dd8433..8f1ae96414 100644 --- a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs +++ b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs @@ -29,6 +29,10 @@ namespace Azure.DataApiBuilder.Mcp.BuiltInTools { public class ReadRecordsTool : IMcpTool { + private const string INVALID_ORDERBY_MESSAGE = + "Argument 'orderby' must be an array of non-empty strings specifying fields and optional directions, " + + "for example [\"name asc\", \"year desc\"]."; + public ToolType ToolType { get; } = ToolType.BuiltIn; public bool IsEnabled(RuntimeConfig config) => config.McpDmlTools?.ReadRecords ?? true; @@ -100,7 +104,7 @@ public async Task ExecuteAsync( string? select = null; string? filter = null; int? first = null; - IEnumerable? orderby = null; + List? orderby = null; string? after = null; // Extract arguments @@ -140,7 +144,23 @@ public async Task ExecuteAsync( if (root.TryGetProperty("orderby", out JsonElement orderbyElement)) { - orderby = (IEnumerable?)orderbyElement.EnumerateArray().Select(e => e.GetString()); + if (orderbyElement.ValueKind != JsonValueKind.Array) + { + return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", INVALID_ORDERBY_MESSAGE, logger); + } + + // Validate and materialize every item before metadata resolution or query parsing. + orderby = new List(); + foreach (JsonElement item in orderbyElement.EnumerateArray()) + { + string? sort = item.ValueKind == JsonValueKind.String ? item.GetString() : null; + if (string.IsNullOrWhiteSpace(sort)) + { + return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", INVALID_ORDERBY_MESSAGE, logger); + } + + orderby.Add(sort); + } } if (root.TryGetProperty("after", out JsonElement afterElement)) @@ -216,20 +236,9 @@ public async Task ExecuteAsync( context.FilterClauseInUrl = sqlMetadataProvider.GetODataParser().GetFilterClause(filterQueryString, $"{context.EntityName}.{context.DatabaseObject.FullName}"); } - if (orderby is not null && orderby.Count() != 0) + if (orderby is not null && orderby.Count != 0) { - string sortQueryString = $"?{RequestParser.SORT_URL}="; - foreach (string param in orderby) - { - if (string.IsNullOrWhiteSpace(param)) - { - return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", "Parameters inside 'orderby' argument cannot be empty or null.", logger); - } - - sortQueryString += $"{param}, "; - } - - sortQueryString = sortQueryString.Substring(0, sortQueryString.Length - 2); + string sortQueryString = $"?{RequestParser.SORT_URL}={string.Join(", ", orderby)}"; (context.OrderByClauseInUrl, context.OrderByClauseOfBackingColumns) = RequestParser.GenerateOrderByLists(context, sqlMetadataProvider, sortQueryString); } diff --git a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs index dca7355ee8..817e442cdc 100644 --- a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs +++ b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs @@ -495,6 +495,66 @@ public async Task AggregateRecords_GroupByDependencyViolation_ReturnsInvalidArgu #endregion + #region Input Validation Tests - Orderby Shape (Issue #3810) + + /// + /// Wrong JSON types must return a tool-specific argument error, regardless of grouping. + /// + [DataTestMethod] + [DataRow("[\"desc\"]")] + [DataRow("[\"title desc\"]")] + [DataRow("[]")] + [DataRow("[null]")] + [DataRow("{}")] + [DataRow("123")] + [DataRow("true")] + [DataRow("false")] + public async Task AggregateRecords_InvalidOrderbyShape_ReturnsActionableInvalidArguments(string orderby) + { + foreach (string grouping in new[] { string.Empty, ",\"groupby\":[]", ",\"groupby\":[\"title\"]" }) + { + string json = $"{{\"entity\":\"Book\",\"function\":\"count\",\"orderby\":{orderby}{grouping}}}"; + CallToolResult result = await ExecuteToolAsync(CreateDefaultServiceProvider(), json); + + string message = AssertErrorResult(result, "InvalidArguments"); + StringAssert.Contains(message, "'orderby'"); + StringAssert.Contains(message, "string"); + StringAssert.Contains(message, "'asc'"); + StringAssert.Contains(message, "'desc'"); + StringAssert.Contains(message, "\"desc\""); + } + } + + /// + /// Preserve existing optional/null/blank handling, normalization, and the grouped descending default. + /// + [DataTestMethod] + [DataRow(null, "desc")] + [DataRow("null", "desc")] + [DataRow("\"\"", "desc")] + [DataRow("\" \"", "desc")] + [DataRow("\"asc\"", "asc")] + [DataRow("\"desc\"", "desc")] + [DataRow("\" ASC \"", "asc")] + [DataRow("\" DeSc \"", "desc")] + public void AggregateRecords_ValidOrderby_PreservesDirectionAndDefault(string? orderby, string expectedDirection) + { + string orderbyProperty = orderby is null ? string.Empty : $",\"orderby\":{orderby}"; + using JsonDocument arguments = JsonDocument.Parse( + $"{{\"entity\":\"Book\",\"function\":\"count\",\"groupby\":[\"title\"]{orderbyProperty}}}"); + object?[] parameters = { arguments, CreateConfig(), "aggregate_records", null, null }; + + CallToolResult? error = InvokePrivateWithMutableArguments( + "TryParseAndValidateArguments", parameters); + + Assert.IsNull(error); + AggregateRecordsTool.AggregateArguments parsed = (AggregateRecordsTool.AggregateArguments)parameters[3]!; + Assert.AreEqual(expectedDirection, parsed.Orderby); + CollectionAssert.AreEqual(new[] { "title" }, parsed.Groupby); + } + + #endregion + #region Input Validation Tests - Orderby Without Groupby (Issue #3279) /// @@ -517,17 +577,9 @@ public async Task AggregateRecords_OrderbyWithoutGroupby_PassesValidation(string 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}"); + // The provider intentionally has no metadata factory. Require that exact boundary, + // rather than allowing an UnexpectedError to pass this compatibility test. + AssertErrorResult(result, "EntityNotFound"); } /// diff --git a/src/Service.Tests/Mcp/BuiltInDmlToolValidationTests.cs b/src/Service.Tests/Mcp/BuiltInDmlToolValidationTests.cs index 884abb2133..10d5c5cc1c 100644 --- a/src/Service.Tests/Mcp/BuiltInDmlToolValidationTests.cs +++ b/src/Service.Tests/Mcp/BuiltInDmlToolValidationTests.cs @@ -142,6 +142,60 @@ public async Task ReadRecords_MissingEntity_ReturnsInvalidArguments() AssertErrorType(result, "InvalidArguments"); } + /// + /// Rejects invalid orderby shapes and members before metadata resolution, with a usable example. + /// + [DataTestMethod] + [DataRow("\"id desc\"")] + [DataRow("\"id\"")] + [DataRow("\"desc\"")] + [DataRow("null")] + [DataRow("123")] + [DataRow("true")] + [DataRow("false")] + [DataRow("{}")] + [DataRow("[123]")] + [DataRow("[true]")] + [DataRow("[false]")] + [DataRow("[{}]")] + [DataRow("[[]]")] + [DataRow("[null]")] + [DataRow("[\"\"]")] + [DataRow("[\" \"]")] + [DataRow("[\"id desc\",123]")] + [DataRow("[\"id desc\",null]")] + [DataRow("[null,123]")] + public async Task ReadRecords_InvalidOrderby_ReturnsActionableInvalidArguments(string orderby) + { + IServiceProvider sp = CreateServiceProvider(CreateConfig()); + CallToolResult result = await ExecuteAsync( + new ReadRecordsTool(), sp, $"{{\"entity\":\"Book\",\"orderby\":{orderby}}}"); + + string message = AssertErrorType(result, "InvalidArguments"); + StringAssert.Contains(message, "'orderby'"); + StringAssert.Contains(message, "array of non-empty strings"); + StringAssert.Contains(message, "[\"name asc\", \"year desc\"]"); + } + + /// + /// Valid string lists, empty lists, and omitted ordering must still reach metadata resolution. + /// This test checks shape validation only, not field resolution or database execution. + /// + [DataTestMethod] + [DataRow("{\"entity\":\"Book\"}")] + [DataRow("{\"entity\":\"Book\",\"orderby\":[]}")] + [DataRow("{\"entity\":\"Book\",\"orderby\":[\"id\"]}")] + [DataRow("{\"entity\":\"Book\",\"orderby\":[\"id asc\"]}")] + [DataRow("{\"entity\":\"Book\",\"orderby\":[\"id desc\"]}")] + [DataRow("{\"entity\":\"Book\",\"orderby\":[\"title asc\",\"id desc\"]}")] + public async Task ReadRecords_ValidOrderbyShape_ReachesMetadataResolution(string arguments) + { + IServiceProvider sp = CreateServiceProvider(CreateConfig()); + CallToolResult result = await ExecuteAsync(new ReadRecordsTool(), sp, arguments); + + AssertErrorType(result, "EntityNotFound"); + } + [TestMethod] public async Task UpdateRecord_MissingFields_ReturnsInvalidArguments() { @@ -229,12 +283,14 @@ private static async Task ExecuteAsync(IMcpTool tool, IServicePr return await tool.ExecuteAsync(args, sp, CancellationToken.None); } - private static void AssertErrorType(CallToolResult result, string expectedType) + private static string AssertErrorType(CallToolResult result, string expectedType) { Assert.IsTrue(result.IsError == true, "Expected an error result."); TextContentBlock block = (TextContentBlock)result.Content[0]; - JsonElement root = JsonDocument.Parse(block.Text).RootElement; - Assert.AreEqual(expectedType, root.GetProperty("error").GetProperty("type").GetString()); + using JsonDocument payload = JsonDocument.Parse(block.Text); + JsonElement error = payload.RootElement.GetProperty("error"); + Assert.AreEqual(expectedType, error.GetProperty("type").GetString()); + return error.GetProperty("message").GetString()!; } private static RuntimeConfig CreateConfig( diff --git a/src/Service.Tests/Mcp/McpServerConfigurationTests.cs b/src/Service.Tests/Mcp/McpServerConfigurationTests.cs index 307e728396..3ae79b7f81 100644 --- a/src/Service.Tests/Mcp/McpServerConfigurationTests.cs +++ b/src/Service.Tests/Mcp/McpServerConfigurationTests.cs @@ -2,10 +2,22 @@ // Licensed under the MIT License. using System; +using System.Collections.Generic; +using System.Linq; +using System.Net.Http; +using System.Text; +using System.Text.Json; +using System.Threading.Tasks; +using Azure.DataApiBuilder.Config.ObjectModel; +using Azure.DataApiBuilder.Mcp.BuiltInTools; using Azure.DataApiBuilder.Mcp.Core; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Microsoft.VisualStudio.TestTools.UnitTesting; +using ModelContextProtocol.AspNetCore; using ModelContextProtocol.Server; namespace Azure.DataApiBuilder.Service.Tests.Mcp @@ -33,5 +45,100 @@ public void ConfigureMcpServer_ConfiguresServerOptions(string? instructions, str Assert.IsNotNull(options.Capabilities.Tools); Assert.AreEqual(expectedInstructions, options.ServerInstructions); } + + /// + /// Exercises discovery and invocation through the real MCP HTTP handler, with no database or network listener. + /// Advertised ordering shapes remain different; malformed calls must return actionable tool errors. + /// + [DataTestMethod] + [DataRow("read_records", "{\"entity\":\"Book\",\"orderby\":\"id desc\"}", "array of non-empty strings")] + [DataRow("read_records", "{\"entity\":\"Book\",\"orderby\":[\"id desc\",123]}", "array of non-empty strings")] + [DataRow("aggregate_records", "{\"entity\":\"Book\",\"function\":\"count\",\"groupby\":[\"title\"],\"orderby\":[\"desc\"]}", "string ('asc' or 'desc')")] + [DataRow("aggregate_records", "{\"entity\":\"Book\",\"function\":\"count\",\"orderby\":[\"desc\"]}", "string ('asc' or 'desc')")] + public async Task ConfigureMcpServer_InvalidOrderby_ReturnsActionableToolError(string toolName, string json, string expectedShape) + { + WebApplicationBuilder builder = WebApplication.CreateBuilder(new WebApplicationOptions + { + EnvironmentName = "Development", + ContentRootPath = AppContext.BaseDirectory + }); + builder.Configuration.Sources.Clear(); + builder.Logging.ClearProviders(); + builder.WebHost.UseTestServer(); + RuntimeConfig config = new( + Schema: "test-schema", + DataSource: new(DatabaseType.MSSQL, ConnectionString: string.Empty, Options: null), + Runtime: new( + Rest: new(), + GraphQL: new(), + Mcp: new(Enabled: true, Path: "/mcp", DmlTools: DmlToolsConfig.Default), + Host: new(Cors: null, Authentication: null, Mode: HostMode.Development)), + Entities: new(new Dictionary())); + builder.Services.AddSingleton(TestHelper.GenerateInMemoryRuntimeConfigProvider(config)); + McpToolRegistry registry = new(); + registry.ReplaceAll([new ReadRecordsTool(), new AggregateRecordsTool()], config); + builder.Services.AddSingleton(registry); + builder.Services.ConfigureMcpServer(instructions: null); + builder.Services.PostConfigure(options => options.Stateless = true); + + await using WebApplication app = builder.Build(); + app.MapMcp("/mcp"); + await app.StartAsync(); + using HttpClient client = app.GetTestClient(); + client.Timeout = TimeSpan.FromSeconds(15); + client.DefaultRequestHeaders.Accept.ParseAdd("application/json"); + client.DefaultRequestHeaders.Accept.ParseAdd("text/event-stream"); + client.DefaultRequestHeaders.Add("MCP-Protocol-Version", "2025-11-25"); + + JsonElement initialized = await PostRpcAsync(client, 1, "initialize", new + { + protocolVersion = "2025-11-25", + capabilities = new { }, + clientInfo = new { name = "orderby-validation-test", version = "1" } + }); + Assert.IsTrue(initialized.TryGetProperty("result", out _)); + + JsonElement listed = await PostRpcAsync(client, 2, "tools/list", new { }); + JsonElement tools = listed.GetProperty("result").GetProperty("tools"); + Assert.AreEqual(2, tools.GetArrayLength()); + foreach (JsonElement tool in tools.EnumerateArray()) + { + string expectedType = tool.GetProperty("name").GetString() == "read_records" ? "array" : "string"; + Assert.AreEqual(expectedType, tool.GetProperty("inputSchema").GetProperty("properties") + .GetProperty("orderby").GetProperty("type").GetString()); + } + + using JsonDocument arguments = JsonDocument.Parse(json); + JsonElement response = await PostRpcAsync(client, 3, "tools/call", new { name = toolName, arguments = arguments.RootElement }); + JsonElement result = response.GetProperty("result"); + Assert.IsTrue(result.GetProperty("isError").GetBoolean()); + using JsonDocument content = JsonDocument.Parse(result.GetProperty("content")[0].GetProperty("text").GetString()!); + Assert.AreEqual(toolName, content.RootElement.GetProperty("toolName").GetString()); + JsonElement error = content.RootElement.GetProperty("error"); + Assert.AreEqual("InvalidArguments", error.GetProperty("type").GetString()); + string message = error.GetProperty("message").GetString()!; + StringAssert.Contains(message, "'orderby'"); + StringAssert.Contains(message, expectedShape); + StringAssert.Contains(message, "for example"); + + await app.StopAsync(); + } + + private static async Task PostRpcAsync(HttpClient client, int id, string method, object parameters) + { + string json = JsonSerializer.Serialize(new { jsonrpc = "2.0", id, method, @params = parameters }); + using StringContent body = new(json, Encoding.UTF8, "application/json"); + using HttpResponseMessage response = await client.PostAsync("/mcp", body); + string payload = await response.Content.ReadAsStringAsync(); + Assert.IsTrue(response.IsSuccessStatusCode, $"HTTP {response.StatusCode}: {payload}"); + + if (response.Content.Headers.ContentType?.MediaType == "text/event-stream") + { + payload = payload.Split('\n').Last(line => line.StartsWith("data:", StringComparison.Ordinal))[5..].Trim(); + } + + using JsonDocument document = JsonDocument.Parse(payload); + return document.RootElement.Clone(); + } } } diff --git a/src/Service.Tests/Mcp/ReadRecordsToolMsSqlIntegrationTests.cs b/src/Service.Tests/Mcp/ReadRecordsToolMsSqlIntegrationTests.cs index 2c009e772e..f88ff8e101 100644 --- a/src/Service.Tests/Mcp/ReadRecordsToolMsSqlIntegrationTests.cs +++ b/src/Service.Tests/Mcp/ReadRecordsToolMsSqlIntegrationTests.cs @@ -138,6 +138,43 @@ public async Task ReadRecords_WithOrderBy_ReturnsSortedResults() } } + /// + /// Verifies sort-field precedence and descending secondary ordering when the first field has duplicate values. + /// + [TestMethod] + public async Task ReadRecords_WithMultipleOrderByFields_RespectsFieldPrecedence() + { + CallToolResult result = await ExecuteReadAsync( + "Book", + select: "id,publisher_id", + filter: "id ge 1 and id le 8", + orderby: new[] { "publisher_id asc", "id desc" }, + first: 8); + + AssertSuccess(result, "ReadRecords with multiple orderby fields should succeed."); + + JsonElement records = GetRecordsArray(ParseResultRoot(result)); + + // The seeded books include repeated publishers. This sequence differs from both + // global id-descending order and the default id-ascending primary-key tie-breaker. + (int Id, int PublisherId)[] expected = + { + (2, 1234), (1, 1234), + (5, 2323), + (8, 2324), (7, 2324), (6, 2324), + (4, 2345), (3, 2345) + }; + + Assert.AreEqual(expected.Length, records.GetArrayLength(), "All eight seeded books should be returned."); + for (int i = 0; i < expected.Length; i++) + { + Assert.AreEqual(expected[i].PublisherId, records[i].GetProperty("publisher_id").GetInt32(), + $"Row {i} should respect publisher_id as the primary ascending sort field."); + Assert.AreEqual(expected[i].Id, records[i].GetProperty("id").GetInt32(), + $"Row {i} should respect id descending within each publisher."); + } + } + /// /// Reads records with first parameter to limit page size. ///