Skip to content

Commit

Permalink
[Configuration] Make optional object sub properties nullable (#1823)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
Aniruddh25 committed Oct 20, 2023
1 parent 34fa935 commit 86d73d2
Show file tree
Hide file tree
Showing 36 changed files with 862 additions and 294 deletions.
33 changes: 23 additions & 10 deletions src/Cli.Tests/EndToEndTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public Task TestInitForCosmosDBNoSql()
Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));

Assert.IsNotNull(runtimeConfig);
Assert.IsTrue(runtimeConfig.Runtime.GraphQL.AllowIntrospection);
Assert.IsTrue(runtimeConfig.AllowIntrospection);
Assert.AreEqual(DatabaseType.CosmosDB_NoSQL, runtimeConfig.DataSource.DatabaseType);
CosmosDbNoSQLDataSourceOptions? cosmosDataSourceOptions = runtimeConfig.DataSource.GetTypedOptions<CosmosDbNoSQLDataSourceOptions>();
Assert.IsNotNull(cosmosDataSourceOptions);
Expand Down Expand Up @@ -92,13 +92,17 @@ public void TestInitForCosmosDBPostgreSql()
Assert.IsNotNull(runtimeConfig);
Assert.AreEqual(DatabaseType.CosmosDB_PostgreSQL, runtimeConfig.DataSource.DatabaseType);
Assert.IsNotNull(runtimeConfig.Runtime);
Assert.IsNotNull(runtimeConfig.Runtime.Rest);
Assert.AreEqual("/rest-api", runtimeConfig.Runtime.Rest.Path);
Assert.IsTrue(runtimeConfig.Runtime.Rest.Enabled);
Assert.IsNotNull(runtimeConfig.Runtime.GraphQL);
Assert.AreEqual("/graphql-api", runtimeConfig.Runtime.GraphQL.Path);
Assert.IsTrue(runtimeConfig.Runtime.GraphQL.Enabled);

HostOptions hostGlobalSettings = runtimeConfig.Runtime.Host;
CollectionAssert.AreEqual(new string[] { "localhost:3000", "www.nolocalhost.com:80" }, hostGlobalSettings.Cors!.Origins);
HostOptions? hostGlobalSettings = runtimeConfig.Runtime?.Host;
Assert.IsNotNull(hostGlobalSettings);
Assert.IsNotNull(hostGlobalSettings.Cors);
CollectionAssert.AreEqual(new string[] { "localhost:3000", "www.nolocalhost.com:80" }, hostGlobalSettings.Cors.Origins);
}

/// <summary>
Expand All @@ -122,10 +126,10 @@ public void TestInitializingRestAndGraphQLGlobalSettings()
Assert.IsNotNull(runtimeConfig);
Assert.AreEqual(DatabaseType.MSSQL, runtimeConfig.DataSource.DatabaseType);
Assert.IsNotNull(runtimeConfig.Runtime);
Assert.AreEqual("/rest-api", runtimeConfig.Runtime.Rest.Path);
Assert.IsFalse(runtimeConfig.Runtime.Rest.Enabled);
Assert.AreEqual("/graphql-api", runtimeConfig.Runtime.GraphQL.Path);
Assert.IsTrue(runtimeConfig.Runtime.GraphQL.Enabled);
Assert.AreEqual("/rest-api", runtimeConfig.Runtime.Rest?.Path);
Assert.IsFalse(runtimeConfig.Runtime.Rest?.Enabled);
Assert.AreEqual("/graphql-api", runtimeConfig.Runtime.GraphQL?.Path);
Assert.IsTrue(runtimeConfig.Runtime.GraphQL?.Enabled);
}

/// <summary>
Expand All @@ -143,7 +147,7 @@ public void TestAddEntity()
// Perform assertions on various properties.
Assert.IsNotNull(runtimeConfig);
Assert.AreEqual(0, runtimeConfig.Entities.Count()); // No entities
Assert.AreEqual(HostMode.Development, runtimeConfig.Runtime.Host.Mode);
Assert.AreEqual(HostMode.Development, runtimeConfig.Runtime?.Host?.Mode);

