-
Notifications
You must be signed in to change notification settings - Fork 327
Warn when dab validate finds zero entities in the config #3306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
eab2698
e2849f8
0459fb3
447b0da
ff044ed
c62ed09
346f079
94aa01d
57c15ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -359,4 +359,29 @@ private async Task ValidatePropertyOptionsFails(ConfigureOptions options) | |
| JsonSchemaValidationResult result = await validator.ValidateConfigSchema(config, TEST_RUNTIME_CONFIG_FILE, mockLoggerFactory.Object); | ||
| Assert.IsFalse(result.IsValid); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates that WarnIfNoEntitiesDefined logs a warning when the config has | ||
| /// an empty entities collection and no autoentities or data-source-files. | ||
| /// This calls the extracted method directly, avoiding any DB connection. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void TestValidateWarnsOnZeroEntities() | ||
| { | ||
| Assert.IsTrue(RuntimeConfigLoader.TryParseConfig(INITIAL_CONFIG, out RuntimeConfig? config)); | ||
|
|
||
| Mock<ILogger<ConfigGenerator>> mockLogger = new(); | ||
| SetLoggerForCliConfigGenerator(mockLogger.Object); | ||
|
|
||
| ConfigGenerator.WarnIfNoEntitiesDefined(config!); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of directly calling the WarnIfNoEntitiesDefined function, test the scenario for which the bug is identified. |
||
|
|
||
| mockLogger.Verify( | ||
| x => x.Log( | ||
| LogLevel.Warning, | ||
| It.IsAny<EventId>(), | ||
| It.Is<It.IsAnyType>((v, t) => v.ToString()!.Contains("No entities are defined in this configuration.")), | ||
| It.IsAny<Exception?>(), | ||
| It.IsAny<Func<It.IsAnyType, Exception?, string>>()), | ||
| Times.Once); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2648,10 +2648,13 @@ public static bool IsConfigValid(ValidateOptions options, FileSystemRuntimeConfi | |
|
|
||
| bool isValid = runtimeConfigValidator.TryValidateConfig(runtimeConfigFile, LoggerFactoryForCli).Result; | ||
|
|
||
| // Additional validation: warn if fields are missing and MCP is enabled | ||
| if (isValid) | ||
| if (runtimeConfigProvider.TryGetConfig(out RuntimeConfig? config) && config is not null) | ||
| { | ||
| if (runtimeConfigProvider.TryGetConfig(out RuntimeConfig? config) && config is not null) | ||
| // Run config-structure warnings regardless of validation outcome. | ||
| WarnIfNoEntitiesDefined(config); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if there are 0 entities, or autoentities, it is still a valid configuration. But do we still need to check whether any entity has missing fields when MCP is enabled - i.e. perform the below validation? We should still Warn if Unauthenticated provider is used with authenticated or custom roles
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are no entities, then the for loop below iterates zero times and this is a no-op, so there wont really be any validation happening; there is nothing to validate. On the other hand, if validation fails for some other reason the user wont see any "missing fields with MCP" warnings, which may be an enhancement we want to add, but shouldn't be scoped to this PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to unauthenticated provider with non-anonymous roles. Since it is inside |
||
|
|
||
| // Additional validation: warn if fields are missing and MCP is enabled | ||
| if (isValid) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please clarify with @JerryNixon, if it is a config validation error or warning in case:
|
||
| { | ||
| bool mcpEnabled = config.IsMcpEnabled; | ||
| if (mcpEnabled) | ||
|
|
@@ -3622,5 +3625,18 @@ private static bool ValidateFields( | |
|
|
||
| return true; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Logs a warning if no entities, autoentities, or data-source-files are configured. | ||
| /// </summary> | ||
| internal static void WarnIfNoEntitiesDefined(RuntimeConfig config) | ||
| { | ||
| if (config.Entities.Entities.Count == 0 | ||
| && config.Autoentities.Autoentities.Count == 0 | ||
| && (config.DataSourceFiles?.SourceFiles is null || !config.DataSourceFiles.SourceFiles.Any())) | ||
| { | ||
| _logger.LogWarning("No entities are defined in this configuration."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.