Skip to content

Commit 27498a7

Browse files
aaronburtleCopilotsouvikghosh04
authored andcommitted
Fix dab validate error messaging when logger is not available (#3311)
## Why make this change? Closes #3268 ## What is this change? When `dab validate` or `dab start` encounters a config parsing error (e.g. missing entities/autoentities), the CLI previously dumped the full exception message and stack trace to stderr. This made the output noisy and unhelpful. After this change, only a clean, descriptive validation message is shown. The key design changes: - `TryParseConfig` is now a pure function that returns an `out string? parseError` message on failure instead of writing to `Console.Error` or `ILogger` internally. Error reporting is the caller's responsibility. - `FileSystemRuntimeConfigLoader.TryLoadConfig` writes the `parseError` to `Console.Error` (because config is parsed before the DI container and logger are available, so the log buffer would never be flushed on a parse failure) and sets an instance-scoped `IsParseErrorEmitted` flag so CLI callers (`ConfigGenerator`) can avoid logging duplicate messages. - `ConfigGenerator.IsConfigValid` now has an explicit early-return path when config parsing fails (via `runtimeConfigProvider.TryGetConfig`), using `IsParseErrorEmitted` to suppress duplicate output. - `ValidateOptions.Handler` uses `LogError("Config is invalid.")`. - Comments referencing which method emits the error to `Console.Error` corrected to `TryLoadConfig` (not `TryParseConfig`). ## How was this tested? * `ValidateConfigTests.cs` — Added `TestValidateConfigWithNoEntitiesProducesCleanError` (new test verifying clean error message, no stack traces). * `EnvironmentTests.cs` — Updated `FailureToStartEngineWhenEnvVarNamedWrong` to match the new single-line clean stderr format. * `EndToEndTests.cs` — Simplified assertions in `TestExitOfRuntimeEngineWithInvalidConfig` (this test is Ignored but updated for consistency). * `RuntimeConfigLoaderTests.cs` — Updated `FailLoadMultiDataSourceConfigDuplicateEntities` to assert `loader.IsParseErrorEmitted` is true. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Souvik Ghosh <souvikofficial04@gmail.com>
1 parent 8f8ee11 commit 27498a7

11 files changed

Lines changed: 103 additions & 47 deletions

src/Cli.Tests/EndToEndTests.cs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,10 +1173,6 @@ public async Task TestExitOfRuntimeEngineWithInvalidConfig(
11731173
Assert.IsNotNull(output);
11741174
StringAssert.Contains(output, $"Deserialization of the configuration file failed.", StringComparison.Ordinal);
11751175

1176-
output = await process.StandardOutput.ReadLineAsync();
1177-
Assert.IsNotNull(output);
1178-
StringAssert.Contains(output, $"Error: Failed to parse the config file: {TEST_RUNTIME_CONFIG_FILE}.", StringComparison.Ordinal);
1179-
11801176
output = await process.StandardOutput.ReadLineAsync();
11811177
Assert.IsNotNull(output);
11821178
StringAssert.Contains(output, $"Failed to start the engine.", StringComparison.Ordinal);

src/Cli.Tests/EnvironmentTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,8 @@ public async Task FailureToStartEngineWhenEnvVarNamedWrong()
163163
);
164164

165165
string? output = await process.StandardError.ReadLineAsync();
166-
Assert.AreEqual("Deserialization of the configuration file failed during a post-processing step.", output);
167-
output = await process.StandardError.ReadToEndAsync();
166+
Assert.IsNotNull(output);
167+
// Clean error message on stderr with no stack trace.
168168
StringAssert.Contains(output, "A valid Connection String should be provided.", StringComparison.Ordinal);
169169
process.Kill();
170170
}

src/Cli.Tests/ValidateConfigTests.cs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,34 @@ public void TestValidateConfigFailsWithNoEntities()
199199
}
200200
}
201201

