Skip to content

Commit

Permalink
Allow multiple types to be registered for one name
Browse files Browse the repository at this point in the history 
Fixes: dotnet/aspnetcore#13346

In 3.0 we added validation to try and report exceptions for some common
HttClient factory usage mistakes in the registration code. Unfortunately
we blocked from legitimate usage cases.

In this case, users are blocked from associating multiple types with the
same logical 'name'.

Ex:

```C#
services.AddHttpClient("Foo").AddTypedClient<A>().AddTypedClient<B>();
```

This is useful and should be allowed because it's a good way to DRY up
configuration code.

----

This change relaxes the validation when `AddTypedClient` is called, but
not when `AddHttpClient` is called without supplying a name.

We still want to block cases like the following:

```C#
services.AddHttpClient<A.SomeClient>();
services.AddHttpClient<B.SomeClient>();
```

The type short name is used as the logical name for the client (named
options) so usage like this is always a bug.
  • Loading branch information
@rynowak
rynowak committed Jan 16, 2020
1 parent b2165d7 commit f198c46
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 35 deletions.
48 changes: 43 additions & 5 deletions src/HttpClientFactory/Http/src/DependencyInjection/HttpClientBuilderExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,13 @@ public static IHttpClientBuilder AddTypedClient<TClient>(this IHttpClientBuilder
throw new ArgumentNullException(nameof(builder));
}

ReserveClient(builder, typeof(TClient), builder.Name);
return AddTypedClientCore<TClient>(builder, validateSingleType: false);
}

internal static IHttpClientBuilder AddTypedClientCore<TClient>(this IHttpClientBuilder builder, bool validateSingleType)
where TClient : class
{
ReserveClient(builder, typeof(TClient), builder.Name, validateSingleType);

builder.Services.AddTransient<TClient>(s =>
{
Expand Down Expand Up @@ -377,7 +383,14 @@ public static IHttpClientBuilder AddTypedClient<TClient>(this IHttpClientBuilder
throw new ArgumentNullException(nameof(builder));
}

ReserveClient(builder, typeof(TClient), builder.Name);
return AddTypedClientCore<TClient, TImplementation>(builder, validateSingleType: false);
}

internal static IHttpClientBuilder AddTypedClientCore<TClient, TImplementation>(this IHttpClientBuilder builder, bool validateSingleType)
where TClient : class
where TImplementation : class, TClient
{
ReserveClient(builder, typeof(TClient), builder.Name, validateSingleType);

builder.Services.AddTransient<TClient>(s =>
{
Expand Down Expand Up @@ -425,7 +438,13 @@ public static IHttpClientBuilder AddTypedClient<TClient>(this IHttpClientBuilder
throw new ArgumentNullException(nameof(factory));
}

ReserveClient(builder, typeof(TClient), builder.Name);
return AddTypedClientCore<TClient>(builder, factory, validateSingleType: false);
}

internal static IHttpClientBuilder AddTypedClientCore<TClient>(this IHttpClientBuilder builder, Func<HttpClient, TClient> factory, bool validateSingleType)
where TClient : class
{
ReserveClient(builder, typeof(TClient), builder.Name, validateSingleType);

builder.Services.AddTransient<TClient>(s =>
{
Expand Down Expand Up @@ -472,7 +491,23 @@ public static IHttpClientBuilder AddTypedClient<TClient>(this IHttpClientBuilder
throw new ArgumentNullException(nameof(factory));
}

ReserveClient(builder, typeof(TClient), builder.Name);
return AddTypedClientCore<TClient>(builder, factory, validateSingleType: false);
}

internal static IHttpClientBuilder AddTypedClientCore<TClient>(this IHttpClientBuilder builder, Func<HttpClient, IServiceProvider, TClient> factory, bool validateSingleType)
where TClient : class
{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}

if (factory == null)
{
throw new ArgumentNullException(nameof(factory));
}

ReserveClient(builder, typeof(TClient), builder.Name, validateSingleType);

builder.Services.AddTransient<TClient>(s =>
{
Expand Down Expand Up @@ -526,7 +561,7 @@ public static IHttpClientBuilder SetHandlerLifetime(this IHttpClientBuilder buil
}

// See comments on HttpClientMappingRegistry.
private static void ReserveClient(IHttpClientBuilder builder, Type type, string name)
private static void ReserveClient(IHttpClientBuilder builder, Type type, string name, bool validateSingleType)
{
var registry = (HttpClientMappingRegistry)builder.Services.Single(sd => sd.ServiceType == typeof(HttpClientMappingRegistry)).ImplementationInstance;
Debug.Assert(registry != null);
Expand All @@ -549,6 +584,9 @@ private static void ReserveClient(IHttpClientBuilder builder, Type type, string
// Check for same name registered to two types. This won't work because we rely on named options for the configuration.
if (registry.NamedClientRegistrations.TryGetValue(name, out var otherType) &&

// Allow using the same name with multiple types in some cases (see callers).
validateSingleType &&

// Allow registering the same name twice to the same type (see above).
type != otherType)
{
Expand Down
24 changes: 12 additions & 12 deletions ...lientFactory/Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection

var name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
var builder = new DefaultHttpClientBuilder(services, name);
builder.AddTypedClient<TClient>();
builder.AddTypedClientCore<TClient>(validateSingleType: true);
return builder;
}

Expand Down Expand Up @@ -247,7 +247,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection

var name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
var builder = new DefaultHttpClientBuilder(services, name);
builder.AddTypedClient<TClient, TImplementation>();
builder.AddTypedClientCore<TClient, TImplementation>(validateSingleType: true);
return builder;
}

Expand Down Expand Up @@ -292,7 +292,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection
AddHttpClient(services);

var builder = new DefaultHttpClientBuilder(services, name);
builder.AddTypedClient<TClient>();
builder.AddTypedClientCore<TClient>(validateSingleType: false); // Name was explicitly provided.
return builder;
}

Expand Down Expand Up @@ -343,7 +343,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection
AddHttpClient(services);

var builder = new DefaultHttpClientBuilder(services, name);
builder.AddTypedClient<TClient, TImplementation>();
builder.AddTypedClientCore<TClient, TImplementation>(validateSingleType: false); // name was explicitly provided
return builder;
}

Expand Down Expand Up @@ -388,7 +388,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection
var name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient>();
builder.AddTypedClientCore<TClient>(validateSingleType: true);
return builder;
}

Expand Down Expand Up @@ -433,7 +433,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection
var name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient>();
builder.AddTypedClientCore<TClient>(validateSingleType: true);
return builder;
}

Expand Down Expand Up @@ -483,7 +483,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection
var name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient, TImplementation>();
builder.AddTypedClientCore<TClient, TImplementation>(validateSingleType: true);
return builder;
}

Expand Down Expand Up @@ -533,7 +533,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection
var name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient, TImplementation>();
builder.AddTypedClientCore<TClient, TImplementation>(validateSingleType: true);
return builder;
}

Expand Down Expand Up @@ -585,7 +585,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection

var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient>();
builder.AddTypedClientCore<TClient>(validateSingleType: false); // name was explicitly provided
return builder;
}

Expand Down Expand Up @@ -637,7 +637,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection

var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient>();
builder.AddTypedClientCore<TClient>(validateSingleType: false); // name was explictly provided
return builder;
}

Expand Down Expand Up @@ -694,7 +694,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection

var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient, TImplementation>();
builder.AddTypedClientCore<TClient, TImplementation>(validateSingleType: false); // name was explicitly provided
return builder;
}

