POST route + custom placeholder pattern is being overridden by GET route + default placeholder pattern

[msheakoski]

In the code below, the GET route is overriding the POST route whenever I use a custom pattern. However, if I try the same POST route with the default pattern, it does correctly match.

<?php

require_once './vendor/autoload.php';

$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\RouteCollector $r) {
    $r->addRoute('GET',  '/user/{name}', 'GET with default placeholder pattern');
    $r->addRoute('POST', '/user/{name:[a-z]+}', 'POST with custom placeholder pattern');

    // This route correctly matches if uncommented
    // $r->addRoute('POST', '/user/{name}', 'POST with default placeholder pattern');
});

function handle($routeInfo) {
    print_r($routeInfo);
    echo "\n";
}

handle($dispatcher->dispatch('GET', '/user/bob'));
handle($dispatcher->dispatch('POST', '/user/fred')); // does not work

[msheakoski]

I created some more test cases to better illustrate what is going on:

<?php

require_once './vendor/autoload.php';

function testRoute($dispatcher, $method, $path) {
    echo "  $method $path:", "\n  ";

    $routeInfo = $dispatcher->dispatch($method, $path);
    switch ($routeInfo[0]) {
        case FastRoute\Dispatcher::NOT_FOUND:
            echo 'NOT FOUND';
            break;
        case FastRoute\Dispatcher::METHOD_NOT_ALLOWED:
            $allowedMethods = implode(', ', $routeInfo[1]);
            echo "METHOD NOT ALLOWED (Allowed methods are $allowedMethods)";
            break;
        case FastRoute\Dispatcher::FOUND:
            echo "matched '{$routeInfo[1]}'";
            break;
    }

    echo "\n\n";
}


echo <<<EOT
*** Test #1: expected behavior
*** POST route with default placeholder pattern correctly matches \n\n
EOT;
$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\RouteCollector $r) {
    $r->addRoute('GET',  '/user/{name}', 'GET with default placeholder pattern');
    $r->addRoute('POST', '/user/{name}', 'POST with default placeholder pattern');
});
testRoute($dispatcher, 'POST', '/user/fred');


echo <<<EOT
*** Test #2: unexpected behavior
*** GET route incorrectly takes priority over the POST route with custom
    placeholder pattern \n\n
EOT;
$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\RouteCollector $r) {
    $r->addRoute('GET',  '/user/{name}', 'GET with default placeholder pattern');
    $r->addRoute('POST', '/user/{name:[a-z]+}', 'POST with custom placeholder pattern');
});
testRoute($dispatcher, 'POST', '/user/fred');


echo <<<EOT
*** Test #3: unexpected behavior
*** POST route with custom placeholder pattern incorrectly takes priority over
    the GET route \n\n
EOT;
$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\RouteCollector $r) {
    $r->addRoute('POST', '/user/{name:[a-z]+}', 'POST with custom placeholder pattern');
    $r->addRoute('GET',  '/user/{name}', 'GET with default placeholder pattern');
});
testRoute($dispatcher, 'GET', '/user/bob');


echo <<<EOT
*** Test #4: unexpected behavior
*** POST route with custom placeholder pattern route is incorrectly ignored when
    GET comes before it, instead matching POST with default placeholder pattern \n\n
EOT;
$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\RouteCollector $r) {
    $r->addRoute('GET',  '/user/{name}', 'GET with default placeholder pattern');
    $r->addRoute('POST', '/user/{name:[a-z]+}', 'POST with custom placeholder pattern');
    $r->addRoute('POST', '/user/{name}', 'POST with default placeholder pattern');
});
testRoute($dispatcher, 'POST', '/user/fred');


echo <<<EOT
*** Test #5: expected behavior
*** POST with custom placeholder pattern route matches when GET comes after it \n\n
EOT;
$dispatcher = FastRoute\simpleDispatcher(function(FastRoute\RouteCollector $r) {
    $r->addRoute('POST', '/user/{name:[a-z]+}', 'POST with custom placeholder pattern');
    $r->addRoute('GET',  '/user/{name}', 'GET with default placeholder pattern');
});
testRoute($dispatcher, 'POST', '/user/fred');

