Skip to content

Commit 31d7bde

Browse files
pakrympakrym
authored
Relax ctor selection rules and improve the error message (Azure#23375)
1 parent 7e36c08 commit 31d7bde

File tree

5 files changed

+134
-34
lines changed

5 files changed

+134
-34
lines changed

‎sdk/extensions/Microsoft.Extensions.Azure/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
### Fixed
1212

13+
- Improved the diagnostic message when a constructor can't be selected.
14+
- The issue where aggressive parameter validation caused constructor not to be selected.
1315

1416
## 1.1.0 (2021-06-09)
1517

‎sdk/extensions/Microsoft.Extensions.Azure/src/Internal/AzureComponentFactory.cs renamed to ‎sdk/extensions/Microsoft.Extensions.Azure/src/AzureComponentFactory.cs

File renamed without changes.

‎sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientFactory.cs

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ public static object CreateClient(Type clientType, Type optionsType, object opti
8080
return constructor.Invoke(arguments.ToArray());
8181
}
8282

83-
throw new InvalidOperationException(BuildErrorMessage(clientType, optionsType));
83+
throw new InvalidOperationException(BuildErrorMessage(configuration, clientType, optionsType));
8484
}
8585

8686
internal static TokenCredential CreateCredential(IConfiguration configuration, TokenCredentialOptions identityClientOptions = null)
@@ -217,10 +217,20 @@ private static bool IsOptionsParameter(ParameterInfo parameter, Type optionsType
217217
parameter.Position == ((ConstructorInfo)parameter.Member).GetParameters().Length - 1;
218218
}
219219

220-
private static string BuildErrorMessage(Type clientType, Type optionsType)
220+
private static string BuildErrorMessage(IConfiguration configuration, Type clientType, Type optionsType)
221221
{
222222
var builder = new StringBuilder();
223-
builder.AppendLine("Unable to find matching constructor. Define one of the follow sets of configuration parameters:");
223+
224+
void TrimTrailingDelimiter()
225+
{
226+
while (builder[builder.Length - 1] is '/' or ',' or ' ')
227+
{
228+
builder.Length--;
229+
}
230+
}
231+
232+
builder.Append("Unable to find matching constructor while trying to create an instance of ").Append(clientType.Name).AppendLine(".");
233+
builder.AppendLine("Expected one of the follow sets of configuration parameters:");
224234

225235
int counter = 1;
226236

@@ -233,31 +243,58 @@ private static string BuildErrorMessage(Type clientType, Type optionsType)
233243

234244
builder.Append(counter).Append(". ");
235245

236-
bool first = true;
237-
238246
foreach (var parameter in constructor.GetParameters())
239247
{
240248
if (IsOptionsParameter(parameter, optionsType))
241249
{
242250
break;
243251
}
244252

245-
if (first)
253+
if (parameter.ParameterType is { IsClass: true } parameterType &&
254+
parameterType != typeof(string) &&
255+
parameterType != typeof(Uri))
246256
{
247-
first = false;
257+
foreach (var parameterConstructor in GetApplicableParameterConstructors(parameterType))
258+
{
259+
foreach (var parameterConstructorParameter in parameterConstructor.GetParameters())
260+
{
261+
builder
262+
.Append(parameter.Name)
263+
.Append(':')
264+
.Append(parameterConstructorParameter.Name)
265+
.Append(", ");
266+
}
267+
268+
TrimTrailingDelimiter();
269+
builder.Append('/');
270+
}
271+
272+
TrimTrailingDelimiter();
248273
}
249274
else
250275
{
251-
builder.Append(", ");
276+
builder.Append(parameter.Name);
252277
}
253278

254-
builder.Append(parameter.Name);
279+
builder.Append(", ");
255280
}
256281

282+
TrimTrailingDelimiter();
283+
257284
builder.AppendLine();
258285
counter++;
259286
}
260287

288+
builder.AppendLine();
289+
builder.Append("Found the following configuration keys: ");
290+
291+
foreach (var child in configuration.AsEnumerable(true))
292+
{
293+
builder.Append(child.Key).Append(", ");
294+
}
295+
296+
TrimTrailingDelimiter();
297+
261298
return builder.ToString();
262299
}
263300

