Skip to content

Exclude regex and alpha constraints when SlimBuilder is used. #46227

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

Merged
merged 40 commits into from
Feb 17, 2023

Conversation

mitchdenny
Copy link
Member

@mitchdenny mitchdenny commented Jan 23, 2023

Background and Motivation

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

Proposed API

namespace Microsoft.Extensions.DependencyInjection;

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

Usage Examples

We will use CreateSlimBuilder(...) to call AddRoutingOptions(...). See PR contents for an example.

Alternative Designs

This PR currently includes an AddRegexConstraint(...) method on the PR, this will likely be removed in favor of just adding the RegexRouteConstraint via the expose dictionary.

Risks

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

@mitchdenny
Copy link
Member Author

Bunch of changes to make unit tests pass. Still need to do some more testing to see if this breaks any typical usage scenarios. I'm feeling great about this change at the moment - I think it carries some risk.

@davidfowl
Copy link
Member

davidfowl commented Jan 25, 2023

We shouldn't change what AddRouting does. The Slim builder should call a different API that removes these constraints. None of the tests should require updating and a new test that uses the new API should be added.

@JamesNK
Copy link
Member

JamesNK commented Jan 26, 2023

What's the error message presented to the user if someone uses the "slim" set of constraints but references regex or alpha in a route? I'm guessing it is something like "No constraint registered for 'regex'."

Here is a crazy idea: The slim set of constraints register a custom IRouteConstraint implementation for regex and alpha. The implementation does nothing but throw an exception in the constructor. The constructor error message provides information about which API someone needs to call to include the real regex and alpha implementations. When real regex and alpha implementations are included, they overwrite the error-throwing one.

The error message could be something like

Unable to create a route that uses the 'regex' constraint. The slim builder does not register this constraint by default. To use this constraint, configure routing and call AddDefaultConstraints().

A custom error can make it very obvious to users what is going on and how to fix their app.

@davidfowl
Copy link
Member

What's the error message presented to the user if someone uses the "slim" set of constraints but references regex or alpha in a route? I'm guessing it is something like "No constraint registered for 'regex'."

See this

Here is a crazy idea: The slim set of constraints register a custom IRouteConstraint implementation for regex and alpha. The implementation does nothing but throw an exception in the constructor. The constructor error message provides information about which API someone needs to call to include the real regex and alpha implementations. When real regex and alpha implementations are included, they overwrite the error-throwing one.

This isn't crazy, it's good 😄. What would be crazy is if we could just make it work.

@mitchdenny mitchdenny force-pushed the exclude-regex-alpha-on-slim-builder branch from 0b9fe16 to a7d32fd Compare January 30, 2023 03:58
@davidfowl
Copy link
Member

We're already calling UseRouting here:

services.AddRouting();

BTW @eerhardt this method name is very confusing:

internal static void UseKestrel(IWebHostBuilder builder)

It does more than adding Kestrel, we should rename it.

@mitchdenny
Copy link
Member Author

Aside from some renames and perhaps alternative placement of AddRouting(...) for the non-slim builder scenario I think this does a pretty good job of avoiding breaking changes except for when someone goes and creates RouteOptions themselves and avoids using either of the two builders.

@mitchdenny mitchdenny marked this pull request as ready for review January 31, 2023 05:02
@mitchdenny mitchdenny requested a review from davidfowl January 31, 2023 10:04
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs unit tests of what happens when a regex constraint is used in a slim builder app - both with and without registering the regex constraint.

Also, did what are your thoughts on providing a friendly error message like I described here - #46227 (comment)

@mitchdenny mitchdenny requested a review from davidfowl February 10, 2023 05:06
@mitchdenny mitchdenny requested a review from a team as a code owner February 17, 2023 02:07
@mitchdenny mitchdenny enabled auto-merge (squash) February 17, 2023 02:27
@mitchdenny mitchdenny disabled auto-merge February 17, 2023 02:27
@mitchdenny mitchdenny merged commit bb3f0d6 into dotnet:main Feb 17, 2023
@ghost ghost added this to the 8.0-preview2 milestone Feb 17, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants