Overhaul of config system #1402
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Finding missing types Refactoring some type names Simplifying the GraphQL naming by doing that when building the config, rather than on demand
Added implementation of JsonConverter.Write methods to generate an initial pass on the config file using CLI
1 task
Removed some tests that weren't testing the CLI but dependencies (such as CommandLineParser or ILogger)
…them simpler and clearer with the new object structure
Moved logic for Entities defaults to RuntimeEntities rather than deserialiser, as that is a more logical place. We always pass that type around, so we can assume the ctor ran, but we aren't always assuming the deserialiser ran (such as what happens with tests)
Aniruddh25
added a commit
that referenced
this pull request
Aug 24, 2023
## 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 <sean.leonard@microsoft.com>
Aniruddh25
added a commit
that referenced
this pull request
Aug 24, 2023
## 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 <sean.leonard@microsoft.com>
Aniruddh25
added a commit
that referenced
this pull request
Aug 30, 2023
## Why make this change? - With the overhaul of config system PR #1402 , we started seeing redundant logs about which config file is being used. ## What is this change? - Store the environment based config file name found at construction time in the property `ConfigFileName`. - Remove Console writeline statements while checking for existence of file since this leads to multiple logs - one from CLI before starting the engine and second from within the engine itself when calling TryPargeConfig - Add a single log of the loaded file in CLI - Also catch exception for incorrect connection string parsing ## How was this tested? - Manually, using the `start` command of CLI as well as triggering the engine directly with argument: `--ConfigFileName` 1. Starting with default config: **Before**:  **After**:  2. Starting with user provided config: **Before**:  **After**:  3. Trying to start with missing file name - same behvior as before 
Aniruddh25
added a commit
that referenced
this pull request
Aug 30, 2023
- With the overhaul of config system PR #1402 , we started seeing redundant logs about which config file is being used. - Store the environment based config file name found at construction time in the property `ConfigFileName`. - Remove Console writeline statements while checking for existence of file since this leads to multiple logs - one from CLI before starting the engine and second from within the engine itself when calling TryPargeConfig - Add a single log of the loaded file in CLI - Also catch exception for incorrect connection string parsing - Manually, using the `start` command of CLI as well as triggering the engine directly with argument: `--ConfigFileName` 1. Starting with default config: **Before**:  **After**:  2. Starting with user provided config: **Before**:  **After**:  3. Trying to start with missing file name - same behvior as before 
Aniruddh25
added a commit
that referenced
this pull request
Aug 30, 2023
Cherry picking #1669 Original description: ## Why make this change? - With the overhaul of config system PR #1402 , we started seeing redundant logs about which config file is being used. ## What is this change? - Store the environment based config file name found at construction time in the property `ConfigFileName`. - Remove Console writeline statements while checking for existence of file since this leads to multiple logs - one from CLI before starting the engine and second from within the engine itself when calling TryPargeConfig - Add a single log of the loaded file in CLI ## How was this tested? - Manually, using the `start` command of CLI as well as triggering the engine directly with argument: `--ConfigFileName` 1. Starting with default config: **Before**:  **After**:  2. Starting with user provided config: **Before**:  **After**:  3. Trying to start with missing file name - same behvior as before 
1 task
Aniruddh25
added a commit
that referenced
this pull request
Sep 6, 2023
## Why make this change? - As per the JSON config schema, not all properties in the config file are mandatory. - With #1402, in versions 0.8.49+ those properties that are not explicitly set, are initialized with defaults before writing to the file system. - However, config files generated using version 0.7.6 or lesser, may not have these properties initialized to their defaults when writing to the file system. Using config files generated with <= 0.7.6, to start engine with 0.8.49 would therefore lead to null reference exceptions when the optional properties are dereferenced. - This PR is to ensure such null reference exceptions are avoided when starting engine with configs that don't have optional properties ## What is this change? - Modify object model constructors to accept nullable arguments for optional properties - represented as fields in the record types. If passed in argument is null, initialize the field with default values. Do this recursively for sub-properties. - To force JSON deserializer to pick up the custom constructor, add `IncludeFields = true` to `JsonSerializerOptions` - Default value of host mode is changed to `development` since `allow-introspection` has a default value of `true` ## How was this tested? - Removed optional properties from config and ensured engine could be started without them. - [X] Unit Tests ## Sample Request(s) - remove `runtime` section from config, starting engine with 0.8.49 leads to NullReferenceException. - After the fix, starting engine is successful even when `runtime` section is absent in the config.
1 task
Aniruddh25
added a commit
that referenced
this pull request
Sep 7, 2023
…o file (#1691) ## Why make this change? - Closes #1687 ## What is this change? - Set the Encoder for JsonSerializationOptions to be [`JavaScriptEncoder.UnsafeRelaxedJsonEscaping`](https://learn.microsoft.com/en-us/dotnet/api/system.text.encodings.web.javascriptencoder.unsaferelaxedjsonescaping?view=net-7.0) instead of the default encoder. - Although [this article](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters) mentions use this encoder with caution, since we are sure the file contents will be deserialized as UTF-8 JSON, we should be safe to use this encoder. - We were using this encoder previously from the fix in #1386, but with #1402, that original fix was essentially not being used since #1402 introduced usage of the new `GetJsonSerializerOptions()` function in `RuntimeConfigLoader`. ## How was this tested? - [X] Unit Tests. `TestSpecialCharactersInConnectionString` is moved to the `ConfigGeneratorTests` since keeping it as part of `InitTests` with the Verify framework wasn't really testing the scenario since we ignore connection strings in the snapshots. - This regression from #1402 was not caught because this test scenario was effectively being skipped. - Note, this test is modified to explicitly compare the expected and actual json string from file since any usage of JsonSerializer/JObject.Parse would require using the same encoder to deserialize making the test itself run similar code which it is testing. Parsing into JSON objects would treat `\u0027` and `'` as equal whereas string comparison wont. Hence, comparison using string is necessary to catch regressions here. ## Sample Request(s) BEFORE: `dab init --host-mode development --database-type mssql --connection-string "@env('my-connection-string')"` creates the configuration file as follows: ``` { "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json", "data-source": { "database-type": "mssql", "connection-string": "@env(\u0027my-connection-string\u0027)", "options": { "set-session-context": false } } ``` Note, the value of connection string. AFTER: Same command creates the following file: ``` { "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json", "data-source": { "database-type": "mssql", "connection-string": "@env('my-connection-string')", "options": { "set-session-context": false } } ``` --------- Co-authored-by: Abhishek Kumar <abhishekkuma@microsoft.com>
Aniruddh25
added a commit
that referenced
this pull request
Sep 7, 2023
…o file (#1691) ## Why make this change? - Closes #1687 ## What is this change? - Set the Encoder for JsonSerializationOptions to be [`JavaScriptEncoder.UnsafeRelaxedJsonEscaping`](https://learn.microsoft.com/en-us/dotnet/api/system.text.encodings.web.javascriptencoder.unsaferelaxedjsonescaping?view=net-7.0) instead of the default encoder. - Although [this article](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters) mentions use this encoder with caution, since we are sure the file contents will be deserialized as UTF-8 JSON, we should be safe to use this encoder. - We were using this encoder previously from the fix in #1386, but with #1402, that original fix was essentially not being used since #1402 introduced usage of the new `GetJsonSerializerOptions()` function in `RuntimeConfigLoader`. ## How was this tested? - [X] Unit Tests. `TestSpecialCharactersInConnectionString` is moved to the `ConfigGeneratorTests` since keeping it as part of `InitTests` with the Verify framework wasn't really testing the scenario since we ignore connection strings in the snapshots. - This regression from #1402 was not caught because this test scenario was effectively being skipped. - Note, this test is modified to explicitly compare the expected and actual json string from file since any usage of JsonSerializer/JObject.Parse would require using the same encoder to deserialize making the test itself run similar code which it is testing. Parsing into JSON objects would treat `\u0027` and `'` as equal whereas string comparison wont. Hence, comparison using string is necessary to catch regressions here. ## Sample Request(s) BEFORE: `dab init --host-mode development --database-type mssql --connection-string "@env('my-connection-string')"` creates the configuration file as follows: ``` { "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json", "data-source": { "database-type": "mssql", "connection-string": "@env(\u0027my-connection-string\u0027)", "options": { "set-session-context": false } } ``` Note, the value of connection string. AFTER: Same command creates the following file: ``` { "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json", "data-source": { "database-type": "mssql", "connection-string": "@env('my-connection-string')", "options": { "set-session-context": false } } ``` --------- Co-authored-by: Abhishek Kumar <abhishekkuma@microsoft.com>
Aniruddh25
pushed a commit
that referenced
this pull request
Sep 14, 2023
…the @model directive, regardless of the name of the GraphQL object type's name. (#1706) ## Why make this change? - Closes #1680 - Regression introduced in 0.8.49 caused required explicitly adding "name" directive for all the model/entity irrespective of whether we are using a different name than the one it originally has. - Regression introduced in #1402 when we started using TryExtractGraphQLName as follows: ``` string typeName = GraphQLUtils.TryExtractGraphQLFieldModelName(underlyingType.Directives, out string? modelName) ? modelName : underlyingType.Name; ``` ## What is this change? Checking Directive "name" exists before accessing its value. ## How was this tested? Tested locally for now. Will be checking Test cases for it. - [x] Integration Tests - [ ] Unit Tests --------- Co-authored-by: Neeraj Sharma <neesharma@microsoft.com>
neeraj-sharma2592
added a commit
that referenced
this pull request
Sep 14, 2023
…the @model directive, regardless of the name of the GraphQL object type's name. (#1706) ## Why make this change? - Closes #1680 - Regression introduced in 0.8.49 caused required explicitly adding "name" directive for all the model/entity irrespective of whether we are using a different name than the one it originally has. - Regression introduced in #1402 when we started using TryExtractGraphQLName as follows: ``` string typeName = GraphQLUtils.TryExtractGraphQLFieldModelName(underlyingType.Directives, out string? modelName) ? modelName : underlyingType.Name; ``` ## What is this change? Checking Directive "name" exists before accessing its value. ## How was this tested? Tested locally for now. Will be checking Test cases for it. - [x] Integration Tests - [ ] Unit Tests --------- Co-authored-by: Neeraj Sharma <neesharma@microsoft.com>
neeraj-sharma2592
added a commit
that referenced
this pull request
Sep 18, 2023
…the @model directive, regardless of the name of the GraphQL object type's name. (#1706) ## Why make this change? - Closes #1680 - Regression introduced in 0.8.49 caused required explicitly adding "name" directive for all the model/entity irrespective of whether we are using a different name than the one it originally has. - Regression introduced in #1402 when we started using TryExtractGraphQLName as follows: ``` string typeName = GraphQLUtils.TryExtractGraphQLFieldModelName(underlyingType.Directives, out string? modelName) ? modelName : underlyingType.Name; ``` ## What is this change? Checking Directive "name" exists before accessing its value. ## How was this tested? Tested locally for now. Will be checking Test cases for it. - [x] Integration Tests - [ ] Unit Tests --------- Co-authored-by: Neeraj Sharma <neesharma@microsoft.com>
neeraj-sharma2592
added a commit
that referenced
this pull request
Sep 18, 2023
…the @model directive, regardless of the name of the GraphQL object type's name. (#1714) ## Why make this change? - Closes #1680 - Regression introduced in 0.8.49 caused required explicitly adding "name" directive for all the model/entity irrespective of whether we are using a different name than the one it originally has. - Regression introduced in #1402 when we started using TryExtractGraphQLName as follows: ``` string typeName = GraphQLUtils.TryExtractGraphQLFieldModelName(underlyingType.Directives, out string? modelName) ? modelName : underlyingType.Name; ``` ## What is this change? Checking Directive "name" exists before accessing its value. ## How was this tested? - [x] Integration Tests Co-authored-by: Neeraj Sharma <neesharma@microsoft.com>
Aniruddh25
added a commit
that referenced
this pull request
Sep 19, 2023
## Why make this change? - As per the JSON config schema, not all properties in the config file are mandatory. - With #1402, in versions 0.8.49+ those properties that are not explicitly set, are initialized with defaults before writing to the file system. - However, config files generated using version 0.7.6 or lesser, may not have these properties initialized to their defaults when writing to the file system. Using config files generated with <= 0.7.6, to start engine with 0.8.49 would therefore lead to null reference exceptions when the optional properties are dereferenced. - This PR is to ensure such null reference exceptions are avoided when starting engine with configs that don't have optional properties ## What is this change? - Modify object model constructors to accept nullable arguments for optional properties - represented as fields in the record types. If passed in argument is null, initialize the field with default values. Do this recursively for sub-properties. - To force JSON deserializer to pick up the custom constructor, add `IncludeFields = true` to `JsonSerializerOptions` - Default value of host mode is changed to `development` since `allow-introspection` has a default value of `true` ## How was this tested? - Removed optional properties from config and ensured engine could be started without them. - [X] Unit Tests ## Sample Request(s) - remove `runtime` section from config, starting engine with 0.8.49 leads to NullReferenceException. - After the fix, starting engine is successful even when `runtime` section is absent in the config.
2 tasks
2 tasks
Aniruddh25
added a commit
that referenced
this pull request
Oct 20, 2023
## Why make this change? - As per the JSON config schema, not all properties in the config file are mandatory. - With #1402, in versions 0.8.49-0.8.52 those properties that are not explicitly set, were initialized with defaults before writing to the file system. - However, config files generated using version 0.7.6 or lesser, may not have these properties initialized to their defaults when writing to the file system. Using config files generated with <= 0.7.6, to start engine with 0.8.49 would therefore lead to null reference exceptions when the optional properties are dereferenced. - With PR #1684 the optional properties were initialized with their default values to avoid the null reference, any config file that is created or modified using the CLI results in a fully expanded file. This is not a problem when a single config is used but leads to undesired behavior when using the merge configuration file feature. A fully expanded higher precedence config file when merged with a base config file will override the values set for the optional properties in the base file with their default values if those properties were not explicitly defined in the higher precedence config file to begin with. In order to retain the base config file values for the optional properties, the higher precedence file would have to define every single optional value exactly the same as in the base file if they desire those to not be overridden. That defeats the purpose of providing the higher precedence file which should ideally only have those properties that need to be overriden. For more details with examples, please refer to #1737. - This PR fixes this problem by allowing optional properties to be null and ensures there are no null reference exceptions as well when starting engine with configs that don't have optional properties. ## What is this change? - Modify object model constructors to accept nullable arguments for optional properties - represented as fields in the record types. The scalar optional properties have not been made nullable yet, will have a follow up PR for those. - All references to optional properties check for nulls - During merge of config files, null + non-null values will pick up non-null values. After the merge, if the value is still null, they are considered as default values. To easily get default values, added some properties to `RuntimeConfig`. - Default value of Host.Mode is `Production`. Keep it that way. Default of GraphQL.AllowIntrospection is true currently - will have another PR to determine it based on host mode. ## How was this tested? - Removed optional properties from config and ensured engine could be started without them. - [X] Unit Tests for deserialization and serialization of nullable optional properties see `RuntimeConfigLoaderJsonDeserializerTests` - [X] In the merge configuration tests, in `TestHelper`, modify base config to contain a non-default value, env based config to not specify that value and confirm the merged config file has the base-config value. ## Sample Request(s) - Before fix, remove `runtime` section from config, run dab start with 0.8.49 leads to NullReferenceException. - After the fix, starting engine is successful even when `runtime` section is absent in the config. // IN THE BASE config file MISSING = use default { empty } = use default // IN THE OVERRIDDEN config file MISSING = use base { empty } = use base Examples: // all subproperties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { empty } merged: "user": { "first": "jerry", "last": "nixon" } // some sub properties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { "first": "jerry" } merged: "user": { "first": "jerry", "last": "nixon" } // parent object property is missing altogether base: "user": { "first": "jerry", "last": "nixon" } override: "user": MISSING (not specified in override) merged: "user": { "first": "jerry", "last": "nixon" } ## TO DO for 0.10-rc - A followup PR to make scalar optional properties nullable too. - A followup PR that makes specifying an explicit NULL imply Default values
Aniruddh25
added a commit
that referenced
this pull request
Oct 20, 2023
## Why make this change? - As per the JSON config schema, not all properties in the config file are mandatory. - With #1402, in versions 0.8.49-0.8.52 those properties that are not explicitly set, were initialized with defaults before writing to the file system. - However, config files generated using version 0.7.6 or lesser, may not have these properties initialized to their defaults when writing to the file system. Using config files generated with <= 0.7.6, to start engine with 0.8.49 would therefore lead to null reference exceptions when the optional properties are dereferenced. - With PR #1684 the optional properties were initialized with their default values to avoid the null reference, any config file that is created or modified using the CLI results in a fully expanded file. This is not a problem when a single config is used but leads to undesired behavior when using the merge configuration file feature. A fully expanded higher precedence config file when merged with a base config file will override the values set for the optional properties in the base file with their default values if those properties were not explicitly defined in the higher precedence config file to begin with. In order to retain the base config file values for the optional properties, the higher precedence file would have to define every single optional value exactly the same as in the base file if they desire those to not be overridden. That defeats the purpose of providing the higher precedence file which should ideally only have those properties that need to be overriden. For more details with examples, please refer to #1737. - This PR fixes this problem by allowing optional properties to be null and ensures there are no null reference exceptions as well when starting engine with configs that don't have optional properties. ## What is this change? - Modify object model constructors to accept nullable arguments for optional properties - represented as fields in the record types. The scalar optional properties have not been made nullable yet, will have a follow up PR for those. - All references to optional properties check for nulls - During merge of config files, null + non-null values will pick up non-null values. After the merge, if the value is still null, they are considered as default values. To easily get default values, added some properties to `RuntimeConfig`. - Default value of Host.Mode is `Production`. Keep it that way. Default of GraphQL.AllowIntrospection is true currently - will have another PR to determine it based on host mode. ## How was this tested? - Removed optional properties from config and ensured engine could be started without them. - [X] Unit Tests for deserialization and serialization of nullable optional properties see `RuntimeConfigLoaderJsonDeserializerTests` - [X] In the merge configuration tests, in `TestHelper`, modify base config to contain a non-default value, env based config to not specify that value and confirm the merged config file has the base-config value. ## Sample Request(s) - Before fix, remove `runtime` section from config, run dab start with 0.8.49 leads to NullReferenceException. - After the fix, starting engine is successful even when `runtime` section is absent in the config. // IN THE BASE config file MISSING = use default { empty } = use default // IN THE OVERRIDDEN config file MISSING = use base { empty } = use base Examples: // all subproperties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { empty } merged: "user": { "first": "jerry", "last": "nixon" } // some sub properties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { "first": "jerry" } merged: "user": { "first": "jerry", "last": "nixon" } // parent object property is missing altogether base: "user": { "first": "jerry", "last": "nixon" } override: "user": MISSING (not specified in override) merged: "user": { "first": "jerry", "last": "nixon" } ## TO DO for 0.10-rc - A followup PR to make scalar optional properties nullable too. - A followup PR that makes specifying an explicit NULL imply Default values
seantleonard
pushed a commit
that referenced
this pull request
Oct 21, 2023
## Why make this change? - As per the JSON config schema, not all properties in the config file are mandatory. - With #1402, in versions 0.8.49-0.8.52 those properties that are not explicitly set, were initialized with defaults before writing to the file system. - However, config files generated using version 0.7.6 or lesser, may not have these properties initialized to their defaults when writing to the file system. Using config files generated with <= 0.7.6, to start engine with 0.8.49 would therefore lead to null reference exceptions when the optional properties are dereferenced. - With PR #1684 the optional properties were initialized with their default values to avoid the null reference, any config file that is created or modified using the CLI results in a fully expanded file. This is not a problem when a single config is used but leads to undesired behavior when using the merge configuration file feature. A fully expanded higher precedence config file when merged with a base config file will override the values set for the optional properties in the base file with their default values if those properties were not explicitly defined in the higher precedence config file to begin with. In order to retain the base config file values for the optional properties, the higher precedence file would have to define every single optional value exactly the same as in the base file if they desire those to not be overridden. That defeats the purpose of providing the higher precedence file which should ideally only have those properties that need to be overriden. For more details with examples, please refer to #1737. - This PR fixes this problem by allowing optional properties to be null and ensures there are no null reference exceptions as well when starting engine with configs that don't have optional properties. ## What is this change? - Modify object model constructors to accept nullable arguments for optional properties - represented as fields in the record types. The scalar optional properties have not been made nullable yet, will have a follow up PR for those. - All references to optional properties check for nulls - During merge of config files, null + non-null values will pick up non-null values. After the merge, if the value is still null, they are considered as default values. To easily get default values, added some properties to `RuntimeConfig`. - Default value of Host.Mode is `Production`. Keep it that way. Default of GraphQL.AllowIntrospection is true currently - will have another PR to determine it based on host mode. ## How was this tested? - Removed optional properties from config and ensured engine could be started without them. - [X] Unit Tests for deserialization and serialization of nullable optional properties see `RuntimeConfigLoaderJsonDeserializerTests` - [X] In the merge configuration tests, in `TestHelper`, modify base config to contain a non-default value, env based config to not specify that value and confirm the merged config file has the base-config value. ## Sample Request(s) - Before fix, remove `runtime` section from config, run dab start with 0.8.49 leads to NullReferenceException. - After the fix, starting engine is successful even when `runtime` section is absent in the config. // IN THE BASE config file MISSING = use default { empty } = use default // IN THE OVERRIDDEN config file MISSING = use base { empty } = use base Examples: // all subproperties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { empty } merged: "user": { "first": "jerry", "last": "nixon" } // some sub properties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { "first": "jerry" } merged: "user": { "first": "jerry", "last": "nixon" } // parent object property is missing altogether base: "user": { "first": "jerry", "last": "nixon" } override: "user": MISSING (not specified in override) merged: "user": { "first": "jerry", "last": "nixon" } ## TO DO for 0.10-rc - A followup PR to make scalar optional properties nullable too. - A followup PR that makes specifying an explicit NULL imply Default values
2 tasks
seantleonard
pushed a commit
that referenced
this pull request
Oct 23, 2023
## Why make this change? - As per the JSON config schema, not all properties in the config file are mandatory. - With #1402, in versions 0.8.49-0.8.52 those properties that are not explicitly set, were initialized with defaults before writing to the file system. - However, config files generated using version 0.7.6 or lesser, may not have these properties initialized to their defaults when writing to the file system. Using config files generated with <= 0.7.6, to start engine with 0.8.49 would therefore lead to null reference exceptions when the optional properties are dereferenced. - With PR #1684 the optional properties were initialized with their default values to avoid the null reference, any config file that is created or modified using the CLI results in a fully expanded file. This is not a problem when a single config is used but leads to undesired behavior when using the merge configuration file feature. A fully expanded higher precedence config file when merged with a base config file will override the values set for the optional properties in the base file with their default values if those properties were not explicitly defined in the higher precedence config file to begin with. In order to retain the base config file values for the optional properties, the higher precedence file would have to define every single optional value exactly the same as in the base file if they desire those to not be overridden. That defeats the purpose of providing the higher precedence file which should ideally only have those properties that need to be overriden. For more details with examples, please refer to #1737. - This PR fixes this problem by allowing optional properties to be null and ensures there are no null reference exceptions as well when starting engine with configs that don't have optional properties. ## What is this change? - Modify object model constructors to accept nullable arguments for optional properties - represented as fields in the record types. The scalar optional properties have not been made nullable yet, will have a follow up PR for those. - All references to optional properties check for nulls - During merge of config files, null + non-null values will pick up non-null values. After the merge, if the value is still null, they are considered as default values. To easily get default values, added some properties to `RuntimeConfig`. - Default value of Host.Mode is `Production`. Keep it that way. Default of GraphQL.AllowIntrospection is true currently - will have another PR to determine it based on host mode. ## How was this tested? - Removed optional properties from config and ensured engine could be started without them. - [X] Unit Tests for deserialization and serialization of nullable optional properties see `RuntimeConfigLoaderJsonDeserializerTests` - [X] In the merge configuration tests, in `TestHelper`, modify base config to contain a non-default value, env based config to not specify that value and confirm the merged config file has the base-config value. ## Sample Request(s) - Before fix, remove `runtime` section from config, run dab start with 0.8.49 leads to NullReferenceException. - After the fix, starting engine is successful even when `runtime` section is absent in the config. // IN THE BASE config file MISSING = use default { empty } = use default // IN THE OVERRIDDEN config file MISSING = use base { empty } = use base Examples: // all subproperties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { empty } merged: "user": { "first": "jerry", "last": "nixon" } // some sub properties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { "first": "jerry" } merged: "user": { "first": "jerry", "last": "nixon" } // parent object property is missing altogether base: "user": { "first": "jerry", "last": "nixon" } override: "user": MISSING (not specified in override) merged: "user": { "first": "jerry", "last": "nixon" } ## TO DO for 0.10-rc - A followup PR to make scalar optional properties nullable too. - A followup PR that makes specifying an explicit NULL imply Default values
2 tasks
seantleonard
added a commit
that referenced
this pull request
Oct 24, 2023
Cherry pick #1823 to 0.9 branch. ## Why make this change? - As per the JSON config schema, not all properties in the config file are mandatory. - With #1402, in versions 0.8.49-0.8.52 those properties that are not explicitly set, were initialized with defaults before writing to the file system. - However, config files generated using version 0.7.6 or lesser, may not have these properties initialized to their defaults when writing to the file system. Using config files generated with <= 0.7.6, to start engine with 0.8.49 would therefore lead to null reference exceptions when the optional properties are dereferenced. - With PR #1684 the optional properties were initialized with their default values to avoid the null reference, any config file that is created or modified using the CLI results in a fully expanded file. This is not a problem when a single config is used but leads to undesired behavior when using the merge configuration file feature. A fully expanded higher precedence config file when merged with a base config file will override the values set for the optional properties in the base file with their default values if those properties were not explicitly defined in the higher precedence config file to begin with. In order to retain the base config file values for the optional properties, the higher precedence file would have to define every single optional value exactly the same as in the base file if they desire those to not be overridden. That defeats the purpose of providing the higher precedence file which should ideally only have those properties that need to be overriden. For more details with examples, please refer to #1737. - This PR fixes this problem by allowing optional properties to be null and ensures there are no null reference exceptions as well when starting engine with configs that don't have optional properties. ## What is this change? - Modify object model constructors to accept nullable arguments for optional properties - represented as fields in the record types. The scalar optional properties have not been made nullable yet, will have a follow up PR for those. - All references to optional properties check for nulls - During merge of config files, null + non-null values will pick up non-null values. After the merge, if the value is still null, they are considered as default values. To easily get default values, added some properties to `RuntimeConfig`. - Default value of Host.Mode is `Production`. Keep it that way. Default of GraphQL.AllowIntrospection is true currently - will have another PR to determine it based on host mode. ## How was this tested? - Removed optional properties from config and ensured engine could be started without them. - [X] Unit Tests for deserialization and serialization of nullable optional properties see `RuntimeConfigLoaderJsonDeserializerTests` - [X] In the merge configuration tests, in `TestHelper`, modify base config to contain a non-default value, env based config to not specify that value and confirm the merged config file has the base-config value. ## Sample Request(s) - Before fix, remove `runtime` section from config, run dab start with 0.8.49 leads to NullReferenceException. - After the fix, starting engine is successful even when `runtime` section is absent in the config. // IN THE BASE config file MISSING = use default { empty } = use default // IN THE OVERRIDDEN config file MISSING = use base { empty } = use base Examples: // all subproperties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { empty } merged: "user": { "first": "jerry", "last": "nixon" } // some sub properties are missing base: "user": { "first": "jerry", "last": "nixon" } override: "user": { "first": "jerry" } merged: "user": { "first": "jerry", "last": "nixon" } // parent object property is missing altogether base: "user": { "first": "jerry", "last": "nixon" } override: "user": MISSING (not specified in override) merged: "user": { "first": "jerry", "last": "nixon" } ## TO DO for 0.10-rc - A followup PR to make scalar optional properties nullable too. - A followup PR that makes specifying an explicit NULL imply Default values --------- Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why make this change?
The original config system has a lot of areas where it doesn't provide concrete types for what we need, due to the flexible nature of the JSON file, and instead forces the code to check at runtime (is the property a bool or an object?) and then have branching logic based on that.
This results in duplication of code, as well as hidden assumptions for what the "expanded" version of shorthand properties (by shorthand properties I mean properties like
"rest": falseon theruntime).With this change we reduce the config C# type system to only handle the fully expanded objects, resulting in a clearer structure for what the types are between JSON and C#, reduce the amount of type casting that needs to be done when checking the config, and unblocking at ability to tackle more complex config features, such as #722.
What is this change?
JsonConverterclasses for parts of the config serializer/deserializer, so that we can expand the config and apply the defaults as expected. The result of this is that if you deserialize then serialize the resulting config can look different as we always serialize to the expanded versionRuntimeConfigand write it to a mock file system, meaning they are a lot more isolated from each otherRuntimeConfigfor the DB and then write out a config (with test-specific features) to the mock file system, which is then given to the test runner to use, rather than writing to disk (although some tests will still write to disk)How was this tested?