[MCP] Fix for inconsistent MCP enabled check#3303
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes inconsistent MCP enablement behavior so the /mcp endpoint is registered by default when the runtime.mcp block is omitted, aligning runtime behavior with mcp.enabled defaulting to true in config/model layers (closes #3284).
Changes:
- Default MCP options resolution now falls back to
new McpRuntimeOptions()whenruntime.mcpis absent, enabling endpoint registration by default. - REST routing guard now throws
GlobalMcpEndpointDisabledonly when MCP is actually disabled. - Added unit tests covering MCP enabled/disabled routing guard behavior and config defaulting of
IsMcpEnabled/McpPath.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Azure.DataApiBuilder.Mcp/Core/McpEndpointRouteBuilderExtensions.cs | Ensures MCP endpoints are mapped by default when runtime.mcp is omitted. |
| src/Core/Services/RestService.cs | Gates the MCP-disabled 404 guard behind IsMcpEnabled. |
| src/Service.Tests/UnitTests/RestServiceUnitTests.cs | Adds unit tests for MCP path guard behavior in GetRouteAfterPathBase. |
| src/Service.Tests/Configuration/McpRuntimeOptionsSerializationTests.cs | Adds tests verifying IsMcpEnabled/McpPath defaults when runtime/mcp blocks are omitted. |
| mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions(); | ||
| return mcpOptions != null; | ||
| } |
There was a problem hiding this comment.
TryGetMcpOptions now returns a default new McpRuntimeOptions() even when the config has no runtime.mcp block, so the method name/XML docs ("found") and the return mcpOptions != null; are misleading/redundant (it will always be true after the assignment). Consider updating the docs/return semantics and simplifying the null checks in MapDabMcp accordingly.
| mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions(); | ||
| return mcpOptions != null; |
There was a problem hiding this comment.
The change makes MCP map by default when runtime.mcp is absent, but there’s no test exercising actual endpoint registration (e.g., a TestServer request to /mcp with a config omitting runtime/mcp). Adding an integration test would prevent regressions of #3284.
| mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions(); | |
| return mcpOptions != null; | |
| McpRuntimeOptions? configuredOptions = runtimeConfig.Runtime?.Mcp; | |
| if (configuredOptions is null) | |
| { | |
| return false; | |
| } | |
| mcpOptions = configuredOptions; | |
| return true; |
| if (route.Equals(_runtimeConfigProvider.GetConfig().McpPath.Substring(1)) | ||
| && !_runtimeConfigProvider.GetConfig().IsMcpEnabled) |
There was a problem hiding this comment.
GetRouteAfterPathBase calls _runtimeConfigProvider.GetConfig() three times and recomputes McpPath.Substring(1) on the hot path for every request. Consider storing the config (and the trimmed MCP route segment) in locals to avoid repeated config access and string operations.
| queryManagerFactory.Setup(x => x.GetQueryBuilder(It.IsAny<DatabaseType>())).Returns(queryBuilder); | ||
| queryManagerFactory.Setup(x => x.GetQueryExecutor(It.IsAny<DatabaseType>())).Returns(queryExecutor); | ||
|
|
||
| Mock<ISqlMetadataProvider> sqlMetadataProvider = new(); |
There was a problem hiding this comment.
InitializeTestWithMcpConfig duplicates most of InitializeTestWithEntityPaths and introduces additional unused locals (e.g., sqlMetadataProvider). Consider reusing the existing initializer and/or removing unused locals to keep the test setup smaller and easier to maintain.
| Mock<ISqlMetadataProvider> sqlMetadataProvider = new(); |
Why make this change?
mcp.enableddefaults are not working #3284MCP endpoint returns 404 when the mcp config block is omitted, despite mcp.enabled defaulting to true across schema, CLI, and model layers.
What is this change?
McpEndpointRouteBuilderExtensions.TryGetMcpOptions: Falls back to newMcpRuntimeOptions()whenRuntime.Mcpisnull, so the/mcpendpoint is registered by default.RestService.GetRouteAfterPathBase: Guards theGlobalMcpEndpointDisabledexception with anIsMcpEnabledcheck so MCP path requests are not unconditionally rejected.How was this tested?
Sample Request(s)
/mcp
/health