dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.12k stars 9.91k forks source link

Add ambiguity breaker (hook) in DefaultEndpointSelector #49547

Closed krystalgamer closed 1 year ago

krystalgamer commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

I have two controllers with exact same routes (can't change them due to business reasons). Instead of getting a AmbiguousMatchException, I'd like to be able to pick a candidate.

Describe the solution you'd like

Create a AmbiguityBreaker which receives a CandidateSet and then has to return at most a candidate.

DefaultEndpointSelector would only call into it (if it exists), in case of an ambuity. Additionally, it'd be great if it supports injecting services that could aid on resolving the ambiguity.

Additional context

No response

davidfowl commented 1 year ago

You can add a MatcherPolicy that implements IEndpointSelectorPolicy to determine which endpoints get selected. Here's a not a great example:

using Microsoft.AspNetCore.Routing.Matching;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddSingleton<MatcherPolicy, SelectTheRighetOne>();

var app = builder.Build();

app.MapGet("/clash", (string name) => $"Hello endpoint1");
app.MapGet("/clash", (string name) => $"Hello endpoint2");

app.Run();

class SelectTheRighetOne : MatcherPolicy, IEndpointSelectorPolicy
{
    public override int Order => 100;

    public bool AppliesToEndpoints(IReadOnlyList<Endpoint> endpoints)
    {
        return endpoints.Any(e => e is RouteEndpoint);
    }

    public Task ApplyAsync(HttpContext httpContext, CandidateSet candidates)
    {
        if (candidates.Count == 1)
        {
            return Task.CompletedTask;
        }

        // TODO: Select the right one
        candidates.SetValidity(0, false);

        return Task.CompletedTask;
    }
}

Consider writing something that is more robust that can pinpoint specific routes based on metadata (or it'll apply to all endpoints).

javiercn commented 1 year ago

@krystalgamer thanks for contacting us.

Can you not give one of the endpoints a lower order? (higher Order number in code). That's the easiest way to disambiguate, if you have some specific criteria you can use to disambiguate, you can write a matcher policy for it.

The example @davidfowl provided is a good start, but I would suggest you use some metadata to decide when you need to apply the policy, otherwise (as David correctly points out) it will apply to all your endpoints, which is not what you want.

javiercn commented 1 year ago

I have two controllers with exact same routes (can't change them due to business reasons). Instead of getting a AmbiguousMatchException, I'd like to be able to pick a candidate.

When you say this, do you mean you can't change the route, or that you can't change the code at all. You could affect the order of one of the routes without changing the other.

ghost commented 1 year ago

Hi @krystalgamer. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

krystalgamer commented 1 year ago

@davidfowl thanks for the suggestion, seems the most promising so far. @javiercn can't change the route. My intention is for this to be dynamic, flight some users to use one instead of the other. Priority would be compile time :/ Even though the match policy needs to be singleton through the context I can resolve services. Even though, not the best solution, it's way better than nothing.

javiercn commented 1 year ago

@krystalgamer thanks for the additional details. MatcherPolicies are exactly designed for that type of scenario, so we suggest you go that route. We don't plan to add an additional extensibility point because there is already one for this purpose.

ghost commented 1 year ago

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.