From 8e9a06f639f74569d8487018c78901502e1d6db3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Akbar=20D=C4=B1zaj=C4=B1?= Date: Tue, 1 Sep 2026 16:01:20 +0400 Subject: [PATCH 1/3] fix(cli): load config in Ignore mode for auto-config-simulate (#3791) TrySimulateAutoentities built its DeserializationVariableReplacementSettings without an envFailureMode, so it defaulted to Throw. `dab init` always writes three OpenTelemetry @env() placeholders (OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, OTEL_SERVICE_NAME) into the generated config, and those variables are normally unset. Deserialization therefore threw and the command aborted with "Failed to read the config file" on a config that `dab start` loads and serves without complaint. Ignore is the correct mode because it is what the engine uses: every engine load goes through TryLoadKnownConfig, which hardcodes Ignore, so unresolved placeholders are tolerated by design. Simulation inspects the same config the engine would run, and must not reject configs the engine accepts. The underlying reason was also invisible. The loader buffers its logs until a logger is attached, and only the `start` verb attaches one, so the parse error was dropped and only the generic message remained. Attach a logger and drain the buffer on the failure path, then suppress the generic message when IsParseErrorEmitted reports the detailed error was already emitted. Because Ignore lets an unresolved reference survive into the connection string, where it would be sent to the database as a literal, reject a connection string still containing @env(' or @akv(' with an actionable message. Tests now assert the specific error rather than a bare IsFalse, which is what lets them distinguish a config-load failure from the check under test, and cover both an unset telemetry placeholder and the connection-string guard. The test loader is constructed with isCliLoader, matching how the CLI builds it; without it a successful load starts a hot-reload watcher over the mock file system whose IO retries add seconds to every test. --- src/Cli.Tests/AutoConfigSimulateTests.cs | 179 ++++++++++++++++++++++- src/Cli/ConfigGenerator.cs | 35 ++++- 2 files changed, 209 insertions(+), 5 deletions(-) diff --git a/src/Cli.Tests/AutoConfigSimulateTests.cs b/src/Cli.Tests/AutoConfigSimulateTests.cs index ef90a587dd..0945629686 100644 --- a/src/Cli.Tests/AutoConfigSimulateTests.cs +++ b/src/Cli.Tests/AutoConfigSimulateTests.cs @@ -24,6 +24,32 @@ public class AutoConfigSimulateTests "Server=tcp:127.0.0.1,1433;Persist Security Info=False;User ID=sa;" + "Password=@env('MSSQL_SA_PASSWORD');MultipleActiveResultSets=False;Connection Timeout=30;"; + /// + /// A fully resolved connection string containing no @env()/@akv() references. It points at a port + /// nothing listens on with a short timeout, so the tests that reach the query stage fail fast + /// without requiring a database. + /// + private const string MSSQL_RESOLVED_CONNECTION_STRING = + "Server=tcp:127.0.0.1,1;Persist Security Info=False;User ID=sa;" + + "Password=placeholder;TrustServerCertificate=True;Connect Timeout=1;"; + + /// + /// Name of an environment variable that is deliberately never set, used to produce an + /// unresolved @env() reference in a connection string. + /// + private const string UNSET_ENV_VAR_NAME = "DAB_TEST_UNSET_CONNECTION_SECRET"; + + /// + /// The OpenTelemetry environment variables that `dab init` always references from the generated + /// config. They are normally unset, which is the scenario covered by issue #3791. + /// + private static readonly string[] _openTelemetryEnvVarNames = new[] + { + "OTEL_EXPORTER_OTLP_ENDPOINT", + "OTEL_EXPORTER_OTLP_HEADERS", + "OTEL_SERVICE_NAME" + }; + private IFileSystem? _fileSystem; private FileSystemRuntimeConfigLoader? _runtimeConfigLoader; @@ -31,7 +57,9 @@ public class AutoConfigSimulateTests public void TestInitialize() { _fileSystem = FileSystemUtils.ProvisionMockFileSystem(); - _runtimeConfigLoader = new FileSystemRuntimeConfigLoader(_fileSystem); + // isCliLoader mirrors how the CLI builds its loader. Without it a successful load starts a + // hot-reload file watcher against the mock file system, whose retries add seconds per test. + _runtimeConfigLoader = new FileSystemRuntimeConfigLoader(_fileSystem, isCliLoader: true); ILoggerFactory loggerFactory = TestLoggerSupport.ProvisionLoggerFactory(); ConfigGenerator.SetLoggerForCliConfigGenerator(loggerFactory.CreateLogger()); @@ -47,14 +75,21 @@ public void TestCleanup() /// /// Tests that the simulate command fails when no autoentities are defined in the config. + /// The config is produced by `dab init`, which always writes unset OpenTelemetry @env() + /// placeholders, so asserting on the specific error also proves the command reached the + /// autoentities check rather than aborting during the config load. /// [TestMethod] public void TestSimulateAutoentities_NoAutoentitiesDefined() { // Arrange: create an MSSQL config without autoentities - InitOptions initOptions = CreateBasicInitOptionsForMsSqlWithConfig(config: TEST_RUNTIME_CONFIG_FILE); + ClearOpenTelemetryEnvironmentVariables(); + InitOptions initOptions = CreateInitOptionsForMsSql(MSSQL_RESOLVED_CONNECTION_STRING); Assert.IsTrue(TryGenerateConfig(initOptions, _runtimeConfigLoader!, _fileSystem!)); + Mock> loggerMock = new(); + SetLoggerForCliConfigGenerator(loggerMock.Object); + AutoConfigSimulateOptions options = new(config: TEST_RUNTIME_CONFIG_FILE); // Act @@ -62,6 +97,79 @@ public void TestSimulateAutoentities_NoAutoentitiesDefined() // Assert Assert.IsFalse(success); + AssertErrorLogged(loggerMock, "No autoentities definitions found in the config file."); + } + + /// + /// Regression test for https://github.com/Azure/data-api-builder/issues/3791. + /// A config generated by `dab init` references OpenTelemetry environment variables that are + /// normally unset. Those unresolved @env() references must not abort the config load, so the + /// command proceeds all the way to the database query stage. + /// + [TestMethod] + public void TestSimulateAutoentities_UnsetTelemetryEnvVars_DoesNotBlockConfigLoad() + { + // Arrange: an init-generated config (unset OpenTelemetry @env() placeholders) with an autoentity. + ClearOpenTelemetryEnvironmentVariables(); + InitOptions initOptions = CreateInitOptionsForMsSql(MSSQL_RESOLVED_CONNECTION_STRING); + Assert.IsTrue(TryGenerateConfig(initOptions, _runtimeConfigLoader!, _fileSystem!)); + + AutoConfigOptions autoConfigOptions = new( + definitionName: "books-filter", + patternsInclude: new[] { "dbo.books" }, + config: TEST_RUNTIME_CONFIG_FILE); + Assert.IsTrue(ConfigGenerator.TryConfigureAutoentities(autoConfigOptions, _runtimeConfigLoader!, _fileSystem!)); + + Mock> loggerMock = new(); + SetLoggerForCliConfigGenerator(loggerMock.Object); + + AutoConfigSimulateOptions options = new(config: TEST_RUNTIME_CONFIG_FILE); + + // Act + bool success = TrySimulateAutoentities(options, _runtimeConfigLoader!, _fileSystem!); + + // Assert: the run fails only because no database is listening, which means the config load, + // the database type check, the autoentities check and the connection string checks all passed. + Assert.IsFalse(success, "No database is listening, so the simulation cannot succeed."); + AssertErrorLogged(loggerMock, "Failed to query the database"); + AssertErrorNotLogged(loggerMock, "Failed to read the config file"); + AssertErrorNotLogged(loggerMock, "No autoentities definitions found"); + } + + /// + /// Tests that an @env() reference which could not be resolved is rejected with an actionable + /// message instead of being sent to the database as a literal. Unresolved references survive the + /// config load because it runs in Ignore mode, so this check is what catches them. + /// + [TestMethod] + public void TestSimulateAutoentities_UnresolvedEnvVarInConnectionString_Fails() + { + // Arrange: a config whose connection string references an environment variable that is not set. + ClearOpenTelemetryEnvironmentVariables(); + Environment.SetEnvironmentVariable(UNSET_ENV_VAR_NAME, null); + + InitOptions initOptions = CreateInitOptionsForMsSql( + "Server=tcp:127.0.0.1,1;User ID=sa;Password=@env('" + UNSET_ENV_VAR_NAME + "');Connect Timeout=1;"); + Assert.IsTrue(TryGenerateConfig(initOptions, _runtimeConfigLoader!, _fileSystem!)); + + AutoConfigOptions autoConfigOptions = new( + definitionName: "books-filter", + patternsInclude: new[] { "dbo.books" }, + config: TEST_RUNTIME_CONFIG_FILE); + Assert.IsTrue(ConfigGenerator.TryConfigureAutoentities(autoConfigOptions, _runtimeConfigLoader!, _fileSystem!)); + + Mock> loggerMock = new(); + SetLoggerForCliConfigGenerator(loggerMock.Object); + + AutoConfigSimulateOptions options = new(config: TEST_RUNTIME_CONFIG_FILE); + + // Act + bool success = TrySimulateAutoentities(options, _runtimeConfigLoader!, _fileSystem!); + + // Assert + Assert.IsFalse(success); + AssertErrorLogged(loggerMock, "unresolved @env() or @akv() reference"); + AssertErrorNotLogged(loggerMock, "Failed to query the database"); } /// @@ -236,4 +344,71 @@ public void TestSimulateAutoentities_WithNonMatchingFilter_OutputsNoMatches() StringAssert.Contains(output, "Matches: 0", "Output should show zero matches."); StringAssert.Contains(output, "(no matches)", "Output should show the 'no matches' message."); } + + /// + /// Creates the init options used to generate an MSSQL config with the given connection string. + /// + /// The connection string written to the generated config. + private static InitOptions CreateInitOptionsForMsSql(string connectionString) + { + return new( + databaseType: DatabaseType.MSSQL, + connectionString: connectionString, + cosmosNoSqlDatabase: null, + cosmosNoSqlContainer: null, + graphQLSchemaPath: null, + setSessionContext: false, + hostMode: HostMode.Development, + corsOrigin: new List(), + authenticationProvider: EasyAuthType.AppService.ToString(), + config: TEST_RUNTIME_CONFIG_FILE); + } + + /// + /// Unsets the OpenTelemetry environment variables referenced by an init-generated config so the + /// tests deterministically exercise the unresolved @env() scenario. + /// + private static void ClearOpenTelemetryEnvironmentVariables() + { + foreach (string name in _openTelemetryEnvVarNames) + { + Environment.SetEnvironmentVariable(name, null); + } + } + + /// + /// Asserts that an error containing the given fragment was logged exactly once. + /// + /// The mocked logger the command wrote to. + /// Fragment expected in the logged error message. + private static void AssertErrorLogged(Mock> loggerMock, string expectedMessageFragment) + { + loggerMock.Verify( + x => x.Log( + LogLevel.Error, + It.IsAny(), + It.Is((o, t) => o.ToString()!.Contains(expectedMessageFragment)), + It.IsAny(), + (Func)It.IsAny()), + Times.Once, + $"Expected an error containing '{expectedMessageFragment}' to be logged."); + } + + /// + /// Asserts that no error containing the given fragment was logged. + /// + /// The mocked logger the command wrote to. + /// Fragment that must not appear in any logged error. + private static void AssertErrorNotLogged(Mock> loggerMock, string unexpectedMessageFragment) + { + loggerMock.Verify( + x => x.Log( + LogLevel.Error, + It.IsAny(), + It.Is((o, t) => o.ToString()!.Contains(unexpectedMessageFragment)), + It.IsAny(), + (Func)It.IsAny()), + Times.Never, + $"Did not expect an error containing '{unexpectedMessageFragment}' to be logged."); + } } diff --git a/src/Cli/ConfigGenerator.cs b/src/Cli/ConfigGenerator.cs index 29838a9abe..f4265c6108 100644 --- a/src/Cli/ConfigGenerator.cs +++ b/src/Cli/ConfigGenerator.cs @@ -3819,11 +3819,29 @@ public static bool TrySimulateAutoentities(AutoConfigSimulateOptions options, Fi return false; } - // Load config with env var replacement so the connection string is fully resolved. - DeserializationVariableReplacementSettings replacementSettings = new(doReplaceEnvVar: true); + // Load config with env var replacement so the connection string is resolved where possible. + // Unresolved @env() references must NOT abort the load: this is deliberately Ignore, matching + // the engine, which only ever loads through TryLoadKnownConfig (also Ignore). A config produced + // by `dab init` always carries OpenTelemetry @env() placeholders that are typically unset, so + // Throw here would reject configs that `dab start` accepts and runs. Placeholders that survive + // into the connection string are caught by the explicit check below. + DeserializationVariableReplacementSettings replacementSettings = new(doReplaceEnvVar: true, envFailureMode: EnvironmentVariableReplacementFailureMode.Ignore); if (!loader.TryLoadConfig(runtimeConfigFile, out RuntimeConfig? runtimeConfig, replacementSettings: replacementSettings)) { - _logger.LogError("Failed to read the config file: {runtimeConfigFile}.", runtimeConfigFile); + // The loader buffers its logs until a logger is attached, and only the `start` verb attaches + // one. Attach and drain here so the underlying parse failure reaches the user instead of + // being dropped along with the generic message below. + loader.SetLogger(LoggerFactoryForCli.CreateLogger()); + loader.FlushLogBuffer(); + + // When IsParseErrorEmitted is true, TryLoadConfig already emitted the + // detailed error to Console.Error. Only log a generic message to avoid + // duplicate output (stderr + stdout). + if (!loader.IsParseErrorEmitted) + { + _logger.LogError("Failed to read the config file: {runtimeConfigFile}.", runtimeConfigFile); + } + return false; } @@ -3846,6 +3864,17 @@ public static bool TrySimulateAutoentities(AutoConfigSimulateOptions options, Fi return false; } + // The config is loaded in Ignore mode, so an @env()/@akv() reference that was not resolved is + // left in place verbatim rather than failing the load. Such a placeholder would be sent to the + // database as a literal, producing a confusing connection error, so reject it here instead. + if (connectionString.Contains("@env('", StringComparison.Ordinal) || connectionString.Contains("@akv('", StringComparison.Ordinal)) + { + _logger.LogError( + "The connection string in the config file contains an unresolved @env() or @akv() reference. " + + "Set the referenced environment variable (or provide it in a .env file) before running the simulation."); + return false; + } + MsSqlQueryBuilder queryBuilder = new(); string query = queryBuilder.BuildGetAutoentitiesQuery(); From 6277bed2cc972114898b03572bc8d6d633db23a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Akbar=20D=C4=B1zaj=C4=B1?= Date: Tue, 1 Sep 2026 17:45:30 +0400 Subject: [PATCH 2/3] test(cli): restore mutated env vars in AutoConfigSimulateTests cleanup ClearOpenTelemetryEnvironmentVariables unset the three real OTEL_* names and never put them back, and the connection-string guard test unset DAB_TEST_UNSET_CONNECTION_SECRET the same way. MSTest runs an assembly's tests in one process, so a cleared variable leaked into every test that ran later, making unrelated tests pass or fail depending on ordering and on whether the host had OTEL_* set. The real names are unavoidable here: the config under test is produced by `dab init`, which hardcodes them, so this cannot be dodged by using fake names the way RuntimeConfigLoaderTests does. Snapshot the values in TestInitialize and restore them in TestCleanup instead, which covers every test in the class rather than relying on each one to clean up after itself. --- src/Cli.Tests/AutoConfigSimulateTests.cs | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/Cli.Tests/AutoConfigSimulateTests.cs b/src/Cli.Tests/AutoConfigSimulateTests.cs index 0945629686..e9e07f854e 100644 --- a/src/Cli.Tests/AutoConfigSimulateTests.cs +++ b/src/Cli.Tests/AutoConfigSimulateTests.cs @@ -50,12 +50,31 @@ public class AutoConfigSimulateTests "OTEL_SERVICE_NAME" }; + /// + /// Every environment variable these tests unset. The OpenTelemetry names are the real ones an + /// init-generated config references, so they may legitimately be set in the host environment. + /// + private static readonly string[] _mutatedEnvVarNames = + _openTelemetryEnvVarNames.Append(UNSET_ENV_VAR_NAME).ToArray(); + private IFileSystem? _fileSystem; private FileSystemRuntimeConfigLoader? _runtimeConfigLoader; + /// + /// Host values of , captured before each test clears them and + /// restored in cleanup. Without this, a cleared variable leaks into every test that runs later in + /// the same process, making unrelated tests fail depending on ordering and host environment. + /// + private readonly Dictionary _originalEnvVarValues = new(); + [TestInitialize] public void TestInitialize() { + foreach (string name in _mutatedEnvVarNames) + { + _originalEnvVarValues[name] = Environment.GetEnvironmentVariable(name); + } + _fileSystem = FileSystemUtils.ProvisionMockFileSystem(); // isCliLoader mirrors how the CLI builds its loader. Without it a successful load starts a // hot-reload file watcher against the mock file system, whose retries add seconds per test. @@ -69,6 +88,12 @@ public void TestInitialize() [TestCleanup] public void TestCleanup() { + foreach (KeyValuePair original in _originalEnvVarValues) + { + Environment.SetEnvironmentVariable(original.Key, original.Value); + } + + _originalEnvVarValues.Clear(); _fileSystem = null; _runtimeConfigLoader = null; } From b90200daf26d69ff87feea1c507dab1aa36c5875 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Akbar=20D=C4=B1zaj=C4=B1?= Date: Tue, 1 Sep 2026 17:47:41 +0400 Subject: [PATCH 3/3] fix(cli): don't report a missing config twice in auto-config-simulate Draining the loader's log buffer on the failure path made the loader's own error visible, which exposed that the generic fallback then repeats it. TryGetConfigFileBasedOnCliPrecedence deliberately does not check that a user-provided path exists, so `--auto-config-simulate -c missing.json` reaches TryLoadConfig, which logs "Unable to find config file: missing.json does not exist." without setting IsParseErrorEmitted. The flush delivered that, and then "Failed to read the config file: missing.json." followed it. Gate the fallback on the file existing as well. The loader attributes both failures it can explain - a parse error via IsParseErrorEmitted, a missing file via its own message - so the fallback is only needed for the one case it stays silent on, a config file that exists but is empty. The new test is the first to exercise the flush path, so it also covers the SetLogger/FlushLogBuffer call itself. --- src/Cli.Tests/AutoConfigSimulateTests.cs | 26 ++++++++++++++++++++++++ src/Cli/ConfigGenerator.cs | 11 ++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/Cli.Tests/AutoConfigSimulateTests.cs b/src/Cli.Tests/AutoConfigSimulateTests.cs index e9e07f854e..38aba1b39e 100644 --- a/src/Cli.Tests/AutoConfigSimulateTests.cs +++ b/src/Cli.Tests/AutoConfigSimulateTests.cs @@ -161,6 +161,32 @@ public void TestSimulateAutoentities_UnsetTelemetryEnvVars_DoesNotBlockConfigLoa AssertErrorNotLogged(loggerMock, "No autoentities definitions found"); } + /// + /// A user-provided config path is not checked for existence before the load (see + /// TryGetConfigFileBasedOnCliPrecedence), so a missing file reaches TryLoadConfig, which logs + /// "Unable to find config file". Draining the loader's buffer now delivers that error, so the + /// generic fallback must stay silent rather than reporting the same failure a second time. + /// + [TestMethod] + public void TestSimulateAutoentities_MissingConfigFile_DoesNotLogGenericError() + { + // Arrange: a config path that was never written to the mock file system. + const string MISSING_CONFIG_FILE = "dab-config.missing.json"; + Assert.IsFalse(_fileSystem!.File.Exists(MISSING_CONFIG_FILE), "The test config file must not exist."); + + Mock> loggerMock = new(); + SetLoggerForCliConfigGenerator(loggerMock.Object); + + AutoConfigSimulateOptions options = new(config: MISSING_CONFIG_FILE); + + // Act + bool success = TrySimulateAutoentities(options, _runtimeConfigLoader!, _fileSystem!); + + // Assert: the loader already reported the missing file, so no duplicate generic error. + Assert.IsFalse(success); + AssertErrorNotLogged(loggerMock, "Failed to read the config file"); + } + /// /// Tests that an @env() reference which could not be resolved is rejected with an actionable /// message instead of being sent to the database as a literal. Unresolved references survive the diff --git a/src/Cli/ConfigGenerator.cs b/src/Cli/ConfigGenerator.cs index f4265c6108..4663fc359f 100644 --- a/src/Cli/ConfigGenerator.cs +++ b/src/Cli/ConfigGenerator.cs @@ -3834,10 +3834,13 @@ public static bool TrySimulateAutoentities(AutoConfigSimulateOptions options, Fi loader.SetLogger(LoggerFactoryForCli.CreateLogger()); loader.FlushLogBuffer(); - // When IsParseErrorEmitted is true, TryLoadConfig already emitted the - // detailed error to Console.Error. Only log a generic message to avoid - // duplicate output (stderr + stdout). - if (!loader.IsParseErrorEmitted) + // The loader explains the two failures it can attribute: a parse error (flagged by + // IsParseErrorEmitted) and a missing file (TryGetConfigFileBasedOnCliPrecedence does not + // check that a user-provided path exists, so it reaches the loader). The flush above has + // now delivered whichever it logged, so emit the generic fallback only for the case the + // loader stays silent on - a config file that exists but is empty - rather than reporting + // the same failure twice. + if (!loader.IsParseErrorEmitted && fileSystem.File.Exists(runtimeConfigFile)) { _logger.LogError("Failed to read the config file: {runtimeConfigFile}.", runtimeConfigFile); }