202+
/// <summary>
203+
/// Validates that when the config has no entities or autoentities, TryParseConfig
204+
/// sets a clean error message (not a raw exception with stack trace) and
205+
/// IsConfigValid returns false without throwing.
206+
/// Regression test for https://github.com/Azure/data-api-builder/issues/3268
207+
/// </summary>
208+
[TestMethod]
209+
public void TestValidateConfigWithNoEntitiesProducesCleanError()
210+
{
211+
string configWithoutEntities = $"{{{SAMPLE_SCHEMA_DATA_SOURCE},{RUNTIME_SECTION}}}";
212+
213+
// Verify TryParseConfig produces a clean error without stack traces.
214+
bool parsed = RuntimeConfigLoader.TryParseConfig(configWithoutEntities, out _, out string? parseError);
215+
216+
Assert.IsFalse(parsed, "Config with no entities should fail to parse.");
217+
Assert.IsNotNull(parseError, "parseError should be set when config parsing fails.");
218+
StringAssert.Contains(parseError,
219+
"Configuration file should contain either at least the entities or autoentities property",
220+
"Parse error should contain the clean validation message.");
221+
Assert.IsFalse(parseError.Contains("StackTrace"),
222+
"Stack trace should not be present in parse error.");
223+
224+
// Verify IsConfigValid also returns false cleanly (no exception thrown).
225+
((MockFileSystem)_fileSystem!).AddFile(TEST_RUNTIME_CONFIG_FILE, configWithoutEntities);
226+
ValidateOptions validateOptions = new(TEST_RUNTIME_CONFIG_FILE);
227+
Assert.IsFalse(ConfigGenerator.IsConfigValid(validateOptions, _runtimeConfigLoader!, _fileSystem!));
228+
}
229+
202230
/// <summary>
203231
/// This Test is used to verify that the validate command is able to catch when data source field is missing.
204232
/// </summary>

src/Cli/Commands/ValidateOptions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public int Handler(ILogger logger, FileSystemRuntimeConfigLoader loader, IFileSy
3838
}
3939
else
4040
{
41-
logger.LogError("Config is invalid. Check above logs for details.");
41+
logger.LogError("Config is invalid.");
4242
}
4343

4444
return isValidConfig ? CliReturnCode.SUCCESS : CliReturnCode.GENERAL_ERROR;

