From 884f1dd1a557435992957ca646570d7e22886311 Mon Sep 17 00:00:00 2001 From: souvikghosh04 Date: Thu, 7 May 2026 14:58:53 +0530 Subject: [PATCH 1/2] aggregate_records should default fields to * --- .../BuiltInTools/AggregateRecordsTool.cs | 24 ++++++++++++++----- .../Mcp/AggregateRecordsToolTests.cs | 13 ++++++++-- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs index 003ff0b5ea..adb79f3259 100644 --- a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs +++ b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs @@ -78,7 +78,8 @@ public class AggregateRecordsTool : IMcpTool }, ""field"": { ""type"": ""string"", - ""description"": ""Field name to aggregate, or * with count to count all rows."" + ""description"": ""Field name to aggregate, or * with count to count all rows. Defaults to '*' when omitted (only valid with function 'count')."", + ""default"": ""*"" }, ""distinct"": { ""type"": ""boolean"", @@ -128,7 +129,7 @@ public class AggregateRecordsTool : IMcpTool ""description"": ""Opaque cursor from a previous endCursor for next-page retrieval. Requires groupby and first. Do not construct manually."" } }, - ""required"": [""entity"", ""function"", ""field""] + ""required"": [""entity"", ""function""] }" ) }; @@ -367,13 +368,24 @@ public async Task ExecuteAsync( $"Invalid function '{function}'. Must be one of: count, avg, sum, min, max.", logger); } - // Parse field - if (!root.TryGetProperty("field", out JsonElement fieldElement) || string.IsNullOrWhiteSpace(fieldElement.GetString())) + // Parse field. When omitted, default to '*' (only valid with function 'count'). + // For non-count functions, an explicit numeric field name is required. + string field; + if (root.TryGetProperty("field", out JsonElement fieldElement) && !string.IsNullOrWhiteSpace(fieldElement.GetString())) { - return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", "Missing required argument 'field'.", logger); + field = fieldElement.GetString()!; + } + else + { + if (function != "count") + { + return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", + $"Missing required argument 'field'. The default value '*' is only valid with function 'count'; for function '{function}', provide a specific numeric field name.", logger); + } + + field = "*"; } - string field = fieldElement.GetString()!; bool isCountStar = function == "count" && field == "*"; if (field == "*" && function != "count") diff --git a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs index dd6587674c..0220486e6d 100644 --- a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs +++ b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs @@ -62,7 +62,15 @@ public void GetToolMetadata_HasRequiredSchemaProperties() CollectionAssert.Contains(requiredFields, "entity"); CollectionAssert.Contains(requiredFields, "function"); - CollectionAssert.Contains(requiredFields, "field"); + CollectionAssert.DoesNotContain(requiredFields, "field", + "'field' must not be required; it defaults to '*'."); + + // Verify the field property declares '*' as its default value. + JsonElement fieldProp = properties.GetProperty("field"); + Assert.IsTrue(fieldProp.TryGetProperty("default", out JsonElement fieldDefault), + "'field' property must declare a default value."); + Assert.AreEqual("*", fieldDefault.GetString(), + "'field' default must be '*'."); // Verify all schema properties exist with correct types AssertSchemaProperty(properties, "entity", "string"); @@ -119,7 +127,8 @@ public async Task AggregateRecords_Disabled_ReturnsToolDisabledError(bool runtim [DataTestMethod] [DataRow("{\"function\": \"count\", \"field\": \"*\"}", null, DisplayName = "Missing entity")] [DataRow("{\"entity\": \"Book\", \"field\": \"*\"}", null, DisplayName = "Missing function")] - [DataRow("{\"entity\": \"Book\", \"function\": \"count\"}", null, DisplayName = "Missing field")] + [DataRow("{\"entity\": \"Book\", \"function\": \"avg\"}", "field", DisplayName = "Missing field for non-count function")] + [DataRow("{\"entity\": \"Book\", \"function\": \"sum\"}", "field", DisplayName = "Missing field for sum function")] [DataRow("{\"entity\": \"Book\", \"function\": \"median\", \"field\": \"price\"}", "median", DisplayName = "Invalid function 'median'")] public async Task AggregateRecords_MissingOrInvalidRequiredArgs_ReturnsInvalidArguments(string json, string expectedInMessage) { From 68066b19d4840c9622f2e19eca1294a87939c50b Mon Sep 17 00:00:00 2001 From: souvikghosh04 Date: Thu, 7 May 2026 16:03:06 +0530 Subject: [PATCH 2/2] Copilot review fixes --- .../BuiltInTools/AggregateRecordsTool.cs | 22 ++++++++++++-- .../Mcp/AggregateRecordsToolTests.cs | 29 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs index adb79f3259..3be49244e0 100644 --- a/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs +++ b/src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs @@ -371,9 +371,27 @@ public async Task ExecuteAsync( // Parse field. When omitted, default to '*' (only valid with function 'count'). // For non-count functions, an explicit numeric field name is required. string field; - if (root.TryGetProperty("field", out JsonElement fieldElement) && !string.IsNullOrWhiteSpace(fieldElement.GetString())) + if (root.TryGetProperty("field", out JsonElement fieldElement)) { - field = fieldElement.GetString()!; + if (fieldElement.ValueKind != JsonValueKind.String) + { + return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", + $"Argument 'field' must be a string. Got: '{fieldElement.ValueKind}'.", logger); + } + + if (!string.IsNullOrWhiteSpace(fieldElement.GetString())) + { + field = fieldElement.GetString()!; + } + else if (function == "count") + { + field = "*"; + } + else + { + return McpResponseBuilder.BuildErrorResult(toolName, "InvalidArguments", + $"Missing required argument 'field'. The default value '*' is only valid with function 'count'; for function '{function}', provide a specific numeric field name.", logger); + } } else { diff --git a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs index 0220486e6d..e1ab32e267 100644 --- a/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs +++ b/src/Service.Tests/Mcp/AggregateRecordsToolTests.cs @@ -164,6 +164,12 @@ public async Task AggregateRecords_NullArguments_ReturnsInvalidArguments() DisplayName = "Star field with avg (must mention count)")] [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": \"*\", \"distinct\": true}", "DISTINCT", DisplayName = "Distinct with count(*)")] + [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"distinct\": true}", "DISTINCT", + DisplayName = "Distinct with implicit count(*) default (issue #3280)")] + [DataRow("{\"entity\": \"Book\", \"function\": \"avg\"}", "field", + DisplayName = "Non-count function without field returns InvalidArguments (issue #3280)")] + [DataRow("{\"entity\": \"Book\", \"function\": \"count\", \"field\": 123}", "field", + DisplayName = "Non-string field value returns InvalidArguments")] public async Task AggregateRecords_InvalidFieldFunctionCombination_ReturnsInvalidArguments(string json, string expectedInMessage) { IServiceProvider sp = CreateDefaultServiceProvider(); @@ -255,6 +261,29 @@ public async Task AggregateRecords_SimpleCountStar_PassesValidation() "Simple count(*) must pass input validation."); } + /// + /// Verifies that count without an explicit field passes validation by defaulting to '*' (issue #3280). + /// + [TestMethod] + public async Task AggregateRecords_CountWithoutField_PassesValidation() + { + IServiceProvider sp = CreateDefaultServiceProvider(); + + CallToolResult result = await ExecuteToolAsync(sp, + "{\"entity\": \"Book\", \"function\": \"count\"}"); + + // May fail at metadata resolution (no real DB), but must NOT fail with InvalidArguments. + if (result.IsError != true) + { + return; + } + + JsonElement content = ParseContent(result); + string errorType = content.GetProperty("error").GetProperty("type").GetString()!; + Assert.AreNotEqual("InvalidArguments", errorType, + "count without field must pass input validation by defaulting to '*' (issue #3280)."); + } + /// /// Verifies that the orderby schema property has no default value (fix for #3279). ///