@@ -282,11 +319,6 @@ private static bool TryConvertArgument(IConfiguration configuration, string para
282319
return TryConvertFromString(configuration, parameterName, s => new Uri(s), out value);
283320
}
284321

285-
if (configuration[parameterName] != null)
286-
{
287-
throw new InvalidOperationException($"Unable to convert value '{configuration[parameterName]}' to parameter type {parameterType.FullName}");
288-
}
289-
290322
return TryCreateObject(parameterType, configuration.GetSection(parameterName), out value);
291323
}
292324

@@ -312,7 +344,7 @@ internal static bool TryCreateObject(Type type, IConfigurationSection configurat
312344
}
313345

314346
List<object> arguments = new List<object>();
315-
foreach (var constructor in type.GetConstructors().OrderByDescending(c => c.GetParameters().Length))
347+
foreach (var constructor in GetApplicableParameterConstructors(type))
316348
{
317349
arguments.Clear();
318350

@@ -337,7 +369,13 @@ internal static bool TryCreateObject(Type type, IConfigurationSection configurat
337369
return true;
338370
}
339371

340-
throw new InvalidOperationException($"Unable to convert section '{configuration.Path}' to parameter type '{type}', unable to find matching constructor.");
372+
value = null;
373+
return false;
374+
}
375+
376+
private static IOrderedEnumerable<ConstructorInfo> GetApplicableParameterConstructors(Type type)
377+
{
378+
return type.GetConstructors(BindingFlags.Public | BindingFlags.Instance).OrderByDescending(c => c.GetParameters().Length);
341379
}
342380
}
343381
}

‎sdk/extensions/Microsoft.Extensions.Azure/tests/ClientFactoryTests.cs

Lines changed: 64 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -81,29 +81,24 @@ public void UsesLongestConstructor()
8181
Assert.AreSame(clientOptions, client.Options);
8282
}
8383

84-
[Test]
85-
public void ThrowsWhenUnableToReadCompositeObject()
86-
{
87-
IConfiguration configuration = GetConfiguration(
88-
new KeyValuePair<string, string>("composite:non-existent", "_"));
89-
90-
var clientOptions = new TestClientOptions();
91-
var exception = Assert.Throws<InvalidOperationException>(() => ClientFactory.CreateClient(typeof(TestClient), typeof(TestClientOptions), clientOptions, configuration, null));
92-
Assert.AreEqual("Unable to convert section 'composite' to parameter type 'Azure.Core.Extensions.Tests.TestClient+CompositeObject', unable to find matching constructor.", exception.Message);
93-
}
94-
9584
[Test]
9685
public void ThrowsExceptionWithInformationAboutArguments()
9786
{
98-
IConfiguration configuration = GetConfiguration();
87+
IConfiguration configuration = GetConfiguration(
88+
new KeyValuePair<string, string>("some section:a", "a"),
89+
new KeyValuePair<string, string>("some section:b:c", "b")
90+
).GetSection("some section");
9991

10092
var clientOptions = new TestClientOptions();
101-
var exception = Assert.Throws<InvalidOperationException>(() => ClientFactory.CreateClient(typeof(TestClient), typeof(TestClientOptions), clientOptions, configuration, null));
102-
Assert.AreEqual("Unable to find matching constructor. Define one of the follow sets of configuration parameters:" + Environment.NewLine +
103-
"1. connectionString" + Environment.NewLine +
104-
"2. uri" + Environment.NewLine +
105-
"3. uri, composite" + Environment.NewLine +
106-
"4. composite" + Environment.NewLine,
93+
var exception = Assert.Throws<InvalidOperationException>(() => ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, null));
94+
Assert.AreEqual("Unable to find matching constructor while trying to create an instance of TestClientWithCredentials." + Environment.NewLine +
95+
"Expected one of the follow sets of configuration parameters:" + Environment.NewLine +
96+
"1. uri" + Environment.NewLine +
97+
"2. uri, credential:key" + Environment.NewLine +
98+
"3. uri, credential:signature" + Environment.NewLine +
99+
"4. uri" + Environment.NewLine +
100+
"" + Environment.NewLine +
101+
"Found the following configuration keys: b, b:c, a",
107102
exception.Message);
108103
}
109104

