AddRoutingCore(this IServiceCollection services) and RouteOptions updates for CreateSlimBuilder

[mitchdenny]

Background and Motivation

Modify RouteOptions to not automatically add the RegexInlineRouteConstraint. Instead AddRouting(...) is modified to call a new method called AddRoutingCore(...) which does everything that AddRouting(...) does, but then adds the RegexInlineRouteConstraint back in to RouteOptions.

Proposed API

namespace Microsoft.Extensions.DependencyInjection;

public static class RoutingServiceCollectionExtensions
{
+    public static IServiceCollection AddRoutingCore(this IServiceCollection services);
}

Usage Examples

This usage of CreateSlimBuilder() will result in no regex constraint being available:

var builder = WebApplication.CreateSlimBuilder();
var app = builder.Build();
app.MapGet("/products/{productId:regex(...)", (string productId) => {});
app.Run();

... the above code would run, but when a request is made to /products/abcd1234 it would throw an InvalidOperationException. We could consider adding an analyzer which detects when CreateSlimBuilder(...) is being used and detect a route pattern with a regex constraint and tell people that they need to add it (possibly with a fixer).

If you do want to add the regex route constraint back in, this is what it would look like:

var builder = WebApplication.CreateSlimBuilder();
builder.Services.AddRoutingSlim().Configure<RouteOptions>(options => {
    options.SetParameterPolicy<RegexInlineRouteConstraint>("regex");
});

Alternative Designs

Rather than using SetParameterPolicy<T>(string) we considered adding a specific method called AddRegexConstraint(...) but it is somewhat over specific. We've also considered changes to Regex to reduce the impact it has on binary size by eliminating backtracking (saves around 800kb).

We have not fully explored using regex source generator support (with the exception of the alpha constraint). The regex source generator relies on a Regex being defined as a field and then the source generator substituting the initialization of that field with a method that returns the source generated Regex instance. This won't work with the regex constraint because the string is passed around as a parameter at runtime.

Once the request delegate source generator is further along we might find a way to achieve this and we can revisit.

Risks

Could be somewhat confusing as to why we are excluding just the reg-ex constraint.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[mitchdenny]

Related to #46227

[halter73]

@eerhardt @mitchdenny @davidfowl @DamianEdwards Looking at #46227 (comment), it seems like there's disagreement on whether any constraints belong in AddRoutingCore() and/or CreateSlimBuilder(). I think AddRoutingCore() being the exact same as AddRouting() but only without the regex constraint is very arbitrary. Maybe if we called it AddRoutingWithoutRegexContstraint() it'd be more understandable 😆.

Based on discussions in the web frameworks sync today, it seems like we're mostly on board to have AddRoutingCore() not include any constraints by default which would make adding a new AddDefaultRouteConstraints() API desirable. The slim builder could still add all the defaults except the RegexInlineRouteConstraint using internal API if we feel that's important for usability. Would everyone be okay with that?

[mitchdenny]

The slim builder could still add all the defaults except the RegexInlineRouteConstraint using internal API if we feel that's important for usability. Would everyone be okay with that?

Backing up a bit, the whole reason we are doing this is the reduce the binary size when using CreateSlimBuilder(...). The discussion was that it created this weird edge case where we had to explain why people might need to add the regex constraint back in - and why.

To be honest, I'm going a bit cold on removing the other constraints. From a usability perspective I think having to tell people to go hunting for the constraint type names which is something that they don't really need to deal with otherwise seems worse than just having a doc somewhere that says add this one line for regex.

I'm updating the PR now with the proposed changes -- removing all constraints and adding AddDefaultRouteConstraints() but will push it up as a separate PR so we can decide how we feel about each approach.

[mitchdenny]

This PR removes all the constraints:

#46522

[mitchdenny]

Update this issue for completeness as it looks like we going with AddRoutingSlim(...) instead of AddRoutingCore(...).

[davidfowl]

We have Core suffixes in the stack. Slim is not a common suffix and I don’t think we should diverge (I’m also hoping Slim won’t stick on the builder).

• AddMvcCore
• AddIdentityCore
• AddSignalRCore
• AddAuthenticationCore
• AddAuthorizationCore

[halter73]

The reason I like AddRoutingSlim() instead of AddRoutingCore() is because of its relation to CreateSlimBuilder(). Considering CreateSlimBuilder() calls this API, and the main purpose behind both APIs is to be AOT and trim friendly while being as functional as possible out of the box, I think reusing "Slim" here makes a lot of sense. "Core" is pretty meaningless, and we overuse that term.

AddMvcCore() and AddSignalRCore() both claim to add "the minimum essential MVC/SignalR services", and there's nothing minimum or essential about AddRoutingCore(). It's exactly the same as AddRouting without regex constraint support by default.

If we were to remove all constraints, the AddRoutingCore() naming might make more sense.

[mitchdenny]

I've converted the PR that implements this back to AddRoutingCore(...) for now. From our sync this morning I think we were planning to settle on AddRoutingCore for now to avoid spinning our wheels on this (Damian's words).

[halter73]

Agreed. We've spun our wheels enough on this one.

API Review Notes:

• Do we want an overload that takes an Action<RouteOptions> configureOptions parameter like we have for AddRouting()? No. This is an advanced API, and you can always call Configure(Action<RouteOptions>) manually if we care. We can also add this overload later if we change our mind.
• Should AddRoutingCore() remove all constraints instead of just the regex one? No. Regex is the only one that costs substantial size, so we feel removing all constraints makes routing far less usable for little gain other than purity perhaps.
• Since AddRoutingCore() is almost exactly like AddRouting() minus the regex constraint, is it really "core", or is it "slim" like CreateSlimBuilder() which calls it? Should we call it CreateSlimBuilder() instead? No. No one really likes "Slim" as a name, and we've used "Core" a lot to add a subset of the full set of services. This particular subset may be particularly large in this case, but that could change. We're preferring consistency with a bunch of other existing IServiceCollection extension methods rather than highlighting the relationship to CreateSlimBuilder().

API approved as proposed!

namespace Microsoft.Extensions.DependencyInjection;

public static class RoutingServiceCollectionExtensions
{
+    public static IServiceCollection AddRoutingCore(this IServiceCollection services);
}

[mitchdenny]

merged.