Improve default implementation of IHttpMessageInvokerFactory

[ArturDorochowicz]

Which version of Duende BFF are you using?
2.2.0

Which version of .NET are you using?
net8.0

Describe the bug

Not a bug, more like ideas.

Bottom line up front - default IHttpMessageInvokerFactory should look like this:

class DefaultHttpMessageInvokerFactory : IHttpMessageInvokerFactory
{
  readonly HttpMessageInvoker _httpClient;

  public DefaultHttpMessageInvokerFactory() : this(new ForwarderHttpClientFactory()) 
  {
  }

  public DefaultHttpMessageInvokerFactory(IForwarderHttpClientFactory factory)
  {
    _httpClient  = factory.CreateClient(
      new ForwarderHttpClientContext { NewConfig = HttpClientConfig.Empty });
  }

  public HttpMessageInvoker CreateClient(string localPath)
  {
    return _httpClient;
  }
}

This basically copies Yarp's DirectForwardingHttpClientProvider, which can't be used directly because it's internal.

Main points here are: 1) reuse Yarp's factory to create the client, 2) singleton client, 3) type is not to be derived from.

This change would improve a couple of things.

It would resolve DuendeSoftware/products#1693 and also guarantee that issue like that would never happen in the future.

It would do away with ConcurrentDictionary. I may be mistaken, but I don't see a point in creating client per local path (at least in the default implementation). It's not what Yarp does. If you use Yarp's direct forwarding (through MapForwarder extensions) it will just use that singleton client exposed from DirectForwardingHttpClientProvider for all endpoints. This makes code simpler and ever so slightly faster.

Current DefaultHttpMessageInvokerFactory is a type to be derived from (public, virtual methods). But it's a bit of a trap. The type is designed to be a singleton, but your AddHttpMessageInvokerFactory, which people would use to register a derived implementation, registers as transient. I think it's a good choice to register unknown third party code like that, but then your default implementation that uses a ConcurrentDictionary should not allow deriving.

To Reproduce

Steps to reproduce the behavior.

Expected behavior

A clear and concise description of what you expected to happen.

Log output/exception with stacktrace

data

Additional context

Add any other context about the problem here.

[ArturDorochowicz]

Edited the suggested code in the initial comment to be exactly like Yarp's DirectForwardingHttpClientProvider. Two constructors are important. If no IForwarderHttpClientFactory implementation is registered (which is the default), the container will use the default ctor. If there's an implementation registered (e.g. Microsoft.Extensions.ServiceDiscovery.Yarp does it here) then the other ctor will be called.

[ArturDorochowicz]

If you were open to some more breaking changes, then it would be interesting to redefine BFF in terms of Yarp's MapForwarder extensions.

MapRemoteBffApi and RemoteApiEndpoint could become something like this:

public static IEndpointConventionBuilder MapRemoteBffApiEndpoint(
  this IEndpointRouteBuilder endpoints,
  PathString localPath,
  string apiAddress)
{
  endpoints.CheckLicense();

  return endpoints.MapForwarder(
    pattern: localPath.Add("/{**catch-all}").Value!,
    destinationPrefix: apiAddress,
    configureTransform: context =>
    {
      context.AddRequestHeaderRemove("Cookie");
      context.AddPathRemovePrefix(localPath);

      var proofService = context.Services.GetRequiredService<IDPoPProofService>();
      var logger = context.Services.GetRequiredService<ILogger<AccessTokenRequestTransform>>();
      context.RequestTransforms.Add(
        new AccessTokenRequestTransform(
          proofService,
          logger,
          localPath,
          apiAddress));
        })
      .WithMetadata(new BffRemoteApiEndpointMetadata());
  }

With much of the current body of RemoteApiEndpoint.Map (the get access token part) folded into the existing AccessTokenRequestTransform.

MapForwarder has a couple of overloads, so here some additional overloads can be defined that take a transform, or transform builder, and/or http client.

This would be a breaking change in a couple of ways:

• no more IHttpMessageInvokerFactory- this is now handled by Yarp, if you want to mock the http client, mock Yarp's IForwarderHttpClientFactory (but it's one instance for all forwarder endpoints)
• no more IHttpTransformerFactory - if you want a custom transform, pass it to an overload of MapRemoteBffApiEndpoint
• no more forwarder error result logging - if it's really needed, it can be added with e.g. an endpoint filter

[AndersAbel]

@ArturDorochowicz Thanks for bringing those ideas up. We discussed this yesterday and came to the same conclusion. Back when we created our code Yarp's direct forwarding feature didn't exist. It looks like we might be able to remove quite a lot our custom code and rely on Yarp's features instead now.

This would indeed be a breaking change, but we are working on a 3.0.0 release of the BFF so this is the right time for breaking changes.

[RolandGuijt]

This is in the works so closing this for now.

[RolandGuijt]

@ArturDorochowicz I've created the above issue to keep track of this.