Allow injecting HttpClient instead of HttpClientHandler and reduce DataServiceClientRequest API surface.

[habbes]

This PR: https://github.com/OData/odata.net/pull/2431 provided support for injecting a custom HttpClientHandler which allowed us to address a number of long-standing issues, especially performance issues related to re-using http connections.

However, injecting HttpClientHandler is not the most ideal choice. I believe it would be better to inject HttpClient instead, namely because this allows customers to use IHttpClientFactory which is the recommended approach for working HttpClient in long-running services. If we only have provisions for HttpClientHandler, this means that customers will not benefit from IHttpClientFactory and will be required to manage the lifetime of HttpClientHandler and potential DNS changes on their own. If they still wish to customize a handler directly, they can still pass a custom handler to instantiate a custom HttpClient. So, this offers the most flexibility.

The reason we exposed HttpClientHandler is because DataServiceClientRequest, and consequently HttpClientRequestMessgae expose (writable) properties that can only be accessed from the handler. But since we allow injecting a custom client, we don't need to provide customization via properties, customers can configure the clients how they wish. Therefore, I propose we get rid of properties such as Credentials, Timeout, etc.

Assemblies affected

Microsoft.OData.Core 7.x

Reproduce steps

The simplest set of steps to reproduce the issue. If possible, reference a commit that demonstrates the issue.

Expected result

What would happen if there wasn't a bug.

Actual result

What is actually happening.

Additional detail

Optional, details of the root cause if known. Delete this section if you have no additional details to add.

[Finickyflame]

Or could it be possible to change the definition of method in the interface IHttpClientHandlerProvider to use HttpMessageHandler instead of HttpClientHandler? This could potentially be a smaller change (still be a public api breaking change), but it would work with the IHttpMessageHandlerFactory.

I'm using this workaround for now:

public static class DataServiceContextExtensions
{
    public static IServiceCollection AddDataServiceContext(this IServiceCollection services, Uri uri)
    {
        services.AddHttpClient(HttpClientHandlerProvider.ClientName);
        services.AddSingleton<IHttpClientHandlerProvider, HttpClientHandlerProvider>();
        services.AddTransient(provider => new DataServiceContext(uri)
        {
            HttpRequestTransportMode = HttpRequestTransportMode.HttpClient,
            HttpClientHandlerProvider = provider.GetRequiredService<IHttpClientHandlerProvider>()
        });
        return services;
    }
}

internal sealed class HttpClientHandlerProvider : IHttpClientHandlerProvider
{
    internal static readonly string ClientName = typeof(DataServiceContext).FullName!;
    
    private readonly IHttpMessageHandlerFactory _factory;

    public HttpClientHandlerProvider(IHttpMessageHandlerFactory factory) => this._factory = factory;

    public HttpClientHandler GetHttpClientHandler() => new HttpMessageHandlerToClientAdapter(this._factory.CreateHandler(ClientName));
}

/// <summary>
/// Adapts the <see cref="HttpMessageHandler"/> to work with the <see cref="Microsoft.OData.Client.DataServiceContext"/>
/// since it only accepts an <see cref="HttpClientHandler"/>.
/// </summary>
internal sealed class HttpMessageHandlerToClientAdapter : HttpClientHandler
{
    private static readonly MethodInfo SendInfo = typeof(HttpMessageHandler).GetMethod(nameof(Send), BindingFlags.Instance | BindingFlags.NonPublic)!;
    private static readonly MethodInfo SendAsyncInfo = typeof(HttpMessageHandler).GetMethod(nameof(SendAsync), BindingFlags.Instance | BindingFlags.NonPublic)!;
    
    private readonly HttpMessageHandler _messageHandler;

    public HttpMessageHandlerToClientAdapter(HttpMessageHandler messageHandler)
    {
        this._messageHandler = messageHandler;
    }

    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken) 
        => (HttpResponseMessage)SendInfo.Invoke(this._messageHandler, new object[] { request, cancellationToken })!;

    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) 
        => (Task<HttpResponseMessage>)SendAsyncInfo.Invoke(this._messageHandler, new object[] { request, cancellationToken })!;

    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            this._messageHandler.Dispose();
        }
    }
}

[dnperfors]

Provider the HttpClientHandler is definitely not enough. I would even go further and would like to pass the HttpClient directly to the constructor of the DataServiceContext. That way I can just register the dataservice as a type HttpClient and configure it as I like (as descibed https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0#typed-clients) If I need to register a Func<HttpClient> on the DataServiceContext in order to used the HttpClient that I pass with the constructor, I would be happy. If I need to implement an interface while I can provide a HttpClient that would work for me as well.

Although I might try the workaround as described above :)

[bytefish]

@habbes Any news on this? It would be great, if we could just inject a HttpClient, which would get resolved from the IServiceCollection#AddHttpClient overload. @Finickyflame Thanks for sharing your workaround!

[habbes]

Hi @bytefish we're making plans for an ODL8 release and this is part of the scope we're discussing.

[habbes]

@bytefish we have released a preview version of ODL 8 that adds support IHttpClientFactory: https://www.nuget.org/packages/Microsoft.OData.Core/8.0.0-preview.1