CarterCommunity / Carter

Carter is framework that is a thin layer of extension methods and functionality over ASP.NET Core allowing code to be more explicit and most importantly more enjoyable.
MIT License
2.14k stars 177 forks source link

Route clash with parametrized routes -> 405 Method not allowed #128

Closed loftum closed 4 years ago

loftum commented 6 years ago

Consider the following:

public class ThingsModule : CarterModule
{
  public ThingsModule() : base("things")
  {
    Get("/", GetAllThings);
    Get("{type}", GetThing);
    Post("horse", PostHorse); //will not work. GET {type} will be selected, and return 405 Method not allowed
  }

  private Task GetAllThings(HttpRequest req, HttpResponse res, RouteData routeData)
  {
    return res.WriteAsync("Horse, anvil, cucumber");
  }

  private Task PostHorse(HttpRequest req, HttpResponse res, RouteData routeData)
  {
    return res.WriteAsync("Horse saved!");
  }

  private Task GetThing(HttpRequest req, HttpResponse res, RouteData routeData)
  {
    var thing = routeData.As<string>("type");
    return res.WriteAsync($"You asked for {WebUtility.HtmlEncode(thing)}");
  }
}

POST /horse will return 405 Method not allowed, because the GET {type} route is selected. Switching the order of the two routes will result in GET /horse not working, because the POST /horse route returns 405.

It would be nice if Carter could find the first valid route (both correct method and route pattern match) to execute.

jchannon commented 6 years ago

I think this is asp.net core routing issue because it matches {type} which is a catch all route essentially first. By having {anthingcangohere} and horse the former is picked as it's a wildcard. Carter could possibly distinguish better with the method in the routing middleware but it'd still have the same issue if you have two post routes setup with the {type} and horse routes. I'll look into a bit more and see if we can do anything

On 9 August 2018 at 10:05, Hans Olav Loftum notifications@github.com wrote:

Consider the following:

public class ThingsModule : CarterModule { public ThingsModule() : base("things") { Get("/", GetAllThings); Get("{type}", GetThing); Post("horse", PostHorse); //will not work. GET {type} will be selected, and return 405 Method not allowed }

private Task GetAllThings(HttpRequest req, HttpResponse res, RouteData routeData) { return res.WriteAsync("Horse, anvil, cucumber"); }

private Task PostHorse(HttpRequest req, HttpResponse res, RouteData routeData) { return res.WriteAsync("Horse saved!"); }

private Task GetThing(HttpRequest req, HttpResponse res, RouteData routeData) { var thing = routeData.As("type"); return res.WriteAsync($"You asked for {WebUtility.HtmlEncode(thing)}"); } }

POST /horse will return 405 Method not allowed, because the GET {type} route is selected. Switching the order of the two routes will result in GET /horse not working, because the POST /horse route returns 405.

It would be nice if Carter could find the first valid route (both correct method and route pattern match) to execute.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/CarterCommunity/Carter/issues/128, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGapvw-mbNIb-rSQp7vEnczzsS5EuFHks5uO_tCgaJpZM4V1UMX .

loftum commented 6 years ago

Aha, I see.

I looked a little into it. I can see Carter uses IRouteBuilder.MapRoute(string template, RequestDelegate handler) to map routes: https://github.com/CarterCommunity/Carter/blob/master/src/CarterExtensions.cs#L47

But there is also an extension for IRouteBuilder.MapVerb(string verb, string template, RequestDelegate handler) (Microsoft.AspNetCore.Routing 2.1.1.0)

Would that do the trick?

jchannon commented 6 years ago

No I don’t think so because there is also MapGet MapPost etc. Whilst it’d fix your problem if you had 2 post routes registered I think we’d still have the same issue.

Not sure if MVC has the same issue too

On Thu, 9 Aug 2018 at 12:22, Hans Olav Loftum notifications@github.com wrote:

Aha, I see.

I looked a little into it. I can see Carter uses IRouteBuilder.MapRoute(string template, RequestDelegate handler) to map routes:

https://github.com/CarterCommunity/Carter/blob/master/src/CarterExtensions.cs#L47

But there is also an extension for IRouteBuilder.MapVerb(string verb, string template, RequestDelegate handler)

Would that do the trick?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CarterCommunity/Carter/issues/128#issuecomment-411724878, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGapoOZNP_AmpbiiDCbClgG6wiA5ZoAks5uPBt2gaJpZM4V1UMX .

jchannon commented 6 years ago

Can you explain what happened when you switched the route order around?

wastaz commented 5 years ago

Ran into this issue again today, it seems a bit weird cause I would expect it to filter on verb first and on matching route after (or something similar). But if this is how aspnet core routing works then I understand if that is hard to change. Do you know if there is an issue for aspnetcore for this issue?

jchannon commented 5 years ago

I don't know if there is an issue, if you find one please link back

wastaz commented 5 years ago

I experimented today with just doing this instead (in pure aspnetcore without any carter or mvc)

            r.MapVerb("GET", "api/test/{id:long}/monkey", async (request, response, route) =>
            {
                await response.ReturnJsonWithStatusCode(HttpStatusCode.OK, new
                {
                    Id = long.Parse(route.Values["id"].ToString()),
                    Test = "Test1"
                });
            });

            r.MapVerb("PUT", "api/test/{id:long}/monkey", async (request, response, route) =>
            {
                await response.ReturnJsonWithStatusCode(HttpStatusCode.OK, new
                {
                    Id = long.Parse(route.Values["id"].ToString()),
                    Test = "Test2"
                });
            });

In plain aspnetcore it seems like this is matching the correct route. When I call this I get the expected things back depending on verb. Doing a similar thing with carter gives 405 method not allowed.

Am I missing something obvious or wouldnt this mean that switching to using MapVerb would solve this for carter?

jchannon commented 5 years ago

You need to create a verb with a catch all route?

r.MapVerb("GET","{type}", async....

wastaz commented 5 years ago

@jchannon Actually, disregard my latest comment. I did some more tests today and I realized that the problem that I am having is not he same problem as the original one in this issue (should have realized this sooner, sorry). I will create a new issue with a reproduction of that one instead of littering this issue with unrelated things.