Expand Down Expand Up @@ -751,7 +751,7 @@ public static IHttpClientBuilder AddHttpClient<TClient>(this IServiceCollection

var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClient<TClient, TImplementation>();
builder.AddTypedClientCore<TClient, TImplementation>(validateSingleType: false); // name was explicitly provided
return builder;
}
}
Expand Down
106 changes: 88 additions & 18 deletions ...Factory/Http/test/DependencyInjection/HttpClientFactoryServiceCollectionExtensionsTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public void AddHttpClient_IsSelfContained_CanCreateClient()
var serviceCollection = new ServiceCollection();

// Act1
serviceCollection.AddHttpClient();
serviceCollection.AddHttpClient();

var services = serviceCollection.BuildServiceProvider();
var options = services.GetRequiredService<IOptionsMonitor<HttpClientFactoryOptions>>();
Expand Down Expand Up @@ -446,21 +446,88 @@ public void AddHttpClient_AddSameNameWithTypedClientTwice_ThrowsError()
}

[Fact]
public void AddHttpClient_AddSameNameWithTypedClientTwice_WithAddTypedClient_ThrowsError()
public void AddHttpClient_AddSameNameWithTypedClientTwice_WithAddTypedClient_IsAllowed()
{
// Arrange
var serviceCollection = new ServiceCollection();
serviceCollection.AddHttpClient<TestTypedClient>();
serviceCollection.AddHttpClient<TestTypedClient>(c =>
{
c.BaseAddress = new Uri("http://example.com");
});

// Act
var ex = Assert.Throws<InvalidOperationException>(() => serviceCollection.AddHttpClient("TestTypedClient").AddTypedClient<AnotherNamespace.TestTypedClient>());
serviceCollection.AddHttpClient("TestTypedClient").AddTypedClient<AnotherNamespace.TestTypedClient>();

var services = serviceCollection.BuildServiceProvider();

// Act
var client = services.GetRequiredService<TestTypedClient>();

// Assert
Assert.Equal(
"The HttpClient factory already has a registered client with the name 'TestTypedClient', bound to the type 'Microsoft.Extensions.Http.TestTypedClient'. " +
"Client names are computed based on the type name without considering the namespace ('TestTypedClient'). " +
"Use an overload of AddHttpClient that accepts a string and provide a unique name to resolve the conflict.",
ex.Message);
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);

// Act
var client2 = services.GetRequiredService<AnotherNamespace.TestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client2.HttpClient.BaseAddress.AbsoluteUri);
}

