Skip to content

[Configuration] Make optional object sub properties nullable#1823

Merged
Aniruddh25 merged 30 commits intomainfrom
makeOptionalPropsNullable
Oct 20, 2023
Merged

[Configuration] Make optional object sub properties nullable#1823
Aniruddh25 merged 30 commits intomainfrom
makeOptionalPropsNullable

Conversation

@Aniruddh25
Copy link
Copy Markdown
Collaborator

@Aniruddh25 Aniruddh25 commented Oct 17, 2023

Why make this change?

  • As per the JSON config schema, not all properties in the config file are mandatory.

  • With Overhaul of config system #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 [Configuration] Initialize optional properties with defaults #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 [Bug]: Merged configuration file always has the optional properties with their default values if they are not specified in the overridden file #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.
  • Unit Tests for deserialization and serialization of nullable optional properties see RuntimeConfigLoaderJsonDeserializerTests
  • 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

## 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.
@Aniruddh25 Aniruddh25 added this to the 0.9 milestone Oct 18, 2023
Copy link
Copy Markdown
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions to help me understand your testing approach and implications of making config properties nullable.

Comment thread src/Cli.Tests/EndToEndTests.cs
Comment thread src/Cli.Tests/TestHelper.cs
Comment thread src/Cli/Utils.cs
Comment thread src/Service.Tests/Configuration/ConfigurationTests.cs
@Aniruddh25 Aniruddh25 enabled auto-merge (squash) October 20, 2023 16:13
Comment thread src/Config/Converters/GraphQLRuntimeOptionsConverterFactory.cs
Comment thread src/Config/Converters/GraphQLRuntimeOptionsConverterFactory.cs
Copy link
Copy Markdown
Contributor

@aaronburtle aaronburtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The improvements to the RuntimeConfig properties are very nice!

@Aniruddh25 Aniruddh25 merged commit 00842f6 into main Oct 20, 2023
@Aniruddh25 Aniruddh25 deleted the makeOptionalPropsNullable branch October 20, 2023 23:01
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
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
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants