Design: IEndpointConventionBuilder Extensions Unusable By Other Extensions

[commonsensesoftware]

Background and Motivation

API Versioning has been investigating support for Minimal APIs per dotnet/aspnet-api-versioning#751. In doing so, it has come to light that the extension methods for IEndpointConventionBuilder are inconsistently implemented and many of them have little-to-no usably by other extensions such as API Versioning.

The primary issues relate to OpenApiRouteHandlerBuilderExtensions.cs. These extensions are very likely to be used by customers in conjunction with API Versioning, but cannot be for the following reasons:

1. RouteHandlerBuilder is sealed
2. The extensions methods accept and return the concrete RouteHandlerBuilder type

For completeness, a similar problem exists for FallbackEndpointRouteBuilderExtensions.cs. Fallback endpoints are not expected to be used with API Versioning, but it could affect other extensions. These extension methods accept and return IEndpointConventionBuilder, which makes them more usable than OpenApiRouteHandlerBuilderExtensions; however, the lack of passing through a more specific type means that the order setup by developers matters.

The design and implementation of each set of extension methods appears to have been done by different people, at different times, and with different design review considerations.

Proposed API

There doesn't appear to be a clear reason why these decisions were made. There seems to be no reason to not implement all of the extension methods using the same approach that @JamesNK used in RoutingEndpointConventionBuilderExtensions.cs. This would mean that all extension methods have the form of:

public static TBuilder SomeExtension<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder { } 

This approach appears to have been lightly discussed in #8902 previously, which might explain why future extension methods did not follow suite.

The proposed change would benefit not just API Versioning, but any other extension that needs to add/change significant parts of the default Minimal API implementation.

Risks

Changing the signature of the existing APIs are a breaking change, but I believe that adding the intended generic implementations can live side-by-side with the existing non-generic variants.

If the proposal were accepted, when would that happen? As it stands, this issue cascades across APIs. API Versioning would be required to reimplement all of the applicable, existing extension methods to retain feature parity for non-versioned Minimal APIs. Furthermore, the unnecessary non-generic extensions methods have to be retained just as they do in ASP.NET Core - likely forever more. If API Versioning doesn't reimplement the extensions methods, then there is a feature gap that must be filled by customers.

API Versioning is looking for guidance to achieve the right level of synergy in both the short and long terms.

Example Usage

The tentative design for Minimal APIs for API Versioning will look something like:

app.DefineApi()
   .HasVersion(1.0)
   .HasVersion(2.0)
   .ReportApiVersions()
   .HasMapping(api => {
       // OpenAPI extensions cannot be used here without reimplementing and maintaining them
   	api.MapGet("/speak", () => "Hello world!");
   	api.MapPost("/speak", (string text) => text).MapToApiVersion(2.0);
   });

cc: @davidfowl @JamesNK

[halter73]

How exactly do you plan to leverage the ability to inherit from RouteHandlerBuilder?

We've returned public concrete IEndpointConventionBuilder's types for a while from Map methods for a a frew releases now. For example:

• MapControllers returns a ControllerActionEndpointConventionBuilder (sealed + internaly-only ctor)
• MapRazorPages returns a PageActionEndpointConventionBuilder (sealed + internaly-only ctor)
• MapHub returns a HubEndpointConventionBuilder (sealed + internal-only ctor)
  • This has the redeeming quality of deriving from an intermediate IHubEndpointConventionBuilder which extension methods could hook off of.
• MapBlazorHub is the same as MapHub but with ComponentEndpointConventionBuilder
• MapConnections returns a ConnectionEndpointRouteBuilder (sealed + internaly-only ctor)

