Merge branch 'main' into copilot/fix-health-check-postgres

Author: Aniruddh25

src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs

@@ -1,8 +1,11 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
+using System.Net;
 using Azure.DataApiBuilder.Mcp.Model;
+using Azure.DataApiBuilder.Service.Exceptions;
 using ModelContextProtocol.Protocol;
+using static Azure.DataApiBuilder.Mcp.Model.McpEnums;
 
 namespace Azure.DataApiBuilder.Mcp.Core
 {
@@ -11,15 +14,42 @@ namespace Azure.DataApiBuilder.Mcp.Core
     /// </summary>
     public class McpToolRegistry
     {
-        private readonly Dictionary<string, IMcpTool> _tools = new();
+        private readonly Dictionary<string, IMcpTool> _tools = new(StringComparer.OrdinalIgnoreCase);
 
         /// <summary>
         /// Registers a tool in the registry
         /// </summary>
+        /// <exception cref="DataApiBuilderException">Thrown when tool name is invalid or duplicate</exception>
         public void RegisterTool(IMcpTool tool)
         {
             Tool metadata = tool.GetToolMetadata();
-            _tools[metadata.Name] = tool;
+            string toolName = metadata.Name?.Trim() ?? string.Empty;
+
+            // Reject empty or whitespace-only tool names
+            if (string.IsNullOrWhiteSpace(toolName))
+            {
+                throw new DataApiBuilderException(
+                    message: "MCP tool name cannot be null, empty, or whitespace.",
+                    statusCode: HttpStatusCode.ServiceUnavailable,
+                    subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
+            }
+
+            // Check for duplicate tool names (case-insensitive)
+            if (_tools.TryGetValue(toolName, out IMcpTool? existingTool))
+            {
+                string existingToolType = existingTool.ToolType == ToolType.BuiltIn ? "built-in" : "custom";
+                string newToolType = tool.ToolType == ToolType.BuiltIn ? "built-in" : "custom";
+
+                throw new DataApiBuilderException(
+                    message: $"Duplicate MCP tool name '{toolName}' detected. " +
+                            $"A {existingToolType} tool with this name is already registered. " +
+                            $"Cannot register {newToolType} tool with the same name. " +
+                            $"Tool names must be unique across all tool types.",
+                    statusCode: HttpStatusCode.ServiceUnavailable,
+                    subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
+            }
+
+            _tools[toolName] = tool;
         }
 
         /// <summary>

src/Cli.Tests/ConfigureOptionsTests.cs

@@ -14,6 +14,7 @@ public class ConfigureOptionsTests : VerifyBase
         private MockFileSystem? _fileSystem;
         private FileSystemRuntimeConfigLoader? _runtimeConfigLoader;
         private const string TEST_RUNTIME_CONFIG_FILE = "test-update-runtime-setting.json";
+        private const string TEST_DATASOURCE_HEALTH_NAME = "My Data Source";
 
         [TestInitialize]
         public void TestInitialize()
@@ -955,6 +956,102 @@ public void TestFailureWhenAddingSetSessionContextToMySQLDatabase()
         }
 
         /// <summary>
+        /// Tests adding data-source.health.name to a config that doesn't have a health section.
+        /// This method verifies that the health.name can be added to a data source configuration
+        /// that doesn't previously have a health section.
+        /// Command: dab configure --data-source.health.name "My Data Source"
+        /// </summary>
+        [TestMethod]
+        public void TestAddDataSourceHealthName()
+        {
+            // Arrange
+            SetupFileSystemWithInitialConfig(INITIAL_CONFIG);
+
+            ConfigureOptions options = new(
+                dataSourceHealthName: TEST_DATASOURCE_HEALTH_NAME,
+                config: TEST_RUNTIME_CONFIG_FILE
+            );
+
+            // Act
+            bool isSuccess = TryConfigureSettings(options, _runtimeConfigLoader!, _fileSystem!);
+
+            // Assert
+            Assert.IsTrue(isSuccess);
+            string updatedConfig = _fileSystem!.File.ReadAllText(TEST_RUNTIME_CONFIG_FILE);
+            Assert.IsTrue(RuntimeConfigLoader.TryParseConfig(updatedConfig, out RuntimeConfig? config));
+            Assert.IsNotNull(config.DataSource);
+            Assert.IsNotNull(config.DataSource.Health);
+            Assert.AreEqual(TEST_DATASOURCE_HEALTH_NAME, config.DataSource.Health.Name);
+            Assert.IsTrue(config.DataSource.Health.Enabled); // Default value
+        }
+
+        /// <summary>
+        /// Tests updating data-source.health.name on a config that already has a health section.
+        /// This method verifies that the health.name can be updated while preserving other health settings.
+        /// Command: dab configure --data-source.health.name "Updated Name"
+        /// </summary>
+        [DataTestMethod]
+        [DataRow("New Name", DisplayName = "Update health name with a simple string")]
+        [DataRow("This is the value", DisplayName = "Update health name with the example from the issue")]
+        public void TestUpdateDataSourceHealthName(string healthName)
+        {
+            // Arrange - Config with existing health section
+            string configWithHealth = @"
+            {
+                ""$schema"": ""test"",
+                ""data-source"": {
+                    ""database-type"": ""mssql"",
+                    ""connection-string"": ""testconnectionstring"",
+                    ""health"": {
+                        ""enabled"": false,
+                        ""threshold-ms"": 2000
+                    }
+                },
+                ""runtime"": {
+                    ""rest"": {
+                        ""enabled"": true,
+                        ""path"": ""/api""
+                    },
+                    ""graphql"": {
+                        ""enabled"": true,
+                        ""path"": ""/graphql"",
+                        ""allow-introspection"": true
+                    },
+                    ""host"": {
+                        ""mode"": ""development"",
+                        ""cors"": {
+                            ""origins"": [],
+                            ""allow-credentials"": false
+                        },
+                        ""authentication"": {
+                            ""provider"": ""StaticWebApps""
+                        }
+                    }
+                },
+                ""entities"": {}
+            }";
+            SetupFileSystemWithInitialConfig(configWithHealth);
+
+            ConfigureOptions options = new(
+                dataSourceHealthName: healthName,
+                config: TEST_RUNTIME_CONFIG_FILE
+            );
+
+            // Act
+            bool isSuccess = TryConfigureSettings(options, _runtimeConfigLoader!, _fileSystem!);
+
+            // Assert
+            Assert.IsTrue(isSuccess);
+            string updatedConfig = _fileSystem!.File.ReadAllText(TEST_RUNTIME_CONFIG_FILE);
+            Assert.IsTrue(RuntimeConfigLoader.TryParseConfig(updatedConfig, out RuntimeConfig? config));
+            Assert.IsNotNull(config.DataSource);
+            Assert.IsNotNull(config.DataSource.Health);
+            Assert.AreEqual(healthName, config.DataSource.Health.Name);
+            // Verify existing health settings are preserved
+            Assert.IsFalse(config.DataSource.Health.Enabled);
+            Assert.AreEqual(2000, config.DataSource.Health.ThresholdMs);
+        }
+
         /// Tests that running "dab configure --runtime.mcp.description {value}" on a config with various values results
         /// in runtime config update. Takes in updated value for mcp.description and 
         /// validates whether the runtime config reflects those updated values

src/Cli/Commands/ConfigureOptions.cs

@@ -28,6 +28,7 @@ public ConfigureOptions(
             string? dataSourceOptionsContainer = null,
             string? dataSourceOptionsSchema = null,
             bool? dataSourceOptionsSetSessionContext = null,
+            string? dataSourceHealthName = null,
             int? depthLimit = null,
             bool? runtimeGraphQLEnabled = null,
             string? runtimeGraphQLPath = null,
@@ -82,6 +83,7 @@ public ConfigureOptions(
             DataSourceOptionsContainer = dataSourceOptionsContainer;
             DataSourceOptionsSchema = dataSourceOptionsSchema;
             DataSourceOptionsSetSessionContext = dataSourceOptionsSetSessionContext;
+            DataSourceHealthName = dataSourceHealthName;
             // GraphQL
             DepthLimit = depthLimit;
             RuntimeGraphQLEnabled = runtimeGraphQLEnabled;
@@ -155,6 +157,9 @@ public ConfigureOptions(
         [Option("data-source.options.set-session-context", Required = false, HelpText = "Enable session context. Allowed values: true (default), false.")]
         public bool? DataSourceOptionsSetSessionContext { get; }
 
+        [Option("data-source.health.name", Required = false, HelpText = "Identifier for data source in health check report.")]
+        public string? DataSourceHealthName { get; }
+
         [Option("runtime.graphql.depth-limit", Required = false, HelpText = "Max allowed depth of the nested query. Allowed values: (0,2147483647] inclusive. Default is infinity. Use -1 to remove limit.")]
         public int? DepthLimit { get; }
 

src/Cli/ConfigGenerator.cs

@@ -684,6 +684,36 @@ private static bool TryUpdateConfiguredDataSourceOptions(
                 dbOptions.Add(namingPolicy.ConvertName(nameof(MsSqlOptions.SetSessionContext)), options.DataSourceOptionsSetSessionContext.Value);
             }
 
+            // Handle health.name option
+            if (options.DataSourceHealthName is not null)
+            {
+                // If there's no existing health config, create one with the name
+                // Note: Passing enabled: null results in Enabled = true at runtime (default behavior)
+                // but UserProvidedEnabled = false, so the enabled property won't be serialized to JSON.
+                // This ensures only the name property is written to the config file.
+                if (datasourceHealthCheckConfig is null)
+                {
+                    datasourceHealthCheckConfig = new DatasourceHealthCheckConfig(enabled: null, name: options.DataSourceHealthName);
+                }
+                else
+                {
+                    // Update the existing health config with the new name while preserving other settings.
+                    // DatasourceHealthCheckConfig is a record (immutable), so we create a new instance.
+                    // Preserve threshold only if it was explicitly set by the user
+                    int? thresholdToPreserve = datasourceHealthCheckConfig.UserProvidedThresholdMs
+                        ? datasourceHealthCheckConfig.ThresholdMs
+                        : null;
+                    // Preserve enabled only if it was explicitly set by the user
+                    bool? enabledToPreserve = datasourceHealthCheckConfig.UserProvidedEnabled
+                        ? datasourceHealthCheckConfig.Enabled
+                        : null;
+                    datasourceHealthCheckConfig = new DatasourceHealthCheckConfig(
+                        enabled: enabledToPreserve,
+                        name: options.DataSourceHealthName,
+                        thresholdMs: thresholdToPreserve);
+                }
+            }
+
             dbOptions = EnumerableUtilities.IsNullOrEmpty(dbOptions) ? null : dbOptions;
             DataSource dataSource = new(dbType, dataSourceConnectionString, dbOptions, datasourceHealthCheckConfig);
             runtimeConfig = runtimeConfig with { DataSource = dataSource };

src/Config/Converters/DatasourceHealthOptionsConvertorFactory.cs

@@ -114,11 +114,21 @@ public HealthCheckOptionsConverter(DeserializationVariableReplacementSettings? r
 
         public override void Write(Utf8JsonWriter writer, DatasourceHealthCheckConfig value, JsonSerializerOptions options)
         {
-            if (value?.UserProvidedEnabled is true)
+            // Write the health object if any of these conditions are met:
+            // - enabled was explicitly provided by the user
+            // - name property has a value
+            // - threshold was explicitly provided by the user
+            if (value?.UserProvidedEnabled is true || value?.Name is not null || value?.UserProvidedThresholdMs is true)
             {
                 writer.WriteStartObject();
-                writer.WritePropertyName("enabled");
-                JsonSerializer.Serialize(writer, value.Enabled, options);
+
+                // Only write enabled if it was explicitly provided by the user
+                if (value?.UserProvidedEnabled is true)
+                {
+                    writer.WritePropertyName("enabled");
+                    JsonSerializer.Serialize(writer, value.Enabled, options);
+                }
+
                 if (value?.Name is not null)
                 {
                     writer.WritePropertyName("name");

src/Core/Configurations/RuntimeConfigValidator.cs

@@ -656,6 +656,7 @@ private void ValidateRestMethods(Entity entity, string entityName)
     /// <summary>
     /// Helper method to validate that the rest path property for the entity is correctly configured.
     /// The rest path should not be null/empty and should not contain any reserved characters.
+    /// Allows sub-directories (forward slashes) in the path.
     /// </summary>
     /// <param name="entityName">Name of the entity.</param>
     /// <param name="pathForEntity">The rest path for the entity.</param>
@@ -672,10 +673,10 @@ private static void ValidateRestPathSettingsForEntity(string entityName, string
                 );
         }
 
-        if (RuntimeConfigValidatorUtil.DoesUriComponentContainReservedChars(pathForEntity))
+        if (!RuntimeConfigValidatorUtil.TryValidateEntityRestPath(pathForEntity, out string? errorMessage))
         {
             throw new DataApiBuilderException(
-                message: $"The rest path: {pathForEntity} for entity: {entityName} contains one or more reserved characters.",
+                message: $"The rest path: {pathForEntity} for entity: {entityName} {errorMessage ?? "contains invalid characters."}",
                 statusCode: HttpStatusCode.ServiceUnavailable,
                 subStatusCode: DataApiBuilderException.SubStatusCodes.ConfigValidationError
                 );

src/Core/Configurations/RuntimeConfigValidatorUtil.cs

@@ -66,6 +66,94 @@ public static bool DoesUriComponentContainReservedChars(string uriComponent)
         return _reservedUriCharsRgx.IsMatch(uriComponent);
     }
 
+    /// <summary>
+    /// Method to validate an entity REST path allowing sub-directories (forward slashes).
+    /// Each segment of the path is validated for reserved characters and path traversal patterns.
+    /// </summary>
+    /// <param name="entityRestPath">The entity REST path to validate.</param>
+    /// <param name="errorMessage">Output parameter containing a specific error message if validation fails.</param>
+    /// <returns>true if the path is valid, false otherwise.</returns>
+    public static bool TryValidateEntityRestPath(string entityRestPath, out string? errorMessage)
+    {
+        errorMessage = null;
+
+        // Check for maximum path length (reasonable limit for URL paths)
+        const int MAX_PATH_LENGTH = 2048;
+        if (entityRestPath.Length > MAX_PATH_LENGTH)
+        {
+            errorMessage = $"exceeds maximum allowed length of {MAX_PATH_LENGTH} characters.";
+            return false;
+        }
+
+        // Check for backslash usage - common mistake
+        if (entityRestPath.Contains('\\'))
+        {
+            errorMessage = "contains a backslash (\\). Use forward slash (/) for path separators.";
+            return false;
+        }
+
+        // Check for percent-encoded characters (URL encoding not allowed in config)
+        if (entityRestPath.Contains('%'))
+        {
+            errorMessage = "contains percent-encoding (%) which is not allowed. Use literal characters only.";
+            return false;
+        }
+
+        // Check for whitespace
+        if (entityRestPath.Any(char.IsWhiteSpace))
+        {
+            errorMessage = "contains whitespace which is not allowed in URL paths.";
+            return false;
+        }
+
+        // Split the path by '/' to validate each segment separately
+        string[] segments = entityRestPath.Split('/');
+
+        // Validate each segment doesn't contain reserved characters
+        foreach (string segment in segments)
+        {
+            if (string.IsNullOrEmpty(segment))
+            {
+                errorMessage = "contains empty path segments. Ensure there are no leading, consecutive, or trailing slashes.";
+                return false;
+            }
+
+            // Check for path traversal patterns
+            if (segment == "." || segment == "..")
+            {
+                errorMessage = "contains path traversal patterns ('.' or '..') which are not allowed.";
+                return false;
+            }
+
+            // Check for specific reserved characters and provide helpful messages
+            if (segment.Contains('?'))
+            {
+                errorMessage = "contains '?' which is reserved for query strings in URLs.";
+                return false;
+            }
+
+            if (segment.Contains('#'))
+            {
+                errorMessage = "contains '#' which is reserved for URL fragments.";
+                return false;
+            }
+
+            if (segment.Contains(':'))
+            {
+                errorMessage = "contains ':' which is a reserved character and not allowed in URL paths.";
+                return false;
+            }
+
+            if (_reservedUriCharsRgx.IsMatch(segment))
+            {
+                errorMessage = "contains reserved characters that are not allowed in URL paths.";
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     /// <summary>
     /// Method to validate if the TTL passed by the user is valid
     /// </summary>