Skip to content

Commit 00842f6

Browse files
Aniruddh25Aniruddh25
authored
[Configuration] Make optional object sub properties nullable (#1823)
## 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
1 parent a12f39f commit 00842f6

36 files changed

+303
-175
lines changed

‎src/Cli.Tests/EndToEndTests.cs

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public Task TestInitForCosmosDBNoSql()
6161
Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
6262

6363
Assert.IsNotNull(runtimeConfig);
64-
Assert.IsTrue(runtimeConfig.Runtime.GraphQL.AllowIntrospection);
64+
Assert.IsTrue(runtimeConfig.AllowIntrospection);
6565
Assert.AreEqual(DatabaseType.CosmosDB_NoSQL, runtimeConfig.DataSource.DatabaseType);
6666
CosmosDbNoSQLDataSourceOptions? cosmosDataSourceOptions = runtimeConfig.DataSource.GetTypedOptions<CosmosDbNoSQLDataSourceOptions>();
6767
Assert.IsNotNull(cosmosDataSourceOptions);
@@ -92,13 +92,17 @@ public void TestInitForCosmosDBPostgreSql()
9292
Assert.IsNotNull(runtimeConfig);
9393
Assert.AreEqual(DatabaseType.CosmosDB_PostgreSQL, runtimeConfig.DataSource.DatabaseType);
9494
Assert.IsNotNull(runtimeConfig.Runtime);
95+
Assert.IsNotNull(runtimeConfig.Runtime.Rest);
9596
Assert.AreEqual("/rest-api", runtimeConfig.Runtime.Rest.Path);
9697
Assert.IsTrue(runtimeConfig.Runtime.Rest.Enabled);
98+
Assert.IsNotNull(runtimeConfig.Runtime.GraphQL);
9799
Assert.AreEqual("/graphql-api", runtimeConfig.Runtime.GraphQL.Path);
98100
Assert.IsTrue(runtimeConfig.Runtime.GraphQL.Enabled);
99101

100-
HostOptions hostGlobalSettings = runtimeConfig.Runtime.Host;
101-
CollectionAssert.AreEqual(new string[] { "localhost:3000", "www.nolocalhost.com:80" }, hostGlobalSettings.Cors!.Origins);
102+
HostOptions? hostGlobalSettings = runtimeConfig.Runtime?.Host;
103+
Assert.IsNotNull(hostGlobalSettings);
104+
Assert.IsNotNull(hostGlobalSettings.Cors);
105+
CollectionAssert.AreEqual(new string[] { "localhost:3000", "www.nolocalhost.com:80" }, hostGlobalSettings.Cors.Origins);
102106
}
103107

104108
/// <summary>
@@ -122,10 +126,10 @@ public void TestInitializingRestAndGraphQLGlobalSettings()
122126
Assert.IsNotNull(runtimeConfig);
123127
Assert.AreEqual(DatabaseType.MSSQL, runtimeConfig.DataSource.DatabaseType);
124128
Assert.IsNotNull(runtimeConfig.Runtime);
125-
Assert.AreEqual("/rest-api", runtimeConfig.Runtime.Rest.Path);
126-
Assert.IsFalse(runtimeConfig.Runtime.Rest.Enabled);
127-
Assert.AreEqual("/graphql-api", runtimeConfig.Runtime.GraphQL.Path);
128-
Assert.IsTrue(runtimeConfig.Runtime.GraphQL.Enabled);
129+
Assert.AreEqual("/rest-api", runtimeConfig.Runtime.Rest?.Path);
130+
Assert.IsFalse(runtimeConfig.Runtime.Rest?.Enabled);
131+
Assert.AreEqual("/graphql-api", runtimeConfig.Runtime.GraphQL?.Path);
132+
Assert.IsTrue(runtimeConfig.Runtime.GraphQL?.Enabled);
129133
}
130134

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

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

