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.1k stars 175 forks source link

Method not allowed when having two routes differing only by verb and parameter name #145

Closed wastaz closed 4 years ago

wastaz commented 5 years ago

If I have two routes that differ only by verb and by the name of one or more of the parameters in the route, then it seems like carter will overwrite all routes except the last.

For example, in the module below if you try to call GET /test/42/blah/d9796528-4631-44e2-b898-6caf2706467d then you will get 405 method not allowed. However PUT /test/42/blah/d9796528-4631-44e2-b898-6caf2706467d will work with a 200 OK and the expected response body.

If you rename the other1 and other2 parameter in the route, the both the GET and PUT routes will work as expected.


    public class TestModule : CarterModule
    {
        public TestModule()
        {

            Put("/test/{id:int}/blah/{otherid1:guid}", async (request, response, route) =>
            {
                var id = route.As<int>("id");
                var otherid = route.As<Guid>("otherid1");

                response.StatusCode = (int) HttpStatusCode.OK;
                await response.AsJson(new
                {
                    Id = id,
                    Other = otherid,
                    Method = "Put"
                });
            });

            Get("/test/{id:int}/blah/{otherid2:guid}", async (request, response, route) =>
            {
                var id = route.As<int>("id");
                var otherid = route.As<Guid>("otherid2");

                response.StatusCode = (int) HttpStatusCode.OK;
                await response.AsJson(new
                {
                    Id = id,
                    Other = otherid,
                    Method = "Get"
                });
            });
        }
    }
jchannon commented 5 years ago

It must be failing here somehow if that's the case https://github.com/CarterCommunity/Carter/blob/master/src/CarterExtensions.cs#L73

However I'd expect the tests to fail here for different verbs https://github.com/CarterCommunity/Carter/blob/master/test/Carter.Tests/TestModule.cs#L74 unless with extra route constraints etc it is an issue.

Probably best to write a test to see if it fails

On Thu, 10 Jan 2019 at 07:16, Fred notifications@github.com wrote:

If I have two routes that differ only by verb and by the name of one or more of the parameters in the route, then it seems like carter will overwrite all routes except the last.

For example, in the module below if you try to call GET /test/42/blah/d9796528-4631-44e2-b898-6caf2706467d then you will get 405 method not allowed. However PUT /test/42/blah/d9796528-4631-44e2-b898-6caf2706467d will work with a 200 OK and the expected response body.

If you rename the other1 and other2 parameter in the route, the both the GET and PUT routes will work as expected.

public class TestModule : CarterModule
{
    public TestModule()
    {

        Put("/test/{id:int}/blah/{otherid1:guid}", async (request, response, route) =>
        {
            var id = route.As<int>("id");
            var otherid = route.As<Guid>("otherid1");

            response.StatusCode = (int) HttpStatusCode.OK;
            await response.AsJson(new
            {
                Id = id,
                Other = otherid,
                Method = "Put"
            });
        });

        Get("/test/{id:int}/blah/{otherid2:guid}", async (request, response, route) =>
        {
            var id = route.As<int>("id");
            var otherid = route.As<Guid>("otherid2");

            response.StatusCode = (int) HttpStatusCode.OK;
            await response.AsJson(new
            {
                Id = id,
                Other = otherid,
                Method = "Get"
            });
        });
    }
}

— 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/145, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGapt6VHI90pPFAsPuWNECUa-pf9Ibiks5vBujLgaJpZM4Z46nG .

jchannon commented 5 years ago

Hi, have started looking at this however I saw the opposite of what you said, when they both have the same other1 routes both work. If the GET has other2 and PUT has other1 and I issue a GET I get a 405

jchannon commented 5 years ago

If we split this down to it's basics like so:

  await WebHost.CreateDefaultBuilder(args)
                .Configure(app =>
                {
                    app.UseRouter(r =>
                        {
                            r.MapRoute("/test/{id:int}/blah/{otherid1:guid}",
                                context => context.Response.WriteAsync("PUT"));
                            r.MapRoute("/test/{id:int}/blah/{otherid2:guid}",
                                context => context.Response.WriteAsync("GET"));
                            r.MapRoute("/test/{id:int}/blah/{otherid2:guid}",
                                context => context.Response.WriteAsync("HEAD"));
                        });
                })
                .ConfigureServices(s => s.AddRouting())
                .Build()
                .RunAsync();

Then send a GET to curl localhost:5000/test/42/blah/d9796528-4631-44e2-b898-6caf2706467d -v the body returned is a PUT

This is because the routing middleware doesn't take into account the names of the args I think so the request we sent in matches the first template it finds which is the PUT and returns it. Need to confirm that's the case. Pinging @davidfowl

In Carter the routing middleware matches the PUT template which has the path "/test/{id:int}/blah/{otherid1:guid}" we then assume the routing middleware has selected the right route including the route constraint names (which I dont think it does) and we go and find the handler for it using the full string to compare on and in this case it doesn't have a GET with "/test/{id:int}/blah/{otherid1:guid}" and returns 405

