Skip to content
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

Added overloads to GetAWSOptions that allow creating service specific client configs #2571

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions extensions/src/AWSSDK.Extensions.NETCore.Setup/AWSOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ public ClientConfig DefaultClientConfig

return this._defaultClientConfig;
}
internal set
{
this._defaultClientConfig = value;
}
}

/// <summary>
Expand All @@ -89,14 +93,6 @@ public ClientConfig DefaultClientConfig
/// </summary>
public LoggingSetting Logging { get; set; }

internal bool IsDefaultClientConfigSet
{
get
{
return this._defaultClientConfig != null;
}
}

/// <summary>
/// Create a service client for the specified service interface using the options set in this instance.
/// For example if T is set to IAmazonS3 then the AmazonS3ServiceClient which implements IAmazonS3 is created
Expand Down
54 changes: 26 additions & 28 deletions extensions/src/AWSSDK.Extensions.NETCore.Setup/ClientFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -219,39 +219,37 @@ private static ClientConfig CreateConfig(Type serviceInterfaceType, AWSOptions o
}

var defaultConfig = options.DefaultClientConfig;
if (options.IsDefaultClientConfigSet)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the one use of the internal IsDefaultClientConfigSet which checks to see if the backing field of DefaultClientConfig is null. If you read DefaultClientConfig like is done above it sets the value so this is always true.

I went ahead and deleted the property in order to not have this gotcha sitting around.

