Fix issue with schema validation for config with env variables #2200

abhishekkumams · 2024-05-02T17:57:59Z

Why make this change?

Closes [Bug]: Schema Validation is not considering environment variables #1979
- the Json sent to schema validation was validated without replacing environment variables, due to which it started throwing error incase of type mismatch.

What is this change?

Added a method that takes in Json as string, and returns the new Json with replaced environment variables as string.

NOTE:

There were certain methods which was doing replacement, but it was written in a way to align with the RuntimeConfig Object. So, it couldn't be used. for example, different converterFactory classes were calling this StringJsonConvertorFactory and then converting it to required type based on the definition in the schema.
Also, trying to deserialize it and then serialize it again changes the formatting. for example:
no by invalid i mean.
if i deserialize the below json

{
"database-type": "mssql"
}

and then deserialize it

{
"database-type" : {
"DatabaseType": 2
}
}

Also, we need the original conversion mechanism to stay since it will be utilized for the workflow outside of dab validate.
Since, the new method takes string as input, it can be used in the future for the hosting scenario as the config file is provided as string.

How was this tested?

Unit Tests

Sample Request(s)

"data-source": {
      "database-type": "@env('DATABASE_TYPE')",
      "options": {
        "database": "@env('DATABASE_NAME')",
        "schema": "@env('GRAPHQL_SCHEMA_PATH')"
      },
      "connection-string": "@env('CONN_STRING')"
    }

Before update

After Update

…validation

abhishekkumams · 2024-05-02T18:02:51Z

/azp run

abhishekkumams · 2024-05-02T18:43:55Z

/azp run

seantleonard

I don't follow why we need new environment variable parsing logic as opposed to using pre-existing code.

seantleonard · 2024-05-02T20:25:23Z

src/Service.Tests/Unittests/SerializationDeserializationTests.cs

This file is for testing DW object serialization. Any reason why config env_var tests are placed here?

Okay, With the name of the file, it was not very evident that it will be used only for DW

seantleonard · 2024-05-02T20:32:29Z

src/Service.Tests/Unittests/SerializationDeserializationTests.cs

+
+        /// <summary>
+        /// This test checks that the ReplaceEnvVarsInJson method throws a JsonException when given an invalid JSON string.
+        /// The test asserts that the exception's message matches the expected error message.


The test asserts that the exception's message matches the expected error message.

nit: While it's good to note that you're checking the emitted error message, your description would be even more beneficial to the reader if it described that you're validating that the error message blames an environment variable replacement failure versus some other JSON parsing issue.

seantleonard · 2024-05-02T20:35:09Z

src/Service.Tests/Unittests/SerializationDeserializationTests.cs

