-
Notifications
You must be signed in to change notification settings - Fork 282
Update variable replacement during deserialization to use replacement settings class and add AKV replacement logic. #2882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2fc665d
50e54aa
455ff18
e0574bf
033eeb0
c7ad08f
049b23e
cfdc2b5
5eb9f0e
bbcc159
f6f92bd
0dd7ad6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,8 @@ namespace Azure.DataApiBuilder.Config.Converters; | |
/// </summary> | ||
internal class AKVRetryPolicyOptionsConverterFactory : JsonConverterFactory | ||
{ | ||
// Determines whether to replace environment variable with its | ||
// value or not while deserializing. | ||
private bool _replaceEnvVar; | ||
// Settings for variable replacement during deserialization. | ||
private readonly DeserializationVariableReplacementSettings? _replacementSettings; | ||
|
||
/// <inheritdoc/> | ||
public override bool CanConvert(Type typeToConvert) | ||
|
@@ -25,27 +24,26 @@ public override bool CanConvert(Type typeToConvert) | |
/// <inheritdoc/> | ||
public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) | ||
{ | ||
return new AKVRetryPolicyOptionsConverter(_replaceEnvVar); | ||
return new AKVRetryPolicyOptionsConverter(_replacementSettings); | ||
} | ||
|
||
/// <param name="replaceEnvVar">Whether to replace environment variable with its | ||
/// value or not while deserializing.</param> | ||
internal AKVRetryPolicyOptionsConverterFactory(bool replaceEnvVar) | ||
/// <param name="replacementSettings">Settings for variable replacement during deserialization. | ||
/// If null, no variable replacement will be performed.</param> | ||
internal AKVRetryPolicyOptionsConverterFactory(DeserializationVariableReplacementSettings? replacementSettings = null) | ||
{ | ||
_replaceEnvVar = replaceEnvVar; | ||
_replacementSettings = replacementSettings; | ||
} | ||
|
||
private class AKVRetryPolicyOptionsConverter : JsonConverter<AKVRetryPolicyOptions> | ||
{ | ||
// Determines whether to replace environment variable with its | ||
// value or not while deserializing. | ||
private bool _replaceEnvVar; | ||
// Settings for variable replacement during deserialization. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as comment above, this applies to all the places where the variable type was changed |
||
private readonly DeserializationVariableReplacementSettings? _replacementSettings; | ||
|
||
/// <param name="replaceEnvVar">Whether to replace environment variable with its | ||
/// value or not while deserializing.</param> | ||
public AKVRetryPolicyOptionsConverter(bool replaceEnvVar) | ||
/// <param name="replacementSettings">Settings for variable replacement during deserialization. | ||
/// If null, no variable replacement will be performed.</param> | ||
public AKVRetryPolicyOptionsConverter(DeserializationVariableReplacementSettings? replacementSettings) | ||
{ | ||
_replaceEnvVar = replaceEnvVar; | ||
_replacementSettings = replacementSettings; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -82,7 +80,7 @@ public AKVRetryPolicyOptionsConverter(bool replaceEnvVar) | |
} | ||
else | ||
{ | ||
mode = EnumExtensions.Deserialize<AKVRetryPolicyMode>(reader.DeserializeString(_replaceEnvVar)!); | ||
mode = EnumExtensions.Deserialize<AKVRetryPolicyMode>(reader.DeserializeString(_replacementSettings)!); | ||
} | ||
|
||
break; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT License. | ||
|
||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
using Azure.DataApiBuilder.Config.ObjectModel; | ||
|
||
namespace Azure.DataApiBuilder.Config.Converters; | ||
|
||
/// <summary> | ||
/// Converter factory for AzureKeyVaultOptions that can optionally perform variable replacement. | ||
/// </summary> | ||
internal class AzureKeyVaultOptionsConverterFactory : JsonConverterFactory | ||
{ | ||
// Determines whether to replace environment variable with its | ||
// value or not while deserializing. | ||
private readonly DeserializationVariableReplacementSettings? _replacementSettings; | ||
|
||
/// <param name="replaceEnvVar">Whether to replace environment variable with its | ||
/// value or not while deserializing.</param> | ||
internal AzureKeyVaultOptionsConverterFactory(DeserializationVariableReplacementSettings? replacementSettings = null) | ||
{ | ||
_replacementSettings = replacementSettings; | ||
} | ||
|
||
/// <inheritdoc/> | ||
public override bool CanConvert(Type typeToConvert) | ||
{ | ||
return typeToConvert.IsAssignableTo(typeof(AzureKeyVaultOptions)); | ||
} | ||
|
||
/// <inheritdoc/> | ||
public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) | ||
{ | ||
return new AzureKeyVaultOptionsConverter(_replacementSettings); | ||
} | ||
|
||
private class AzureKeyVaultOptionsConverter : JsonConverter<AzureKeyVaultOptions> | ||
{ | ||
// Determines whether to replace environment variable with its | ||
// value or not while deserializing. | ||
private readonly DeserializationVariableReplacementSettings? _replacementSettings; | ||
|
||
/// <param name="replaceEnvVar">Whether to replace environment variable with its | ||
/// value or not while deserializing.</param> | ||
public AzureKeyVaultOptionsConverter(DeserializationVariableReplacementSettings? replacementSettings) | ||
{ | ||
_replacementSettings = replacementSettings; | ||
} | ||
|
||
/// <summary> | ||
/// Reads AzureKeyVaultOptions with optional variable replacement. | ||
/// </summary> | ||
public override AzureKeyVaultOptions? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
{ | ||
if (reader.TokenType is JsonTokenType.StartObject) | ||
{ | ||
string? endpoint = null; | ||
AKVRetryPolicyOptions? retryPolicy = null; | ||
|
||
while (reader.Read()) | ||
{ | ||
if (reader.TokenType is JsonTokenType.EndObject) | ||
{ | ||
return new AzureKeyVaultOptions | ||
{ | ||
Endpoint = endpoint, | ||
RetryPolicy = retryPolicy | ||
}; | ||
} | ||
|
||
string? property = reader.GetString(); | ||
reader.Read(); | ||
|
||
switch (property) | ||
{ | ||
case "endpoint": | ||
if (reader.TokenType is JsonTokenType.String) | ||
{ | ||
endpoint = reader.DeserializeString(_replacementSettings); | ||
} | ||
|
||
break; | ||
|
||
case "retry-policy": | ||
if (reader.TokenType is JsonTokenType.StartObject) | ||
{ | ||
// Pass the replaceEnvVar setting to the retry policy converter | ||
retryPolicy = JsonSerializer.Deserialize<AKVRetryPolicyOptions>(ref reader, options); | ||
} | ||
|
||
break; | ||
|
||
default: | ||
reader.Skip(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we want to skip an incorrect property? Shouldn't we want to throw an error? |
||
break; | ||
} | ||
} | ||
} | ||
else if (reader.TokenType is JsonTokenType.Null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think it would be better to put this on top so it is consistent with other ConverterFactories |
||
{ | ||
return null; | ||
} | ||
|
||
throw new JsonException("Invalid AzureKeyVaultOptions format"); | ||
} | ||
|
||
public override void Write(Utf8JsonWriter writer, AzureKeyVaultOptions value, JsonSerializerOptions options) | ||
{ | ||
JsonSerializer.Serialize(writer, value, options); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we always want to write the options back into the config? I would expect to only write values if the user wants to do it in the first place. |
||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make the comment a bit more descriptive, to show that it can replace the variable with either AKV, an environment variable, or nothing during deserialization.