186+
Assert.IsNotNull(runtimeConfig.Runtime);
187+
Assert.IsNotNull(runtimeConfig.Runtime.Host);
182188
Assert.AreEqual("AzureAD", runtimeConfig.Runtime.Host.Authentication?.Provider);
183189
Assert.AreEqual("aud-xxx", runtimeConfig.Runtime.Host.Authentication?.Jwt?.Audience);
184190
Assert.AreEqual("issuer-xxx", runtimeConfig.Runtime.Host.Authentication?.Jwt?.Issuer);
@@ -204,7 +210,7 @@ public void EnsureHostModeEnumIsCaseInsensitive(string hostMode, HostMode hostMo
204210
if (expectSuccess)
205211
{
206212
Assert.IsNotNull(runtimeConfig);
207-
Assert.AreEqual(hostModeEnumType, runtimeConfig.Runtime.Host.Mode);
213+
Assert.AreEqual(hostModeEnumType, runtimeConfig.Runtime?.Host?.Mode);
208214
}
209215
else
210216
{
@@ -225,7 +231,7 @@ public void TestAddEntityWithoutIEnumerable()
225231

226232
Assert.IsNotNull(runtimeConfig);
227233
Assert.AreEqual(0, runtimeConfig.Entities.Count()); // No entities
228-
Assert.AreEqual(HostMode.Production, runtimeConfig.Runtime.Host.Mode);
234+
Assert.AreEqual(HostMode.Production, runtimeConfig.Runtime?.Host?.Mode);
229235

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

822829
if (apiType is ApiType.REST)
823830
{
831+
Assert.IsNotNull(runtimeConfig.Runtime);
832+
Assert.IsNotNull(runtimeConfig.Runtime.Rest);
824833
Assert.AreEqual(expectedEnabledFlagValueInConfig, runtimeConfig.Runtime.Rest.Enabled);
825834
}
826835
else
827836
{
837+
Assert.IsNotNull(runtimeConfig.Runtime);
838+
Assert.IsNotNull(runtimeConfig.Runtime.GraphQL);
828839
Assert.AreEqual(expectedEnabledFlagValueInConfig, runtimeConfig.Runtime.GraphQL.Enabled);
829840
}
830841
}
@@ -855,6 +866,8 @@ public void TestRestRequestBodyStrictMode(bool includeRestRequestBodyStrictFlag,
855866
Program.Execute(initArgs, _cliLogger!, _fileSystem!, _runtimeConfigLoader!);
856867

857868
Assert.IsTrue(_runtimeConfigLoader!.TryLoadConfig(TEST_RUNTIME_CONFIG_FILE, out RuntimeConfig? runtimeConfig));
869+
Assert.IsNotNull(runtimeConfig.Runtime);
870+
Assert.IsNotNull(runtimeConfig.Runtime.Rest);
858871
Assert.AreEqual(isRequestBodyStrict, runtimeConfig.Runtime.Rest.RequestBodyStrict);
859872
}
860873
}

‎src/Cli.Tests/ModuleInitializer.cs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,20 @@ public static void Init()
2525
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.CosmosDataSourceUsed);
2626
// Ignore the SqlDataSourceUsed as that's unimportant from a test standpoint.
2727
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.SqlDataSourceUsed);
28+
// Ignore the IsRequestBodyStrict as that's unimportant from a test standpoint.
29+
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsRequestBodyStrict);
30+
// Ignore the IsGraphQLEnabled as that's unimportant from a test standpoint.
31+
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsGraphQLEnabled);
32+
// Ignore the IsRestEnabled as that's unimportant from a test standpoint.
33+
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsRestEnabled);
34+
// Ignore the IsStaticWebAppsIdentityProvider as that's unimportant from a test standpoint.
35+
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.IsStaticWebAppsIdentityProvider);
36+
// Ignore the RestPath as that's unimportant from a test standpoint.
37+
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.RestPath);
38+
// Ignore the GraphQLPath as that's unimportant from a test standpoint.
39+
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.GraphQLPath);
40+
// Ignore the AllowIntrospection as that's unimportant from a test standpoint.
41+
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.AllowIntrospection);
2842
// Ignore the JSON schema path as that's unimportant from a test standpoint.
2943
VerifierSettings.IgnoreMember<RuntimeConfig>(config => config.Schema);
3044
// Ignore the message as that's not serialized in our config file anyway.