string[] addArgs = {"add", "todo", "-c", TEST_RUNTIME_CONFIG_FILE, "--source", "s001.todo",
"--rest", "todo", "--graphql", "todo", "--permissions", "anonymous:*"};
Expand Down Expand Up @@ -179,6 +183,8 @@ public void TestVerifyAuthenticationOptions()
Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
Assert.IsNotNull(runtimeConfig);

Assert.IsNotNull(runtimeConfig.Runtime);
Assert.IsNotNull(runtimeConfig.Runtime.Host);
Assert.AreEqual("AzureAD", runtimeConfig.Runtime.Host.Authentication?.Provider);
Assert.AreEqual("aud-xxx", runtimeConfig.Runtime.Host.Authentication?.Jwt?.Audience);
Assert.AreEqual("issuer-xxx", runtimeConfig.Runtime.Host.Authentication?.Jwt?.Issuer);
Expand All @@ -204,7 +210,7 @@ public void EnsureHostModeEnumIsCaseInsensitive(string hostMode, HostMode hostMo
if (expectSuccess)
{
Assert.IsNotNull(runtimeConfig);
Assert.AreEqual(hostModeEnumType, runtimeConfig.Runtime.Host.Mode);
Assert.AreEqual(hostModeEnumType, runtimeConfig.Runtime?.Host?.Mode);
}
else
{
Expand All @@ -225,7 +231,7 @@ public void TestAddEntityWithoutIEnumerable()

Assert.IsNotNull(runtimeConfig);
Assert.AreEqual(0, runtimeConfig.Entities.Count()); // No entities
Assert.AreEqual(HostMode.Production, runtimeConfig.Runtime.Host.Mode);
Assert.AreEqual(HostMode.Production, runtimeConfig.Runtime?.Host?.Mode);

string[] addArgs = { "add", "book", "-c", TEST_RUNTIME_CONFIG_FILE, "--source", "s001.book", "--permissions", "anonymous:*" };
Program.Execute(addArgs, _cliLogger!, _fileSystem!, _runtimeConfigLoader!);
Expand Down Expand Up @@ -765,6 +771,7 @@ public void TestBaseRouteIsConfigurableForSWA(string authProvider, bool isExcept
{
Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
Assert.IsNotNull(runtimeConfig);
Assert.IsNotNull(runtimeConfig.Runtime);
Assert.AreEqual("/base-route", runtimeConfig.Runtime.BaseRoute);
}
}
Expand Down Expand Up @@ -821,10 +828,14 @@ public void TestEnabledDisabledFlagsForApis(

if (apiType is ApiType.REST)
{
Assert.IsNotNull(runtimeConfig.Runtime);
Assert.IsNotNull(runtimeConfig.Runtime.Rest);
Assert.AreEqual(expectedEnabledFlagValueInConfig, runtimeConfig.Runtime.Rest.Enabled);
}
else
{
Assert.IsNotNull(runtimeConfig.Runtime);
Assert.IsNotNull(runtimeConfig.Runtime.GraphQL);
Assert.AreEqual(expectedEnabledFlagValueInConfig, runtimeConfig.Runtime.GraphQL.Enabled);
}
}
Expand Down Expand Up @@ -855,6 +866,8 @@ public void TestRestRequestBodyStrictMode(bool includeRestRequestBodyStrictFlag,
Program.Execute(initArgs, _cliLogger!, _fileSystem!, _runtimeConfigLoader!);

Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
Assert.IsNotNull(runtimeConfig.Runtime);
Assert.IsNotNull(runtimeConfig.Runtime.Rest);
Assert.AreEqual(isRequestBodyStrict, runtimeConfig.Runtime.Rest.RequestBodyStrict);
}
}
14 changes: 14 additions & 0 deletions src/Cli.Tests/ModuleInitializer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ public static void Init()
{
// Ignore the connection string from the output to avoid committing it.
VerifierSettings.IgnoreMember<DataSource>(dataSource => dataSource.ConnectionString);
// Ignore the IsRequestBodyStrict as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsRequestBodyStrict);
// Ignore the IsGraphQLEnabled as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsGraphQLEnabled);
// Ignore the IsRestEnabled as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsRestEnabled);
// Ignore the IsStaticWebAppsIdentityProvider as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsStaticWebAppsIdentityProvider);
// Ignore the RestPath as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.RestPath);
// Ignore the GraphQLPath as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.GraphQLPath);
// Ignore the AllowIntrospection as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.AllowIntrospection);
// Ignore the JSON schema path as that's unimportant from a test standpoint.
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.Schema);
// Ignore the message as that's not serialized in our config file anyway.
Expand Down
9 changes: 2 additions & 7 deletions src/Cli.Tests/TestHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@ public static Process ExecuteDabCommand(string command, string flags)
""graphql"": {
""path"": ""/graphql"",
""enabled"": true,
""allow-introspection"": true
""allow-introspection"": false
},
""host"": {
""mode"": ""production"",
Expand Down Expand Up @@ -917,11 +917,6 @@ public static Process ExecuteDabCommand(string command, string flags)
""connection-string"": ""localhost:5000;User ID={USER_NAME};Password={USER_PASSWORD};MultipleActiveResultSets=False;""
},
""runtime"": {
""graphql"": {
""path"": ""/graphql"",
""enabled"": true,
""allow-introspection"": true
},
""host"": {
""mode"": ""production"",
""cors"": {
Expand Down Expand Up @@ -990,7 +985,7 @@ public static Process ExecuteDabCommand(string command, string flags)
""graphql"": {
""path"": ""/graphql"",
""enabled"": true,
""allow-introspection"": true
""allow-introspection"": false
},
""host"": {
""mode"": ""production"",
Expand Down
4 changes: 2 additions & 2 deletions src/Cli/ConfigGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,7 @@ public static bool VerifyCanUpdateRelationship(RuntimeConfig runtimeConfig, stri
}

// Add/Update of relationship is not allowed when GraphQL is disabled in Global Runtime Settings
if (!runtimeConfig.Runtime.GraphQL.Enabled)
if (!runtimeConfig.IsGraphQLEnabled)
{
_logger.LogError("Cannot add/update relationship as GraphQL is disabled in the global runtime settings of the config.");
return false;
Expand Down Expand Up @@ -1073,7 +1073,7 @@ public static bool TryStartEngineWithOptions(StartOptions options, FileSystemRun
else
{
minimumLogLevel = Startup.GetLogLevelBasedOnMode(deserializedRuntimeConfig);
HostMode hostModeType = deserializedRuntimeConfig.Runtime.Host.Mode;
HostMode hostModeType = deserializedRuntimeConfig.IsDevelopmentMode() ? HostMode.Development : HostMode.Production;

_logger.LogInformation("Setting default minimum LogLevel: {minimumLogLevel} for {hostMode} mode.", minimumLogLevel, hostModeType);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Cli/Exporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ private static void ExportGraphQL(ExportOptions options, RuntimeConfig runtimeCo
HttpClient client = new( // CodeQL[SM02185] Loading internal server connection
new HttpClientHandler { ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator }
)
{ BaseAddress = new Uri($"https://localhost:5001{runtimeConfig.Runtime.GraphQL.Path}") };
{ BaseAddress = new Uri($"https://localhost:5001{runtimeConfig.GraphQLPath}") };

IntrospectionClient introspectionClient = new();
Task<HotChocolate.Language.DocumentNode> response = introspectionClient.DownloadSchemaAsync(client);
Expand Down
44 changes: 0 additions & 44 deletions src/Cli/Utils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -177,50 +177,6 @@ public static bool IsURIComponentValid(string? uriComponent)
return !DoesUriComponentContainReservedChars(uriComponent);
}

/// <summary>
/// Returns the default host Global Settings
/// If the user doesn't specify host mode. Default value to be used is Production.
/// Sample:
// "host": {
// "mode": "production",
// "cors": {
// "origins": [],
// "allow-credentials": true
// },
// "authentication": {
// "provider": "StaticWebApps"
// }
// }
/// </summary>
public static HostOptions GetDefaultHostOptions(
HostMode hostMode,
IEnumerable<string>? corsOrigin,
string authenticationProvider,
string? audience,
string? issuer)
{
string[]? corsOriginArray = corsOrigin is null ? new string[] { } : corsOrigin.ToArray();
CorsOptions cors = new(Origins: corsOriginArray);
AuthenticationOptions AuthenticationOptions;
if (Enum.TryParse<EasyAuthType>(authenticationProvider, ignoreCase: true, out _)
|| AuthenticationOptions.SIMULATOR_AUTHENTICATION.Equals(authenticationProvider))
{
AuthenticationOptions = new(Provider: authenticationProvider, null);
}
else
{
AuthenticationOptions = new(
Provider: authenticationProvider,
Jwt: new(audience, issuer)
);
}

return new(
Mode: hostMode,
Cors: cors,
Authentication: AuthenticationOptions);
}

/// <summary>
/// Returns an object of type Policy
/// If policyRequest or policyDatabase is provided. Otherwise, returns null.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ private class GraphQLRuntimeOptionsConverter : JsonConverter<GraphQLRuntimeOptio
{
public override GraphQLRuntimeOptions? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
if (reader.TokenType == JsonTokenType.True)
if (reader.TokenType == JsonTokenType.True || reader.TokenType == JsonTokenType.Null)
{
return new GraphQLRuntimeOptions();
}

if (reader.TokenType == JsonTokenType.Null || reader.TokenType == JsonTokenType.False)
if (reader.TokenType == JsonTokenType.False)
{
return new GraphQLRuntimeOptions(Enabled: false);
}
Expand Down
4 changes: 2 additions & 2 deletions src/Config/Converters/RestRuntimeOptionsConverterFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ private class RestRuntimeOptionsConverter : JsonConverter<RestRuntimeOptions>
{
public override RestRuntimeOptions? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
if (reader.TokenType == JsonTokenType.True)
if (reader.TokenType == JsonTokenType.True || reader.TokenType == JsonTokenType.Null)
{
return new RestRuntimeOptions();
}

if (reader.TokenType == JsonTokenType.Null || reader.TokenType == JsonTokenType.False)
if (reader.TokenType == JsonTokenType.False)
{
return new RestRuntimeOptions(Enabled: false);
}
Expand Down
8 changes: 1 addition & 7 deletions src/Config/ObjectModel/AuthenticationOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Azure.DataApiBuilder.Config.ObjectModel;
/// </param>
/// <param name="Jwt">Settings enabling validation of the received JWT token.
/// Required only when Provider is other than EasyAuth.</param>
public record AuthenticationOptions(string Provider, JwtOptions? Jwt)
public record AuthenticationOptions(string Provider = nameof(EasyAuthType.StaticWebApps), JwtOptions? Jwt = null)
{
public const string SIMULATOR_AUTHENTICATION = "Simulator";
public const string CLIENT_PRINCIPAL_HEADER = "X-MS-CLIENT-PRINCIPAL";
Expand All @@ -36,10 +36,4 @@ public record AuthenticationOptions(string Provider, JwtOptions? Jwt)
/// </summary>
/// <returns>True if the provider is enabled for JWT, otherwise false.</returns>
public bool IsJwtConfiguredIdentityProvider() => !IsEasyAuthAuthenticationProvider() && !IsAuthenticationSimulatorEnabled();

/// <summary>
/// A shorthand method to determine whether Static Web Apps is configured for the current authentication provider.
/// </summary>
/// <returns>True if the provider is enabled for Static Web Apps, otherwise false.</returns>
public bool IsStaticWebAppsIdentityProvider() => EasyAuthType.StaticWebApps.ToString().Equals(Provider, StringComparison.OrdinalIgnoreCase);
};
2 changes: 1 addition & 1 deletion src/Config/ObjectModel/HostOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@

namespace Azure.DataApiBuilder.Config.ObjectModel;

public record HostOptions(CorsOptions? Cors, AuthenticationOptions? Authentication, HostMode Mode = HostMode.Development);
public record HostOptions(CorsOptions? Cors, AuthenticationOptions? Authentication, HostMode Mode = HostMode.Production);
Loading

0 comments on commit 86d73d2

Please sign in to comment.