Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +53 to 54
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
mcpOptions = runtimeConfig?.Runtime?.Mcp ?? new McpRuntimeOptions();
return mcpOptions != null;
McpRuntimeOptions? configuredOptions = runtimeConfig.Runtime?.Mcp;
if (configuredOptions is null)
{
return false;
}
mcpOptions = configuredOptions;
return true;

Copilot uses AI. Check for mistakes.
}
Comment on lines +53 to 55
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
Expand Down
3 changes: 2 additions & 1 deletion src/Core/Services/RestService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +413 to +414
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
{
throw new DataApiBuilderException(
message: $"Route {route} was not found.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,95 @@ public void TestBackwardCompatibilityDeserializationWithoutDescriptionField()
Assert.IsNull(deserializedConfig.Runtime.Mcp.Description, "Description should be null when not present in JSON");
}

/// <summary>
/// When the mcp and runtime config blocks are omitted entirely, IsMcpEnabled should default to true.
/// This validates the fix for GitHub issue #3284.
/// </summary>
[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");
}

/// <summary>
/// When the mcp config block is present but enabled is not specified,
/// IsMcpEnabled should default to true.
/// </summary>
[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");
}

/// <summary>
/// When mcp.enabled is explicitly set to false, IsMcpEnabled should return false.
/// </summary>
[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");
}

/// <summary>
/// Creates a minimal RuntimeConfig with the specified MCP options for testing.
/// </summary>
Expand Down
127 changes: 127 additions & 0 deletions src/Service.Tests/UnitTests/RestServiceUnitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,46 @@ public void SinglePathMatchingTest()

#endregion

#region MCP Path Guard Tests

/// <summary>
/// When MCP is explicitly disabled and the route matches the MCP path (default or custom),
/// GetRouteAfterPathBase should throw GlobalMcpEndpointDisabled.
/// </summary>
[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<DataApiBuilderException>(
() => _restService.GetRouteAfterPathBase(route));
Assert.AreEqual(HttpStatusCode.NotFound, ex.StatusCode);
Assert.AreEqual(DataApiBuilderException.SubStatusCodes.GlobalMcpEndpointDisabled, ex.SubStatusCode);
}

/// <summary>
/// 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.
/// </summary>
[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<DataApiBuilderException>(
() => _restService.GetRouteAfterPathBase("mcp"));
Assert.AreEqual(HttpStatusCode.BadRequest, ex.StatusCode);
Assert.AreEqual(DataApiBuilderException.SubStatusCodes.BadRequest, ex.SubStatusCode);
}

#endregion

#region Helper Functions

/// <summary>
Expand Down Expand Up @@ -306,6 +346,93 @@ private static void InitializeTestWithEntityPaths(string restRoutePrefix, Dictio
provider,
requestValidator);
}

/// <summary>
/// Initializes RestService with a specific MCP configuration for testing MCP path guard behavior.
/// </summary>
/// <param name="restRoutePrefix">REST path prefix (e.g., "/api").</param>
/// <param name="mcpOptions">MCP options, or null to simulate absent mcp config block.</param>
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<string, Entity>())
);

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> dbExceptionParser = new(provider);
Mock<ILogger<QueryExecutor<SqlConnection>>> queryExecutorLogger = new();
Mock<ILogger<IQueryEngine>> queryEngineLogger = new();
Mock<IHttpContextAccessor> httpContextAccessor = new();
Mock<IMetadataProviderFactory> metadataProviderFactory = new();
Mock<IAbstractQueryManagerFactory> queryManagerFactory = new();
Mock<IQueryEngineFactory> queryEngineFactory = new();

MsSqlQueryExecutor queryExecutor = new(
provider,
dbExceptionParser.Object,
queryExecutorLogger.Object,
httpContextAccessor.Object);

queryManagerFactory.Setup(x => x.GetQueryBuilder(It.IsAny<DatabaseType>())).Returns(queryBuilder);
queryManagerFactory.Setup(x => x.GetQueryExecutor(It.IsAny<DatabaseType>())).Returns(queryExecutor);

Mock<ISqlMetadataProvider> sqlMetadataProvider = new();
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
Mock<ISqlMetadataProvider> sqlMetadataProvider = new();

Copilot uses AI. Check for mistakes.
Mock<IAuthorizationService> authorizationService = new();
DefaultHttpContext context = new();
httpContextAccessor.Setup(_ => _.HttpContext).Returns(context);
AuthorizationResolver authorizationResolver = new(provider, metadataProviderFactory.Object);
GQLFilterParser gQLFilterParser = new(provider, metadataProviderFactory.Object);

Mock<IFusionCache> 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<DatabaseType>())).Returns(queryEngine);

SqlMutationEngine mutationEngine =
new(
queryManagerFactory.Object,
metadataProviderFactory.Object,
queryEngineFactory.Object,
authorizationResolver,
gQLFilterParser,
httpContextAccessor.Object,
provider);

Mock<IMutationEngineFactory> mutationEngineFactory = new();
mutationEngineFactory.Setup(x => x.GetMutationEngine(It.IsAny<DatabaseType>())).Returns(mutationEngine);
RequestValidator requestValidator = new(metadataProviderFactory.Object, provider);

_restService = new RestService(
queryEngineFactory.Object,
mutationEngineFactory.Object,
metadataProviderFactory.Object,
httpContextAccessor.Object,
authorizationService.Object,
provider,
requestValidator);
}
#endregion
}
}
Loading