Output:

*** Test #1: expected behavior
*** POST route with default placeholder pattern correctly matches 

  POST /user/fred:
  matched 'POST with default placeholder pattern'

*** Test #2: unexpected behavior
*** GET route incorrectly takes priority over the POST route with custom
    placeholder pattern 

  POST /user/fred:
  METHOD NOT ALLOWED (Allowed methods are GET)

*** Test #3: unexpected behavior
*** POST route with custom placeholder pattern incorrectly takes priority over
    the GET route 

  GET /user/bob:
  METHOD NOT ALLOWED (Allowed methods are POST)

*** Test #4: unexpected behavior
*** POST route with custom placeholder pattern route is incorrectly ignored when
    GET comes before it, instead matching POST with default placeholder pattern 

  POST /user/fred:
  matched 'POST with default placeholder pattern'

*** Test #5: expected behavior
*** POST with custom placeholder pattern route matches when GET comes after it 

  POST /user/fred:
  matched 'POST with custom placeholder pattern'

[nikic]

Wow, this looks like a pretty serious issue. Currently the router first finds a matching route and then checks if it is allowed for this method. As your example shows, this doesn't work correctly if there are two routes for different methods, where one matches a subset of the other.

So, just a brain dump on how we might fix this: The first approach would be to not stop at the first matched route if the method is not allowed - instead remember the allowed methods and continue doing further matches. If we find a route that matches and is allowed later on, we use it. Otherwise return METHOD_NOT_ALLOWED.

At which point another question comes up: What is the "allowed method list" supposed to look like in this case? Currently it's the allowed methods for the first matched route, but probably it should be the union of the allowed methods of all matching routes.

But I don't think it's actually possible to implement this approach - the way routes are matched is designed to always provide the first matching route, skipping a route during matching would probably be rather complicated and/or slow.

The alternative is to invert the matching order: Have separate route maps for different methods and only match the routes for the given method (with HEAD fallback as usual). If a route matches, it will be the right one for this method.

However it is less clear what should be done if there is no match. To distinguish between a "method not allowed" and a "not found" and to determine the allowed methods list would, at least naively, require matching against the routes of all other methods as well, which does not sound terribly efficient.

On the other hand, the performance of the error case usually isn't the important bit, so maybe that's quite okay.

[msheakoski]

I'm still trying to get a better understanding of the inner workings of FastRoute, but you mentioned METHOD_NOT_ALLOWED possibly playing a role. I always wondered what the purpose of this value being returned is, because as an end user of the library, aren't we only concerned with: "did the route match? yes or no"? To me, a matching path but non-matching method would simply be considered a NOT_FOUND, and I think that's also the convention of other popular routers such as the one in Rails.

What is the use case for having METHOD_NOT_ALLOWED (aside from providing a more informative error message) and if it was removed, would that help to solve this issue?

[nikic]

@msheakoski The METHOD_NOT_ALLOWED state allows you to send a 405 Method Not Allowed response (and the allowed method list is used for the Allow header that is required in this case) instead of a generic 404 Not Found.

[msheakoski]

Regarding my question on the usefulness of METHOD_NOT_ALLOWED, I was actually wrong in my assumption about Rails. Both FastRoute and Rails follow the popular convention of responding with 405. I originally thought that a resource is the combination of method+path, but it is actually just the path, with the method being a constraint. I learned something new!

[mruz]

I can confirm this issue.

[nikic]

Should be fixed by 5b16723.

[msheakoski]

Thank you, @nikic! This looks great! Do you plan on bumping the version as well for composer users?

[nikic]

Done: https://github.com/nikic/FastRoute/releases/tag/v0.3.0, https://packagist.org/packages/nikic/fast-route