Merge branch 'main' into copilot/support-xml-data-type-mssql

Author: JerryNixon

schemas/dab.draft.schema.json

@@ -819,15 +819,23 @@
               "additionalProperties": false,
               "properties": {
                 "mcp": {
-                  "type": "object",
-                  "description": "MCP endpoint configuration",
-                  "additionalProperties": false,
-                  "properties": {
-                    "dml-tools": {
+                  "oneOf": [
+                    {
                       "$ref": "#/$defs/boolean-or-string",
-                      "description": "Enable/disable all DML tools with default settings."
+                      "description": "Boolean shorthand: true enables dml-tools only (custom-tool remains false), false disables all MCP functionality."
+                    },
+                    {
+                      "type": "object",
+                      "description": "MCP endpoint configuration",
+                      "additionalProperties": false,
+                      "properties": {
+                        "dml-tools": {
+                          "$ref": "#/$defs/boolean-or-string",
+                          "description": "Enable/disable all DML tools with default settings."
+                        }
+                      }
                     }
-                  }
+                  ]
                 },
                 "rest": {
                   "type": "object",

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<CallToolResult> 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<CallToolResult> 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<string, double>? havingOperators = null;
             List<double>? havingInValues = null;
@@ -481,24 +483,24 @@ public async Task<CallToolResult> ExecuteAsync(
         }
 
         /// <summary>
-        /// 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).
         /// </summary>
-        private static CallToolResult? ValidateGroupByDependencies(
+        internal static CallToolResult? ValidateGroupByDependencies(
             int groupbyCount,
-            bool userProvidedOrderby,
+            ref bool userProvidedOrderby,
             int? first,
             string? after,
             string toolName,
             ILogger? logger)
         {
             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)
                 {

src/Cli.Tests/ModuleInitializer.cs

@@ -65,6 +65,8 @@ public static void Init()
         VerifierSettings.IgnoreMember<Entity>(entity => entity.EntityFirst);
         // Ignore the entity IsLinkingEntity as that's unimportant from a test standpoint.
         VerifierSettings.IgnoreMember<Entity>(entity => entity.IsLinkingEntity);
+        // Ignore the entity IsAutoentity as that's unimportant from a test standpoint.
+        VerifierSettings.IgnoreMember<Entity>(entity => entity.IsAutoentity);
         // Ignore the UserProvidedTtlOptions. They aren't serialized to our config file, enforced by EntityCacheOptionsConverter.
         VerifierSettings.IgnoreMember<EntityCacheOptions>(cacheOptions => cacheOptions.UserProvidedTtlOptions);
         // Ignore the UserProvidedEnabledOptions. They aren't serialized to our config file, enforced by EntityCacheOptionsConverter.

src/Config/ObjectModel/Entity.cs

@@ -45,6 +45,9 @@ public record Entity
     [JsonIgnore]
     public bool IsLinkingEntity { get; init; }
 
+    [JsonIgnore]
+    public bool IsAutoentity { get; init; }
+
     [JsonConstructor]
     public Entity(
         EntitySource Source,
@@ -58,7 +61,8 @@ public Entity(
         bool IsLinkingEntity = false,
         EntityHealthCheckConfig? Health = null,
         string? Description = null,
-        EntityMcpOptions? Mcp = null)
+        EntityMcpOptions? Mcp = null,
+        bool IsAutoentity = false)
     {
         this.Health = Health;
         this.Source = Source;
@@ -72,6 +76,7 @@ public Entity(
         this.IsLinkingEntity = IsLinkingEntity;
         this.Description = Description;
         this.Mcp = Mcp;
+        this.IsAutoentity = IsAutoentity;
     }
 
     /// <summary>

src/Config/ObjectModel/RuntimeConfig.cs

@@ -269,6 +269,11 @@ public bool TryAddGeneratedAutoentityNameToDataSourceName(string entityName, str
         return false;
     }
 
+    public bool RemoveGeneratedAutoentityNameFromDataSourceName(string entityName)
+    {
+        return _entityNameToDataSourceName.Remove(entityName);
+    }
+
     /// <summary>
     /// Constructor for runtimeConfig.
     /// To be used when setting up from cli json scenario.

src/Core/Configurations/RuntimeConfigProvider.cs

@@ -426,4 +426,32 @@ public void AddMergedEntitiesToConfig(Dictionary<string, Entity> newEntities)
         };
         _configLoader.EditRuntimeConfig(newRuntimeConfig);
     }
+
+    public void RemoveGeneratedAutoentitiesFromConfig()
+    {
+        Dictionary<string, Entity> entities = new(_configLoader.RuntimeConfig!.Entities);
+        List<string> removingEntities = new();
+
+        // Add entities that will be removed to a list first to avoid modifying the collection while iterating over it.
+        foreach ((string name, Entity entity) in entities)
+        {
+            if (entity.IsAutoentity)
+            {
+                removingEntities.Add(name);
+            }
+        }
+
+        // Remove all autoentities from the config.
+        foreach (string name in removingEntities)
+        {
+            entities.Remove(name);
+            _configLoader.RuntimeConfig!.RemoveGeneratedAutoentityNameFromDataSourceName(name);
+        }
+
+        RuntimeConfig newRuntimeConfig = _configLoader.RuntimeConfig! with
+        {
+            Entities = new(entities)
+        };
+        _configLoader.EditRuntimeConfig(newRuntimeConfig);
+    }
 }

src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs

@@ -349,7 +349,8 @@ protected override async Task GenerateAutoentitiesIntoEntities(IReadOnlyDictiona
                         Health: autoentity.Template.Health,
                         Fields: null,
                         Relationships: null,
-                        Mappings: new());
+                        Mappings: new(),
+                        IsAutoentity: true);
 
                     // Add the generated entity to the linking entities dictionary.
                     // This allows the entity to be processed later during metadata population.

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs

@@ -353,6 +353,12 @@ public async Task InitializeAsync()
 
             GenerateRestPathToEntityMap();
             InitODataParser();
+
+            if (_isValidateOnly)
+            {
+                RemoveGeneratedAutoentities();
+            }
+
             timer.Stop();
             _logger.LogTrace($"Done inferring Sql database schema in {timer.ElapsedMilliseconds}ms.");
         }
@@ -714,6 +720,15 @@ protected virtual Task GenerateAutoentitiesIntoEntities(IReadOnlyDictionary<stri
             throw new NotSupportedException($"{GetType().Name} does not support Autoentities yet.");
         }
 
+        /// <summary>
+        /// Removes the entities that were generated from the autoentities property.
+        /// This should only be done when we only want to validate the entities.
+        /// </summary>
+        private void RemoveGeneratedAutoentities()
+        {
+            _runtimeConfigProvider.RemoveGeneratedAutoentitiesFromConfig();
+        }
+
         protected void PopulateDatabaseObjectForEntity(
             Entity entity,
             string entityName,

src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs

@@ -64,6 +64,7 @@ private static void GenerateConfigFile(
         string entityBackingColumn = "title",
         string entityExposedName = "title",
         string mcpEnabled = "true",
+        string autoentityName = "autoentity_{object}",
         string configFileName = CONFIG_FILE_NAME)
     {
         File.WriteAllText(configFileName, @"
@@ -180,6 +181,29 @@ private static void GenerateConfigFile(
                           }
                         ]
                       }
+                    },
+                    ""autoentities"": {
+                      ""BooksAutoentities"": {
+                        ""patterns"": {
+                          ""include"": [ ""%book%"" ],
+                          ""name"": """ + autoentityName + @"""
+                        },
+                        ""template"": {
+                          ""rest"": {
+                            ""enabled"": true
+                          }
+                        },
+                        ""permissions"": [
+                          {
+                            ""role"": ""anonymous"",
+                            ""actions"": [
+                              {
+                                ""action"": ""*""
+                              }
+                            ]
+                          }
+                        ]
+                      }
                     }
                 }");
     }