+        [TestMethod]
+        public void TestEnvVariableReplacementWithInvalidJson()
+        {
+            // Create an invalid JSON string


nit: note which part is invalid. (I'm assuming lack of closing quotation marks after the ENV_STRING.)

src/Service.Tests/Unittests/SerializationDeserializationTests.cs

seantleonard · 2024-05-02T20:38:46Z

src/Service.Tests/Unittests/SerializationDeserializationTests.cs

+            catch (JsonException ex)
+            {
+                Assert.AreEqual("Failed to replace environment variables in given JSON. "
+                    + "Environment variable 'MISSING_ENV_VAR' is not set.", ex.Message);


Wouldn't the following scenarios result in this error message? I suppose "unset environment variable" would apply to both of these:

MISSING_ENV_VAR exists but doesn't have a value -> null

MISSING_ENV_VAR doesn't exist -> null

well, actually not. environment files are read as test. environment values are read as string,i.e. "null", and then added to the json as a null value. So, if a user wants to set a null value. it will be assigned as null and no error should be thorwn. there is a test for the same.

src/Core/Configurations/RuntimeConfigValidator.cs

seantleonard · 2024-05-02T21:03:14Z

src/Config/Converters/Utf8JsonReaderExtensions.cs

+    /// </summary>
+    /// <param name="inputJson">The JSON string to process.</param>
+    /// <exception cref="JsonException">Thrown when the input is null or when an error occurs while processing the JSON.</exception>
+    public static string? ReplaceEnvVarsInJson(string? inputJson)


why do we need this function when StringJsonConverterFactory() already exists which has helpers to parse out envvironment variable placeholders? that class also uses regex to detect instances of @env()

why not something like:

TryParseConfig( json: json, config: out config, connectionString: _connectionString, replaceEnvVar: true)

Is there something about dab validate that requires a completely different code path + new code?
if so , please add to pr description and code comments.

please elaborate in PR description. this doesn't really tell us much about "why" it can't be used.

There were certain methods which was doing replacement, but it was written in a way to align with the RuntimeConfig Object. So, it couldn't be used.

Also, we need the original conversion mechanism to stay since it will be utilized for the workflow outside of dab validate.

Not clear why original conversion would need to change.

Aniruddh25

Some questions on the approach

src/Config/Converters/Utf8JsonReaderExtensions.cs

Aniruddh25 · 2024-05-03T06:18:26Z

src/Core/Configurations/RuntimeConfigValidator.cs

+        // The config file may contain some environment variables that need to be replaced before validation.
+        try
+        {
+            jsonData = Utf8JsonReaderExtensions.ReplaceEnvVarsInJson(jsonData);


We already have the runtimeConfig deserialized from the configFilePath.

Wouldnt that mean we already had the jsonData with the environment variables replaced?
Or does the runtimeConfig object doesnt have the environment variable values?
Instead of reading from the file path, cant we get the string from which the runtimeConfig object was formed - to do the schema validation on?

no we cant. schemaValidator require a json string. i tried converting the deserialized runtimeConfig to string and then passing it to the schema validator but it was not forming a valid json string.

can you provide an example? invalid json string because of extra escape characters? is 'invalid json string' indicative of a different bug?

no by invalid i mean.
if i deserialize the below json

{ "database-type": "mssql" }

and then serialize it again

{ "database-type" : { "DatabaseType": 2 } }

that seems like another bug

it's just how enums are serialized i believe, don't think it's a bug

it's just how enums are serialized i believe, don't think it's a bug

I think we would need to have custom (de)serialization that handles the case of these enums so that they go back to the intended format. Deserialization/Serialization ought to be reversable so this does seem like a bug that just hasnt arisen yet.

using string jsonData = runtimeConfig.ToJson() instead of all the custom parsing code and then provided jsonData to:
await jsonConfigSchemaValidator.ValidateJsonConfigWithSchemaAsync(jsonSchema, jsonData!);

works great and doesn't present any of the issues mentioned earlier in this thread. Am I missing something?

do the tests added in this PR pass with your proposed changes? If yes, we may not need the custom parsing code mentioned here.

…validation

Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>

…validation

aaronburtle · 2024-05-09T23:41:57Z

src/Config/Converters/Utf8JsonReaderExtensions.cs

+                break;
+            case JsonValueKind.String:
+                string? value = element.GetString();
+                if (value is not null && value.StartsWith("@env('") && value.EndsWith("')"))


Is this assuming that the start of the string is the beginning of the env('<match>')?

What happens if the env('<match>') happens in the middle of the string?

For example, if the person had

{
"database-type": "env('db_type')env('db_lang')"
}

where their environment variable are db_type=ms, and db_lang=sql

will this replace env('db_lang') with sql?

Seems like this would skip all of the replacements except the first and only replace the first when the string starts with the env variable.

If you look at the StringJsonConverterFactory there is logic that handles all of our edge cases with regex match and replace. If you can't use that class explicitly, perhaps you can factor out the logic that is being used for parsing the env variables to be shared?

aaronburtle · 2024-05-10T03:16:13Z

src/Config/Converters/Utf8JsonReaderExtensions.cs

+
+                break;
+            default:
+                // Write the JSON element to the writer as is


So if the env('') doesnt start the string we end up here, but then we don't do any replacements?

aaronburtle

Some questions about how and when we are matching the environment variables, and if we can share some code.

…validation

## Why make this change? - Closes #2200, an issue with `dab validate -c "path_to_config.json"` where environment variable `@env('ENV_VAR_NAME')` references are unresolved during config schema validation and schema validation always fails. The issue specifically calls out an error when an environment variable is used for the property `database-type` can't resolve an environment variable and properly set an enum value (mssql, pgsql, mysql, etc) for the property. ## What is this change? - This change corrects how `RuntimeConfigValidator::ValidateConfigSchema(...)` resolves environment variables in the runtime config. The usage of `string jsonData = runtimeConfig.ToJson();` converts the runtime config object (which already has environment variables resolved) into valid json which can then be validated against `dab.draft.schema.json`. ## How was this tested? - [x] Integration Tests: added a test which exercises the code path that validates the dab runtime config against the dab json schema. By ensuring that no schema validation errors occur due to unresolved environment variables. ## Sample Request(s) Add the file `launchSettings.json` to `src/Cli/Properties` with the following contents: ```json { "profiles": { "start": { "commandName": "Project", "commandLineArgs": "start -c src\\Cli\\dab-config.json", "environmentVariables": { "DATABASE_TYPE": "mssql", "DATABASE_NAME": "<your DB name", "CONN_STRING": "<CONN_STRING>", "AUTH_METHOD": "Simulator", "PUB_ENT_SRC": "publishers" }, "httpPort": 5002 }, "validate": { "commandName": "Project", "commandLineArgs": "validate -c src\\Cli\\dab-config.json", "environmentVariables": { "DATABASE_TYPE": "mssql", "DATABASE_NAME": "<your DB name>", "CONN_STRING": "<CONN_STRING>", "AUTH_METHOD": "Simulator", "PUB_ENT_SRC": "publishers" }, "httpPort": 5002 } } } ``` and modify your dab-config.json to have the following contents (assumes that you've deployed the DAB test project's .sql script: ```json { "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.13.0-rc/dab.draft.schema.json", "data-source": { "database-type": "@env('DATABASE_TYPE')", "connection-string": "@env('CONN_STRING')" }, "runtime": { "rest": { "enabled": true, "path": "/api" }, "graphql": { "allow-introspection": true, "enabled": true, "path": "/graphql" }, "host": { "mode": "development", "cors": { "origins": [ "http://localhost:5000" ], "allow-credentials": false }, "authentication": { "provider": "@env('AUTH_METHOD')" } } }, "entities": { "Query": { "source": "books", "permissions": [ { "role": "anonymous", "actions": [ "read" ] } ], "graphql": { "type": { "singular": "ProductCatalogue", "plural": "ProductCatalogue" } } }, "Product": { "source": "@env('PUB_ENT_SRC')", "permissions": [ { "role": "authenticated", "actions": [ "read", "delete", "update" ] } ] } } } ```

fixing issue with json schema validation with env variables

ec45d82

abhishekkumams requested review from mbhaskar, Aniruddh25, seantleonard, Mathos1432, gledis69, aaronburtle, tarazou9, ayush3797, aaronpowell, severussundar, ravishetye, rohkhann, neeraj-sharma2592 and sourabh1007 as code owners May 2, 2024 17:58

abhishekkumams and others added 2 commits May 2, 2024 23:29

fix formatting

bb767ab

Merge branch 'main' into dev/abhishekkuma/replace-env-var-for-schema-…

ebdeb04

…validation

abhishekkumams added 2 commits May 3, 2024 00:12

fixing issue with null values

e02177a

merging changes from main

75a2c19

seantleonard requested changes May 2, 2024

View reviewed changes

Aniruddh25 requested changes May 3, 2024

View reviewed changes

Aniruddh25 added this to the 1.1rc milestone May 3, 2024

ayush3797 and others added 3 commits May 7, 2024 21:42

Merge branch 'main' into dev/abhishekkuma/replace-env-var-for-schema-…

d5f2813

…validation

Update src/Service.Tests/Unittests/SerializationDeserializationTests.cs

850abd7

Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>

Merge branch 'main' into dev/abhishekkuma/replace-env-var-for-schema-…

8d55b74

…validation

aaronburtle reviewed May 9, 2024

View reviewed changes

aaronburtle reviewed May 10, 2024

View reviewed changes

seantleonard modified the milestones: 1.1rc, 1.1 Patch 01 May 11, 2024

Merge branch 'main' into dev/abhishekkuma/replace-env-var-for-schema-…

22a72ab

…validation

seantleonard mentioned this pull request May 13, 2024

Fix dab validate env_var parsing. #2215

Merged

1 task

seantleonard closed this in #2215 May 14, 2024

Fix issue with schema validation for config with env variables #2200

Fix issue with schema validation for config with env variables #2200

Uh oh!

Conversation

abhishekkumams commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why make this change?

What is this change?

NOTE:

How was this tested?

Sample Request(s)

Before update

After Update

Uh oh!

abhishekkumams commented May 2, 2024

Uh oh!

abhishekkumams commented May 2, 2024

Uh oh!

seantleonard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aniruddh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhishekkumams May 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronburtle May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronburtle May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronburtle May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronburtle left a comment

abhishekkumams commented May 2, 2024 •

edited

Loading

abhishekkumams May 4, 2024 •

edited

Loading

aaronburtle May 9, 2024 •

edited

Loading

aaronburtle May 9, 2024 •

edited

Loading

aaronburtle May 10, 2024 •

edited

Loading