From 321b8fc1dccda3925bafc581ea998d87bf1bce9d Mon Sep 17 00:00:00 2001 From: souvikghosh04 Date: Mon, 23 Mar 2026 10:30:43 +0530 Subject: [PATCH 1/4] [MCP] Fix for inconsistent MCP enabled check --- .../Core/McpEndpointRouteBuilderExtensions.cs | 2 +- src/Core/Services/RestService.cs | 3 +- .../McpRuntimeOptionsSerializationTests.cs | 89 ++++++++++++ .../UnitTests/RestServiceUnitTests.cs | 127 ++++++++++++++++++ 4 files changed, 219 insertions(+), 2 deletions(-) diff --git a/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs b/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs index 6401e17e22..1f633643f3 100644 --- a/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs +++ b/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs @@ -50,7 +50,7 @@ private static bool TryGetMcpOptions(RuntimeConfigProvider runtimeConfigProvider return false; } - mcpOptions = runtimeConfig?.Runtime?.Mcp; + mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions(); return mcpOptions != null; } } diff --git a/src/Core/Services/RestService.cs b/src/Core/Services/RestService.cs index 2bfab7e05f..41b7848963 100644 --- a/src/Core/Services/RestService.cs +++ b/src/Core/Services/RestService.cs @@ -410,7 +410,8 @@ public string GetRouteAfterPathBase(string route) // forward slash '/'. configuredRestPathBase = configuredRestPathBase.Substring(1); - if (route.Equals(_runtimeConfigProvider.GetConfig().McpPath.Substring(1))) + if (route.Equals(_runtimeConfigProvider.GetConfig().McpPath.Substring(1)) + && !_runtimeConfigProvider.GetConfig().IsMcpEnabled) { throw new DataApiBuilderException( message: $"Route {route} was not found.", diff --git a/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs b/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs index eefa1f08f4..714c33d6f8 100644 --- a/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs +++ b/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs @@ -191,6 +191,95 @@ public void TestBackwardCompatibilityDeserializationWithoutDescriptionField() Assert.IsNull(deserializedConfig.Runtime.Mcp.Description, "Description should be null when not present in JSON"); } + /// + /// When the mcp and runtime config blocks are omitted entirely, IsMcpEnabled should default to true. + /// This validates the fix for GitHub issue #3284. + /// + [TestMethod] + public void TestIsMcpEnabledDefaultsTrueWhenMcpAndRuntimeBlocksAbsent() + { + // Arrange - config with no runtime/mcp block + string configJson = @"{ + ""$schema"": ""test-schema"", + ""data-source"": { + ""database-type"": ""mssql"", + ""connection-string"": ""Server=test;Database=test;"" + }, + ""entities"": {} + }"; + + // Act + bool parseSuccess = RuntimeConfigLoader.TryParseConfig(configJson, out RuntimeConfig config); + + // Assert + Assert.IsTrue(parseSuccess); + Assert.IsNull(config.Runtime, "Runtime should be null when not specified"); + Assert.IsNull(config.Runtime?.Mcp, "Mcp should be null when not specified"); + Assert.IsTrue(config.IsMcpEnabled, "IsMcpEnabled should default to true when mcp block is absent"); + Assert.AreEqual(McpRuntimeOptions.DEFAULT_PATH, config.McpPath, "McpPath should return default when mcp block is absent"); + } + + /// + /// When the mcp config block is present but enabled is not specified, + /// IsMcpEnabled should default to true. + /// + [TestMethod] + public void TestIsMcpEnabledDefaultsTrueWhenEnabledNotSpecified() + { + // Arrange - config with mcp block but no enabled field + string configJson = @"{ + ""$schema"": ""test-schema"", + ""data-source"": { + ""database-type"": ""mssql"", + ""connection-string"": ""Server=test;Database=test;"" + }, + ""runtime"": { + ""mcp"": { + ""path"": ""/mcp"" + } + }, + ""entities"": {} + }"; + + // Act + bool parseSuccess = RuntimeConfigLoader.TryParseConfig(configJson, out RuntimeConfig config); + + // Assert + Assert.IsTrue(parseSuccess); + Assert.IsNotNull(config.Runtime?.Mcp); + Assert.IsTrue(config.IsMcpEnabled, "IsMcpEnabled should default to true when enabled is not specified"); + } + + /// + /// When mcp.enabled is explicitly set to false, IsMcpEnabled should return false. + /// + [TestMethod] + public void TestIsMcpEnabledReturnsFalseWhenExplicitlyDisabled() + { + // Arrange + string configJson = @"{ + ""$schema"": ""test-schema"", + ""data-source"": { + ""database-type"": ""mssql"", + ""connection-string"": ""Server=test;Database=test;"" + }, + ""runtime"": { + ""mcp"": { + ""enabled"": false + } + }, + ""entities"": {} + }"; + + // Act + bool parseSuccess = RuntimeConfigLoader.TryParseConfig(configJson, out RuntimeConfig config); + + // Assert + Assert.IsTrue(parseSuccess); + Assert.IsNotNull(config.Runtime?.Mcp); + Assert.IsFalse(config.IsMcpEnabled, "IsMcpEnabled should be false when explicitly disabled"); + } + /// /// Creates a minimal RuntimeConfig with the specified MCP options for testing. /// diff --git a/src/Service.Tests/UnitTests/RestServiceUnitTests.cs b/src/Service.Tests/UnitTests/RestServiceUnitTests.cs index ac7b3667e6..36393bc353 100644 --- a/src/Service.Tests/UnitTests/RestServiceUnitTests.cs +++ b/src/Service.Tests/UnitTests/RestServiceUnitTests.cs @@ -167,6 +167,46 @@ public void SinglePathMatchingTest() #endregion + #region MCP Path Guard Tests + + /// + /// When MCP is explicitly disabled and the route matches the MCP path (default or custom), + /// GetRouteAfterPathBase should throw GlobalMcpEndpointDisabled. + /// + [DataTestMethod] + [DataRow("/mcp", "mcp", DisplayName = "MCP disabled with default path")] + [DataRow("/custom-mcp", "custom-mcp", DisplayName = "MCP disabled with custom path")] + public void McpPathThrowsWhenMcpDisabled(string mcpPath, string route) + { + InitializeTestWithMcpConfig("/api", new McpRuntimeOptions(Enabled: false, Path: mcpPath)); + DataApiBuilderException ex = Assert.ThrowsException( + () => _restService.GetRouteAfterPathBase(route)); + Assert.AreEqual(HttpStatusCode.NotFound, ex.StatusCode); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.GlobalMcpEndpointDisabled, ex.SubStatusCode); + } + + /// + /// When MCP is enabled (explicitly or by default when config is absent), + /// the MCP path route should NOT throw GlobalMcpEndpointDisabled. + /// It falls through to the normal path-base check. + /// + [DataTestMethod] + [DataRow(true, DisplayName = "MCP explicitly enabled")] + [DataRow(null, DisplayName = "MCP config absent (defaults to enabled)")] + public void McpPathDoesNotThrowGlobalMcpEndpointDisabledWhenMcpEnabled(bool? mcpEnabled) + { + McpRuntimeOptions mcpOptions = mcpEnabled.HasValue ? new McpRuntimeOptions(Enabled: mcpEnabled.Value) : null; + InitializeTestWithMcpConfig("/api", mcpOptions); + // "mcp" doesn't start with "api", so it should throw BadRequest (invalid path), + // NOT GlobalMcpEndpointDisabled. + DataApiBuilderException ex = Assert.ThrowsException( + () => _restService.GetRouteAfterPathBase("mcp")); + Assert.AreEqual(HttpStatusCode.BadRequest, ex.StatusCode); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.BadRequest, ex.SubStatusCode); + } + + #endregion + #region Helper Functions /// @@ -306,6 +346,93 @@ private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictio provider, requestValidator); } + + /// + /// Initializes RestService with a specific MCP configuration for testing MCP path guard behavior. + /// + /// REST path prefix (e.g., "/api"). + /// MCP options, or null to simulate absent mcp config block. + private static void InitializeTestWithMcpConfig(string restRoutePrefix, McpRuntimeOptions mcpOptions) + { + RuntimeConfig mockConfig = new( + Schema: "", + DataSource: new(DatabaseType.PostgreSQL, "", new()), + Runtime: new( + Rest: new(Path: restRoutePrefix), + GraphQL: new(), + Mcp: mcpOptions, + Host: new(null, null) + ), + Entities: new(new Dictionary()) + ); + + MockFileSystem fileSystem = new(); + fileSystem.AddFile(FileSystemRuntimeConfigLoader.DEFAULT_CONFIG_FILE_NAME, new MockFileData(mockConfig.ToJson())); + FileSystemRuntimeConfigLoader loader = new(fileSystem); + RuntimeConfigProvider provider = new(loader); + MsSqlQueryBuilder queryBuilder = new(); + Mock dbExceptionParser = new(provider); + Mock>> queryExecutorLogger = new(); + Mock> queryEngineLogger = new(); + Mock httpContextAccessor = new(); + Mock metadataProviderFactory = new(); + Mock queryManagerFactory = new(); + Mock queryEngineFactory = new(); + + MsSqlQueryExecutor queryExecutor = new( + provider, + dbExceptionParser.Object, + queryExecutorLogger.Object, + httpContextAccessor.Object); + + queryManagerFactory.Setup(x => x.GetQueryBuilder(It.IsAny())).Returns(queryBuilder); + queryManagerFactory.Setup(x => x.GetQueryExecutor(It.IsAny())).Returns(queryExecutor); + + Mock sqlMetadataProvider = new(); + Mock authorizationService = new(); + DefaultHttpContext context = new(); + httpContextAccessor.Setup(_ => _.HttpContext).Returns(context); + AuthorizationResolver authorizationResolver = new(provider, metadataProviderFactory.Object); + GQLFilterParser gQLFilterParser = new(provider, metadataProviderFactory.Object); + + Mock cache = new(); + DabCacheService cacheService = new(cache.Object, logger: null, httpContextAccessor.Object); + + SqlQueryEngine queryEngine = new( + queryManagerFactory.Object, + metadataProviderFactory.Object, + httpContextAccessor.Object, + authorizationResolver, + gQLFilterParser, + queryEngineLogger.Object, + provider, + cacheService); + + queryEngineFactory.Setup(x => x.GetQueryEngine(It.IsAny())).Returns(queryEngine); + + SqlMutationEngine mutationEngine = + new( + queryManagerFactory.Object, + metadataProviderFactory.Object, + queryEngineFactory.Object, + authorizationResolver, + gQLFilterParser, + httpContextAccessor.Object, + provider); + + Mock mutationEngineFactory = new(); + mutationEngineFactory.Setup(x => x.GetMutationEngine(It.IsAny())).Returns(mutationEngine); + RequestValidator requestValidator = new(metadataProviderFactory.Object, provider); + + _restService = new RestService( + queryEngineFactory.Object, + mutationEngineFactory.Object, + metadataProviderFactory.Object, + httpContextAccessor.Object, + authorizationService.Object, + provider, + requestValidator); + } #endregion } } From 50eaab91effd593c5cb461956d5c1605f3aa1652 Mon Sep 17 00:00:00 2001 From: souvikghosh04 Date: Tue, 24 Mar 2026 11:29:00 +0530 Subject: [PATCH 2/4] Fixed self reviews and refactoring --- .../Core/McpEndpointRouteBuilderExtensions.cs | 28 ++----- src/Core/Services/RestService.cs | 7 +- .../UnitTests/RestServiceUnitTests.cs | 83 +------------------ 3 files changed, 15 insertions(+), 103 deletions(-) diff --git a/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs b/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs index 1f633643f3..bb3a1e5dba 100644 --- a/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs +++ b/src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs @@ -22,7 +22,14 @@ public static IEndpointRouteBuilder MapDabMcp( RuntimeConfigProvider runtimeConfigProvider, [StringSyntax("Route")] string pattern = "") { - if (!TryGetMcpOptions(runtimeConfigProvider, out McpRuntimeOptions? mcpOptions) || mcpOptions == null || !mcpOptions.Enabled) + if (!runtimeConfigProvider.TryGetConfig(out RuntimeConfig? runtimeConfig)) + { + return endpoints; + } + + McpRuntimeOptions mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions(); + + if (!mcpOptions.Enabled) { return endpoints; } @@ -34,24 +41,5 @@ public static IEndpointRouteBuilder MapDabMcp( return endpoints; } - - /// - /// Gets MCP options from the runtime configuration - /// - /// Runtime config provider - /// MCP options - /// True if MCP options were found, false otherwise - private static bool TryGetMcpOptions(RuntimeConfigProvider runtimeConfigProvider, out McpRuntimeOptions? mcpOptions) - { - mcpOptions = null; - - if (!runtimeConfigProvider.TryGetConfig(out RuntimeConfig? runtimeConfig)) - { - return false; - } - - mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions(); - return mcpOptions != null; - } } } diff --git a/src/Core/Services/RestService.cs b/src/Core/Services/RestService.cs index 41b7848963..5014d942bb 100644 --- a/src/Core/Services/RestService.cs +++ b/src/Core/Services/RestService.cs @@ -402,7 +402,8 @@ private bool TryGetStoredProcedureRESTVerbs(string entityName, [NotNullWhen(true /// does not match the configured REST path or the global REST endpoint is disabled. public string GetRouteAfterPathBase(string route) { - string configuredRestPathBase = _runtimeConfigProvider.GetConfig().RestPath; + RuntimeConfig runtimeConfig = _runtimeConfigProvider.GetConfig(); + string configuredRestPathBase = runtimeConfig.RestPath; // Strip the leading '/' from the REST path provided in the runtime configuration // because the input argument 'route' has no starting '/'. @@ -410,8 +411,8 @@ public string GetRouteAfterPathBase(string route) // forward slash '/'. configuredRestPathBase = configuredRestPathBase.Substring(1); - if (route.Equals(_runtimeConfigProvider.GetConfig().McpPath.Substring(1)) - && !_runtimeConfigProvider.GetConfig().IsMcpEnabled) + if (route.Equals(runtimeConfig.McpPath.Substring(1)) + && !runtimeConfig.IsMcpEnabled) { throw new DataApiBuilderException( message: $"Route {route} was not found.", diff --git a/src/Service.Tests/UnitTests/RestServiceUnitTests.cs b/src/Service.Tests/UnitTests/RestServiceUnitTests.cs index 36393bc353..88a34b903d 100644 --- a/src/Service.Tests/UnitTests/RestServiceUnitTests.cs +++ b/src/Service.Tests/UnitTests/RestServiceUnitTests.cs @@ -255,7 +255,7 @@ public static void InitializeTestWithMultipleEntityPaths(string restRoutePrefix, /// /// Core helper to initialize REST Service with specified entity path mappings. /// - private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictionary entityPaths) + private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictionary entityPaths, McpRuntimeOptions mcpOptions = null, bool useMcpParam = false) { RuntimeConfig mockConfig = new( Schema: "", @@ -263,7 +263,7 @@ private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictio Runtime: new( Rest: new(Path: restRoutePrefix), GraphQL: new(), - Mcp: new(), + Mcp: useMcpParam ? mcpOptions : (mcpOptions ?? new()), Host: new(null, null) ), Entities: new(new Dictionary()) @@ -354,84 +354,7 @@ private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictio /// MCP options, or null to simulate absent mcp config block. private static void InitializeTestWithMcpConfig(string restRoutePrefix, McpRuntimeOptions mcpOptions) { - RuntimeConfig mockConfig = new( - Schema: "", - DataSource: new(DatabaseType.PostgreSQL, "", new()), - Runtime: new( - Rest: new(Path: restRoutePrefix), - GraphQL: new(), - Mcp: mcpOptions, - Host: new(null, null) - ), - Entities: new(new Dictionary()) - ); - - MockFileSystem fileSystem = new(); - fileSystem.AddFile(FileSystemRuntimeConfigLoader.DEFAULT_CONFIG_FILE_NAME, new MockFileData(mockConfig.ToJson())); - FileSystemRuntimeConfigLoader loader = new(fileSystem); - RuntimeConfigProvider provider = new(loader); - MsSqlQueryBuilder queryBuilder = new(); - Mock dbExceptionParser = new(provider); - Mock>> queryExecutorLogger = new(); - Mock> queryEngineLogger = new(); - Mock httpContextAccessor = new(); - Mock metadataProviderFactory = new(); - Mock queryManagerFactory = new(); - Mock queryEngineFactory = new(); - - MsSqlQueryExecutor queryExecutor = new( - provider, - dbExceptionParser.Object, - queryExecutorLogger.Object, - httpContextAccessor.Object); - - queryManagerFactory.Setup(x => x.GetQueryBuilder(It.IsAny())).Returns(queryBuilder); - queryManagerFactory.Setup(x => x.GetQueryExecutor(It.IsAny())).Returns(queryExecutor); - - Mock sqlMetadataProvider = new(); - Mock authorizationService = new(); - DefaultHttpContext context = new(); - httpContextAccessor.Setup(_ => _.HttpContext).Returns(context); - AuthorizationResolver authorizationResolver = new(provider, metadataProviderFactory.Object); - GQLFilterParser gQLFilterParser = new(provider, metadataProviderFactory.Object); - - Mock cache = new(); - DabCacheService cacheService = new(cache.Object, logger: null, httpContextAccessor.Object); - - SqlQueryEngine queryEngine = new( - queryManagerFactory.Object, - metadataProviderFactory.Object, - httpContextAccessor.Object, - authorizationResolver, - gQLFilterParser, - queryEngineLogger.Object, - provider, - cacheService); - - queryEngineFactory.Setup(x => x.GetQueryEngine(It.IsAny())).Returns(queryEngine); - - SqlMutationEngine mutationEngine = - new( - queryManagerFactory.Object, - metadataProviderFactory.Object, - queryEngineFactory.Object, - authorizationResolver, - gQLFilterParser, - httpContextAccessor.Object, - provider); - - Mock mutationEngineFactory = new(); - mutationEngineFactory.Setup(x => x.GetMutationEngine(It.IsAny())).Returns(mutationEngine); - RequestValidator requestValidator = new(metadataProviderFactory.Object, provider); - - _restService = new RestService( - queryEngineFactory.Object, - mutationEngineFactory.Object, - metadataProviderFactory.Object, - httpContextAccessor.Object, - authorizationService.Object, - provider, - requestValidator); + InitializeTestWithEntityPaths(restRoutePrefix, new Dictionary(), mcpOptions, useMcpParam: true); } #endregion } From 64eea164a32334c4bcbd24e82e9b2b2baf9a94f1 Mon Sep 17 00:00:00 2001 From: souvikghosh04 Date: Tue, 24 Mar 2026 14:13:46 +0530 Subject: [PATCH 3/4] Add check in config generator --- src/Cli/ConfigGenerator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cli/ConfigGenerator.cs b/src/Cli/ConfigGenerator.cs index 8cec9dd239..02f6d6ec57 100644 --- a/src/Cli/ConfigGenerator.cs +++ b/src/Cli/ConfigGenerator.cs @@ -2653,7 +2653,7 @@ public static bool IsConfigValid(ValidateOptions options, FileSystemRuntimeConfi { if (runtimeConfigProvider.TryGetConfig(out RuntimeConfig? config) && config is not null) { - bool mcpEnabled = config.Runtime?.Mcp?.Enabled == true; + bool mcpEnabled = config.IsMcpEnabled; if (mcpEnabled) { foreach (KeyValuePair entity in config.Entities) From a58fa8fbf894fa393185a9b4d2f0ee6cce24c130 Mon Sep 17 00:00:00 2001 From: souvikghosh04 Date: Wed, 25 Mar 2026 08:51:58 +0530 Subject: [PATCH 4/4] Removed the issue reference from comments/XML doc --- .../Configuration/McpRuntimeOptionsSerializationTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs b/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs index 714c33d6f8..f706048107 100644 --- a/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs +++ b/src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs @@ -193,7 +193,6 @@ public void TestBackwardCompatibilityDeserializationWithoutDescriptionField() /// /// When the mcp and runtime config blocks are omitted entirely, IsMcpEnabled should default to true. - /// This validates the fix for GitHub issue #3284. /// [TestMethod] public void TestIsMcpEnabledDefaultsTrueWhenMcpAndRuntimeBlocksAbsent()