@@ -788,6 +812,41 @@ await WaitForConditionAsync(
         Assert.AreEqual(HttpStatusCode.OK, restResult.StatusCode);
     }
 
+    /// <summary>
+    /// Hot reload the configuration file so that it changes the name of the autoentity properties.
+    /// Then we assert that the hot reload is successful by sending a request to the newly created autoentity.
+    /// </summary>
+    [TestCategory(MSSQL_ENVIRONMENT)]
+    [TestMethod]
+    public async Task HotReloadAutoentities()
+    {
+        // Arrange
+        _writer = new StringWriter();
+        Console.SetOut(_writer);
+
+        // Act
+        HttpResponseMessage restResult = await _testClient.GetAsync($"rest/autoentity_books");
+
+        GenerateConfigFile(
+            connectionString: $"{ConfigurationTests.GetConnectionStringFromEnvironmentConfig(TestCategory.MSSQL).Replace("\\", "\\\\")}",
+            autoentityName: "HotReload_{object}");
+        await WaitForConditionAsync(
+          () => WriterContains(HOT_RELOAD_SUCCESS_MESSAGE),
+          TimeSpan.FromSeconds(HOT_RELOAD_TIMEOUT_SECONDS),
+          TimeSpan.FromMilliseconds(500));
+
+        HttpResponseMessage failRestResult = await _testClient.GetAsync($"rest/autoentity_books");
+        HttpResponseMessage hotReloadRestResult = await _testClient.GetAsync($"rest/HotReload_books");
+
+        // Assert
+        Assert.AreEqual(HttpStatusCode.OK, restResult.StatusCode,
+                    $"REST request before hot-reload failed when it was expected to succeed. Response: {await restResult.Content.ReadAsStringAsync()}");
+        Assert.AreEqual(HttpStatusCode.NotFound, failRestResult.StatusCode,
+                    $"REST request after hot-reload succeeded when it was expected to fail. Response: {await failRestResult.Content.ReadAsStringAsync()}");
+        Assert.AreEqual(HttpStatusCode.OK, hotReloadRestResult.StatusCode,
+                    $"REST request after hot-reload failed when it was expected to succeed. Response: {await hotReloadRestResult.Content.ReadAsStringAsync()}");
+    }
+
     /// <summary>
     /// /// (Warning: This test only currently works in the pipeline due to constrains of not
     /// being able to change from one database type to another, under normal circumstances