Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize route matching for status codes #1873

Closed
awwright opened this issue Aug 11, 2015 · 6 comments
Closed

Generalize route matching for status codes #1873

awwright opened this issue Aug 11, 2015 · 6 comments

Comments

@awwright
Copy link

Pyramid's route handler currently allows matching on various properties of the request, like request-line and headers.

However, it always uses 404 to indicate no route has been matched. 404 has a very particular meaning in HTTP (the resource does not exist), whereas it's just one way a route can fail to match in Pyramid (e.g. the resource might exist, but the method is incorrect). Choosing 404 is just as arbitrary as sending back 401, 403, or 405. If route matching is indeed generic, it's inappropriate and unfair (to automated user-agents consuming an API, in particular) to assume that 404 is going to be an appropriate default.

Therefore, Pyramid's ability to match routes based on arbitrary values (like the request method) is either (1) insufficently generalized, as there is only one way to fail and mismatch here will not trigger the correct error code (e.g. 415 to indicate the resource was defined, but has no view because of an incorrect method); or (2) routes should only be used for URI matching, and handling anything else is the responsibility of the view.

The documentation suggests that the intended purpose is the former:

add_route
request_method

A string representing an HTTP method name, e.g. GET, POST, HEAD, DELETE, PUT or a tuple of elements containing HTTP method names. If this argument is not specified, this route will match if the request has any request method. If this predicate returns False, route matching continues.

This is written as if I should be able to use this to separate my PUT vs. GET views. Suppose I implement an API and implements routes and views for just PUT /item/{id}. I instruct a bot to update the resource, so it GETs the resource to obtain an ETag (intended for use with the If-Match header). Since this request returns 404, it assumes the resource doesn't exist and fails. If 405 were returned instead, it would be able to at least make a dangerous PUT, updating the resource without knowing its current contents.

If the intended purpose of these arguments is to allow Pyramid to be flexible for the sake of flexibility, but shouldn't ordinarily be used; then arguments such as accept and request_method should not exist at all, but just a generic callback for filtering, or an explanation that compliant HTTP behavior is implemented after the route step.

Otherwise, Pyramid must be responsible for returning the correct status code.

Examples of expected behavior (typically, first matching error from this list will be returned):

  1. PUT over a resource that does not exist, and no view for creating resources exists → 403 Forbidden
  2. PUT over a resource that does not exist, but the user is not authorized to create resources → 401 Not Authorized
  3. No route with that resource was defined → 404 Not Found
  4. The resource is defined, but no view to handle POST is defined on it → 405 Method Not Allowed
  5. POST to a resource that doesn't know how to handle the supplied Content-Type → 415 Unsupported Media Type
  6. Server doesn't know how to brew that kind of beverage → 418 I'm A Teapot
  7. Server doesn't know how to reply with any media type specified by the Accept request-header → 406 Not Acceptable

Note how 404 isn't even the first or last item in the list. It seems like it's the default just because it's a pretty common error; or someone thought 404 Not Found means "no matching Pyramid route" instead of "no such resource" (an entirely different concept). If this is the case, then the default error should be 400, the generic client error.

Solutions to this issue:

  1. Ability to define routes so they can fail in a certain way
  2. Default error should be 400, not 404
  3. Correct documentation so invalid HTTP behavior isn't being taught as the default
  4. In general, Pyramid should not be making invalid HTTP behavior cheaper than valid behavior. Valid behavior should be cheap and default.

Related issues: #1526 #1573 #1261

@digitalresistor
Copy link
Member

This is a known issue, and one that I don't think is going to get fixed. Generally you don't place restrictions on the route in Pyramid, instead you place them on your view. And Pyramid doesn't know that a predicate (which is what accept/request_method is) doesn't match and thus a view didn't match because of that, or due to for example a missing variable because it simply skips over that particular view because of it's filtering.

At the end of looking for a view that matches it raises a NotFound because there wasn't a view that was found for that particular request.

@awwright
Copy link
Author

I'd figured that if this were a known problem, then there would be an issue for it :p

It seems like the solution to the fundamental problem involves changing the status code based on partial matches. If a URI matched, then it's not a 404 that's going to be returned. And so on. Something like:

  1. No route matched by request-URI and request-method is not PUT? 404
  2. No route matched by request-URI and method? 405
  3. No route matched by all of above and request Content-Type? 415
  4. No route matched by all of above and accept? 406
  5. etc
  6. No route matched by all predicates? 400

But this also impacts the documentation. The assertion that

Generally you don't place restrictions on the route in Pyramid, instead you place them on your view

Is completely at odds with what the documentation currently suggests to do.

@digitalresistor
Copy link
Member

Except that nothing in Pyramid requires the concept of a URI. You can match for example on resources based upon parts of the URI, or even none of it.

You are going to have to be more specific on where it is at odds with the documentation.

You CAN place accept/request on an config.add_route(), but in most cases you would have those in your view configuration (i.e. config.add_view() or @view_config()).

@awwright
Copy link
Author

If there's a route that performs no matching against the request URI, then point (1) above will always be skipped. There's no additional requirements being imposed on users. So what are you referring to, where I might have implied something "requires the concept of a URI"?

I quoted a sample of the documentation for add_route. I would venture to guess since it exists, that's a good place to implement different logic by method, e.g. one for GET, one for PUT. If that's not an appropriate place to implement such a switch, it should say so.

@digitalresistor
Copy link
Member

It's just ONE place to implement different logic by method, it is not the ONLY place. You can write an entire Pyramid application without ever calling add_route().

All of the logic can for example be placed on views. At that point it becomes difficult if not impossible to implement the aforementioned rules you posted.

For example:

Let's start at number 2, number 1 is almost the current case of what we have:

The response MUST include an Allow header containing a list of valid methods for the requested resource.

Pyramid DOESN'T know what all the valid methods are for the requested resource, it is entirely possible that no such resource even exists in which case a 404 is much more preferable, even for a PUT request. Even if a resource exists it is entirely possible that for example 10+ views match, but they are all disqualified for other reasons (such as not having the proper query parameters) and unfortunately there is no way to differentiate between a predicate mismatch due to an accept/method versus a user defined predicate.

Number 3:

Returning of a 415 is not valid either, since Pyramid can't know for example that a view with a different content-type would have succeeded, because Pyramid doesn't know that there may be other views that could have matched if the content type matches.

Number 4:

Unless it was a HEAD request, the response SHOULD include an entity containing a list of available entity characteristics and location(s) from which the user or user agent can choose the one most appropriate.

Once again Pyramid can't return that information, for a given resource/location there may not even be a valid answer.

@mmerickel
Copy link
Member

Closing this as a dup of #1526.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants