[API Proposal]: Add IHttpClientBuilder to builder interfaces returned by AddStandardResilienceHandler(), AddStandardHedgingHandler()

[abatishchev]

Background and motivation

Currently the top-level API methods for Microsoft.Extensions.Http.Resilience are:

public static IHttpStandardResiliencePipelineBuilder AddStandardResilienceHandler(this IHttpClientBuilder builder);

public static IStandardHedgingHandlerBuilder AddStandardHedgingHandler(this IHttpClientBuilder builder);

Both return an inner builder:

public interface IHttpStandardResiliencePipelineBuilder
{
    string PipelineName { get; }
    IServiceCollection Services { get; }
}

public interface IStandardHedgingHandlerBuilder
{
    string Name { get; }
    IServiceCollection Services { get; }
    IRoutingStrategyBuilder RoutingStrategyBuilder { get; }
}

Each has a reference to IServiceCollection, but not to IHttpClientBuilder.

API Proposal

Update the builder interfaces with a reference to the original IHttpClientBuilder:

public interface IHttpStandardResiliencePipelineBuilder
{
  // ...
  IHttpClientBuilder HttpClientBuilder { get; }
}

public interface IStandardHedgingHandlerBuilder
{
  // ...
  IHttpClientBuilder HttpClientBuilder { get; }
}

API Usage

Currently, the user has to store a reference to IHttpClientBuilder manually:

var httpClientBuilder = services.AddHttpClient(...);
httpClientBuilder.AddStandardResilienceHandler(...);
httpClientBuilder.AddStandardHedgingHandler(...);

Instead, this would be possible:

services.AddHttpClient(...)
        .AddStandardResilienceHandler(...)
        .HttpClientBuilder // <-- the newly added property
        .AddStandardHedgingHandler(...);

Alternative Designs

No response

Risks

Minimal since both HttpStandardResiliencePipelineBuilder and StandardHedgingHandlerBuilder are currently private classes (records).

[joperezr]

cc @iliar-turdushev can you please take a look? Proposal seems straight forward, so if there aren't any concerns implementation-wise, we can mark it as ready-for-review.

My 2 cents here of course the ideal would have been for both of those AddStandard.. methods to return the httpClientBuilder, but they didn't and it would be a breaking change to change that. Given the state we are in, it isn't immediately obvious to me why calling a property is necessarily better than your workaround example where the caller just stores the http client builder.

[joperezr]

cc: @martintmk

[abatishchev]

@joperezr, @martintmk what should be the next step? If you agree, I'd like to give it a try and open a pull request with the change

[martintmk]

For the context, when we were initially designing the API, both IStandardHedgingHandlerBuilder and IStandardHedgingHandlerBuilder contained reference to IHttpClientBuilder.

In the API review, nesting builders was considered as anti-pattern and we removed the reference. I see the benefits though, so if everyone agrees we can introduce this change.

cc @geeknoid Your thoughts on this Proposal?

[abatishchev]

@geeknoid ping, please

[geeknoid]

Extending a public interface type would be a breaking change, right?

[abatishchev]

Technically, yes. But I think it's extremely unlikely anyone uses a non-default implementation of any builder interface. Such interfaces interfaces aren't a public contact but a standard way to implement the fluent/builder API.

[geeknoid]

@joperezr If you're OK with the potential breaking change, then I'm also OK with the change.

[joperezr]

I of course would rather not do a breaking change unless we absolutely have to, but as called out I also think this is very unlikely to break people. That said, I'm not really a SME on builder APIs, so I'll mark this as ready for review and check what experts like @halter73 think to make sure they agree this breaking change is okay.

[joperezr]

Actually, wait a sec, would this break if some code compiled against the older version of the interface (via an old package) but then ends up using a newer package with the new interface and doesn't re-compile?

(mainly asking as that would change my assessment of "unlikely to break people")

[RussKie]

Should we get advice from ARB folks?

[martintmk]

How about exposing HttpClientBuilder extension method instead? Usage would be very similar:

services.AddHttpClient(...)
        .AddStandardResilienceHandler(...)
        .HttpClientBuilder()
        .AddStandardHedgingHandler(...);

[abatishchev]

.HttpClientBuilder()