{
var emptyArray = new object[0];
var singleArray = new object[1];
var emptyArray = new object[0];
var singleArray = new object[1];

var clientConfigTypeInfo = typeof(ClientConfig).GetTypeInfo();
foreach (var property in clientConfigTypeInfo.DeclaredProperties)
var clientConfigTypeInfo = options.DefaultClientConfig.GetType();
var properties = clientConfigTypeInfo.GetProperties(BindingFlags.Public | BindingFlags.Instance);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the uses of GetTypeInfo to be GetType + GetProperties with binding flags in order to get inherited properties as well as ones on the declaring type.

This is also probably good to do because GetTypeInfo is possibly going to be deprecated as a result of this dotnet/runtime#53217

Copy link
Contributor

Choose a reason for hiding this comment

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

Since options is gauranteed to have a value I agree that it's fine to use GetType() here

foreach (var property in properties)
{
if (property.GetMethod != null && property.SetMethod != null)
{
if (property.GetMethod != null && property.SetMethod != null)
{
// Skip RegionEndpoint because it is set below and calling the get method on the
// property triggers the default region fallback mechanism.
if (string.Equals(property.Name, "RegionEndpoint", StringComparison.Ordinal))
continue;
// Skip RegionEndpoint because it is set below and calling the get method on the
// property triggers the default region fallback mechanism.
if (string.Equals(property.Name, "RegionEndpoint", StringComparison.Ordinal))
continue;

// DefaultConfigurationMode is skipped from the DefaultClientConfig because it is expected to be set
// at the top level of AWSOptions which is done before this loop.
if (string.Equals(property.Name, "DefaultConfigurationMode", StringComparison.Ordinal))
continue;
// DefaultConfigurationMode is skipped from the DefaultClientConfig because it is expected to be set
// at the top level of AWSOptions which is done before this loop.
if (string.Equals(property.Name, "DefaultConfigurationMode", StringComparison.Ordinal))
continue;

// Skip setting RetryMode if it is set to legacy but the DefaultConfigurationMode is not legacy.
// This will allow the retry mode to be configured from the DefaultConfiguration.
// This is a workaround to handle the inability to tell if RetryMode was explicitly set.
if (string.Equals(property.Name, "RetryMode", StringComparison.Ordinal) &&
defaultConfig.RetryMode == RequestRetryMode.Legacy &&
config.DefaultConfigurationMode != DefaultConfigurationMode.Legacy)
continue;
// Skip setting RetryMode if it is set to legacy but the DefaultConfigurationMode is not legacy.
// This will allow the retry mode to be configured from the DefaultConfiguration.
// This is a workaround to handle the inability to tell if RetryMode was explicitly set.
if (string.Equals(property.Name, "RetryMode", StringComparison.Ordinal) &&
defaultConfig.RetryMode == RequestRetryMode.Legacy &&
config.DefaultConfigurationMode != DefaultConfigurationMode.Legacy)
continue;

singleArray[0] = property.GetMethod.Invoke(defaultConfig, emptyArray);
if (singleArray[0] != null)
{
property.SetMethod.Invoke(config, singleArray);
}
singleArray[0] = property.GetMethod.Invoke(defaultConfig, emptyArray);
if (singleArray[0] != null)
{
property.SetMethod.Invoke(config, singleArray);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,33 @@ public static AWSOptions GetAWSOptions(this IConfiguration config)
/// <returns>The AWSOptions containing the values set in configuration system.</returns>
public static AWSOptions GetAWSOptions(this IConfiguration config, string configSection)
{
var options = new AWSOptions();
return GetAWSOptions<DefaultClientConfig>(config, configSection);
}

/// <summary>
/// Constructs an AWSOptions class with the options specified in the "AWS" section in the IConfiguration object.
/// </summary>
/// <typeparam name="TConfig">The AWS client config to be used in creating clients, like AmazonS3Config.</typeparam>
/// <param name="config"></param>
/// <returns>The AWSOptions containing the values set in configuration system.</returns>
public static AWSOptions GetAWSOptions<TConfig>(this IConfiguration config) where TConfig : ClientConfig, new()
{
return GetAWSOptions<TConfig>(config, DEFAULT_CONFIG_SECTION);
}

/// <summary>
/// Constructs an AWSOptions class with the options specified in the "AWS" section in the IConfiguration object.
/// </summary>
/// <typeparam name="TConfig">The AWS client config to be used in creating clients, like AmazonS3Config.</typeparam>
/// <param name="config"></param>
/// <param name="configSection">The config section to extract AWS options from.</param>
/// <returns>The AWSOptions containing the values set in configuration system.</returns>
public static AWSOptions GetAWSOptions<TConfig>(this IConfiguration config, string configSection) where TConfig : ClientConfig, new()
{
var options = new AWSOptions
{
DefaultClientConfig = new TConfig(),
};

IConfiguration section;
if (string.IsNullOrEmpty(configSection))
Expand All @@ -64,12 +90,13 @@ public static AWSOptions GetAWSOptions(this IConfiguration config, string config
if (section == null)
return options;

var clientConfigTypeInfo = typeof(ClientConfig).GetTypeInfo();
foreach(var element in section.GetChildren())
var clientConfigType = typeof(TConfig);
var properties = clientConfigType.GetProperties(BindingFlags.Public | BindingFlags.Instance);
foreach (var element in section.GetChildren())
{
try
{
var property = clientConfigTypeInfo.DeclaredProperties.SingleOrDefault(p => p.Name.Equals(element.Key, StringComparison.OrdinalIgnoreCase));
var property = properties.SingleOrDefault(p => p.Name.Equals(element.Key, StringComparison.OrdinalIgnoreCase));
if (property == null || property.SetMethod == null)
continue;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace Amazon.Extensions.NETCore.Setup
/// </summary>
internal class DefaultClientConfig : ClientConfig
{
internal DefaultClientConfig()
public DefaultClientConfig()
{

}
Expand Down
43 changes: 43 additions & 0 deletions extensions/test/NETCore.SetupTests/ConfigurationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ public void GetClientConfigSettingsTest()
client = options.CreateServiceClient<IAmazonS3>();
Assert.Equal(DefaultConfigurationMode.Standard, client.Config.DefaultConfigurationMode);
Assert.Equal(RequestRetryMode.Adaptive, client.Config.RetryMode);

// verify S3 config specific settings are not configured
var clientConfig = client.Config as AmazonS3Config;
Assert.NotNull(clientConfig);
Assert.False(clientConfig.ForcePathStyle);
}

[Fact]
Expand Down Expand Up @@ -204,5 +209,43 @@ public void TestRegionFoundFromEnvironmentVariable()
FallbackRegionFactory.Reset();
}
}

[Fact]
public void ClientConfigTypeOverloadTest()
{
var config = new ConfigurationBuilder()
.AddJsonFile("./TestFiles/GetClientConfigSettingsTest.json")
.Build();

var options = config.GetAWSOptions<AmazonS3Config>();

Assert.Equal(RegionEndpoint.USWest2, options.Region);
Assert.True(options.DefaultClientConfig.UseHttp);
Assert.Equal(6, options.DefaultClientConfig.MaxErrorRetry);
Assert.Equal(TimeSpan.FromMilliseconds(1000), options.DefaultClientConfig.Timeout);
Assert.Equal("us-east-1", options.DefaultClientConfig.AuthenticationRegion);
Assert.Equal(DefaultConfigurationMode.Standard, options.DefaultConfigurationMode);

var client = options.CreateServiceClient<IAmazonS3>();
Assert.NotNull(client);
Assert.Equal(RegionEndpoint.USWest2, client.Config.RegionEndpoint);
Assert.True(client.Config.UseHttp);
Assert.Equal(6, client.Config.MaxErrorRetry);
Assert.Equal(TimeSpan.FromMilliseconds(1000), client.Config.Timeout);
Assert.Equal("us-east-1", client.Config.AuthenticationRegion);
Assert.Equal(DefaultConfigurationMode.Standard, client.Config.DefaultConfigurationMode);
Assert.Equal(RequestRetryMode.Standard, client.Config.RetryMode);

// Verify that setting the standard mode doesn't override explicit settings of retry mode to a non-legacy mode.
options.DefaultClientConfig.RetryMode = RequestRetryMode.Adaptive;
client = options.CreateServiceClient<IAmazonS3>();
Assert.Equal(DefaultConfigurationMode.Standard, client.Config.DefaultConfigurationMode);
Assert.Equal(RequestRetryMode.Adaptive, client.Config.RetryMode);

// verify S3 config specific settings are configured
var clientConfig = client.Config as AmazonS3Config;
Assert.NotNull(clientConfig);
Assert.True(clientConfig.ForcePathStyle);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"AuthenticationRegion": "us-east-1",
"Region": "us-west-2",
"DefaultsMode": "Standard",
"ForcePathStyle": true,
"RetryMode": "Standard"
}
}