Conversation
Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
dab autoentities-configure
- Fix GraphQL template to only serialize 'enabled' property (not 'type') - Improve error messages to show all valid parameter values - Use EnumExtensions.GenerateMessageForInvalidInput for EntityCacheLevel - Update MCP dml-tool error message to show valid values Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
- Require permissions when creating a new autoentity definition - Allow updating existing definitions without providing permissions - Improve error handling to distinguish between parsing failure and missing permissions Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
RubenCerna2079
requested review from
Aniruddh25,
aaronburtle,
anushakolan and
souvikghosh04
as code owners
RubenCerna2079
requested review from
neeraj-sharma2592 and
sourabh1007
as code owners
RubenCerna2079
requested review from
Alekhya-Polavarapu,
rusamant and
vadeveka
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds a new DAB CLI verb to upsert autoentities definitions in the runtime config, so users can configure auto-entity patterns/template/permissions without manually editing JSON. This also updates JSON converters to correctly persist new template options.
Changes:
- Added
autoentities-configureCLI command/options and handler logic (TryConfigureAutoentities) to upsert autoentities definitions. - Updated autoentities JSON converters to adjust GraphQL template serialization and to include MCP template serialization.
- Added unit tests for the new CLI behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Config/Converters/AutoentityTemplateConverter.cs | Changes template serialization (GraphQL now writes only enabled; adds MCP serialization). |
| src/Config/Converters/AutoentityConverter.cs | Ensures template is emitted when MCP user-provided; avoids writing empty permissions. |
| src/Cli/Program.cs | Registers AutoentitiesConfigureOptions with the CLI parser. |
| src/Cli/ConfigGenerator.cs | Implements TryConfigureAutoentities and builders for patterns/template/permissions. |
| src/Cli/Commands/AutoentitiesConfigureOptions.cs | Adds CLI verb/options and handler for autoentities-configure. |
| src/Cli.Tests/AutoentitiesConfigureTests.cs | Adds unit tests covering create/update and invalid inputs. |
| if (value?.UserProvidedMcpOptions is true) | ||
| { | ||
| writer.WritePropertyName("mcp"); | ||
| JsonSerializer.Serialize(writer, value.Mcp, options); |
There was a problem hiding this comment.
autoentities.template.mcp is serialized via JsonSerializer.Serialize(writer, value.Mcp, options), which will use EntityMcpOptionsConverterFactory and may emit the boolean shorthand (true/false) when only dml-tools was set. The JSON schema for autoentities.template.mcp only allows an object with the dml-tools property (no boolean shorthand and no custom-tool), so the CLI can end up writing a config that fails schema validation (dab validate). Consider writing mcp manually as an object that only includes dml-tools when UserProvidedMcpOptions is true (and avoid emitting/propagating custom-tool in autoentities templates).
| if (value?.UserProvidedMcpOptions is true) | |
| { | |
| writer.WritePropertyName("mcp"); | |
| JsonSerializer.Serialize(writer, value.Mcp, options); | |
| if (value?.UserProvidedMcpOptions is true && value.Mcp is not null) | |
| { | |
| writer.WritePropertyName("mcp"); | |
| writer.WriteStartObject(); | |
| if (value.Mcp.DmlTools is not null) | |
| { | |
| writer.WritePropertyName("dml-tools"); | |
| JsonSerializer.Serialize(writer, value.Mcp.DmlTools, options); | |
| } | |
| writer.WriteEndObject(); |
| if (options.TemplateCacheTtlSeconds is not null) | ||
| { | ||
| cacheTtl = options.TemplateCacheTtlSeconds.Value; | ||
| cacheUpdated = true; | ||
| _logger.LogInformation("Updated template.cache.ttl-seconds for definition '{DefinitionName}'", options.DefinitionName); | ||
| } |
There was a problem hiding this comment.
template.cache.ttl-seconds is accepted and written without validation. The schema requires a minimum of 1 second, so values like 0 or negative numbers can be persisted by the CLI and then fail schema validation. Consider validating options.TemplateCacheTtlSeconds (e.g., > 0, reusing RuntimeConfigValidatorUtil.IsTTLValid) and returning null/false with a clear error message when invalid.
src/Cli/ConfigGenerator.cs
Outdated
| // Get existing autoentity definition or create a new one | ||
| Autoentity? existingAutoentity = null; | ||
| if (autoEntitiesDictionary.TryGetValue(options.DefinitionName, out Autoentity? value)) | ||
| { | ||
| existingAutoentity = value; | ||
| } | ||
|
|
||
| // Build patterns | ||
| AutoentityPatterns patterns = BuildAutoentityPatterns(options, existingAutoentity); | ||
|
|
||
| // Build template | ||
| AutoentityTemplate? template = BuildAutoentityTemplate(options, existingAutoentity); | ||
| if (template is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Build permissions | ||
| EntityPermission[]? permissions = BuildAutoentityPermissions(options, existingAutoentity); | ||
|
|
There was a problem hiding this comment.
The implementation currently allows creating a brand-new autoentities definition without any permissions (and even with no other options, resulting in an empty {} definition written to config). This contradicts the PR description stating permissions are required for new definitions and can lead to generated entities with no access rules configured. Consider adding an explicit check when existingAutoentity is null to require --permissions (and/or require at least one configurable option) before upserting the new definition.
| public void TestConfigureAutoentitiesDefinition_WithTemplateOptions() | ||
| { | ||
| // Arrange | ||
| InitOptions initOptions = CreateBasicInitOptionsForMsSqlWithConfig(config: TEST_RUNTIME_CONFIG_FILE); | ||
| Assert.IsTrue(ConfigGenerator.TryGenerateConfig(initOptions, _runtimeConfigLoader!, _fileSystem!)); | ||
|
|
||
| AutoentitiesConfigureOptions options = new( | ||
| definitionName: "test-def", | ||
| templateRestEnabled: true, | ||
| templateGraphqlEnabled: false, | ||
| templateMcpDmlTool: "true", | ||
| templateCacheEnabled: true, | ||
| templateCacheTtlSeconds: 30, | ||
| templateCacheLevel: "L1", | ||
| templateHealthEnabled: true, | ||
| config: TEST_RUNTIME_CONFIG_FILE | ||
| ); | ||
|
|
||
| // Act | ||
| bool success = ConfigGenerator.TryConfigureAutoentities(options, _runtimeConfigLoader!, _fileSystem!); | ||
|
|
||
| // Assert | ||
| Assert.IsTrue(success); | ||
| Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? config)); | ||
|
|
||
| Autoentity autoentity = config.Autoentities!.AutoEntities["test-def"]; | ||
| Assert.IsTrue(autoentity.Template.Rest.Enabled); | ||
| Assert.IsFalse(autoentity.Template.GraphQL.Enabled); | ||
| Assert.IsTrue(autoentity.Template.Mcp!.DmlToolEnabled); | ||
| Assert.AreEqual(true, autoentity.Template.Cache.Enabled); | ||
| Assert.AreEqual(30, autoentity.Template.Cache.TtlSeconds); | ||
| Assert.AreEqual(EntityCacheLevel.L1, autoentity.Template.Cache.Level); | ||
| Assert.IsTrue(autoentity.Template.Health.Enabled); | ||
| } |
There was a problem hiding this comment.
The unit tests validate Template.Mcp.DmlToolEnabled on the in-memory object, but they don’t assert the actual JSON persisted to disk. Given EntityMcpOptions can serialize as a boolean shorthand, it would be good to add an assertion that the written config contains autoentities.<def>.template.mcp as an object with a dml-tools property (matching the schema), and that GraphQL template serialization only writes enabled (no type). This would catch regressions in the converters.
| @@ -67,6 +67,7 @@ public static int Execute(string[] args, ILogger cliLogger, IFileSystem fileSyst | |||
| (ValidateOptions options) => options.Handler(cliLogger, loader, fileSystem), | |||
| (AddTelemetryOptions options) => options.Handler(cliLogger, loader, fileSystem), | |||
| (ConfigureOptions options) => options.Handler(cliLogger, loader, fileSystem), | |||
| (AutoentitiesConfigureOptions options) => options.Handler(cliLogger, loader, fileSystem), | |||
| (ExportOptions options) => options.Handler(cliLogger, loader, fileSystem), | |||
There was a problem hiding this comment.
ParseArguments<...> type parameter order must match the subsequent .MapResult(...) handler order. Here, ExportOptions is listed before AddTelemetryOptions in ParseArguments, but the MapResult lambdas handle AddTelemetryOptions/ConfigureOptions/AutoentitiesConfigureOptions before ExportOptions. This should fail to compile (handler delegate types won’t line up). Reorder either the ParseArguments<...> generic list or the MapResult lambdas so ExportOptions’ handler is in the same position as ExportOptions in the type list.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Why make this change?
dab autoentities configure#2964Implements CLI command to configure autoentities definitions via command line. Previously, users had to manually edit the config file to add or modify autoentities entries.
What is this change?
Added
autoentities-configurecommand that upserts autoentities definitions with support for:--patterns.include,--patterns.exclude,--patterns.name)role:actionsformatImplementation:
AutoentitiesConfigureOptions.cs- Command options class following existing CLI patternsConfigGenerator.TryConfigureAutoentities()- Main handler with builder methods for patterns, template, and permissionsProgram.csparserBug fixes:
AutoentityConverterandAutoentityTemplateConverter- Added missing MCP options serialization logic that prevented MCP settings from persisting to config fileAutoentityTemplateConverter- Fixed GraphQL template serialization to only writeenabledproperty, excludingtypeobject (singular/plural) which is determined by generated entitiesImprovements:
EnumExtensions.GenerateMessageForInvalidInput<T>()pattern--permissionsonly when creating a new autoentity definition. When updating an existing definition, permissions are optional and preserved from the existing configuration.How was this tested?
Added 8 unit tests covering:
Sample Request(s)
Result in config:
{ "autoentities": { "my-def": { "patterns": { "include": ["dbo.%", "sys.%"], "exclude": ["dbo.internal%"], "name": "{schema}_{table}" }, "template": { "rest": { "enabled": true }, "graphql": { "enabled": true }, "mcp": false, "cache": { "enabled": true, "ttl-seconds": 60, "level": "l1l2" } }, "permissions": [ { "role": "anonymous", "actions": [ { "action": "read" } ] } ] } } }Notes:
"graphql": { "enabled": true }without the emptytypeobject that was previously being written.Original prompt