From a890ed63ba4314c29a55c31b2341587f49b5c48a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 06:23:04 +0000 Subject: [PATCH 1/5] Initial plan From 871bf54eb1aeb445799726128433ea173f97b828 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 06:36:58 +0000 Subject: [PATCH 2/5] Add duplicate MCP tool name detection and validation Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com> --- .../Core/McpToolRegistry.cs | 20 + src/Service.Tests/Mcp/McpToolRegistryTests.cs | 354 ++++++++++++++++++ 2 files changed, 374 insertions(+) create mode 100644 src/Service.Tests/Mcp/McpToolRegistryTests.cs diff --git a/src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs b/src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs index 9c9b96d72b..481d532a00 100644 --- a/src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs +++ b/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 { @@ -16,9 +19,26 @@ public class McpToolRegistry /// /// Registers a tool in the registry /// + /// Thrown when a tool with the same name is already registered public void RegisterTool(IMcpTool tool) { Tool metadata = tool.GetToolMetadata(); + + // Check for duplicate tool names + if (_tools.TryGetValue(metadata.Name, 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 '{metadata.Name}' 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[metadata.Name] = tool; } diff --git a/src/Service.Tests/Mcp/McpToolRegistryTests.cs b/src/Service.Tests/Mcp/McpToolRegistryTests.cs new file mode 100644 index 0000000000..845232d038 --- /dev/null +++ b/src/Service.Tests/Mcp/McpToolRegistryTests.cs @@ -0,0 +1,354 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Azure.DataApiBuilder.Mcp.Core; +using Azure.DataApiBuilder.Mcp.Model; +using Azure.DataApiBuilder.Service.Exceptions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using ModelContextProtocol.Protocol; +using static Azure.DataApiBuilder.Mcp.Model.McpEnums; + +namespace Azure.DataApiBuilder.Service.Tests.Mcp +{ + /// + /// Tests for McpToolRegistry to ensure tool name uniqueness validation. + /// + [TestClass] + public class McpToolRegistryTests + { + /// + /// Test that registering a tool with a unique name succeeds. + /// + [TestMethod] + public void RegisterTool_WithUniqueName_Succeeds() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool tool = new MockMcpTool("unique_tool", ToolType.BuiltIn); + + // Act & Assert - should not throw + registry.RegisterTool(tool); + + // Verify tool was registered + bool found = registry.TryGetTool("unique_tool", out IMcpTool? retrievedTool); + Assert.IsTrue(found); + Assert.IsNotNull(retrievedTool); + Assert.AreEqual("unique_tool", retrievedTool.GetToolMetadata().Name); + } + + /// + /// Test that registering multiple tools with unique names succeeds. + /// + [TestMethod] + public void RegisterTool_WithMultipleUniqueNames_Succeeds() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool tool1 = new MockMcpTool("tool_one", ToolType.BuiltIn); + IMcpTool tool2 = new MockMcpTool("tool_two", ToolType.Custom); + IMcpTool tool3 = new MockMcpTool("tool_three", ToolType.BuiltIn); + + // Act & Assert - should not throw + registry.RegisterTool(tool1); + registry.RegisterTool(tool2); + registry.RegisterTool(tool3); + + // Verify all tools were registered + IEnumerable allTools = registry.GetAllTools(); + Assert.AreEqual(3, allTools.Count()); + } + + /// + /// Test that registering two built-in tools with the same name throws an exception. + /// + [TestMethod] + public void RegisterTool_WithDuplicateBuiltInToolName_ThrowsException() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool tool1 = new MockMcpTool("duplicate_tool", ToolType.BuiltIn); + IMcpTool tool2 = new MockMcpTool("duplicate_tool", ToolType.BuiltIn); + + // Act - Register first tool + registry.RegisterTool(tool1); + + // Assert - Second registration should throw + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(tool2) + ); + + // Verify exception details + Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'duplicate_tool' detected")); + Assert.IsTrue(exception.Message.Contains("built-in tool with this name is already registered")); + Assert.IsTrue(exception.Message.Contains("Cannot register built-in tool with the same name")); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); + } + + /// + /// Test that registering two custom tools with the same name throws an exception. + /// + [TestMethod] + public void RegisterTool_WithDuplicateCustomToolName_ThrowsException() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool tool1 = new MockMcpTool("my_custom_tool", ToolType.Custom); + IMcpTool tool2 = new MockMcpTool("my_custom_tool", ToolType.Custom); + + // Act - Register first tool + registry.RegisterTool(tool1); + + // Assert - Second registration should throw + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(tool2) + ); + + // Verify exception details + Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'my_custom_tool' detected")); + Assert.IsTrue(exception.Message.Contains("custom tool with this name is already registered")); + Assert.IsTrue(exception.Message.Contains("Cannot register custom tool with the same name")); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); + } + + /// + /// Test that registering a custom tool with the same name as a built-in tool throws an exception. + /// + [TestMethod] + public void RegisterTool_CustomToolConflictsWithBuiltIn_ThrowsException() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool builtInTool = new MockMcpTool("create_record", ToolType.BuiltIn); + IMcpTool customTool = new MockMcpTool("create_record", ToolType.Custom); + + // Act - Register built-in tool first + registry.RegisterTool(builtInTool); + + // Assert - Custom tool registration should throw + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(customTool) + ); + + // Verify exception details + Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'create_record' detected")); + Assert.IsTrue(exception.Message.Contains("built-in tool with this name is already registered")); + Assert.IsTrue(exception.Message.Contains("Cannot register custom tool with the same name")); + Assert.IsTrue(exception.Message.Contains("Tool names must be unique across all tool types")); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); + } + + /// + /// Test that registering a built-in tool with the same name as a custom tool throws an exception. + /// + [TestMethod] + public void RegisterTool_BuiltInToolConflictsWithCustom_ThrowsException() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool customTool = new MockMcpTool("my_stored_proc", ToolType.Custom); + IMcpTool builtInTool = new MockMcpTool("my_stored_proc", ToolType.BuiltIn); + + // Act - Register custom tool first + registry.RegisterTool(customTool); + + // Assert - Built-in tool registration should throw + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(builtInTool) + ); + + // Verify exception details + Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'my_stored_proc' detected")); + Assert.IsTrue(exception.Message.Contains("custom tool with this name is already registered")); + Assert.IsTrue(exception.Message.Contains("Cannot register built-in tool with the same name")); + Assert.IsTrue(exception.Message.Contains("Tool names must be unique across all tool types")); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); + } + + /// + /// Test that tool name comparison is case-sensitive. + /// Tools with different casing should be allowed. + /// + [TestMethod] + public void RegisterTool_WithDifferentCasing_Succeeds() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool tool1 = new MockMcpTool("my_tool", ToolType.BuiltIn); + IMcpTool tool2 = new MockMcpTool("My_Tool", ToolType.Custom); + IMcpTool tool3 = new MockMcpTool("MY_TOOL", ToolType.Custom); + + // Act & Assert - All should register successfully (case-sensitive) + registry.RegisterTool(tool1); + registry.RegisterTool(tool2); + registry.RegisterTool(tool3); + + // Verify all tools were registered + IEnumerable allTools = registry.GetAllTools(); + Assert.AreEqual(3, allTools.Count()); + } + + /// + /// Test that GetAllTools returns all registered tools. + /// + [TestMethod] + public void GetAllTools_ReturnsAllRegisteredTools() + { + // Arrange + McpToolRegistry registry = new(); + registry.RegisterTool(new MockMcpTool("tool_a", ToolType.BuiltIn)); + registry.RegisterTool(new MockMcpTool("tool_b", ToolType.Custom)); + registry.RegisterTool(new MockMcpTool("tool_c", ToolType.BuiltIn)); + + // Act + IEnumerable allTools = registry.GetAllTools(); + + // Assert + Assert.AreEqual(3, allTools.Count()); + Assert.IsTrue(allTools.Any(t => t.Name == "tool_a")); + Assert.IsTrue(allTools.Any(t => t.Name == "tool_b")); + Assert.IsTrue(allTools.Any(t => t.Name == "tool_c")); + } + + /// + /// Test that TryGetTool returns false for non-existent tool. + /// + [TestMethod] + public void TryGetTool_WithNonExistentName_ReturnsFalse() + { + // Arrange + McpToolRegistry registry = new(); + registry.RegisterTool(new MockMcpTool("existing_tool", ToolType.BuiltIn)); + + // Act + bool found = registry.TryGetTool("non_existent_tool", out IMcpTool? tool); + + // Assert + Assert.IsFalse(found); + Assert.IsNull(tool); + } + + /// + /// Test edge case: empty tool name (if allowed by validation) + /// + [TestMethod] + public void RegisterTool_WithEmptyToolName_CanRegisterOnce() + { + // Arrange + McpToolRegistry registry = new(); + IMcpTool tool1 = new MockMcpTool("", ToolType.BuiltIn); + IMcpTool tool2 = new MockMcpTool("", ToolType.Custom); + + // Act - Register first tool with empty name + registry.RegisterTool(tool1); + + // Assert - Second tool with empty name should throw + Assert.ThrowsException( + () => registry.RegisterTool(tool2) + ); + } + + /// + /// Test realistic scenario with actual built-in tool names. + /// + [TestMethod] + public void RegisterTool_WithRealisticBuiltInToolNames_DetectsDuplicates() + { + // Arrange + McpToolRegistry registry = new(); + + // Simulate registering built-in tools + registry.RegisterTool(new MockMcpTool("create_record", ToolType.BuiltIn)); + registry.RegisterTool(new MockMcpTool("read_records", ToolType.BuiltIn)); + registry.RegisterTool(new MockMcpTool("update_record", ToolType.BuiltIn)); + registry.RegisterTool(new MockMcpTool("delete_record", ToolType.BuiltIn)); + registry.RegisterTool(new MockMcpTool("describe_entities", ToolType.BuiltIn)); + + // Try to register a custom tool with a conflicting name + IMcpTool customTool = new MockMcpTool("read_records", ToolType.Custom); + + // Assert - Should throw + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(customTool) + ); + + Assert.IsTrue(exception.Message.Contains("read_records")); + Assert.IsTrue(exception.Message.Contains("built-in tool")); + } + + /// + /// Test that entity names converted to snake_case that result in same tool name are caught. + /// This validates the comment from @JerryNixon about entity aliases being unique. + /// Although entity names must be unique at config level, this ensures tool name + /// conversion doesn't create conflicts (e.g., GetUser and get_user would both become get_user). + /// + [TestMethod] + public void RegisterTool_WithConflictingSnakeCaseNames_DetectsDuplicates() + { + // Arrange + McpToolRegistry registry = new(); + + // First tool with snake_case name + IMcpTool tool1 = new MockMcpTool("get_user", ToolType.Custom); + + // Second tool that would convert to the same snake_case + // In reality, these would come from different entity names that convert to same tool name + IMcpTool tool2 = new MockMcpTool("get_user", ToolType.Custom); + + // Act - Register first tool + registry.RegisterTool(tool1); + + // Assert - Second registration should throw + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(tool2) + ); + + // Verify exception details + Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'get_user' detected")); + } + + #region Mock Tool Implementation + + /// + /// Mock implementation of IMcpTool for testing purposes. + /// + private class MockMcpTool : IMcpTool + { + private readonly string _toolName; + + public MockMcpTool(string toolName, ToolType toolType) + { + _toolName = toolName; + ToolType = toolType; + } + + public ToolType ToolType { get; } + + public Tool GetToolMetadata() + { + return new Tool + { + Name = _toolName, + Description = $"Mock {ToolType} tool", + InputSchema = JsonSerializer.Deserialize("{\"type\": \"object\"}") + }; + } + + public Task ExecuteAsync( + JsonDocument? arguments, + IServiceProvider serviceProvider, + CancellationToken cancellationToken = default) + { + // Not used in these tests + throw new NotImplementedException(); + } + } + + #endregion + } +} From b1891289fc86a424f1127a3c7a91b7307bce1aa2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 06:38:29 +0000 Subject: [PATCH 3/5] Fix nullable reference type annotations in McpToolRegistryTests Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com> --- src/Service.Tests/Mcp/McpToolRegistryTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Service.Tests/Mcp/McpToolRegistryTests.cs b/src/Service.Tests/Mcp/McpToolRegistryTests.cs index 845232d038..102f35c12e 100644 --- a/src/Service.Tests/Mcp/McpToolRegistryTests.cs +++ b/src/Service.Tests/Mcp/McpToolRegistryTests.cs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#nullable enable + using System; using System.Collections.Generic; using System.Linq; From a3aaf81ad8e218d77787da4fa838b49e04f9a3af Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 06:40:17 +0000 Subject: [PATCH 4/5] Address code review feedback: fix JsonElement handling and clarify test comments Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com> --- src/Service.Tests/Mcp/McpToolRegistryTests.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Service.Tests/Mcp/McpToolRegistryTests.cs b/src/Service.Tests/Mcp/McpToolRegistryTests.cs index 102f35c12e..f21b5ada1b 100644 --- a/src/Service.Tests/Mcp/McpToolRegistryTests.cs +++ b/src/Service.Tests/Mcp/McpToolRegistryTests.cs @@ -284,10 +284,10 @@ public void RegisterTool_WithRealisticBuiltInToolNames_DetectsDuplicates() } /// - /// Test that entity names converted to snake_case that result in same tool name are caught. - /// This validates the comment from @JerryNixon about entity aliases being unique. - /// Although entity names must be unique at config level, this ensures tool name - /// conversion doesn't create conflicts (e.g., GetUser and get_user would both become get_user). + /// Test that duplicate tool names (regardless of how they're generated) are caught. + /// This validates the duplicate detection mechanism works correctly. + /// Note: Entity names are already required to be unique at config level, + /// but this ensures the tool registry properly detects any duplicates that might occur. /// [TestMethod] public void RegisterTool_WithConflictingSnakeCaseNames_DetectsDuplicates() @@ -298,8 +298,7 @@ public void RegisterTool_WithConflictingSnakeCaseNames_DetectsDuplicates() // First tool with snake_case name IMcpTool tool1 = new MockMcpTool("get_user", ToolType.Custom); - // Second tool that would convert to the same snake_case - // In reality, these would come from different entity names that convert to same tool name + // Second tool with the same name (duplicate) IMcpTool tool2 = new MockMcpTool("get_user", ToolType.Custom); // Act - Register first tool @@ -333,11 +332,13 @@ public MockMcpTool(string toolName, ToolType toolType) public Tool GetToolMetadata() { + // Create a simple JSON object for the input schema + using JsonDocument doc = JsonDocument.Parse("{\"type\": \"object\"}"); return new Tool { Name = _toolName, Description = $"Mock {ToolType} tool", - InputSchema = JsonSerializer.Deserialize("{\"type\": \"object\"}") + InputSchema = doc.RootElement.Clone() }; } From d2188d8cd5793fc9f7ad993d24b45054f9536c74 Mon Sep 17 00:00:00 2001 From: souvikghosh04 Date: Mon, 9 Feb 2026 10:33:33 +0530 Subject: [PATCH 5/5] Minor fixes and refactoring of tests --- .../Core/McpToolRegistry.cs | 28 ++- src/Service.Tests/Mcp/McpToolRegistryTests.cs | 196 ++++++------------ 2 files changed, 85 insertions(+), 139 deletions(-) diff --git a/src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs b/src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs index 481d532a00..0ba182b6db 100644 --- a/src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs +++ b/src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs @@ -14,32 +14,42 @@ namespace Azure.DataApiBuilder.Mcp.Core /// public class McpToolRegistry { - private readonly Dictionary _tools = new(); + private readonly Dictionary _tools = new(StringComparer.OrdinalIgnoreCase); /// /// Registers a tool in the registry /// - /// Thrown when a tool with the same name is already registered + /// Thrown when tool name is invalid or duplicate public void RegisterTool(IMcpTool tool) { Tool metadata = tool.GetToolMetadata(); + string toolName = metadata.Name?.Trim() ?? string.Empty; - // Check for duplicate tool names - if (_tools.TryGetValue(metadata.Name, out IMcpTool? existingTool)) + // 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 '{metadata.Name}' 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.", + 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[metadata.Name] = tool; + _tools[toolName] = tool; } /// diff --git a/src/Service.Tests/Mcp/McpToolRegistryTests.cs b/src/Service.Tests/Mcp/McpToolRegistryTests.cs index f21b5ada1b..c8fa6a9768 100644 --- a/src/Service.Tests/Mcp/McpToolRegistryTests.cs +++ b/src/Service.Tests/Mcp/McpToolRegistryTests.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net; using System.Text.Json; using System.Threading; using System.Threading.Tasks; @@ -24,26 +25,6 @@ namespace Azure.DataApiBuilder.Service.Tests.Mcp [TestClass] public class McpToolRegistryTests { - /// - /// Test that registering a tool with a unique name succeeds. - /// - [TestMethod] - public void RegisterTool_WithUniqueName_Succeeds() - { - // Arrange - McpToolRegistry registry = new(); - IMcpTool tool = new MockMcpTool("unique_tool", ToolType.BuiltIn); - - // Act & Assert - should not throw - registry.RegisterTool(tool); - - // Verify tool was registered - bool found = registry.TryGetTool("unique_tool", out IMcpTool? retrievedTool); - Assert.IsTrue(found); - Assert.IsNotNull(retrievedTool); - Assert.AreEqual("unique_tool", retrievedTool.GetToolMetadata().Name); - } - /// /// Test that registering multiple tools with unique names succeeds. /// @@ -67,15 +48,21 @@ public void RegisterTool_WithMultipleUniqueNames_Succeeds() } /// - /// Test that registering two built-in tools with the same name throws an exception. + /// Test that registering duplicate tools of the same type throws an exception. + /// Validates that both built-in and custom tools enforce name uniqueness within their own type. /// - [TestMethod] - public void RegisterTool_WithDuplicateBuiltInToolName_ThrowsException() + [DataTestMethod] + [DataRow(ToolType.BuiltIn, "duplicate_tool", "built-in", DisplayName = "Duplicate Built-In Tools")] + [DataRow(ToolType.Custom, "my_custom_tool", "custom", DisplayName = "Duplicate Custom Tools")] + public void RegisterTool_WithDuplicateSameType_ThrowsException( + ToolType toolType, + string toolName, + string expectedToolTypeText) { // Arrange McpToolRegistry registry = new(); - IMcpTool tool1 = new MockMcpTool("duplicate_tool", ToolType.BuiltIn); - IMcpTool tool2 = new MockMcpTool("duplicate_tool", ToolType.BuiltIn); + IMcpTool tool1 = new MockMcpTool(toolName, toolType); + IMcpTool tool2 = new MockMcpTool(toolName, toolType); // Act - Register first tool registry.RegisterTool(tool1); @@ -86,113 +73,72 @@ public void RegisterTool_WithDuplicateBuiltInToolName_ThrowsException() ); // Verify exception details - Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'duplicate_tool' detected")); - Assert.IsTrue(exception.Message.Contains("built-in tool with this name is already registered")); - Assert.IsTrue(exception.Message.Contains("Cannot register built-in tool with the same name")); + Assert.IsTrue(exception.Message.Contains($"Duplicate MCP tool name '{toolName}' detected")); + Assert.IsTrue(exception.Message.Contains($"{expectedToolTypeText} tool with this name is already registered")); + Assert.IsTrue(exception.Message.Contains($"Cannot register {expectedToolTypeText} tool with the same name")); Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); + Assert.AreEqual(HttpStatusCode.ServiceUnavailable, exception.StatusCode); } /// - /// Test that registering two custom tools with the same name throws an exception. + /// Test that registering tools with conflicting names across different types throws an exception. + /// Validates that tool names must be unique across all tool types (built-in and custom). /// - [TestMethod] - public void RegisterTool_WithDuplicateCustomToolName_ThrowsException() + [DataTestMethod] + [DataRow("create_record", ToolType.BuiltIn, ToolType.Custom, "built-in", "custom", DisplayName = "Built-In then Custom conflict")] + [DataRow("read_records", ToolType.BuiltIn, ToolType.Custom, "built-in", "custom", DisplayName = "Built-In then Custom conflict (read_records)")] + [DataRow("my_stored_proc", ToolType.Custom, ToolType.BuiltIn, "custom", "built-in", DisplayName = "Custom then Built-In conflict")] + public void RegisterTool_WithCrossTypeConflict_ThrowsException( + string toolName, + ToolType firstToolType, + ToolType secondToolType, + string expectedExistingType, + string expectedNewType) { // Arrange McpToolRegistry registry = new(); - IMcpTool tool1 = new MockMcpTool("my_custom_tool", ToolType.Custom); - IMcpTool tool2 = new MockMcpTool("my_custom_tool", ToolType.Custom); + IMcpTool existingTool = new MockMcpTool(toolName, firstToolType); + IMcpTool conflictingTool = new MockMcpTool(toolName, secondToolType); // Act - Register first tool - registry.RegisterTool(tool1); - - // Assert - Second registration should throw - DataApiBuilderException exception = Assert.ThrowsException( - () => registry.RegisterTool(tool2) - ); - - // Verify exception details - Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'my_custom_tool' detected")); - Assert.IsTrue(exception.Message.Contains("custom tool with this name is already registered")); - Assert.IsTrue(exception.Message.Contains("Cannot register custom tool with the same name")); - Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); - } - - /// - /// Test that registering a custom tool with the same name as a built-in tool throws an exception. - /// - [TestMethod] - public void RegisterTool_CustomToolConflictsWithBuiltIn_ThrowsException() - { - // Arrange - McpToolRegistry registry = new(); - IMcpTool builtInTool = new MockMcpTool("create_record", ToolType.BuiltIn); - IMcpTool customTool = new MockMcpTool("create_record", ToolType.Custom); - - // Act - Register built-in tool first - registry.RegisterTool(builtInTool); - - // Assert - Custom tool registration should throw - DataApiBuilderException exception = Assert.ThrowsException( - () => registry.RegisterTool(customTool) - ); - - // Verify exception details - Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'create_record' detected")); - Assert.IsTrue(exception.Message.Contains("built-in tool with this name is already registered")); - Assert.IsTrue(exception.Message.Contains("Cannot register custom tool with the same name")); - Assert.IsTrue(exception.Message.Contains("Tool names must be unique across all tool types")); - Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); - } - - /// - /// Test that registering a built-in tool with the same name as a custom tool throws an exception. - /// - [TestMethod] - public void RegisterTool_BuiltInToolConflictsWithCustom_ThrowsException() - { - // Arrange - McpToolRegistry registry = new(); - IMcpTool customTool = new MockMcpTool("my_stored_proc", ToolType.Custom); - IMcpTool builtInTool = new MockMcpTool("my_stored_proc", ToolType.BuiltIn); - - // Act - Register custom tool first - registry.RegisterTool(customTool); + registry.RegisterTool(existingTool); - // Assert - Built-in tool registration should throw + // Assert - Second tool registration should throw DataApiBuilderException exception = Assert.ThrowsException( - () => registry.RegisterTool(builtInTool) + () => registry.RegisterTool(conflictingTool) ); // Verify exception details - Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'my_stored_proc' detected")); - Assert.IsTrue(exception.Message.Contains("custom tool with this name is already registered")); - Assert.IsTrue(exception.Message.Contains("Cannot register built-in tool with the same name")); + Assert.IsTrue(exception.Message.Contains($"Duplicate MCP tool name '{toolName}' detected")); + Assert.IsTrue(exception.Message.Contains($"{expectedExistingType} tool with this name is already registered")); + Assert.IsTrue(exception.Message.Contains($"Cannot register {expectedNewType} tool with the same name")); Assert.IsTrue(exception.Message.Contains("Tool names must be unique across all tool types")); Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); + Assert.AreEqual(HttpStatusCode.ServiceUnavailable, exception.StatusCode); } /// /// Test that tool name comparison is case-sensitive. - /// Tools with different casing should be allowed. + /// Tools with different casing should not be allowed. /// [TestMethod] - public void RegisterTool_WithDifferentCasing_Succeeds() + public void RegisterTool_WithDifferentCasing_ThrowsException() { // Arrange McpToolRegistry registry = new(); IMcpTool tool1 = new MockMcpTool("my_tool", ToolType.BuiltIn); IMcpTool tool2 = new MockMcpTool("My_Tool", ToolType.Custom); - IMcpTool tool3 = new MockMcpTool("MY_TOOL", ToolType.Custom); - // Act & Assert - All should register successfully (case-sensitive) + // Act - Register first tool registry.RegisterTool(tool1); - registry.RegisterTool(tool2); - registry.RegisterTool(tool3); - // Verify all tools were registered - IEnumerable allTools = registry.GetAllTools(); - Assert.AreEqual(3, allTools.Count()); + // Assert - Case-insensitive duplicate should throw + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(tool2) + ); + + Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name")); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); } /// @@ -236,23 +182,22 @@ public void TryGetTool_WithNonExistentName_ReturnsFalse() } /// - /// Test edge case: empty tool name (if allowed by validation) + /// Test edge case: empty tool name should throw exception. /// [TestMethod] - public void RegisterTool_WithEmptyToolName_CanRegisterOnce() + public void RegisterTool_WithEmptyToolName_ThrowsException() { // Arrange McpToolRegistry registry = new(); - IMcpTool tool1 = new MockMcpTool("", ToolType.BuiltIn); - IMcpTool tool2 = new MockMcpTool("", ToolType.Custom); + IMcpTool tool = new MockMcpTool("", ToolType.BuiltIn); - // Act - Register first tool with empty name - registry.RegisterTool(tool1); - - // Assert - Second tool with empty name should throw - Assert.ThrowsException( - () => registry.RegisterTool(tool2) + // Assert - Empty tool names should be rejected + DataApiBuilderException exception = Assert.ThrowsException( + () => registry.RegisterTool(tool) ); + + Assert.IsTrue(exception.Message.Contains("cannot be null, empty, or whitespace")); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); } /// @@ -284,36 +229,27 @@ public void RegisterTool_WithRealisticBuiltInToolNames_DetectsDuplicates() } /// - /// Test that duplicate tool names (regardless of how they're generated) are caught. - /// This validates the duplicate detection mechanism works correctly. - /// Note: Entity names are already required to be unique at config level, - /// but this ensures the tool registry properly detects any duplicates that might occur. + /// Test that registering a tool with leading/trailing whitespace in the name is treated as a duplicate of the trimmed name. + /// Note: during tool registration, the registry should trim whitespace and detect duplicates accordingly. /// [TestMethod] - public void RegisterTool_WithConflictingSnakeCaseNames_DetectsDuplicates() + public void RegisterTool_WithLeadingTrailingWhitespace_DetectsDuplicate() { // Arrange McpToolRegistry registry = new(); + IMcpTool tool1 = new MockMcpTool("my_tool", ToolType.BuiltIn); + IMcpTool tool2 = new MockMcpTool(" my_tool ", ToolType.Custom); - // First tool with snake_case name - IMcpTool tool1 = new MockMcpTool("get_user", ToolType.Custom); - - // Second tool with the same name (duplicate) - IMcpTool tool2 = new MockMcpTool("get_user", ToolType.Custom); - - // Act - Register first tool + // Act registry.RegisterTool(tool1); - // Assert - Second registration should throw - DataApiBuilderException exception = Assert.ThrowsException( + // Assert - trimmed name should collide + Assert.ThrowsException( () => registry.RegisterTool(tool2) ); - - // Verify exception details - Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'get_user' detected")); } - #region Mock Tool Implementation + #region Private helpers /// /// Mock implementation of IMcpTool for testing purposes. @@ -352,6 +288,6 @@ public Task ExecuteAsync( } } - #endregion + #endregion Private helpers } }