@@ -205,6 +200,57 @@ public void IgnoresConstructorWhenCredentialsNull()
205200
Assert.AreSame(clientOptions, client.Options);
206201
}
207202

203+
[Test]
204+
public void IgnoresNonTokenCredentialConstructors()
205+
{
206+
IConfiguration configuration = GetConfiguration(
207+
new KeyValuePair<string, string>("uri", "http://localhost"),
208+
new KeyValuePair<string, string>("credential", "managedidentity"),
209+
new KeyValuePair<string, string>("clientId", "secret")
210+
);
211+
212+
var clientOptions = new TestClientOptions();
213+
var client = (TestClientWithCredentials)ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, new DefaultAzureCredential());
214+
215+
Assert.AreEqual("http://localhost/", client.Uri.ToString());
216+
Assert.AreSame(clientOptions, client.Options);
217+
Assert.NotNull(client.Credential);
218+
}
219+
220+
[Test]
221+
public void CanUseAzureKeyCredential()
222+
{
223+
IConfiguration configuration = GetConfiguration(
224+
new KeyValuePair<string, string>("uri", "http://localhost"),
225+
new KeyValuePair<string, string>("credential:key", "key"),
226+
new KeyValuePair<string, string>("clientId", "secret")
227+
);
228+
229+
var clientOptions = new TestClientOptions();
230+
var client = (TestClientWithCredentials)ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, new DefaultAzureCredential());
231+
232+
Assert.AreEqual("http://localhost/", client.Uri.ToString());
233+
Assert.AreSame(clientOptions, client.Options);
234+
Assert.AreEqual("key", client.AzureKeyCredential.Key);
235+
}
236+
237+
[Test]
238+
public void CanUseAzureSasCredential()
239+
{
240+
IConfiguration configuration = GetConfiguration(
241+
new KeyValuePair<string, string>("uri", "http://localhost"),
242+
new KeyValuePair<string, string>("credential:signature", "key"),
243+
new KeyValuePair<string, string>("clientId", "secret")
244+
);
245+
246+
var clientOptions = new TestClientOptions();
247+
var client = (TestClientWithCredentials)ClientFactory.CreateClient(typeof(TestClientWithCredentials), typeof(TestClientOptions), clientOptions, configuration, new DefaultAzureCredential());
248+
249+
Assert.AreEqual("http://localhost/", client.Uri.ToString());
250+
Assert.AreSame(clientOptions, client.Options);
251+
Assert.AreEqual("key", client.AzureSasCredential.Signature);
252+
}
253+
208254
private IConfiguration GetConfiguration(params KeyValuePair<string, string>[] items)
209255
{
210256
return new ConfigurationBuilder().AddInMemoryCollection(items).Build();

‎sdk/extensions/Microsoft.Extensions.Azure/tests/TestClientWithCredentials.cs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,25 @@ namespace Azure.Core.Extensions.Tests
88
internal class TestClientWithCredentials : TestClient
99
{
1010
public TokenCredential Credential { get; }
11+
public AzureKeyCredential AzureKeyCredential { get; }
12+
public AzureSasCredential AzureSasCredential { get; }
1113

1214
public TestClientWithCredentials(Uri uri, TestClientOptions options) : base(uri, options)
1315
{
1416
}
1517

18+
public TestClientWithCredentials(Uri uri, AzureKeyCredential credential, TestClientOptions options) : base(uri, options)
19+
{
20+
if (credential == null) throw new ArgumentNullException(nameof(credential));
21+
AzureKeyCredential = credential;
22+
}
23+
24+
public TestClientWithCredentials(Uri uri, AzureSasCredential credential, TestClientOptions options) : base(uri, options)
25+
{
26+
if (credential == null) throw new ArgumentNullException(nameof(credential));
27+
AzureSasCredential = credential;
28+
}
29+
1630
public TestClientWithCredentials(Uri uri, TokenCredential credential, TestClientOptions options) : base(uri, options)
1731
{
1832
if (credential == null) throw new ArgumentNullException(nameof(credential));

0 commit comments

Comments
 (0)