The big reason we sealed RouteHandlerBuilder (other than that unsealing later on isn't breaking while sealing would be) is consistency with these other types. As far as I know, RouteHandlerBuilder is the first of these classes we've shipped specific extension methods for (in OpenApiRouteHandlerBuilderExtensions) which shows the weakness of this pattern.

Rightly or wrongly, we wanted the OpenApiRouteHandlerBuilderExtensions to only apply to minimal route handlers so they extended the concrete RouteHandlerBuilder type rather than IEndpointConventionBuilder. We felt these extension methods might not make sense for a SignalR Hub for example. Perhaps we could revisit this.

While you could in theory derive from RouteHandlerBuilder and keep support for all the OpenApiRouteHandlerBuilderExtensions without reimplementing them, this is a trick you could only really do once. Imagine you have MyWrappedRouteHandlerBuilder and SomeoneElsesWrappedRouteHandlerBuilder which both had their own extension methods and derived directly from RouteHandlerBuilder. You'd only be able to get extension methods for one or the other. There's no object that you could return that would support all the extension methods for both MyWrappedRouteHandlerBuilder and SomeoneElsesWrappedRouteHandlerBuilder unless one inherits from the other or implements the other's interface(s).

If you want to add support for additional extension methods on a ControllerActionEndpointConventionBuilder, PageActionEndpointConventionBuilder, HubEndpointConventionBuilder, etc..., you'd be defining a bunch of types inheriting from different classes (assuming we unseal them and make the ctors public). And doing so still wouldn't fix the composability issues of supporting multiple classes that derive from a single one of these types. At least RouteHandlerBuilder has a public constructor so you can create your own instance.

Changing the signature of the existing APIs are a breaking change, but I believe that adding the intended generic implementations can live side-by-side with the existing non-generic variants.

I tested adding the following overloads to OpenApiRouteHandlerBuilderExtensions.Accepts(...):

// Old
public static RouteHandlerBuilder Accepts<TRequest>(this RouteHandlerBuilder builder,
    string contentType, params string[] additionalContentTypes) where TRequest : notnull

// New
public static TBuilder Accepts<TBuilder, TRequest>(this TBuilder builder,
    string contentType, params string[] additionalContentTypes) where TBuilder : IEndpointConventionBuilder where TRequest : notnull

// New
public static TBuilder Accepts<TBuilder, TRequest>(this TBuilder builder,
    string contentType, params string[] additionalContentTypes) where TBuilder : RouteHandlerBuilder where TRequest : notnull

The problem is that the C# compiler seems to always prefer the overload with less generic parameters unless you specify the generic parameters explicitly. The following would still use the old overload:

app.MapPost("/accepts-xml", () => Accepted()).Accepts<Person>("application/xml");

I verified this is also the case for overloads that previously had no generic parameters.

[halter73]

Maybe it's okay if the existing overloads OpenApiRouteHandlerBuilderExtensions are preferred when given a RouteHandlerBuilder as long as the new overloads also work on any IEndpointConventionBuilder. This means being able to call app.MapHub<MyHub>("/myhub").Accepts<Person>("application/xml") which is weird but perhaps the lesser evil.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[commonsensesoftware]

Inheriting from RouteHandlerBuilder wouldn't really help much. As you've astutely outlined, it doesn't really address the crux of the problem. There are potentially some less than ideal ways it could be beaten into submission, but since the type is sealed that avenue is a dead end. At least for API Versioning, there is little interest in extending RouteHandlerBuilder. I just wanted to call out that it's not an option for any extender. That facet seems to be by design, which would be OK if the extension methods weren't coupled to that type.

The current plan of action is that the builder at this level in composition from API Versioning would return it's own VersionedEndpointMetadataBuilder that would implement IEndpointConventionBuilder with all the addition stuff specific to API versioning. API Explorer extensions for OpenAPI exist today and will certainly be asked for. It is very undesirable to maintain a separate, parallel copy that would essentially be a fork of the implementation in ASP.NET Core.

If we agree that the generic variants makes sense, the fact that RouteHandlerBuilder is sealed is irrelevant. In fact, that probably even helps. As I understand, the compiler will chose an extension method with a matching, explicit type over a compatible generic overload. That would seem to be the ideal, long-term solution IMO. There wouldn't be confusion between extension methods from a usage perspective because the RouteHandlerBuilder extensions would never into an API Versioning extension since it's intrinsically not extensible.

For the scenarios that require multiple, generic parameters, I understand the challenge/problem. I also understand why/how this led down a path where a concrete type such as RouteHandlerBuilder had to be used. For example, the proposed new signature:

public static TBuilder Accepts<TBuilder, TRequest>(
    this TBuilder builder,
    string contentType,
    params string[] additionalContentTypes)
    where TBuilder : IEndpointConventionBuilder
    where TRequest : notnull

will not work because the compiler does not allow specifying only some of the generic type parameters explicitly this way. It would have to be builder.Accept<VersionedRouteHandlerBuilder, Person>("application/xml"), which is yucky.

However, I feel with some additional thinking and refactoring, this is a solvable problem albeit it another form. For example, if the second generic type parameter was lifted out into a callback, the following could work:

public class AcceptMetadataBuilder
{
    public AcceptMetadataBuilder Of<TRequest>(string contentType) where TRequest : notnull
    {
        // ...
        return this;
    }

    public AcceptMetadata Build() { /* ... */  }
}

// ...

public static TBuilder Accept<TBuilder>(this TBuilder builder, Action<AcceptMetadataBuilder> configure)
    where TBuilder : IEndpointConventionBuilder
{
     var acceptMetadata = new AcceptMetadataBuilder();
     configure(acceptMetadata);
     builder.WithMetadata(acceptMetadata.Build());
    return builder;
}

// ...

builder.Accept(request => request.Of<Person>("application/json"));

That's just one possible idea. I'm sure there are other possible ways. Of course, the flavor that doesn't use the TRequest is also an option:

builder.Accept(typeof(Person), "application/json");

[commonsensesoftware]

I now have a working example for API Versioning that you can peek at here. Hopefully, that helps illustrate the amount of forking and/or implementation of existing code required to retain parity. It's not everything, but it's also not zero.

This is related to and might be exacerbated by #36007.

[commonsensesoftware]

@halter73 API Versioning has an official Preview release with this support. As expected, the community is immediately champing at the bit for parity with the OpenApiRouteHandlerBuilderExtensions. In my attempt to appease the masses, I realized that the current state of the union is even worse than I thought. The current IProducesResponseTypeMetadata and IAcceptsMetadata implementations are internal classes and have internal properties, which means I can't even fork their implementations. I've had to resort to using Reflection with compiled lambdas to preserve the intrinsic behavior 😞. If you're curious, you can see the solution I came up with here.

I'm trying to get the compatible .NET 6.0 release out within the next month (or less). Ideally, I'd like to have signature parity with wherever this issue is going to land. That will minimize the impact for the .NET 7.0 release. Expanding upon what's already been discussed, here's the current preview solution I've come up with:

api.MapGet("/people/{id:int}", (int id) => new Person(){ Id = id })
   .Produces(response => response.Body<Person>())
   .HasApiVersion(1.0);

api.MapPost("/people", (Person person) => Results.Created("/people/" + person.Id, person))
   .Accepts(request => request.Body<Person>())
   .Produces(response => response.Body<Person>(), 201)
   .HasApiVersion(1.0);

api.MapPatch("/people/{id:int}", (int id, Person person) => Results.NoContent())
   .Accepts(request => request.Body<Person>().FormattedAs("application/merge-patch+json"))
   .Produces(response => response.Body<Person>().FormattedAs("application/json"), 204)
   .HasApiVersion(1.0);

To see the complete picture, here's the end-to-end Minimal OpenAPI Example.

[halter73]

There doesn't appear to be a clear reason why these decisions were made. There seems to be no reason to not implement all of the extension methods using the same approach that @JamesNK used in RoutingEndpointConventionBuilderExtensions.cs. This would mean that all extension methods have the form of:

public static TBuilder SomeExtension<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder { } 

@commonsensesoftware We recently did some of this as part of #41428 and then also did this for AddEndpointFilter and WithOpenApi as part of #41427.

The Produces, ProducesProblem, ProducesValidationProblem and Accepts extension methods were notably excluded from this treatment even though I originally proposed we update these too. The main issue with updating these APIs were the type arguments. Having an additional type argument on top of TBuilder is annoying because then you have to explicitly pass the TBuilder argument then. And as mentioned in the API Review Notes, WithMetadata can be used manually with attributes if needed. But as you note above, there is no public IProducesResponseTypeMetadata implementation at the moment, so setting a response type on a non-RouteHandlerBuilder is even less convenient than it otherwise would be.

I'll admit that this API review was focused on the MapGroup use case and less on the API Versioning use case. I realize that the extension methods we left out are exactly the methods you proposed updating in this issue. Would you be okay with having to write something like .Accepts<VersionedEndpointMetadataBuilder, RequestType>(...)? Or is there something else you would prefer? Are there any other methods you'd like to see use this TBuilder pattern?

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.