From dcfea49dc00a929d08c15515d440c23a14631a61 Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Fri, 4 Sep 2020 15:25:02 -0700 Subject: [PATCH] Improve integration of configuration based clients with other features (#14859) * Improve integration of configuration based clients with other features * Fixes --- .../src/Internal/IClientOptionsFactory.cs | 12 +++ .../src/AzureClientFactoryBuilder.cs | 1 - .../src/Internal/AzureClientFactory.cs | 9 +- .../src/Internal/AzureClientsGlobalOptions.cs | 1 - .../src/Internal/ClientOptionsFactory.cs | 31 ++++++- .../src/Internal/DefaultClientOptionsSetup.cs | 38 -------- .../Internal/FallbackAzureClientFactory.cs | 43 +++++---- .../src/Internal/IClientOptionsFactory.cs | 10 ++ .../tests/AzureClientFactoryTests.cs | 91 +++++++++++++++++++ 9 files changed, 170 insertions(+), 66 deletions(-) create mode 100644 sdk/core/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs delete mode 100644 sdk/extensions/Microsoft.Extensions.Azure/src/Internal/DefaultClientOptionsSetup.cs create mode 100644 sdk/extensions/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs diff --git a/sdk/core/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs b/sdk/core/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs new file mode 100644 index 0000000000000..834d7cf6cf14e --- /dev/null +++ b/sdk/core/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs @@ -0,0 +1,12 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using Azure.Core; + +namespace Microsoft.Extensions.Azure +{ + internal interface IClientOptionsFactory + { + object CreateOptions(string name); + } +} \ No newline at end of file diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientFactoryBuilder.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientFactoryBuilder.cs index 6b9332a075702..b67f56dbff80c 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientFactoryBuilder.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientFactoryBuilder.cs @@ -100,7 +100,6 @@ IAzureClientBuilder IAzureClientFactoryBuilderWithCredential. _serviceCollection.TryAddSingleton(typeof(IConfigureOptions>), typeof(DefaultCredentialClientOptionsSetup)); _serviceCollection.TryAddSingleton(typeof(IOptionsMonitor), typeof(ClientOptionsMonitor)); _serviceCollection.TryAddSingleton(typeof(ClientOptionsFactory), typeof(ClientOptionsFactory)); - _serviceCollection.TryAddSingleton(typeof(IConfigureOptions), typeof(DefaultClientOptionsSetup)); _serviceCollection.TryAddSingleton(typeof(IAzureClientFactory), typeof(AzureClientFactory)); _serviceCollection.TryAddSingleton( typeof(TClient), diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientFactory.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientFactory.cs index 485364813f31a..c4cff27597864 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientFactory.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientFactory.cs @@ -12,15 +12,18 @@ internal class AzureClientFactory: IAzureClientFactory> _clientRegistrations; private readonly IServiceProvider _serviceProvider; + private readonly IOptionsMonitor _globalOptions; private readonly IOptionsMonitor> _clientsOptions; private readonly IOptionsMonitor _monitor; private readonly EventSourceLogForwarder _logForwarder; + private FallbackAzureClientFactory _fallbackFactory; public AzureClientFactory( IServiceProvider serviceProvider, + IOptionsMonitor globalOptions, IOptionsMonitor> clientsOptions, IEnumerable> clientRegistrations, IOptionsMonitor monitor, @@ -33,6 +36,7 @@ public AzureClientFactory( } _serviceProvider = serviceProvider; + _globalOptions = globalOptions; _clientsOptions = clientsOptions; _monitor = monitor; _logForwarder = logForwarder; @@ -41,9 +45,12 @@ public AzureClientFactory( public TClient CreateClient(string name) { _logForwarder.Start(); + if (!_clientRegistrations.TryGetValue(name, out ClientRegistration registration)) { - throw new InvalidOperationException($"Unable to find client registration with type '{typeof(TClient).Name}' and name '{name}'."); + _fallbackFactory ??= new FallbackAzureClientFactory(_globalOptions, _serviceProvider, _logForwarder); + + return _fallbackFactory.CreateClient(name); } return registration.GetClient(_monitor.Get(name), _clientsOptions.Get(name).CredentialFactory(_serviceProvider)); diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientsGlobalOptions.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientsGlobalOptions.cs index 03884bdc98eb7..a4224a4df3714 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientsGlobalOptions.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureClientsGlobalOptions.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using Azure.Core; -using Azure.Core.Pipeline; using Azure.Identity; using Microsoft.Extensions.Configuration; diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientOptionsFactory.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientOptionsFactory.cs index 78d497a810123..1808866fe2ada 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientOptionsFactory.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientOptionsFactory.cs @@ -4,23 +4,33 @@ using System; using System.Collections.Generic; using System.Reflection; +using Azure.Core; using Microsoft.Extensions.Options; namespace Microsoft.Extensions.Azure { // Slightly adjusted copy of https://github.com/aspnet/Extensions/blob/master/src/Options/Options/src/OptionsFactory.cs - internal class ClientOptionsFactory where TOptions : class + internal class ClientOptionsFactory: IClientOptionsFactory where TOptions : class { private readonly IEnumerable> _setups; private readonly IEnumerable> _postConfigures; private readonly IEnumerable> _clientRegistrations; + private readonly IOptions _defaultOptions; + private readonly IServiceProvider _serviceProvider; - public ClientOptionsFactory(IEnumerable> setups, IEnumerable> postConfigures, IEnumerable> clientRegistrations) + public ClientOptionsFactory( + IEnumerable> setups, + IEnumerable> postConfigures, + IEnumerable> clientRegistrations, + IOptions defaultOptions, + IServiceProvider serviceProvider) { _setups = setups; _postConfigures = postConfigures; _clientRegistrations = clientRegistrations; + _defaultOptions = defaultOptions; + _serviceProvider = serviceProvider; } private TOptions CreateOptions(string name) @@ -35,7 +45,17 @@ private TOptions CreateOptions(string name) } } - return (TOptions)ClientFactory.CreateClientOptions(version, typeof(TOptions)); + var options = (TOptions)ClientFactory.CreateClientOptions(version, typeof(TOptions)); + + if (options is ClientOptions clientOptions) + { + foreach (var globalConfigureOption in _defaultOptions.Value.ConfigureOptionDelegates) + { + globalConfigureOption(clientOptions, _serviceProvider); + } + } + + return options; } /// @@ -62,5 +82,10 @@ public TOptions Create(string name) return options; } + + object IClientOptionsFactory.CreateOptions(string name) + { + return Create(name); + } } } \ No newline at end of file diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/DefaultClientOptionsSetup.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/DefaultClientOptionsSetup.cs deleted file mode 100644 index 539fedcd0f52f..0000000000000 --- a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/DefaultClientOptionsSetup.cs +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -using System; -using Azure.Core; -using Azure.Core.Pipeline; -using Microsoft.Extensions.Options; - -namespace Microsoft.Extensions.Azure -{ - internal class DefaultClientOptionsSetup : IConfigureNamedOptions where T : class - { - private readonly IOptions _defaultOptions; - private readonly IServiceProvider _serviceProvider; - - public DefaultClientOptionsSetup(IOptions defaultOptions, IServiceProvider serviceProvider) - { - _defaultOptions = defaultOptions; - _serviceProvider = serviceProvider; - } - - public void Configure(T options) - { - if (options is ClientOptions clientOptions) - { - foreach (var globalConfigureOption in _defaultOptions.Value.ConfigureOptionDelegates) - { - globalConfigureOption(clientOptions, _serviceProvider); - } - } - } - - public void Configure(string name, T options) - { - Configure(options); - } - } -} \ No newline at end of file diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/FallbackAzureClientFactory.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/FallbackAzureClientFactory.cs index ca5915c0c1160..409d9894ccf4c 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/FallbackAzureClientFactory.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/FallbackAzureClientFactory.cs @@ -7,25 +7,30 @@ using System.Reflection; using Azure.Core; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; namespace Microsoft.Extensions.Azure { internal class FallbackAzureClientFactory: IAzureClientFactory { - private readonly IOptionsMonitor _globalOptions; + private readonly AzureClientsGlobalOptions _globalOptions; private readonly IServiceProvider _serviceProvider; private readonly EventSourceLogForwarder _logForwarder; private readonly Dictionary> _clientRegistrations; private readonly Type _clientOptionType; + private readonly IConfiguration _configurationRoot; + private readonly IClientOptionsFactory _optionsFactory; public FallbackAzureClientFactory( IOptionsMonitor globalOptions, IServiceProvider serviceProvider, EventSourceLogForwarder logForwarder) { - _globalOptions = globalOptions; _serviceProvider = serviceProvider; + _globalOptions = globalOptions.CurrentValue; + _configurationRoot = _globalOptions.ConfigurationRootResolver?.Invoke(_serviceProvider); + _logForwarder = logForwarder; _clientRegistrations = new Dictionary>(); @@ -43,23 +48,29 @@ public FallbackAzureClientFactory( { throw new InvalidOperationException("Unable to detect the client option type"); } + + _optionsFactory = (IClientOptionsFactory)ActivatorUtilities.CreateInstance(serviceProvider, typeof(ClientOptionsFactory<,>).MakeGenericType(typeof(TClient), _clientOptionType)); } public TClient CreateClient(string name) { - _logForwarder.Start(); + if (_configurationRoot == null) + { + throw new InvalidOperationException($"Unable to find client registration with type '{typeof(TClient).Name}' and name '{name}'."); + } - var globalOptions = _globalOptions.CurrentValue; + _logForwarder.Start(); FallbackClientRegistration registration; lock (_clientRegistrations) { if (!_clientRegistrations.TryGetValue(name, out registration)) { - var section = globalOptions.ConfigurationRootResolver?.Invoke(_serviceProvider).GetSection(name); + var section = _configurationRoot.GetSection(name); + if (!section.Exists()) { - throw new InvalidOperationException($"Unable to find a configuration section with the name {name} to configure the client with or the configuration root wasn't set."); + throw new InvalidOperationException($"Unable to find a configuration section with the name {name} to configure the client with."); } registration = new FallbackClientRegistration( @@ -71,24 +82,12 @@ public TClient CreateClient(string name) } } - - return registration.GetClient( - GetClientOptions(globalOptions, registration.Configuration), - ClientFactory.CreateCredential(registration.Configuration) ?? globalOptions.CredentialFactory(_serviceProvider)); + var currentOptions = _optionsFactory.CreateOptions(name); + registration.Configuration.Bind(currentOptions); + return registration.GetClient(currentOptions, + ClientFactory.CreateCredential(registration.Configuration) ?? _globalOptions.CredentialFactory(_serviceProvider)); } - private object GetClientOptions(AzureClientsGlobalOptions globalOptions, IConfiguration section) - { - var clientOptions = (ClientOptions) ClientFactory.CreateClientOptions(null, _clientOptionType); - foreach (var globalConfigureOptions in globalOptions.ConfigureOptionDelegates) - { - globalConfigureOptions(clientOptions, _serviceProvider); - } - - section.Bind(clientOptions); - - return clientOptions; - } private class FallbackClientRegistration: ClientRegistration { diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs new file mode 100644 index 0000000000000..403fdf81c18aa --- /dev/null +++ b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/IClientOptionsFactory.cs @@ -0,0 +1,10 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +namespace Microsoft.Extensions.Azure +{ + internal interface IClientOptionsFactory + { + object CreateOptions(string name); + } +} \ No newline at end of file diff --git a/sdk/extensions/Microsoft.Extensions.Azure/tests/AzureClientFactoryTests.cs b/sdk/extensions/Microsoft.Extensions.Azure/tests/AzureClientFactoryTests.cs index d781617af450e..2254c819a67dd 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/tests/AzureClientFactoryTests.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/tests/AzureClientFactoryTests.cs @@ -334,6 +334,97 @@ public void CanCreateClientWithoutRegistration() Assert.AreEqual("ConfigurationTenantId", clientSecretCredential.TenantId); } + [Test] + public void RegistrationOverridesConfigurationBasedClient() + { + var configuration = GetConfiguration( + new KeyValuePair("TestClient:connectionString", "http://localhost/"), + new KeyValuePair("AnotherTestClient:connectionString", "http://betterhost/")); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddAzureClients(builder => { + builder.AddTestClient(configuration.GetSection("AnotherTestClient")).WithName("TestClient"); + builder.UseConfiguration(_ => configuration); + }); + + ServiceProvider provider = serviceCollection.BuildServiceProvider(); + IAzureClientFactory factory = provider.GetService>(); + TestClient client = factory.CreateClient("TestClient"); + + Assert.AreEqual("http://betterhost/", client.ConnectionString); + } + + [Test] + public void RegistrationFallsBackToConfigurationBasedClient() + { + var configuration = GetConfiguration( + new KeyValuePair("TestClient:connectionString", "http://localhost/"), + new KeyValuePair("AnotherTestClient:connectionString", "http://betterhost/")); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddAzureClients(builder => { + builder.AddTestClient(configuration.GetSection("AnotherTestClient")); + builder.UseConfiguration(_ => configuration); + }); + + ServiceProvider provider = serviceCollection.BuildServiceProvider(); + IAzureClientFactory factory = provider.GetService>(); + TestClient client = factory.CreateClient("TestClient"); + TestClient defaultTestClient = provider.GetService(); + + Assert.AreEqual("http://localhost/", client.ConnectionString); + Assert.AreEqual("http://betterhost/", defaultTestClient.ConnectionString); + } + + [Test] + public void CanSetClientOptionsInConfigurationBasedClientsViaConfigureOptions() + { + var configuration = GetConfiguration( + new KeyValuePair("TestClient:connectionString", "http://localhost/"), + new KeyValuePair("TestClient2:connectionString", "http://localhost2/") + ); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddAzureClients(builder => { + builder.UseConfiguration(_ => configuration); + }); + + serviceCollection.ConfigureAll(options => options.Property = "client option value"); + serviceCollection.Configure("TestClient", options => options.IntProperty = 2); + + ServiceProvider provider = serviceCollection.BuildServiceProvider(); + IAzureClientFactory factory = provider.GetService>(); + TestClient client = factory.CreateClient("TestClient"); + TestClient client2 = factory.CreateClient("TestClient2"); + + Assert.AreEqual("http://localhost/", client.ConnectionString); + Assert.AreEqual("client option value", client.Options.Property); + Assert.AreEqual(2, client.Options.IntProperty); + + Assert.AreEqual("http://localhost2/", client2.ConnectionString); + Assert.AreEqual("client option value", client2.Options.Property); + } + + [Test] + public void CanSetClientOptionsInConfigurationBasedClients() + { + var configuration = GetConfiguration( + new KeyValuePair("TestClient:connectionString", "http://localhost/"), + new KeyValuePair("TestClient:Property", "client option value")); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddAzureClients(builder => { + builder.UseConfiguration(_ => configuration); + }); + + ServiceProvider provider = serviceCollection.BuildServiceProvider(); + IAzureClientFactory factory = provider.GetService>(); + TestClient client = factory.CreateClient("TestClient"); + + Assert.AreEqual("http://localhost/", client.ConnectionString); + Assert.AreEqual("client option value", client.Options.Property); + } + [Test] public void CanCreateClientWithoutRegistrationUsingConnectionString() {