‎src/Cli.Tests/TestHelper.cs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -870,7 +870,7 @@ public static Process ExecuteDabCommand(string command, string flags)
870870
""graphql"": {
871871
""path"": ""/graphql"",
872872
""enabled"": true,
873-
""allow-introspection"": true
873+
""allow-introspection"": false
874874
},
875875
""host"": {
876876
""mode"": ""production"",
@@ -917,11 +917,6 @@ public static Process ExecuteDabCommand(string command, string flags)
917917
""connection-string"": ""localhost:5000;User ID={USER_NAME};Password={USER_PASSWORD};MultipleActiveResultSets=False;""
918918
},
919919
""runtime"": {
920-
""graphql"": {
921-
""path"": ""/graphql"",
922-
""enabled"": true,
923-
""allow-introspection"": true
924-
},
925920
""host"": {
926921
""mode"": ""production"",
927922
""cors"": {
@@ -990,7 +985,7 @@ public static Process ExecuteDabCommand(string command, string flags)
990985
""graphql"": {
991986
""path"": ""/graphql"",
992987
""enabled"": true,
993-
""allow-introspection"": true
988+
""allow-introspection"": false
994989
},
995990
""host"": {
996991
""mode"": ""production"",

‎src/Cli/ConfigGenerator.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,7 @@ public static bool VerifyCanUpdateRelationship(RuntimeConfig runtimeConfig, stri
939939
}
940940

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

10781078
_logger.LogInformation("Setting default minimum LogLevel: {minimumLogLevel} for {hostMode} mode.", minimumLogLevel, hostModeType);
10791079
}

‎src/Cli/Exporter.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ private static void ExportGraphQL(ExportOptions options, RuntimeConfig runtimeCo
7272
HttpClient client = new( // CodeQL[SM02185] Loading internal server connection
7373
new HttpClientHandler { ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator }
7474
)
75-
{ BaseAddress = new Uri($"https://localhost:5001{runtimeConfig.Runtime.GraphQL.Path}") };
75+
{ BaseAddress = new Uri($"https://localhost:5001{runtimeConfig.GraphQLPath}") };
7676

7777
IntrospectionClient introspectionClient = new();
7878
Task<HotChocolate.Language.DocumentNode> response = introspectionClient.DownloadSchemaAsync(client);

‎src/Cli/Utils.cs

Lines changed: 0 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -177,50 +177,6 @@ public static bool IsURIComponentValid(string? uriComponent)
177177
return !DoesUriComponentContainReservedChars(uriComponent);
178178
}
179179

180-
/// <summary>
181-
/// Returns the default host Global Settings
182-
/// If the user doesn't specify host mode. Default value to be used is Production.
183-
/// Sample:
184-
// "host": {
185-
// "mode": "production",
186-
// "cors": {
187-
// "origins": [],
188-
// "allow-credentials": true
189-
// },
190-
// "authentication": {
191-
// "provider": "StaticWebApps"
192-
// }
193-
// }
194-
/// </summary>
195-
public static HostOptions GetDefaultHostOptions(
196-
HostMode hostMode,
197-
IEnumerable<string>? corsOrigin,
198-
string authenticationProvider,
199-
string? audience,
200-
string? issuer)
201-
{
202-
string[]? corsOriginArray = corsOrigin is null ? new string[] { } : corsOrigin.ToArray();
203-
CorsOptions cors = new(Origins: corsOriginArray);
204-
AuthenticationOptions AuthenticationOptions;
205-
if (Enum.TryParse<EasyAuthType>(authenticationProvider, ignoreCase: true, out _)
206-
|| AuthenticationOptions.SIMULATOR_AUTHENTICATION.Equals(authenticationProvider))
207-
{
208-
AuthenticationOptions = new(Provider: authenticationProvider, null);
209-
}
210-
else
211-
{
212-
AuthenticationOptions = new(
213-
Provider: authenticationProvider,
214-
Jwt: new(audience, issuer)
215-
);
216-
}
217-
218-
return new(
219-
Mode: hostMode,
220-
Cors: cors,
221-
Authentication: AuthenticationOptions);
222-
}
223-
224180
/// <summary>
225181
/// Returns an object of type Policy
226182
/// If policyRequest or policyDatabase is provided. Otherwise, returns null.

‎src/Config/Converters/GraphQLRuntimeOptionsConverterFactory.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@ private class GraphQLRuntimeOptionsConverter : JsonConverter<GraphQLRuntimeOptio
2525
{
2626
public override GraphQLRuntimeOptions? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
2727
{
28-
if (reader.TokenType == JsonTokenType.True)
28+
if (reader.TokenType == JsonTokenType.True || reader.TokenType == JsonTokenType.Null)
2929
{
3030
return new GraphQLRuntimeOptions();
3131
}
3232

33-
if (reader.TokenType == JsonTokenType.Null || reader.TokenType == JsonTokenType.False)
33+
if (reader.TokenType == JsonTokenType.False)
3434
{
3535
return new GraphQLRuntimeOptions(Enabled: false);
3636
}

‎src/Config/Converters/RestRuntimeOptionsConverterFactory.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@ private class RestRuntimeOptionsConverter : JsonConverter<RestRuntimeOptions>
2525
{
2626
public override RestRuntimeOptions? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
2727
{
28-
if (reader.TokenType == JsonTokenType.True)
28+
if (reader.TokenType == JsonTokenType.True || reader.TokenType == JsonTokenType.Null)
2929
{
3030
return new RestRuntimeOptions();
3131
}
3232

33-
if (reader.TokenType == JsonTokenType.Null || reader.TokenType == JsonTokenType.False)
33+
if (reader.TokenType == JsonTokenType.False)
3434
{
3535
return new RestRuntimeOptions(Enabled: false);
3636
}

‎src/Config/ObjectModel/AuthenticationOptions.cs

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ namespace Azure.DataApiBuilder.Config.ObjectModel;
1111
/// </param>
1212
/// <param name="Jwt">Settings enabling validation of the received JWT token.
1313
/// Required only when Provider is other than EasyAuth.</param>
14-
public record AuthenticationOptions(string Provider, JwtOptions? Jwt)
14+
public record AuthenticationOptions(string Provider = nameof(EasyAuthType.StaticWebApps), JwtOptions? Jwt = null)
1515
{
1616
public const string SIMULATOR_AUTHENTICATION = "Simulator";
1717
public const string CLIENT_PRINCIPAL_HEADER = "X-MS-CLIENT-PRINCIPAL";
@@ -36,10 +36,4 @@ public record AuthenticationOptions(string Provider, JwtOptions? Jwt)
3636
/// </summary>
3737
/// <returns>True if the provider is enabled for JWT, otherwise false.</returns>
3838
public bool IsJwtConfiguredIdentityProvider() => !IsEasyAuthAuthenticationProvider() && !IsAuthenticationSimulatorEnabled();
39-
40-
/// <summary>
41-
/// A shorthand method to determine whether Static Web Apps is configured for the current authentication provider.
42-
/// </summary>
43-
/// <returns>True if the provider is enabled for Static Web Apps, otherwise false.</returns>
44-
public bool IsStaticWebAppsIdentityProvider() => EasyAuthType.StaticWebApps.ToString().Equals(Provider, StringComparison.OrdinalIgnoreCase);
4539
};

‎src/Config/ObjectModel/HostOptions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@
33

44
namespace Azure.DataApiBuilder.Config.ObjectModel;
55

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

0 commit comments

Comments
 (0)