diff --git a/sdk/extensions/Microsoft.Extensions.Azure/CHANGELOG.md b/sdk/extensions/Microsoft.Extensions.Azure/CHANGELOG.md index ccd6f09b1387..03f0d07e6ae9 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/CHANGELOG.md +++ b/sdk/extensions/Microsoft.Extensions.Azure/CHANGELOG.md @@ -10,6 +10,8 @@ ### Fixed +- Improved the diagnostic message when a constructor can't be selected. +- The issue where aggressive parameter validation caused constructor not to be selected. ## 1.1.0 (2021-06-09) diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureComponentFactory.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/AzureComponentFactory.cs similarity index 100% rename from sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureComponentFactory.cs rename to sdk/extensions/Microsoft.Extensions.Azure/src/AzureComponentFactory.cs diff --git a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientFactory.cs b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientFactory.cs index 18d09a05e25b..24056b2f04c7 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientFactory.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientFactory.cs @@ -80,7 +80,7 @@ public static object CreateClient(Type clientType, Type optionsType, object opti return constructor.Invoke(arguments.ToArray()); } - throw new InvalidOperationException(BuildErrorMessage(clientType, optionsType)); + throw new InvalidOperationException(BuildErrorMessage(configuration, clientType, optionsType)); } internal static TokenCredential CreateCredential(IConfiguration configuration, TokenCredentialOptions identityClientOptions = null) @@ -217,10 +217,20 @@ private static bool IsOptionsParameter(ParameterInfo parameter, Type optionsType parameter.Position == ((ConstructorInfo)parameter.Member).GetParameters().Length - 1; } - private static string BuildErrorMessage(Type clientType, Type optionsType) + private static string BuildErrorMessage(IConfiguration configuration, Type clientType, Type optionsType) { var builder = new StringBuilder(); - builder.AppendLine("Unable to find matching constructor. Define one of the follow sets of configuration parameters:"); + + void TrimTrailingDelimiter() + { + while (builder[builder.Length - 1] is '/' or ',' or ' ') + { + builder.Length--; + } + } + + builder.Append("Unable to find matching constructor while trying to create an instance of ").Append(clientType.Name).AppendLine("."); + builder.AppendLine("Expected one of the follow sets of configuration parameters:"); int counter = 1; @@ -233,8 +243,6 @@ private static string BuildErrorMessage(Type clientType, Type optionsType) builder.Append(counter).Append(". "); - bool first = true; - foreach (var parameter in constructor.GetParameters()) { if (IsOptionsParameter(parameter, optionsType)) @@ -242,22 +250,51 @@ private static string BuildErrorMessage(Type clientType, Type optionsType) break; } - if (first) + if (parameter.ParameterType is { IsClass: true } parameterType && + parameterType != typeof(string) && + parameterType != typeof(Uri)) { - first = false; + foreach (var parameterConstructor in GetApplicableParameterConstructors(parameterType)) + { + foreach (var parameterConstructorParameter in parameterConstructor.GetParameters()) + { + builder + .Append(parameter.Name) + .Append(':') + .Append(parameterConstructorParameter.Name) + .Append(", "); + } + + TrimTrailingDelimiter(); + builder.Append('/'); + } + + TrimTrailingDelimiter(); } else { - builder.Append(", "); + builder.Append(parameter.Name); } - builder.Append(parameter.Name); + builder.Append(", "); } + TrimTrailingDelimiter(); + builder.AppendLine(); counter++; } + builder.AppendLine(); + builder.Append("Found the following configuration keys: "); + + foreach (var child in configuration.AsEnumerable(true)) + { + builder.Append(child.Key).Append(", "); + } + + TrimTrailingDelimiter(); + return builder.ToString(); } @@ -282,11 +319,6 @@ private static bool TryConvertArgument(IConfiguration configuration, string para return TryConvertFromString(configuration, parameterName, s => new Uri(s), out value); } - if (configuration[parameterName] != null) - { - throw new InvalidOperationException($"Unable to convert value '{configuration[parameterName]}' to parameter type {parameterType.FullName}"); - } - return TryCreateObject(parameterType, configuration.GetSection(parameterName), out value); } @@ -312,7 +344,7 @@ internal static bool TryCreateObject(Type type, IConfigurationSection configurat } List arguments = new List(); - foreach (var constructor in type.GetConstructors().OrderByDescending(c => c.GetParameters().Length)) + foreach (var constructor in GetApplicableParameterConstructors(type)) { arguments.Clear(); @@ -337,7 +369,13 @@ internal static bool TryCreateObject(Type type, IConfigurationSection configurat return true; } - throw new InvalidOperationException($"Unable to convert section '{configuration.Path}' to parameter type '{type}', unable to find matching constructor."); + value = null; + return false; + } + + private static IOrderedEnumerable GetApplicableParameterConstructors(Type type) + { + return type.GetConstructors(BindingFlags.Public | BindingFlags.Instance).OrderByDescending(c => c.GetParameters().Length); } } } \ No newline at end of file diff --git a/sdk/extensions/Microsoft.Extensions.Azure/tests/ClientFactoryTests.cs b/sdk/extensions/Microsoft.Extensions.Azure/tests/ClientFactoryTests.cs index 8231ce019e9c..05d91a271947 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/tests/ClientFactoryTests.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/tests/ClientFactoryTests.cs @@ -81,29 +81,24 @@ public void UsesLongestConstructor() Assert.AreSame(clientOptions, client.Options); } - [Test] - public void ThrowsWhenUnableToReadCompositeObject() - { - IConfiguration configuration = GetConfiguration( - new KeyValuePair("composite:non-existent", "_")); - - var clientOptions = new TestClientOptions(); - var exception = Assert.Throws(() => ClientFactory.CreateClient(typeof(TestClient), typeof(TestClientOptions), clientOptions, configuration, null)); - Assert.AreEqual("Unable to convert section 'composite' to parameter type 'Azure.Core.Extensions.Tests.TestClient+CompositeObject', unable to find matching constructor.", exception.Message); - } - [Test] public void ThrowsExceptionWithInformationAboutArguments() { - IConfiguration configuration = GetConfiguration(); + IConfiguration configuration = GetConfiguration( + new KeyValuePair("some section:a", "a"), + new KeyValuePair("some section:b:c", "b") + ).GetSection("some section"); var clientOptions = new TestClientOptions(); - var exception = Assert.Throws(() => ClientFactory.CreateClient(typeof(TestClient), typeof(TestClientOptions), clientOptions, configuration, null)); - Assert.AreEqual("Unable to find matching constructor. Define one of the follow sets of configuration parameters:" + Environment.NewLine + - "1. connectionString" + Environment.NewLine + - "2. uri" + Environment.NewLine + - "3. uri, composite" + Environment.NewLine + - "4. composite" + Environment.NewLine, + var exception = Assert.Throws(() => ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, null)); + Assert.AreEqual("Unable to find matching constructor while trying to create an instance of TestClientWithCredentials." + Environment.NewLine + + "Expected one of the follow sets of configuration parameters:" + Environment.NewLine + + "1. uri" + Environment.NewLine + + "2. uri, credential:key" + Environment.NewLine + + "3. uri, credential:signature" + Environment.NewLine + + "4. uri" + Environment.NewLine + + "" + Environment.NewLine + + "Found the following configuration keys: b, b:c, a", exception.Message); } @@ -205,6 +200,57 @@ public void IgnoresConstructorWhenCredentialsNull() Assert.AreSame(clientOptions, client.Options); } + [Test] + public void IgnoresNonTokenCredentialConstructors() + { + IConfiguration configuration = GetConfiguration( + new KeyValuePair("uri", "http://localhost"), + new KeyValuePair("credential", "managedidentity"), + new KeyValuePair("clientId", "secret") + ); + + var clientOptions = new TestClientOptions(); + var client = (TestClientWithCredentials)ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, new DefaultAzureCredential()); + + Assert.AreEqual("http://localhost/", client.Uri.ToString()); + Assert.AreSame(clientOptions, client.Options); + Assert.NotNull(client.Credential); + } + + [Test] + public void CanUseAzureKeyCredential() + { + IConfiguration configuration = GetConfiguration( + new KeyValuePair("uri", "http://localhost"), + new KeyValuePair("credential:key", "key"), + new KeyValuePair("clientId", "secret") + ); + + var clientOptions = new TestClientOptions(); + var client = (TestClientWithCredentials)ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, new DefaultAzureCredential()); + + Assert.AreEqual("http://localhost/", client.Uri.ToString()); + Assert.AreSame(clientOptions, client.Options); + Assert.AreEqual("key", client.AzureKeyCredential.Key); + } + + [Test] + public void CanUseAzureSasCredential() + { + IConfiguration configuration = GetConfiguration( + new KeyValuePair("uri", "http://localhost"), + new KeyValuePair("credential:signature", "key"), + new KeyValuePair("clientId", "secret") + ); + + var clientOptions = new TestClientOptions(); + var client = (TestClientWithCredentials)ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, new DefaultAzureCredential()); + + Assert.AreEqual("http://localhost/", client.Uri.ToString()); + Assert.AreSame(clientOptions, client.Options); + Assert.AreEqual("key", client.AzureSasCredential.Signature); + } + private IConfiguration GetConfiguration(params KeyValuePair[] items) { return new ConfigurationBuilder().AddInMemoryCollection(items).Build(); diff --git a/sdk/extensions/Microsoft.Extensions.Azure/tests/TestClientWithCredentials.cs b/sdk/extensions/Microsoft.Extensions.Azure/tests/TestClientWithCredentials.cs index ce329617c6ba..723900a27e9f 100644 --- a/sdk/extensions/Microsoft.Extensions.Azure/tests/TestClientWithCredentials.cs +++ b/sdk/extensions/Microsoft.Extensions.Azure/tests/TestClientWithCredentials.cs @@ -8,11 +8,25 @@ namespace Azure.Core.Extensions.Tests internal class TestClientWithCredentials : TestClient { public TokenCredential Credential { get; } + public AzureKeyCredential AzureKeyCredential { get; } + public AzureSasCredential AzureSasCredential { get; } public TestClientWithCredentials(Uri uri, TestClientOptions options) : base(uri, options) { } + public TestClientWithCredentials(Uri uri, AzureKeyCredential credential, TestClientOptions options) : base(uri, options) + { + if (credential == null) throw new ArgumentNullException(nameof(credential)); + AzureKeyCredential = credential; + } + + public TestClientWithCredentials(Uri uri, AzureSasCredential credential, TestClientOptions options) : base(uri, options) + { + if (credential == null) throw new ArgumentNullException(nameof(credential)); + AzureSasCredential = credential; + } + public TestClientWithCredentials(Uri uri, TokenCredential credential, TestClientOptions options) : base(uri, options) { if (credential == null) throw new ArgumentNullException(nameof(credential));