`HttpController`'s `ADD_METHOD_TO` breaks in some cases

[Mis1eader-dev]

Describe the bug I have a few HttpControllers and HttpSimpleControllers mixed together, and noticed today that only one of the HttpControllers was returning status 400, which is a status code I use for incorrect input in the URL. I went exactly to the controller in question and added a print statement to check if it was the one getting called, and it turns out it isn't. I then prefixed its URI with something that differs from all the other controllers and then I hit the new URI, and it returned a status 200. I went to another controller which has about the same structure but has a constant in its URI, and put a print statement and saw it had been hitting that incorrect API endpoint.

To Reproduce Couldn't reproduce it within a small project, here's how the two API endpoints in question roughly look like:

class Get : public HttpController<Get>
{
public:
    void asyncHandleHttpRequest(
        const HttpRequestPtr& req,
        std::function<void (const HttpResponsePtr&)>&& callback,
        string&& city,
        string&& id
    );

    METHOD_LIST_BEGIN
        ADD_METHOD_TO(
            Get::asyncHandleHttpRequest,
            "/api/clients/{client-city}/{client-id}",
            HttpMethod::Get,
        );
    METHOD_LIST_END
};

class Get2 : public HttpController<Get2>
{
public:
    void asyncHandleHttpRequest(
        const HttpRequestPtr& req,
        std::function<void (const HttpResponsePtr&)>&& callback,
        string&& id
    );

    METHOD_LIST_BEGIN
        ADD_METHOD_TO(
            Get2::asyncHandleHttpRequest,
            "/api/clients/users/{user-id}",
            HttpMethod::Get,
        );
    METHOD_LIST_END
};

Here the API endpoint that overshadowed the other was Get.

Will see if I can make a reproduceable example instead once I have time.

Expected behavior Should have hit the API endpoint that looks closest to the actual URL.

[Mis1eader-dev]

Turns out it's because there were two HttpSimpleController APIs, one with no query params, one with query params, and the one was getting hit that required a query params.

[Mis1eader-dev]

Actually I thought it was another API causing this, but the API in question is still not working unless I change the prefix of the API endpoint, here is the API that is not working:

METHOD_LIST_BEGIN
    ADD_METHOD_TO(
        GetSelf::asyncHandleHttpRequest,
        "/api/clients/device-groups/{device-group-id}",
        API_LOGGED_IN_FILTERS,
        HttpMethod::Get,
    );
METHOD_LIST_END

And here is the API that overshadows the above API:

METHOD_LIST_BEGIN
    ADD_METHOD_TO(
        Get::asyncHandleHttpRequest,
        "/api/clients/{user-group-city}/{user-group-id}",
        API_LOGGED_IN_FILTERS,
        HttpMethod::Get,
    );
METHOD_LIST_END

[Mis1eader-dev]

I believe Drogon is picking the 2nd one greedily, not knowing there is another API that matches the prefix closer to the actual req->path()

[Mis1eader-dev]

The plugin I wrote in this PR is not greedy and picks the one that matches the closest to the actual path, we can improve upon it and use it as both a matcher and redirector built into Drogon

@an-tao what is your opinion on this? Or do we just make the current algorithm not greedy?

[Mis1eader-dev]

New findings, if I change the not working API to this:

"/api/clientss/device-groups/{device-group-id}"

Note the double 's' in clients, it works.

However if I do this:

"/api/clients/device-groupss/{device-group-id}"

Note the double 's' in device-groups, then it doesn't work.

There is something strange happening with the current algorithm.

I also tried increasing and decreasing the length of the string to see if the algorithm breaks these two APIs apart, but it didn't work for either case:

"/api/clients/device-groups/{device-group-id-longer}"

"/api/clients/device-groups/{device}"

[an-tao]

The plugin I wrote in this PR is not greedy and picks the one that matches the closest to the actual path, we can improve upon it and use it as both a matcher and redirector built into Drogon

@an-tao what is your opinion on this? Or do we just make the current algorithm not greedy?

I just feel that the logic of this redirector solution is too complex for users—it's even confusing me. And some other developers have expressed similar views, so I put it on hold.

[Mis1eader-dev]

@an-tao the PR aside, what is the plan for this bug in method matchers? Do I need to provide an example branch that replicates the bug?

[an-tao]

@an-tao the PR aside, what is the plan for this bug in method matchers? Do I need to provide an example branch that replicates the bug?

Yes, please, thanks.

[Mis1eader-dev]

@an-tao this is a reproduceable example of the bug.

The G controller overshadows the D controller.

[Mis1eader-dev]

Turns out all the other controllers aren't necessary to trigger this bug. See this commit which produces the same bug.

Strangely enough, swapping the order of their declarations will make the bug go away.