davidfowl commented 5 years ago

cc @JamesNK @rynowak

JamesNK commented 5 years ago

Routing doesn't care about the name when matching. How could it? The parameter name is just the name the route value is given when the route has matched.

IRouter routing doesn't have any built in logic for the HTTP method.

jchannon commented 5 years ago

"IRouter routing doesn't have any built in logic for the HTTP method."

What do you mean here, there are MapVerb and MapPost etc when you use those it doesn't take into account the verb when deciding which route to execute?

rynowak commented 5 years ago

IRouter has a simpler priority system - the routes run in the order you specified them and the first match wins. If your example a few comments up based on IRouter changed the order of the routes to have "HEAD" first, then that's what would be returned.

Since endpoint routing gets data from a variety of sources, we order it ourselves. There are built-in concepts like precedence (based on route template) and some of the policies apply ordering as well (a route with and HTTP method is more specific than one without). There's also an Order value which you can set yourself if you want things to be sequential. We use order to turn MVC conventional routes into endpoints so we can replicate the old behaviour.

rynowak commented 5 years ago

"IRouter routing doesn't have any built in logic for the HTTP method."

What do you mean here, there are MapVerb and MapPost etc when you use those it doesn't take into account the verb when deciding which route to execute?

Yeah @JamesNK is just wrong 😆. Those features aren't used by MVC, but they do exist.

JamesNK commented 5 years ago

Ah, ok. I've never seen the IRouter HTTP method matching.

jchannon commented 5 years ago

I mean I could look to change Carter with the current IRouter implementation although this issue is a bit obscure. Are the asp.net core 3.0 endpoint routing packages ready to be beta tested for 3rd party frameworks? If so I ( we? ;) ) could look to move Carter over to endpoint routing which might address this issue at the same time

rynowak commented 5 years ago

I think we'd be super interested in your feedback. The major changes to E/R in 3.0 from us are going to be fixing a few functional bugs, and redoing the startup experience (which I think is a no-op for you). The core routing engine in 3.0 doesn't really have much planned work.

jchannon commented 5 years ago

I've raised something with ER on one of the aspnet repos and discussed it with @JamesNK but now I can't find it (it was a PR he did for a sample third party fwk you guys have I think to test ER) hopefully it won't be much of a change for Carter but if you could point me in the right direction for package feeds etc I could start to look to move it over

JamesNK commented 5 years ago

https://github.com/davidfowl/Web.Framework/pull/6

It's a little crusty. I should update it for preview 1. Names might be slightly different but the concepts haven't changed.

JamesNK commented 5 years ago

https://github.com/aspnet/AspNetCore/issues/4221

jchannon commented 5 years ago

Ah brill!

Thanks for the help, pointers and transparency

jchannon commented 5 years ago

Just out of interest is there a list of routes eg verbs and templates in the routing middleware or part of the httpcontext that is accessible currently so I can see what routes the fwk knows about?

jchannon commented 5 years ago

Guess this is it?https://stackoverflow.com/a/50696890

jchannon commented 5 years ago

Or easier https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.routing.routebuilder.routes

jchannon commented 5 years ago

@wastaz @JamesNK @rynowak this issue can be closed once ASP.NET Core 3 is released. I have moved Carter to ASP.NET Core 3 on this branch https://github.com/CarterCommunity/Carter/pull/149 and the work there resolves this issue.

@JamesNK @rynowak it seemed relatively straight forward to move to the new routing I feel I have missed something! Are there planned blog posts etc on what else can be done with the new routing? One thing I noticed in the nightlies was a UseEndpoint() which I thought I needed but didn't but also couldn't really see what it did. Just wondering if I could improve Carter more but don't know what else the routing brings, like I don't really understand the datasources is and the params object[] metadata arguments are but thanks for the all the help so far.

This was also tweeted: https://twitter.com/CarterLibs/status/1085248206168969216

JamesNK commented 5 years ago

UseEndpoint is middleware to explicitly run the matched endpoint. The matched endpoint gets implicitly run at the end the middleware pipeline so you don't need it.

Your PR looks good to me.

Your routes don't need a datasource because they don't change during runtime.

Metadata is stored in Endpoint.Metadata and can be used by systems that use endpoints. For example, MapGet automatically adds IHttpMethodMetadata to the endpoint with the GET verb. The matcher then uses that metadata to do HTTP matching.

rynowak commented 5 years ago

Are there planned blog posts etc on what else can be done with the new routing?

So much this. We plan to talk about it in more detail once the details about how to wire it up are further along. Based on current direction, I wouldn't expect much significant change.

abazanov commented 5 years ago

Guys, I don't want to raise a new issue because it might just be me, but I am getting the same response 405 Method Not Allowed when I try...

Get<FindSomeObjectMeta>("/{q}", async (ctx) => ...

Where q is just a string and then I call [route]/?q=whatever


EDIT

I went for Get<FindSomeObjectMeta>("/find/{q}", async (ctx) => ... and it works because it does not conflict with the base Get. Maybe will stick with this.