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.

Sign up for GitHub

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

Fix conflicting HttpClient configs #110999

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+48 −3
Open

Fix conflicting HttpClient configs #110999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f047aac
Fix conflicting HttpClient configs
Dec 30, 2024
6eba467
Change documentation according to new changes
stormaref Dec 31, 2024
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
4 changes: 2 additions & 2 deletions ...t.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ public static IHttpClientBuilder AddHttpClient(this IServiceCollection services,
/// <summary>
/// Adds the <see cref="IHttpClientFactory"/> and related services to the <see cref="IServiceCollection"/> and configures
/// a binding between the <typeparamref name="TClient" /> type and a named <see cref="HttpClient"/>. The client name will
/// be set to the type name of <typeparamref name="TClient"/>.
/// be set to the type name of <typeparamref name="TImplementation"/>.
/// </summary>
/// <typeparam name="TClient">
/// The type of the typed client. The type specified will be registered in the service collection as
Expand Down Expand Up @@ -450,7 +450,7 @@ public static IHttpClientBuilder AddHttpClient(this IServiceCollection services,

AddHttpClient(services);

string name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
string name = TypeNameHelper.GetTypeDisplayName(typeof(TImplementation), fullName: false);
Copy link

Choose a reason for hiding this comment

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

This makes the documentation on the method (and any that call it) now incorrect. (line 440)

Copy link
Author

Choose a reason for hiding this comment

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

You're right I'm gonna change the documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this introduces a breaking change. However, if you're looking to have multiple implementations for an interface, you can take advantage of the options available for named and typed HttpClients. It’s a great way to handle different scenarios!

Copy link
Author

@stormaref stormaref Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There are indeed several ways to achieve this, but this is something that ought to work as expected out of the box. With dependency injection, the expected behavior is to allow different implementations of an interface to coexist without conflicts. However, the current setup leads to issues, as evidenced by many people encountering this problem.

Here’s a related discussion on StackOverflow that highlights this issue: https://stackoverflow.com/questions/74005464/add-httpclients-with-same-interface-ended-up-having-the-same-base-url-asp-net-co

Also, what is the problem with changing this behavior? What do you think could go wrong with allowing multiple HttpClient registrations under the same interface to retain their independent configurations?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that this code might be missing a few other implementations. Just wanted to point it out.

runtime/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs

Lines 707 to 716 in 6eba467

public static IHttpClientBuilder AddHttpClient<TClient, TImplementation>(this IServiceCollection services, Func<HttpClient, TImplementation> factory)
where TClient : class
where TImplementation : class, TClient
{
ThrowHelper.ThrowIfNull(services);
ThrowHelper.ThrowIfNull(factory);
string name = TypeNameHelper.GetTypeDisplayName(typeof(TClient), fullName: false);
return AddHttpClient<TClient, TImplementation>(services, name, factory);
}

About your question:

what is the problem with changing this behavior?

I don't know, but @CarnaViire can help.

I encourage you to explore these links and review the related issues.

runtime/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientBuilderExtensions.cs

Lines 765 to 785 in 95814d0

if (registry.NamedClientRegistrations.TryGetValue(name, out Type? 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.
type != otherType)
{
string message =
$"The HttpClient factory already has a registered client with the name '{name}', bound to the type '{otherType.FullName}'. " +
$"Client names are computed based on the type name without considering the namespace ('{otherType.Name}'). " +
$"Use an overload of AddHttpClient that accepts a string and provide a unique name to resolve the conflict.";
throw new InvalidOperationException(message);
}
if (validateSingleType)
{
registry.NamedClientRegistrations[name] = type;
}
}

  1. Two httpclients configured by httpclientfactory for classes with same name conflict extensions#960
  2. Fix #519 extensions#1563

Copy link
Author

Choose a reason for hiding this comment

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

If you consider this change acceptable, I would consider the modification of other overloads as well.
Thank you for sharing these links. I have reviewed them, and I believe this change is necessary.

Copy link
Author

Choose a reason for hiding this comment

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

@CarnaViire would you please clarify this?

var builder = new DefaultHttpClientBuilder(services, name);
builder.ConfigureHttpClient(configureClient);
builder.AddTypedClientCore<TClient, TImplementation>(validateSingleType: true);
Expand Down
30 changes: 29 additions & 1 deletion ...nsions.Http.Tests/DependencyInjection/HttpClientFactoryServiceCollectionExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Linq;
using System.Net;
using System.Net.Http;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Http;
Expand Down Expand Up @@ -236,6 +237,33 @@ public void AddHttpClient_WithTypedClientAndImplementation_ConfiguresNamedClient
Assert.Equal("http://example.com/", client.HttpClient.BaseAddress.AbsoluteUri);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
public void AddMultipleHttpClients_WithSameTypedClientAndImplementation_ConfiguresNamedClient_WithImplementationsFullName()
{
// Arrange
var serviceCollection = new ServiceCollection();

// Act
serviceCollection.AddHttpClient<ITestTypedClient, TestTypedClient>(config =>
{
config.BaseAddress = new Uri("http://exampleA.com");
});

serviceCollection.AddHttpClient<ITestTypedClient, TestAnotherTypedClient>(config =>
{
config.BaseAddress = new Uri("http://exampleB.com");
});

var services = serviceCollection.BuildServiceProvider();

// Act2
var clients = services.GetRequiredService<IEnumerable<ITestTypedClient>>();
var baseAddresses = clients.Select(c=>c.HttpClient.BaseAddress).ToList();

// Assert
Assert.NotEqual(baseAddresses.First(), baseAddresses.Last());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
public void AddHttpClient_WithTypedClient_AndName_ConfiguresNamedClient()
{
Expand Down Expand Up @@ -315,7 +343,7 @@ public void AddHttpClient_WithTypedClientAndImplementation_AndDelegate_Configure
// Arrange
var serviceCollection = new ServiceCollection();

serviceCollection.Configure<HttpClientFactoryOptions>(nameof(ITestTypedClient), options =>
serviceCollection.Configure<HttpClientFactoryOptions>(nameof(TestTypedClient), options =>
{
options.HttpClientActions.Add((c) => c.BaseAddress = new Uri("http://example.com"));
});
Expand Down
17 changes: 17 additions & 0 deletions ...Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/TestAnotherTypedClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Net.Http;

namespace Microsoft.Extensions.Http
{
public class TestAnotherTypedClient : ITestTypedClient
{
public TestAnotherTypedClient(HttpClient httpClient)
{
HttpClient = httpClient;
}

public HttpClient HttpClient { get; }
}
}