Wouldn't this be awkward and inconsistent comparing to the rest of the API? e.g. the property Services already exists and is widely used.

[halter73]

Technically, yes. But I think it's extremely unlikely anyone uses a non-default implementation of any builder interface. Such interfaces interfaces aren't a public contact but a standard way to implement the fluent/builder API.

This isn't something we've been super consistent with. MVC, Blazor and SignalR use interfaces for their builders. IApplicationBuilder, IEndpointRouteBuilder, IEndpointConventionBuilder and IHttpClientBuilder are all interfaces too, but AuthenticationBuilder, OptionsBuilder<TOption> don't implement any interfaces.

An upside of interfaces is that you can have a builder that implements multiple builders or otherwise augments the builder like WebApplication which implements IApplicationBuilder and IEndpointRouteBuilder, but it does make it harder to add anything to the builder particularly if you're targeting netstandard or net4xx where I don't think we can rely on C# 8 default interface methods (DIM). You could also argue the interfaces make it easier to write certain kinds of test cases, but I don't find that super compelling.

Actually, wait a sec, would this break if some code compiled against the older version of the interface (via an old package) but then ends up using a newer package with the new interface and doesn't re-compile?

(mainly asking as that would change my assessment of "unlikely to break people")

This would break implementers of the interface unless there is a DIM and the implementing type is loaded by a runtime that supports DIMs. It shouldn't break code simply calling interface methods unless those methods were removed. That said, I don't think it's worth accepting any level of breaking change just to save people from storing the IHttpClientBuilder in a variable.

[abatishchev]

It shouldn't break code simply calling interface methods unless those methods were removed

So this is not a breaking change, correct?

just to save people from storing the IHttpClientBuilder in a variable.

The existing API is less fluent, less convenient, more chatty. Hence the proposal.

[iliar-turdushev]

What if we add IHttpClientBuilder only on those .NET versions that support C# 8, i.e. support default interface property implementation:

public interface IHttpStandardResiliencePipelineBuilder
{
#if NET6_0_OR_GREATER
  IHttpClientBuilder HttpClientBuilder { get => null; }
#endif
}

Then HttpClientBuilder property will become available on .NET 6 and later. .NET Framework 4.6.2 will not support it though. Such a change will not break our customers.

[geeknoid]

@iliar-turdushev That sounds quite reasonable.

[RussKie]

What if we add IHttpClientBuilder only on those .NET versions that support C# 8, i.e. support default interface property implementation:

public interface IHttpStandardResiliencePipelineBuilder
{
#if NET6_0_OR_GREATER
  IHttpClientBuilder HttpClientBuilder { get => null; }
#endif
}

Then HttpClientBuilder property will become available on .NET 6 and later. .NET Framework 4.6.2 will not support it though. Such a change will not break our customers.

Mm.... This makes me concerned. Would this pass package validation?

[iliar-turdushev]

Mm.... This makes me concerned. Would this pass package validation?

@RussKie It seems that the validation passes. Please, take a look at the following PR #5215. All checks have passed. Does it mean that we are good? The HttpClientBuilder property I've added is Experimental though. It shouldn't affect the validation, right?

UPDATE Making the property stable breaks the build. After I removed the Experimental attribute the package validation started to fail, see #5220. We already have a package with the same issue. To fix it the warning is simply suppressed. Are we good to apply the same approach (suppress the warning) here?

[abatishchev]

@iliar-turdushev ping on this, please. who should we follow up with to respond to your latest update?

[joperezr]

Langversion is not tied to a TFM, but instead to the tooling which can vary. I'm not a fan of forking the API surface between TFMs to be honest unless we absolutely have to which doesn't seem to be the case here. Package Validation won't fail with the proposal because the two TFMs aren't compatible with each other (meaning you can't compile against net6.0 but then execute against net462)

[joperezr]

@abatishchev I really appreciate the suggestion here, and I also believe that if we were to redo this we would have probably have these properties to begin with, but given we have already shipped and the potential breaking changes called above, I'm leaning to following @halter73's advice here (who is the SME on aspnetcore APIs) and not take the breaking change given the workaround is really straight forward (just store the httpclientbuilder in a variable). @iliar-turdushev or @abatishchev is there any scenario where doing the workaround wouldn't be possible? Mainly asking as that could influence whether or not we think we can take this.