[Fact]
public void AddHttpClient_AddSameNameWithTypedClientTwice_WithExplicitName_IsAllowed()
{
// Arrange
var serviceCollection = new ServiceCollection();
serviceCollection.AddHttpClient<TestTypedClient>(c =>
{
c.BaseAddress = new Uri("http://example.com");
});

// Act
serviceCollection.AddHttpClient<AnotherNamespace.TestTypedClient>("TestTypedClient");

var services = serviceCollection.BuildServiceProvider();

// Act
var client = services.GetRequiredService<TestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);

// Act
var client2 = services.GetRequiredService<AnotherNamespace.TestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client2.HttpClient.BaseAddress.AbsoluteUri);
}

[Fact]
public void AddHttpClient_RegisteringMultipleTypes_WithAddTypedClient_IsAllowed()
{
// Arrange
var serviceCollection = new ServiceCollection();

// Act
serviceCollection.AddHttpClient("Test", c =>
{
c.BaseAddress = new Uri("http://example.com");
})
.AddTypedClient<TestTypedClient>()
.AddTypedClient<AnotherNamespace.TestTypedClient>();

var services = serviceCollection.BuildServiceProvider();

// Act
var client = services.GetRequiredService<TestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);

// Act
var client2 = services.GetRequiredService<AnotherNamespace.TestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client2.HttpClient.BaseAddress.AbsoluteUri);
}

[Fact]
Expand All @@ -475,7 +542,7 @@ public void AddHttpClient_AddTypedClient_WithServiceDelegate_ConfiguresNamedClie
});

// Act
serviceCollection.AddHttpClient("test").AddTypedClient<TestTypedClient>((c,s) =>
serviceCollection.AddHttpClient("test").AddTypedClient<TestTypedClient>((c, s) =>
{
Assert.Equal("http://example.com/", c.BaseAddress.AbsoluteUri);
c.BaseAddress = new Uri("http://example2.com");
Expand Down Expand Up @@ -564,7 +631,7 @@ public void AddHttpClient_WithTypedClient_AndServiceDelegate_ConfiguresClient()
});

// Act1
serviceCollection.AddHttpClient<TestTypedClient>((s,c) =>
serviceCollection.AddHttpClient<TestTypedClient>((s, c) =>
{
var options = s.GetRequiredService<IOptions<OtherTestOptions>>();
c.BaseAddress = new Uri(options.Value.BaseAddress);
Expand All @@ -574,7 +641,7 @@ public void AddHttpClient_WithTypedClient_AndServiceDelegate_ConfiguresClient()

// Act2
var client = services.GetRequiredService<TestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);
}
Expand All @@ -591,7 +658,7 @@ public void AddHttpClient_WithTypedClientAndImplementation_AndServiceDelegate_Co
});

// Act1
serviceCollection.AddHttpClient<ITestTypedClient, TestTypedClient>((s,c) =>
serviceCollection.AddHttpClient<ITestTypedClient, TestTypedClient>((s, c) =>
{
var options = s.GetRequiredService<IOptions<OtherTestOptions>>();
c.BaseAddress = new Uri(options.Value.BaseAddress);
Expand All @@ -601,7 +668,7 @@ public void AddHttpClient_WithTypedClientAndImplementation_AndServiceDelegate_Co

// Act2
var client = services.GetRequiredService<ITestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);
}
Expand All @@ -618,7 +685,7 @@ public void AddHttpClient_WithTypedClient_AndServiceDelegate_ConfiguresNamedClie
});

// Act1
serviceCollection.AddHttpClient<TestTypedClient>("test", (s,c) =>
serviceCollection.AddHttpClient<TestTypedClient>("test", (s, c) =>
{
var options = s.GetRequiredService<IOptions<OtherTestOptions>>();
c.BaseAddress = new Uri(options.Value.BaseAddress);
Expand All @@ -628,7 +695,7 @@ public void AddHttpClient_WithTypedClient_AndServiceDelegate_ConfiguresNamedClie

// Act2
var client = services.GetRequiredService<TestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);
}
Expand All @@ -645,7 +712,7 @@ public void AddHttpClient_WithTypedClientAndImplementation_AndServiceDelegate_Co
});

// Act1
serviceCollection.AddHttpClient<ITestTypedClient, TestTypedClient>("test", (s,c) =>
serviceCollection.AddHttpClient<ITestTypedClient, TestTypedClient>("test", (s, c) =>
{
var options = s.GetRequiredService<IOptions<OtherTestOptions>>();
c.BaseAddress = new Uri(options.Value.BaseAddress);
Expand All @@ -655,7 +722,7 @@ public void AddHttpClient_WithTypedClientAndImplementation_AndServiceDelegate_Co

// Act2
var client = services.GetRequiredService<ITestTypedClient>();

// Assert
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);
}
Expand Down Expand Up @@ -1171,6 +1238,9 @@ public class TestTypedClient
{
public TestTypedClient(HttpClient httpClient)
{
HttpClient = httpClient;
}

public HttpClient HttpClient { get; }
}
}

0 comments on commit f198c46

Please sign in to comment.