src/Cli/ConfigGenerator.cs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2564,7 +2564,14 @@ public static bool TryStartEngineWithOptions(StartOptions options, FileSystemRun
25642564
// Replaces all the environment variables while deserializing when starting DAB.
25652565
if (!loader.TryLoadKnownConfig(out RuntimeConfig? deserializedRuntimeConfig, replaceEnvVar: true))
25662566
{
2567-
_logger.LogError("Failed to parse the config file: {runtimeConfigFile}.", runtimeConfigFile);
2567+
// When IsParseErrorEmitted is true, TryLoadConfig already emitted the
2568+
// detailed error to Console.Error. Only log a generic message to avoid
2569+
// duplicate output (stderr + stdout).
2570+
if (!loader.IsParseErrorEmitted)
2571+
{
2572+
_logger.LogError("Failed to parse the config file: {runtimeConfigFile}.", runtimeConfigFile);
2573+
}
2574+
25682575
return false;
25692576
}
25702577
else
@@ -2641,6 +2648,19 @@ public static bool IsConfigValid(ValidateOptions options, FileSystemRuntimeConfi
26412648

26422649
RuntimeConfigProvider runtimeConfigProvider = new(loader);
26432650

2651+
if (!runtimeConfigProvider.TryGetConfig(out RuntimeConfig? _))
2652+
{
2653+
// When IsParseErrorEmitted is true, TryLoadConfig already emitted the
2654+
// detailed error to Console.Error. Only log a generic message to avoid
2655+
// duplicate output (stderr + stdout).
2656+
if (!loader.IsParseErrorEmitted)
2657+
{
2658+
_logger.LogError("Failed to parse the config file.");
2659+
}
2660+
2661+
return false;
2662+
}
2663+
26442664
ILogger<RuntimeConfigValidator> runtimeConfigValidatorLogger = LoggerFactoryForCli.CreateLogger<RuntimeConfigValidator>();
26452665
RuntimeConfigValidator runtimeConfigValidator = new(runtimeConfigProvider, fileSystem, runtimeConfigValidatorLogger, true);
26462666

src/Config/FileSystemRuntimeConfigLoader.cs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,12 @@ public class FileSystemRuntimeConfigLoader : RuntimeConfigLoader
8484
/// </summary>
8585
public string ConfigFilePath { get; internal set; }
8686

87+
/// <summary>
88+
/// Indicates whether the most recent TryLoadConfig call encountered a parse error
89+
/// that was already emitted to Console.Error.
90+
/// </summary>
91+
public bool IsParseErrorEmitted { get; private set; }
92+
8793
public FileSystemRuntimeConfigLoader(
8894
IFileSystem fileSystem,
8995
HotReloadEventHandler<HotReloadEventArgs>? handler = null,
@@ -205,6 +211,7 @@ public bool TryLoadConfig(
205211
bool? isDevMode = null,
206212
DeserializationVariableReplacementSettings? replacementSettings = null)
207213
{
214+
IsParseErrorEmitted = false;
208215
if (_fileSystem.File.Exists(path))
209216
{
210217
SendLogToBufferOrLogger(LogLevel.Information, $"Loading config file from {_fileSystem.Path.GetFullPath(path)}.");
@@ -241,11 +248,12 @@ public bool TryLoadConfig(
241248
// Use default replacement settings if none provided
242249
replacementSettings ??= new DeserializationVariableReplacementSettings();
243250

251+
string? parseError = null;
244252
if (!string.IsNullOrEmpty(json) && TryParseConfig(
245253
json,
246254
out RuntimeConfig,
255+
out parseError,
247256
replacementSettings,
248-
logger: null,
249257
connectionString: _connectionString))
250258
{
251259
if (TrySetupConfigFileWatcher())
@@ -281,6 +289,12 @@ public bool TryLoadConfig(
281289
RuntimeConfig = LastValidRuntimeConfig;
282290
}
283291

292+
if (parseError is not null)
293+
{
294+
Console.Error.WriteLine(parseError);
295+
IsParseErrorEmitted = true;
296+
}
297+
284298
config = null;
285299
return false;
286300
}

src/Config/RuntimeConfigLoader.cs

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
using Azure.DataApiBuilder.Product;
1414
using Azure.DataApiBuilder.Service.Exceptions;
1515
using Microsoft.Data.SqlClient;
16-
using Microsoft.Extensions.Logging;
1716
using Microsoft.Extensions.Primitives;
1817
using Npgsql;
1918
using static Azure.DataApiBuilder.Config.DabConfigEvents;
@@ -179,16 +178,34 @@ protected void SignalConfigChanged(string message = "")
179178
/// </summary>
180179
/// <param name="json">JSON that represents the config file.</param>
181180
/// <param name="config">The parsed config, or null if it parsed unsuccessfully.</param>
181+
/// <param name="parseError">A clean error message when parsing fails, or null on success.</param>
182182
/// <param name="replacementSettings">Settings for variable replacement during deserialization. If null, no variable replacement will be performed.</param>
183-
/// <param name="logger">logger to log messages</param>
184183
/// <param name="connectionString">connectionString to add to config if specified</param>
185184
/// <returns>True if the config was parsed, otherwise false.</returns>
186185
public static bool TryParseConfig(string json,
187186
[NotNullWhen(true)] out RuntimeConfig? config,
188187
DeserializationVariableReplacementSettings? replacementSettings = null,
189-
ILogger? logger = null,
190188
string? connectionString = null)
191189
{
190+
return TryParseConfig(json, out config, out _, replacementSettings, connectionString);
191+
}
192+
193+
/// <summary>
194+
/// Parses a JSON string into a <c>RuntimeConfig</c> object for single database scenario.
195+
/// </summary>
196+
/// <param name="json">JSON that represents the config file.</param>
197+
/// <param name="config">The parsed config, or null if it parsed unsuccessfully.</param>
198+
/// <param name="parseError">A clean error message when parsing fails, or null on success.</param>
199+
/// <param name="replacementSettings">Settings for variable replacement during deserialization. If null, no variable replacement will be performed.</param>
200+
/// <param name="connectionString">connectionString to add to config if specified</param>
201+
/// <returns>True if the config was parsed, otherwise false.</returns>
202+
public static bool TryParseConfig(string json,
203+
[NotNullWhen(true)] out RuntimeConfig? config,
204+
out string? parseError,
205+
DeserializationVariableReplacementSettings? replacementSettings = null,
206+
string? connectionString = null)
207+
{
208+
parseError = null;
192209
// First pass: extract AzureKeyVault options if AKV replacement is requested
193210
if (replacementSettings?.DoReplaceAkvVar is true)
194211
{
@@ -263,18 +280,9 @@ public static bool TryParseConfig(string json,
263280
ex is JsonException ||
264281
ex is DataApiBuilderException)
265282
{
266-
string errorMessage = ex is JsonException ? "Deserialization of the configuration file failed." :
267-
"Deserialization of the configuration file failed during a post-processing step.";
268-
269-
// logger can be null when called from CLI
270-
if (logger is null)
271-
{
272-
Console.Error.WriteLine(errorMessage + $"\n" + $"Message:\n {ex.Message}\n" + $"Stack Trace:\n {ex.StackTrace}");
273-
}
274-
else
275-
{
276-
logger.LogError(exception: ex, message: errorMessage);
277-
}
283+
parseError = ex is DataApiBuilderException
284+
? ex.Message
285+
: $"Deserialization of the configuration file failed. {ex.Message}";
278286

279287
config = null;
280288
return false;

src/Core/Configurations/RuntimeConfigProvider.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@ public async Task<bool> Initialize(
188188
if (RuntimeConfigLoader.TryParseConfig(
189189
configuration,
190190
out RuntimeConfig? runtimeConfig,
191+
out _,
191192
replacementSettings: null))
192193
{
193194
_configLoader.RuntimeConfig = runtimeConfig;
@@ -269,7 +270,7 @@ public async Task<bool> Initialize(
269270

270271
IsLateConfigured = true;
271272

272-
if (RuntimeConfigLoader.TryParseConfig(jsonConfig, out RuntimeConfig? runtimeConfig, replacementSettings))
273+
if (RuntimeConfigLoader.TryParseConfig(jsonConfig, out RuntimeConfig? runtimeConfig, out _, replacementSettings))
273274
{
274275
_configLoader.RuntimeConfig = runtimeConfig.DataSource.DatabaseType switch
275276
{

src/Service.Tests/Configuration/ConfigurationTests.cs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2500,8 +2500,7 @@ public async Task TestSPRestDefaultsForManuallyConstructedConfigs(
25002500
configJson,
25012501
out RuntimeConfig deserializedConfig,
25022502
replacementSettings: new(),
2503-
logger: null,
2504-
GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL));
2503+
connectionString: GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL));
25052504
string configFileName = "custom-config.json";
25062505
File.WriteAllText(configFileName, deserializedConfig.ToJson());
25072506
string[] args = new[]
@@ -2588,8 +2587,7 @@ public async Task SanityTestForRestAndGQLRequestsWithoutMultipleMutationFeatureF
25882587
configJson,
25892588
out RuntimeConfig deserializedConfig,
25902589
replacementSettings: new(),
2591-
logger: null,
2592-
GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL)));
2590+
connectionString: GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL)));
25932591
string configFileName = "custom-config.json";
25942592
File.WriteAllText(configFileName, deserializedConfig.ToJson());
25952593
string[] args = new[]
@@ -3619,8 +3617,7 @@ public async Task ValidateStrictModeAsDefaultForRestRequestBody(bool includeExtr
36193617
configJson,
36203618
out RuntimeConfig deserializedConfig,
36213619
replacementSettings: new(),
3622-
logger: null,
3623-
GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL));
3620+
connectionString: GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL));
36243621
const string CUSTOM_CONFIG = "custom-config.json";
36253622
File.WriteAllText(CUSTOM_CONFIG, deserializedConfig.ToJson());
36263623
string[] args = new[]

src/Service.Tests/Configuration/RuntimeConfigLoaderTests.cs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,11 @@ public async Task FailLoadMultiDataSourceConfigDuplicateEntities(string configPa
9696
Console.SetError(sw);
9797

9898
loader.TryLoadConfig("dab-config.json", out RuntimeConfig _);
99-
string error = sw.ToString();
10099

101-
Assert.IsTrue(error.StartsWith("Deserialization of the configuration file failed during a post-processing step."));
102-
Assert.IsTrue(error.Contains("An item with the same key has already been added."));
100+
Assert.IsTrue(loader.IsParseErrorEmitted,
101+
"IsParseErrorEmitted should be true when config parsing fails.");
102+
Assert.IsFalse(string.IsNullOrWhiteSpace(sw.ToString()),
103+
"An error message should have been emitted to Console.Error.");
103104
}
104105

105106
/// <summary>

0 commit comments

Comments
 (0)