Replace environment variable only for dab start (#1641)

## Why make this change?

- Closes #1632 
- it's an unintended side effect of an optimisation pathway was added to
the deserializer in #1402.
Previously, deserializing the JSON config and replacing environment
variables were done as two separate parses of the config JSON blob.
First the JSON would be read from disk and then we'd create a JSON
reader that would walk all the tokens in the file, we'd check the token
to see if it matched the regex for an environment variable, we'd replace
it and then return the updated JSON string. Next this would be given to
the deserializer and it would be turned into the .NET object that we
would use throughout the engine. This meant that we'd end up walking the
AST for the JSON twice.
- With #1402, when we are doing deserialization we would also do the
environment variable replacement, meaning that we'd only walk the JSON
AST once, thus improving performance.
- There is a downside to this - the CLI doesn't need to do environment
variable replacements (since we don't need to actually connect to the DB
or anything, except for start, but that doesn't write out an updated
config).

## What is this change?

- Introduces a flag replaceEnvVar to determine whether to do the
environment variable replacement in the JsonConverters when being used
from the CLI commands. For `start` command this will be `true`, for all
other commands this is `false`.
- To minimize changes, default value of this argument was kept false.
All occurrences of callers requiring this flag were modified and set to
`true` in case of `start` or running the engine.
- For Enums, always replace the environment variable with its value for
appropriate deserialization into an Enum value.

## How was this tested?

- [X] Integration Tests - Modified the End2EndTests, to test the
commands `add`, `update` dont replace the environment variables, but
`start` replaces it.
- [X] Unit Tests - Modified the `CheckConfigEnvParsingTest` to test when
replaceEnvVar is `false` we dont replace them. The `true` scenario was
already being tested. Also modified the `database-type` property to be
an env variable for testing enums are always replaced with values.

---------

Co-authored-by: Sean Leonard <[email protected]>

Author: Aniruddh25

src/Cli.Tests/EndToEndTests.cs

@@ -35,6 +35,8 @@ public void TestInitialize()
         _cliLogger = loggerFactory.CreateLogger<Program>();
         SetLoggerForCliConfigGenerator(loggerFactory.CreateLogger<ConfigGenerator>());
         SetCliUtilsLogger(loggerFactory.CreateLogger<Utils>());
+
+        Environment.SetEnvironmentVariable($"connection-string", TEST_CONNECTION_STRING);
     }
 
     [TestCleanup]
@@ -52,7 +54,7 @@ public void TestCleanup()
     public Task TestInitForCosmosDBNoSql()
     {
         string[] args = { "init", "-c", TEST_RUNTIME_CONFIG_FILE, "--database-type", "cosmosdb_nosql",
-                          "--connection-string", "localhost:5000", "--cosmosdb_nosql-database",
+                          "--connection-string", TEST_ENV_CONN_STRING, "--cosmosdb_nosql-database",
                           "graphqldb", "--cosmosdb_nosql-container", "planet", "--graphql-schema", TEST_SCHEMA_FILE, "--cors-origin", "localhost:3000,www.nolocalhost.com:80" };
         Program.Execute(args, _cliLogger!, _fileSystem!, _runtimeConfigLoader!);
 
@@ -109,7 +111,10 @@ public void TestInitializingRestAndGraphQLGlobalSettings()
         string[] args = { "init", "-c", TEST_RUNTIME_CONFIG_FILE, "--connection-string", SAMPLE_TEST_CONN_STRING, "--database-type", "mssql", "--rest.path", "/rest-api", "--rest.disabled", "--graphql.path", "/graphql-api" };
         Program.Execute(args, _cliLogger!, _fileSystem!, _runtimeConfigLoader!);
 
-        Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
+        Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(
+            TEST_RUNTIME_CONFIG_FILE,
+            out RuntimeConfig? runtimeConfig,
+            replaceEnvVar: true));
 
         SqlConnectionStringBuilder builder = new(runtimeConfig.DataSource.ConnectionString);
         Assert.AreEqual(DEFAULT_APP_NAME, builder.ApplicationName);
@@ -129,7 +134,8 @@ public void TestInitializingRestAndGraphQLGlobalSettings()
     [TestMethod]
     public void TestAddEntity()
     {
-        string[] initArgs = { "init", "-c", TEST_RUNTIME_CONFIG_FILE, "--host-mode", "development", "--database-type", "mssql", "--connection-string", SAMPLE_TEST_CONN_STRING, "--auth.provider", "StaticWebApps" };
+        string[] initArgs = { "init", "-c", TEST_RUNTIME_CONFIG_FILE, "--host-mode", "development", "--database-type",
+            "mssql", "--connection-string", TEST_ENV_CONN_STRING, "--auth.provider", "StaticWebApps" };
         Program.Execute(initArgs, _cliLogger!, _fileSystem!, _runtimeConfigLoader!);
 
         Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
@@ -145,6 +151,7 @@ public void TestAddEntity()
 
         Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? addRuntimeConfig));
         Assert.IsNotNull(addRuntimeConfig);
+        Assert.AreEqual(TEST_ENV_CONN_STRING, addRuntimeConfig.DataSource.ConnectionString);
         Assert.AreEqual(1, addRuntimeConfig.Entities.Count()); // 1 new entity added
         Assert.IsTrue(addRuntimeConfig.Entities.ContainsKey("todo"));
         Entity entity = addRuntimeConfig.Entities["todo"];
@@ -379,7 +386,7 @@ public Task TestConfigGeneratedAfterAddingEntityWithSourceWithDefaultType()
     public void TestUpdateEntity()
     {
         string[] initArgs = { "init", "-c", TEST_RUNTIME_CONFIG_FILE, "--database-type",
-                              "mssql", "--connection-string", SAMPLE_TEST_CONN_STRING };
+                              "mssql", "--connection-string", TEST_ENV_CONN_STRING };
         Program.Execute(initArgs, _cliLogger!, _fileSystem!, _runtimeConfigLoader!);
 
         Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
