Fix autoentity validation & CLI/Schema mismatch bugs#3416
Fix autoentity validation & CLI/Schema mismatch bugs#3416RubenCerna2079 wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses two CLI/autoentities bugs in Data API builder: aligning the dab auto-config option naming with the JSON schema and improving dab validate behavior/error messaging when autoentity generation fails due to conflicts.
Changes:
- Renames the CLI option from
template.mcp.dml-tooltotemplate.mcp.dml-toolsto match the schema and updates related wiring in config generation. - Updates the autoentity/entity name conflict message in the MSSQL metadata provider to be more actionable.
- Wraps CLI validation in try/catch to avoid unhandled exceptions and log errors instead.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs | Updates conflict exception text for autoentity generation. |
| src/Cli/ConfigGenerator.cs | Adds exception handling to IsConfigValid and renames template MCP dml-tools option usage. |
| src/Cli/Commands/AutoConfigOptions.cs | Renames the CLI option to template.mcp.dml-tools and updates the corresponding property. |
| { | ||
| throw new DataApiBuilderException( | ||
| message: $"Entity with name '{entityName}' already exists. Cannot create new entity from autoentities definition '{autoentityName}'.", | ||
| message: $"Entity '{entityName}' conflicts with autoentity pattern '{autoentityName}'. Use --patterns.exclude to skip it.", |
There was a problem hiding this comment.
This exception message comes from the runtime metadata provider (not just the CLI) but it points users to the CLI switch --patterns.exclude. Consider rewording to reference the config path (e.g., autoentities.<definition>.patterns.exclude) or mention both, so the guidance is actionable when the service is started from an existing config file.
| message: $"Entity '{entityName}' conflicts with autoentity pattern '{autoentityName}'. Use --patterns.exclude to skip it.", | |
| message: $"Entity '{entityName}' conflicts with autoentity pattern '{autoentityName}'. Exclude it in config at autoentities.{autoentityName}.patterns.exclude or use the CLI switch --patterns.exclude.", |
| } | ||
| catch (AggregateException ex) | ||
| { | ||
| foreach (Exception exception in ex.InnerExceptions) |
There was a problem hiding this comment.
The catch for AggregateException iterates ex.InnerExceptions directly, which can miss nested exceptions (AggregateExceptions can be nested). Consider using ex.Flatten().InnerExceptions so all underlying validation errors get logged.
| foreach (Exception exception in ex.InnerExceptions) | |
| foreach (Exception exception in ex.Flatten().InnerExceptions) |
| bool isValid = false; | ||
| try | ||
| { | ||
| if (runtimeConfigProvider.TryGetConfig(out RuntimeConfig? config) && config is not null) | ||
| isValid = runtimeConfigValidator.TryValidateConfig(runtimeConfigFile, LoggerFactoryForCli).Result; | ||
|
|
||
| // Additional validation: warn if fields are missing and MCP is enabled | ||
| if (isValid) | ||
| { | ||
| bool mcpEnabled = config.IsMcpEnabled; | ||
| if (mcpEnabled) | ||
| if (runtimeConfigProvider.TryGetConfig(out RuntimeConfig? config) && config is not null) | ||
| { | ||
| foreach (KeyValuePair<string, Entity> entity in config.Entities) | ||
| bool mcpEnabled = config.IsMcpEnabled; | ||
| if (mcpEnabled) | ||
| { | ||
| if (entity.Value.Fields == null || !entity.Value.Fields.Any()) | ||
| foreach (KeyValuePair<string, Entity> entity in config.Entities) | ||
| { | ||
| _logger.LogWarning($"Entity '{entity.Key}' is missing 'fields' definition while MCP is enabled. " + | ||
| "It's recommended to define fields explicitly to ensure optimal performance with MCP."); | ||
| if (entity.Value.Fields == null || !entity.Value.Fields.Any()) | ||
| { | ||
| _logger.LogWarning($"Entity '{entity.Key}' is missing 'fields' definition while MCP is enabled. " + | ||
| "It's recommended to define fields explicitly to ensure optimal performance with MCP."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Warn if Unauthenticated provider is used with authenticated or custom roles | ||
| if (config.Runtime?.Host?.Authentication?.IsUnauthenticatedAuthenticationProvider() == true) | ||
| { | ||
| bool hasNonAnonymousRoles = config.Entities | ||
| .Where(e => e.Value.Permissions is not null) | ||
| .SelectMany(e => e.Value.Permissions!) | ||
| .Any(p => !p.Role.Equals("anonymous", StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (hasNonAnonymousRoles) | ||
| // Warn if Unauthenticated provider is used with authenticated or custom roles | ||
| if (config.Runtime?.Host?.Authentication?.IsUnauthenticatedAuthenticationProvider() == true) | ||
| { | ||
| _logger.LogWarning( | ||
| "Authentication provider is 'Unauthenticated' but some entities have permissions configured for non-anonymous roles. " + | ||
| "All requests will be treated as anonymous."); | ||
| bool hasNonAnonymousRoles = config.Entities | ||
| .Where(e => e.Value.Permissions is not null) | ||
| .SelectMany(e => e.Value.Permissions!) | ||
| .Any(p => !p.Role.Equals("anonymous", StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (hasNonAnonymousRoles) | ||
| { | ||
| _logger.LogWarning( | ||
| "Authentication provider is 'Unauthenticated' but some entities have permissions configured for non-anonymous roles. " + | ||
| "All requests will be treated as anonymous."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return isValid; | ||
| } | ||
| catch (AggregateException ex) | ||
| { | ||
| foreach (Exception exception in ex.InnerExceptions) | ||
| { | ||
| _logger.LogError(exception, exception.Message); | ||
| } | ||
|
|
||
| return isValid; | ||
| return isValid; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, ex.Message); | ||
| return isValid; | ||
| } |
There was a problem hiding this comment.
IsConfigValid now has new behavior for validation failures coming from the engine (catching/logging AggregateException instead of crashing). There are existing ValidateConfigTests for ConfigGenerator.IsConfigValid, but none cover the autoentity/entity name conflict scenario from #3335. Adding a regression test that asserts IsConfigValid returns false and prints the friendly conflict message would prevent reintroducing the unhandled-exception behavior.
| [Option("template.mcp.dml-tools", Required = false, HelpText = "Enable/disable DML tools for generated entities. Allowed values: true, false. Default: true")] | ||
| public string? TemplateMcpDmlTools { get; } |
There was a problem hiding this comment.
Changing the CLI option name from template.mcp.dml-tool to template.mcp.dml-tools fixes the schema mismatch, but it’s also a breaking CLI change for anyone using the old flag. Consider accepting both as aliases (and potentially marking the old one hidden/deprecated) to avoid silently breaking existing scripts.
| public AutoConfigOptions( | ||
| string definitionName, | ||
| IEnumerable<string>? patternsInclude = null, | ||
| IEnumerable<string>? patternsExclude = null, | ||
| string? patternsName = null, | ||
| string? templateMcpDmlTool = null, | ||
| bool? templateRestEnabled = null, | ||
| bool? templateGraphqlEnabled = null, | ||
| bool? templateCacheEnabled = null, | ||
| int? templateCacheTtlSeconds = null, | ||
| string? templateCacheLevel = null, | ||
| bool? templateHealthEnabled = null, | ||
| IEnumerable<string>? permissions = null, | ||
| string? config = null) | ||
| : base(config) | ||
| { | ||
| DefinitionName = definitionName; | ||
| PatternsInclude = patternsInclude; | ||
| PatternsExclude = patternsExclude; | ||
| PatternsName = patternsName; | ||
| TemplateMcpDmlTool = templateMcpDmlTool; | ||
| TemplateMcpDmlTools = templateMcpDmlTool; | ||
| TemplateRestEnabled = templateRestEnabled; |
There was a problem hiding this comment.
In the constructor assignment TemplateMcpDmlTools = templateMcpDmlTool;, the parameter name remains singular (templateMcpDmlTool) while the option/property are now plural (dml-tools). Renaming the parameter (and any call sites) to templateMcpDmlTools would reduce confusion and keep naming consistent.
souvikghosh04
left a comment
There was a problem hiding this comment.
Added some comments
| [Option("template.mcp.dml-tool", Required = false, HelpText = "Enable/disable DML tools for generated entities. Allowed values: true, false. Default: true")] | ||
| public string? TemplateMcpDmlTool { get; } | ||
| [Option("template.mcp.dml-tools", Required = false, HelpText = "Enable/disable DML tools for generated entities. Allowed values: true, false. Default: true")] | ||
| public string? TemplateMcpDmlTools { get; } |
There was a problem hiding this comment.
do we have test coverage for this and the below changes?
| { | ||
| foreach (KeyValuePair<string, Entity> entity in config.Entities) | ||
| bool mcpEnabled = config.IsMcpEnabled; | ||
| if (mcpEnabled) |
There was a problem hiding this comment.
nit: just use config.IsMcpEnabled instead of a new variable
| { | ||
| foreach (Exception exception in ex.InnerExceptions) | ||
| { | ||
| _logger.LogError(exception, exception.Message); |
There was a problem hiding this comment.
you might not need to pass the exception object. just the message as its done in other places.
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, ex.Message); |
Why make this change?
autoentities.<def-name>.template.mcp.dml-tools, need to ensure that they are the same.dab validateproduces the wrong output message.What is this change?
For issue #3375:
AutoConfigOption.csfile so that it uses the proper name and changed the name of the variable to also match the schema in theConfigGenerator.cs.For issue #3335:
MsSqlMetadataProvider.csso that it is easier for the user to understand the error.ConfigGenerator.csso that instead of outputing an exception when dab throws an error it just logs the exception.How was this tested?
The issues were related to mismatches or to the output of log messages that can only be tested locally.
Sample Request(s)
dab auto-config --template.mcp.dml-tools true/false
dab validate --config test.json