[Routing] A route requirements failure is overridden by a method failure #22739

origaminal · 2017-05-18T02:38:09Z

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no
Symfony version	4.0.0

I faced with the error that when you have routes with same paths, different methods and one of the routes has a condition. If the condition will fail during matching, UrlMatcher will throw MethodNotAllowedException with an incorrect set of allowed methods while it seems supposed to throw ResourceNotFoundException on a failed condition.

This test can reproduce the issue:

    /**
     * @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
     */
    public function testRequirementFailureIsNotOverridenWithMethodFailure()
    {
        $coll = new RouteCollection();
        $coll->add('foo1', new Route('/foo', array(), array(), array(), '', array(), array('get')));
        $coll->add('foo2', new Route('/foo', array(), array(), array(), '', array('https'), array('post')));

        $matcher = new UrlMatcher($coll, new RequestContext('', 'POST'));
        $matcher->match('/foo');
    }

The solution for UrlMatcher should not be a hard task but there is also the matcher produced by PhpMatcherDumper which logic seems inverted towards UrlMatcher's logic (i.e.: the condition is checked before the method).

The text was updated successfully, but these errors were encountered:

ogizanagi · 2017-05-18T21:53:36Z

I don't think it's a bug. Considering the test you wrote, the behavior looks legit to me:
A client tries to reach the /foo resource which does only accept GET, so a POST is a Method Not Allowed (405), not a Not Found (404). The resource still exists at /foo URI, whether the client used http or https, so it's not a 404.

origaminal · 2017-05-19T00:24:14Z

Maybe array('post') of the foo2 route evaded from your sight cause it is placed beyond the viewport. But it is there. And therefore GET (get a resource representation) and POST (send data to be handled by a resource) are both applicable to the /foo resource. So MethodNotAllowed is untruth when returned in a response to the POST /foo request.

Btw, https is used just as the simplest way to provide Route with a condition. request.getContentType() == 'json' could be a condition instead, for example.

NotFoundResource also seems not the optimal response for a condition failure. BadRequest (400) seems more correct.

ogizanagi · 2017-05-19T06:32:41Z

Maybe array('post') of the foo2 route evaded from your sight cause it is placed beyond the viewport. But it is there. And therefore GET (get a resource representation) and POST (send data to be handled by a resource) are both applicable to the /foo resource.

I did see it actually: the /foo resource does only accept POST using https, but not using http. Still a 405 to me.

Btw, https is used just as the simplest way to provide Route with a condition. request.getContentType() == 'json' could be a condition instead, for example.

It may seem less legit with this example, but it entirely depends what you do inside your condition and what you assume it should return. For instance, for api versioning, I might use something like:

@Route("/foo", methods={"GET"})
@Route("/foo", methods={"POST"}, condition="1.1 >= request.attributes.get('api_version')")

The /foo resource still exists, but POST is only available since v1.1. As a v1.0 client using any HTTP verb else than GET, I'd expect a 405 Method Not Allowed.

So in most of the cases, I'd say MethodNotAllowed is the most appropriated response when using the methods attribute (only my opinion, let's see). I don't really see how the url matcher can assume anything else for you to determine it should rather be a 404, unless finding a simple way to indicate what to return on condition failure.

origaminal · 2017-05-20T00:02:17Z

A resource is identified by a URI:

Each resource is identified by a Uniform Resource Identifier (URI)
https://tools.ietf.org/html/rfc7231#section-2

So if we take (https was a bad example as it is a part of URI and http/https resources should be considered as different ones)

    public function testRequirementFailureIsNotOverridenWithMethodFailure()
    {
        $coll = new RouteCollection();
        $coll->add('foo1', new Route('/foo', array(), array(), array(), '', array(), array('get')));
        $coll->add('foo2', new Route('/foo', array(), array(), array(), '', array(), array('post'), 'equest.getContentType() == 'json''));

        $matcher = new UrlMatcher($coll, new RequestContext('', 'POST'));
        $matcher->match('/foo');
    }

Both MethodNotAllowed and ResourceNotFound are bad choices. Just I find MethodNotAllowed more confusing.

In case of you example the current behavior will work out correctly. Its semantics is clear and MethodNotAllowed is a legit response. In terms of HTTP it will be interpreted that /1.0/foo and /1.1/foo are different resources with different sets of allowed methods.

So seems that the both behaviors (current one or ResourceNotFound one) will fail under some circumstances.

For me BadRequest seems a better solution in case of a condition failure. My assumption is that most of the time a condition will test topics beyond ResourceNotFound (a URI matched) and MethodNotAllowed (a method is matched), like whether a request format if json, is it a xhr request, etc.

ogizanagi · 2017-05-20T07:49:32Z

For me BadResponse seems a better solution in case of a condition failure. My assumption is that most of the time a condition will test topics beyond ResourceNotFound (a URI matched) and MethodNotAllowed (a method is matched), like whether a request format if json, is it a xhr request, etc.

You're right. A BadRequest is the appropriated response for such checks. Only the thing is it's not really the routing component / url matcher's responsibility to validate the request format. It only tries to find a match, based on the URI, method used, and some excluding conditions. Hence it only deals with MethodNotAllowed and ResourceNotFound exceptions (both have a significance for a routing component).

I believe when you're using routes with such conditions on the request format and expect a proper BadRequest response, you're acting beyond the routing scope, and accept it'll only allow the router to exclude matching routes under come circumstances.
But if you do want a proper request format validation, you need to set your own in a listener or in your controller in order to send the appropriated response.

ro0NL · 2017-05-20T07:54:45Z

What about something like InvalidConditionException => BadRequestException?

origaminal · 2017-05-20T19:42:23Z

UrlMatcher implements Symfony\Component\Routing\Matcher\RequestMatcherInterface which name semantics and PhpDoc description

Tries to match a request with a set of routes.

imply that a request is what matched through this service. Therefore my assumption is that tests are not scopes by URI, method and some excluding conditions, the testing of request headers supposed to be legal.

The example in the docs seems like a confirmation:

condition: "context.getMethod() in ['GET', 'HEAD'] and request.headers.get('User-Agent') matches '/firefox/i'"

And if a testing of user-agent, content-type, xhr attributes is legal then BadRequest seems as a proper solution to indicate a condition failure.

nicolas-grekas · 2018-02-23T15:54:56Z

Unless I misunderstood the issue, this has been fixed in #26100

…match (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [Routing] Don't throw 405 when scheme requirement doesn't match | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22739 | License | MIT | Doc PR | - Commits ------- 9d70ef0 [Routing] Don't throw 405 when scheme requirement doesn't match

xabbuh added Bug Routing labels May 18, 2017

carsonbot added the Status: Needs Review label May 18, 2017

nicolas-grekas closed this as completed Feb 23, 2018

This was referenced Feb 25, 2018

[Routing] Don't throw 405 when scheme requirement doesn't match #26312

Merged

[Routing] support scheme requirement without redirectable dumped matcher #26304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Routing] A route requirements failure is overridden by a method failure #22739

[Routing] A route requirements failure is overridden by a method failure #22739

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Routing] A route requirements failure is overridden by a method failure #22739

[Routing] A route requirements failure is overridden by a method failure #22739

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!