@@ -421,6 +428,7 @@ public void TestUpdateEntity()
 
         Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? updateRuntimeConfig));
         Assert.IsNotNull(updateRuntimeConfig);
+        Assert.AreEqual(TEST_ENV_CONN_STRING, updateRuntimeConfig.DataSource.ConnectionString);
         Assert.AreEqual(2, updateRuntimeConfig.Entities.Count()); // No new entity added
 
         Assert.IsTrue(updateRuntimeConfig.Entities.ContainsKey("todo"));

src/Cli.Tests/EnvironmentTests.cs

@@ -123,7 +123,8 @@ public void TestSystemEnvironmentVariableIsUsedInAbsenceOfEnvironmentFile()
     [TestMethod]
     public void TestStartWithEnvFileIsSuccessful()
     {
-        BootstrapTestEnvironment("CONN_STRING=test_connection_string");
+        string expectedEnvVarName = "CONN_STRING";
+        BootstrapTestEnvironment(expectedEnvVarName + "=test_connection_string", expectedEnvVarName);
 
         // Trying to start the runtime engine
         using Process process = ExecuteDabCommand(
@@ -148,10 +149,11 @@ public void TestStartWithEnvFileIsSuccessful()
     /// I feel confident that the overarching scenario is covered through other testing
     /// so disabling temporarily while we investigate should be acceptable.
     /// </summary>
-    [TestMethod, Ignore]
+    [TestMethod]
     public async Task FailureToStartEngineWhenEnvVarNamedWrong()
     {
-        BootstrapTestEnvironment("COMM_STRINX=test_connection_string");
+        string expectedEnvVarName = "WRONG_CONN_STRING";
+        BootstrapTestEnvironment("COMM_STRINX=test_connection_string", expectedEnvVarName);
 
         // Trying to start the runtime engine
         using Process process = ExecuteDabCommand(
@@ -160,11 +162,12 @@ public async Task FailureToStartEngineWhenEnvVarNamedWrong()
         );
 
         string? output = await process.StandardError.ReadLineAsync();
-        StringAssert.Contains(output, "Environmental Variable, CONN_STRING, not found.", StringComparison.Ordinal);
+        StringAssert.Contains(output, "Environmental Variable, "
+            + expectedEnvVarName + ", not found.", StringComparison.Ordinal);
         process.Kill();
     }
 
-    private static void BootstrapTestEnvironment(string envFileContents)
+    private static void BootstrapTestEnvironment(string envFileContents, string connStringEnvName)
     {
         // Creating environment variable file
         File.Create(".env").Close();
@@ -174,7 +177,8 @@ private static void BootstrapTestEnvironment(string envFileContents)
             File.Delete(TEST_RUNTIME_CONFIG_FILE);
         }
 
-        string[] initArgs = { "init", "-c", TEST_RUNTIME_CONFIG_FILE, "--database-type", "mssql", "--connection-string", "@env('CONN_STRING')" };
+        string[] initArgs = { "init", "-c", TEST_RUNTIME_CONFIG_FILE, "--database-type", "mssql",
+            "--connection-string", "@env('" + connStringEnvName + "')" };
         Program.Main(initArgs);
     }
 

src/Cli.Tests/TestHelper.cs

@@ -8,6 +8,9 @@ public static class TestHelper
         // Config file name for tests
         public const string TEST_RUNTIME_CONFIG_FILE = "dab-config-test.json";
 
+        public const string TEST_CONNECTION_STRING = "testconnectionstring";
+        public const string TEST_ENV_CONN_STRING = "@env('connection-string')";
+
         public const string SAMPLE_TEST_CONN_STRING = "Data Source=<>;Initial Catalog=<>;User ID=<>;Password=<>;";
 
         // test schema for cosmosDB
@@ -41,6 +44,7 @@ public static Process ExecuteDabCommand(string command, string flags)
                 StartInfo =
                 {
                     FileName = @"./Microsoft.DataApiBuilder",
+                    CreateNoWindow = true,
                     Arguments = $"{command} {flags}",
                     WindowStyle = ProcessWindowStyle.Hidden,
                     UseShellExecute = false,

src/Cli/ConfigGenerator.cs

@@ -969,7 +969,8 @@ public static bool TryStartEngineWithOptions(StartOptions options, FileSystemRun
             loader.UpdateBaseConfigFileName(runtimeConfigFile);
 
             // Validates that config file has data and follows the correct json schema
-            if (!loader.TryLoadKnownConfig(out RuntimeConfig? deserializedRuntimeConfig))
+            // Replaces all the environment variables while deserializing when starting DAB.
+            if (!loader.TryLoadKnownConfig(out RuntimeConfig? deserializedRuntimeConfig, replaceEnvVar: true))
             {
                 _logger.LogError("Failed to parse the config file: {configFile}.", runtimeConfigFile);
                 return false;

src/Cli/Exporter.cs

@@ -26,7 +26,10 @@ public static void Export(ExportOptions options, ILogger logger, FileSystemRunti
                 return;
             }
 
-            if (!loader.TryLoadConfig(runtimeConfigFile, out RuntimeConfig? runtimeConfig) || runtimeConfig is null)
+            if (!loader.TryLoadConfig(
+                    runtimeConfigFile,
+                    out RuntimeConfig? runtimeConfig,
+                    replaceEnvVar: true) || runtimeConfig is null)
             {
                 logger.LogError("Failed to read the config file: {runtimeConfigFile}.", runtimeConfigFile);
                 return;