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.

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

Decouple the MatcherPolicy from MVC and make it solely based on routing. #751

Closed
davidfowl opened this issue Jun 25, 2021 · 14 comments
Closed

Comments

@davidfowl
Copy link
Member

The implementation could be made more generic by using endpoint metadata directly instead of the ActionDescriptor to stash the API information. Instead of doing SetProperty the data could be stored here.

I haven't looked deeply to see what other tentacles reach into MVC.

@commonsensesoftware
Copy link
Collaborator

Interesting that you mention that. A few people have asked how that might be achieved.

Switching from storing things in an ActionDescriptor to EndpointMetadata, or more specifically Endpoint.Metadata, is feasible - with the right amount of refactoring. API Versioning was around before Endpoint Routing (and helped flush out some of the design with Ryan and James). The two main reasons the information is still in a property are:

  1. Historical
  2. Continues to work for those still using legacy routing (they're still out there)

#2 can be addressed by the fact that you can't use both routing systems simultaneously. A strategy can be used to control where the information is accessed to avoid a bunch of branching conditions. Today, this would normally be detected through MvcOptions.EnableEndpointRouting. If MVC were removed, then it would probably be assumed true and only possibly switch when integrated with MVC.

I've also be considering breaking things further apart; something like Abstractions. This work and feature would likely align to those changes. Any thoughts or suggestions are most certainly welcome.

TL;DR

The fundamental challenge to removing MVC completely is how the metamodel is currently discovered. Version discovery and collation are largely done through the ApplicationModels namespace. In the strictest sense, API Versioning doesn't really care about that and isn't bound to it. A fairly early design point was API version information is purely metadata. It is provided by way of IApiVersionProvider. This is most commonly achieved by way of attributes, but there is a conventions fluent API as well. The Application Model afforded by MVC provides a nice way to inspect and apply API version metadata by interrogating the developer's code.

API version collation is one of the more difficult challenges. Despite what some people may think, it's achieved using the controller's name. Again - it's doesn't have to, but it was a natural choice. It might seem more logical to group by route template, but this can go sideways pretty quickly. For example, api/v{ver:apiVersion}/{id} and api/v{version:apiVersion}/{id:int} are semantically equivalent (and obvious to developers reading it), but is quite difficult - as well as likely slow - to match up. There are other scenarios where the route template just doesn't give you enough information. For example, what if value/{id} and value/{id}/subvalue/list should, or shouldn't, be grouped together? It's a bit more nuanced than that, but hopefully that paints a picture.

Armed with that information, what I conceptually think could work if those other issues are addressed would be something like:

app.UseEndpoints(endpoints =>
{
	// version-neutral api
	endpoints.MapGet("/ping", c => Task.CompletedTask)
	         .IsApiVersionNeutral();

	// 1.0 endpoint
	endpoints.MapGet("/hello", c => c.Response.WriteAsync("Hello world v1!"))
	         .HasApiVersion(1, 0);

	// 2.0 endpoints (grouped for convience)
	endpoints.WithApiVersion(2, 0, api =>
	{
	      api.MapGet("/hello", c => c.Response.WriteAsync("Hello world v2!"));
	});
});

I suspect this approach breaks or inhibits API Explorer features for those that care about it - usually for OpenAPI documents. I believe that's already an issue and an understood decision point for choosing this type of setup.

@davidfowl
Copy link
Member Author

We're interested in this so we'll investigate what it would take to make it work in the .NET 7 timeframe.

@commonsensesoftware
Copy link
Collaborator

@davidfowl,

I just wanted to let you know that this work has been completed (but I'm not closing the issue just yet). Due to changes in the project that have just been announced, we're still a little bit out from publishing the corresponding packages. If you're interested in taking a peek, you can see the implementation here. Things are now split apart from core HTTP and MVC Core, where API Versioning is possible without any part or direct references to the MVC Core assembly. This also sets up the foundation for Minimal API support.

@commonsensesoftware
Copy link
Collaborator

As it relates to Minimal API, everything will tie together like this:

builder.Services.AddApiVersioning();

var app = builder.Build();

app.DefineApi()
   .HasApiVersion( 1.0 )
   .HasApiVersion( 2.0 )
   .ReportApiVersions()
   .HasMapping( api =>
    {
        // GET /weatherforecast?api-version=1.0
        api.MapGet( "/weatherforecast", () =>
            {
                return Enumerable.Range( 1, 5 ).Select( index =>
                    new WeatherForecast
                    (
                        DateTime.Now.AddDays( index ),
                        Random.Shared.Next( -20, 55 ),
                        summaries[Random.Shared.Next( summaries.Length )]
                    ) );
            } )
           .MapToApiVersion( 1.0 );

        // GET /weatherforecast?api-version=2.0
        api.MapGet( "/weatherforecast", () =>
            {
                return Enumerable.Range( 0, summaries.Length ).Select( index =>
                    new WeatherForecast
                    (
                        DateTime.Now.AddDays( index ),
                        Random.Shared.Next( -20, 55 ),
                        summaries[Random.Shared.Next( summaries.Length )]
                    ) );
            } )
           .MapToApiVersion( 2.0 );

        // POST /weatherforecast?api-version=2.0
        api.MapPost( "/weatherforecast", ( WeatherForecast forecast ) => { } )
           .MapToApiVersion( 2.0 );

        // DELETE /weatherforecast
        api.MapDelete( "/weatherforecast", () => { } )
           .IsApiVersionNeutral();
    } );

@davidfowl
Copy link
Member Author

davidfowl commented Mar 19, 2022

@commonsensesoftware We're also doing route grouping in .NET 7 dotnet/aspnetcore#36007 so we should make sure this gels with that. I'm curious to play with this though!

cc @halter73 @captainsafia

@commonsensesoftware
Copy link
Collaborator

@davidfowl,

Preview 2 is now available. The 🐀 🧶 has been eliminated. To the greatest extent possible, everything now only considers endpoints and their metadata.

New Packages:

Examples:

The discussion on route grouping is progressing well, but some help may be needed in dotnet/aspnetcore#39604. Customers don't want to wait until .NET 7 for me to release an iteration. Ideally, I'd like to release within the next month. Since things are in preview now with a lot of breaking changes from previous versions, I'm ok with a few more. I don't want to repeat that in the .NET 7 release. Anything that we can to align direction (e.g. design + signatures) should be good for all. As can be seen here, I've had to resort to some pretty 🤮 hackery to retain parity with the out-of-the-box Minimal API support for the API Explorer.

@halter73 and I have had some discussion, but nothing is settled - yet.

@davidfowl
Copy link
Member Author

Hey @commonsensesoftware, super happy to see this landing but I'm very concerned with the amount of code copied and the layering of the approach you landed on here. I think there are 2 things being coupled here that I'd like to see split up:

  • The metadata for defining an API version policy, and the matcher policy
  • The builder API that defines that policy for a single endpoint or group of endpoints.

What you have right now in Asp.Versioning.Http looks like its both things. This coupled with the fact that to get the desired API you've had to replicate all of the extensions (and the associated bugs) bothers me quite a bit.

I wonder if we could have an API version policy object that could be added to multiple endpoints manually.

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.
builder.Services.AddApiVersioning();

var app = builder.Build();

var apiPolicy = new ApiVersionPolicyBuilder()
            .HasApiVersion(1.0)
            .HasApiVersion(2.0)
            .ReportApiVersions()
            .Build();

// GET /weatherforecast?api-version=1.0
api.MapGet("/weatherforecast", () =>
{
        return Enumerable.Range(1, 5).Select(index =>
            new WeatherForecast
            (
                DateTime.Now.AddDays(index),
                Random.Shared.Next(-20, 55),
                summaries[Random.Shared.Next(summaries.Length)]
            ) );
})
.WithApiVersionPolicy(apiPolicy)
.MapToApiVersion(1.0);

// GET /weatherforecast?api-version=2.0
app.MapGet("/weatherforecast", () =>
{
    return Enumerable.Range(0, summaries.Length ).Select(index =>
        new WeatherForecast
        (
            DateTime.Now.AddDays( index ),
            Random.Shared.Next( -20, 55 ),
            summaries[Random.Shared.Next(summaries.Length)]
        ) );
})
.WithApiVersionPolicy(apiPolicy)
.MapToApiVersion( 2.0 );

// POST /weatherforecast?api-version=2.0
app.MapPost("/weatherforecast", (WeatherForecast forecast ) => { })
    .WithApiVersionPolicy(apiPolicy)
    .MapToApiVersion(2.0);

// DELETE /weatherforecast
app.MapDelete("/weatherforecast", () => { } )
    .WithApiVersionPolicy(apiPolicy)
    .IsApiVersionNeutral();

I'd prefer to have you focus on the pure API versioning data model and let us worry about how to apply policies to multiple endpoints (via route grouping).

@commonsensesoftware
Copy link
Collaborator

@davidfowl ,

First, thanks for taking to the time to provide feedback.

Philosophically, we are aligned. The goal of API Versioning has always been to provide ways to bolt on the metadata and let you define routes as you always have. In the most naive sense, the mental model is meant to be that API versions serve as nothing more than a way to disambiguate otherwise ambiguous endpoints. When going through the initial design review with Ryan and James, this was, and still is, achieved with a custom IEndpointSelectorPolicy.

Certain 404 responses as well as 405 and 415 have always been a challenge for API Versioning, but #744 really highlights the issue. I thought it might have been a mistake in the routing system itself. After some discussion with Javier, it became clear that a custom INodeBuilderPolicy is also necessary so that versioned endpoints are eliminated before other policies and the correct response is returned. This steams from the fact that, while you know that a candidate is invalid, you don't know why. The addition of a INodeBuilderPolicy addresses each of these problems and has an added benefit of eliminating candidates faster. This workflow illustrates the endpoint candidate selection process.

Building a versioning policy by hand is possible, but puts the onus on the developer and is primed for errors. Collation and construction of a policy per endpoint has always been one of the key features of API Versioning.

It's important to understand a declared API version versus a mapped API version. Declaring an API version is what is exposed on the surface area and reported. Mapping an API version only disambiguates a specific endpoint API version. Think of mapping as analogous to method overriding or interface mapping; e.g. for given endpoint E and method M, map it to API version A.

Let's reconsider a sample setup with more annotations:

app.DefineApi()          // ← MapGroup (?); hope to align signatures in 6.0 so 7.0 uses final design
   .HasApiVersion( 1.0 ) // ← all endpoints in this group declare 1.0
   .HasApiVersion( 2.0 ) // ← all endpoints in this group declare 2.0
   .ReportApiVersions()  // ← all endpoints in this group reports their versions
   .HasMapping( api =>
    {
        // GET /weatherforecast?api-version=1.0
        api.MapGet( "/weatherforecast", () => new WeatherForecast[]{ new() } )
           .MapToApiVersion( 1.0 );   // ← explicitly maps to 1.0

        // GET /weatherforecast?api-version=2.0
        api.MapGet( "/weatherforecast", () => new WeatherForecast[]{ new() } )
           .MapToApiVersion( 2.0 );    // ← explicitly maps to 2.0

        // GET /weatherforecast/{id}?api-version=1.0|2.0    ← implicitly maps to 1.0 and 2.0
        api.MapGet( "/weatherforecast/{id}", ( int id ) => new WeatherForecast() );

        // POST /weatherforecast?api-version=1.0|2.0    ← implicitly maps to 1.0 and 2.0
        api.MapPost( "/weatherforecast", ( WeatherForecast forecast ) => { } );

        // DELETE /weatherforecast[?api-version=1.0|2.0]   ← implicitly maps to 1.0, 2.0, or unspecified
        api.MapDelete( "/weatherforecast", () => { } ).IsApiVersionNeutral();
    } );

When the /weatherforecast endpoint reports its versions, all endpoints with the exception of DELETE (because it's version-neutral), will report the header:

api-supported-versions: 1.0, 2.0

In MVC, this is achieved using IActionFilter via ReportApiVersionsAttribute. For Minimal APIs, it is achieved using a decorator which is applied as a convention. The developer doesn't have to add each and every convention. They express their intent and the right thing happens. This invariably means customizations to the underlying builders so that a Minimal API can live up to its name.

While I appreciate, and even like, the simplicity of attaching a policy, there's a bit more to it. I feel it's important that the API Versioning setup be as similar to a standard Minimal API setup as possible. API Versioning has already had its own conventions API for a long time. The current design is inspired by melding those two together. I don't want the Minimal API setup to be a completely new and different approach.

Ultimately, I do not want to copy or duplicate a bunch of intrinsic code that supports Minimal APIs. For the 6.0 release, I doubt there is any way around that. It's not too late for the 7.0 release though. I'm calling out the pain points and limitations I'm facing as early has possible so that there is a path to an agreeable solution. I'm highly interested and invested in following the Grouping work so we are aligned. I'd also like to see the limitations in the existing design remedied. Several things are internal, use internal members, or are sealed for no particularly good reason. I need some help driving that.

Sidebar: I faced similar challenges in the legacy IActionSelector design. While some of the bits were opened up in later releases, quite a bit of internal stuff had to be duplicated because there was no other way to use or consume it.

@davidfowl
Copy link
Member Author

The problem is that it will exist forever and it does not compose with the existing APIs. Think about what would happen if other libraries did the same thing? They would all create their own grouping constructs that don't compose and it would be chaos (that's why we're doing it in .NET 7).

As a compromise, can you split the builder API into a separate opt-in package?

PS: This is your package and feel free to ignore this feedback but I feel pretty strongly about this and I would argue this isn't minimal APIs, it's your own version of it. When issues show up because people move code from the top level to the version API version builder, we'll have to treat them as external bugs.

I'm highly interested and invested in following the Grouping work so we are aligned. I'd also like to see the limitations in the existing design remedied. Several things are internal, use internal members, or are sealed for no particularly good reason. I need some help driving that.

👍🏾

@commonsensesoftware
Copy link
Collaborator

@davidfowl,

Agreed. I strive to avoid forking code whenever I can. I'm not interested in owning it. I'm also aware of a few scenarios with teams who shall not be named that have not followed that recommendation and it bit them down the road. I'll just say that I'd rather not be a target of the meme:

"When David Fowler gives you sound advice and you don't follow it."

I'd like a mulligan. I've gone back to the whiteboard using the skeleton of what you proposed. Here's what I've come up with:

// shortcut for: app.Services.GetRequiredService<IApiVersionSetBuilderFactory>().Create(default(string));
var versionSet = app.NewApiVersionSet()
                    .HasApiVersion( 1.0 )
                    .HasApiVersion( 2.0 )
                    .ReportApiVersions()
                    .Build();

// GET /weatherforecast?api-version=1.0
app.MapGet( "/weatherforecast", () => default(WeatherForecast))
   .UseApiVersioning( versionSet )
   .MapToApiVersion( 1.0 );

// GET /weatherforecast?api-version=2.0
app.MapGet( "/weatherforecast", () => default(WeatherForecast))
   .UseApiVersioning( versionSet )
   .MapToApiVersion( 2.0 );

Names are important. Policy sounds great, but it's a misnomer in this context. The object is not fully constructed at this point. A policy would be more indicative of the final thing constructed. This might be possible completely out-of-band, but it would be verbose and look ugly methinks. There also doesn't seem to be a way to reference the endpoints out-of-band to make the correlations. The API versioning information can be collated across more than one endpoint.

A version set seemed to more accurately represent that, but I'm not entirely married to that name. The established WithXXX extension methods add metadata to the endpoint. At this point in the composition, the metadata is not added. Only when the Endpoint is finally constructed is ApiVersionMetadata applied from all builder operations.

I hacked and slashed all the preview code. No code is forked, copied, or duplicated now. However, the existing ASP.NET Core 6.0 design has some limitations IMO.

  1. The builder pipeline is difficult or impossible to extend because you can only apply a convention
  2. The IEndpointConventionBuilder has no access to services

The first issue is annoying, but it can be worked around. The second issue is more painful. Since there is neither a way to access the information that has been collected in the builder chain (say a monad of builder metadata) and there is no access to services, I was forced to bridge the two with an interface that unions the necessary behaviors. I don't see a way around this without sacrificing or changing long established features and behaviors.

public interface IVersionedEndpointConventionBuilder :
    IEndpointConventionBuilder,
    IMapToApiVersionConventionBuilder
{
    bool ReportApiVersions { get; set; }
    ApiVersionMetadata Build();
}

This allows the result of UseApiVersioning to retain the intrinsic built-in functionality, while adding API Versioning specific behaviors. This also removes builder ordering issues. UseApiVersioning will be required for even the most simple setup because there is no other way to get or request information such as the configured ApiVersioningOptions. This can't and doesn't need to change for 6.0, but at least adding IEndpointConventionBuilder.Services in 7.0 should be considered.

Even though I don't need to as a one-person show, I try to be in the habit of creating PRs so the community can see what I'm doing and potentially comment on it. As such, PR #816 is up with all the current changes I'm considering for Preview 3. A big part of the PR is this refactoring. If you'd like to comment directly on it, feel free to do so. All the changes for this topic are in a single commit. If that is TL;DR and you just want to see the changes, you can look at the source branch here. The example projects have been updated as well. Any additional thoughts or ideas are certainly welcome.

@captainsafia
Copy link
Member

The IEndpointConventionBuilder has no access to services

dotnet/aspnetcore#41238 adds access to the ServiceProvider to the EndpointBuilder class which means that you can access it when adding conventions to your builder. I'm not sure if that is enough of an access point for your particular implementation but it should be available to play around with in .NET 7.

@commonsensesoftware
Copy link
Collaborator

@captainsafia this is great news! Having access to services, even just on the EndpointBuilder, will be an improvement. Armed with this knowledge, I will the necessary adjustments for the 6.0 release knowing this change will come in 7.0. Thanks for sharing.

@commonsensesoftware
Copy link
Collaborator

@davidfowl, 6.0 is finally signed and published with all the of aforementioned goodness. I'll start the process of looking into adding the officially supported grouping constructs for .NET 7.0 and see how things line up.

I'll also look at the changes provided by @captainsafia for ways to streamline policy configuration and make it more natural.

Thanks for all the discussion and feedback. The project landed in a better place for it.

@davidfowl
Copy link
Member Author

You are a blessing to the .NET community @commonsensesoftware! Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants