Vertx-web: Failed Content Negotiation Should Return 415

[AshtonKem]

Version

• vert.x core: 3.3.3
• vert.x web: 3.3.3
  

  Context

Route supports declaring routes by Content-Type via the consumes method, which is then used to match against a request's Content-Type. If a request matches a path but has the wrong Content-Type for a given handler it will not route.

Unfortunately if a request matches the path of one or more routes but has the wrong Content-Type, Vertx by default returns a 404. The more correct response would be a 415, because the route exists but the provided resource was not acceptable to the server.

Do you have a reproducer?

The following router snippet is sufficient to demonstrate the problem.

router.post("/foo").consumes("application/xml").blockingHandler(new FakeHandler());

Curling "/foo" against this server with Content-Type: application/json would return a 404, not a 415.

Steps to reproduce

1. Create route with consumes tag
2. Curl route with wrong Content-Type
3. Observe 404

[aesteve]

I was surprised by this behaviour, too.

In fact since you declare the route as matching a specific content-type, if the Accept header doesn't match the declared mime-type, Vert.x imply considers that no route matches. I guess Vert.x Router (as it is implemented now) doesn't know the difference between "no route matches" and "a route matches the same path and the same method, but with a different mime-type".

https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java#L99

I wanted to implement it and it doesn't look trivial, at all. Good enhancement though, same behaviour could be implemented for Method Not Allowed, too.

[pmlopes]

The reason why this is happening is because the router will first attempt a match based on content type and if fails passes the request to the next handler and once there are no more handlers the default error is not found.

Nevertheless the status code should be 415

[pmlopes]

@aesteve please review if the PR does fulfil your requirements: 405, 406 and 415

[aesteve]

I just reviewed the PR, it fulfills my requirements indeed.

[robkid]

Two other scenarios that I think merit consideration:

Order of route registration impacting response codes
This has already been partly referenced by brunoais's comment on the code, but wanted to call out a specific scenario. Consider the following routes:

router.post("/foo").consumes("application/json").handler(...);
router.put("/foo").consumes("application/x-www-form-urlencoded").handler(...);

If I issue a POST request to /foo that provides application/x-www-form-urlencoded, I believe the pull request would respond 405, since the last route that matches the path has a different method. If I reverse the ordering of the routes, I would get 415, since the last route that matches has a different content type. While I don't know when this exact scenario would occur (two different routes for the same path with divergent methods and content types), having the response be dependent on the order of route registration seems fragile to me.

"Functionality" handlers versus "response" handlers
Consider a case where I want to secure all PUT and POST requests to a section of my API, but want to publicly allow all GET requests (in the classic blog sample application, anyone can see anything, but I have to know who you are if you want to create/edit blog posts/comments). I might have something like:

router.post("/api/blog/*").handler(redirectAuthHandler);
router.put("/api/blog/*").handler(redirectAuthHandler);
router.post("/api/blog/post").consumes("application/json").handler(...);
router.get("/api/blog/post/:id").handler(...);
router.put("/api/blog/post/:id").consumes("application/json").handler(...);

If someone then tries to GET to /api/blog/entry, I believe they'd get a 405, since the path matches both of the first two routes but no other route, and both of those routes have the wrong method. But those routes aren't "real" routes in the sense of intending to provide a response; they're merely there to provide functionality around a section of the application. I would argue that 404 is appropriate here.

Similar things could be said of session handlers, logging handlers, timing handlers, etc. It would be nice to be able to mark those kinds of "routes" so that they are not part of the response code selection algorithm. From a typing perspective, they're in some ways a different type of route, though with the use of functional interfaces it becomes harder to identify them by types.

Thoughts on these
For the first scenario, it reinforces in my mind that it feels like the routing algorithm needs to in some way look at the routes holistically. Maybe having a natural ordering of status codes as I think was referenced somewhere is sufficient; at least I haven't come up with a counter example as of yet.

For the second, it seems like having some sort of way to specify different types of routes is necessary. From my experience and use cases, the functionality routes tend to be more complex than I want to provide in a lambda (we use Vert.X in a Java application), so even just a marker interface might work, as we'd have a full handler class anyway. Not sure that perfectly translates to all of the other languages though.

[lecogiteur]

Hi,

@pmlopes I see your code in vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java .Is it possible to do add an handler in order to intercept error and create a custom response for HTTP error:

Actually:

if (failed()){
        unhandledFailure(statusCode, failure, router);
     } else {
      // Send back default error code
      response().setStatusCode(defaultErrorCode);
       if (request().method() == HttpMethod.HEAD) {
         // HEAD responses don't have a body
         response().end();
       } else {
         response()
                 .putHeader(HttpHeaderNames.CONTENT_TYPE, "text/html; charset=utf-8")
                .end(String.format(DEFAULT_ERROR_MESSAGE, HttpResponseStatus.valueOf(defaultErrorCode).reasonPhrase()));
       }
     }

Is possible to do something like:

if (failed()){
        unhandledFailure(statusCode, failure, router);
     } else if (router.httpErrorResponseHandler() != null) {
        router.httpErrorResponseHandler().handle(new HttpException(HttpResponseStatus.valueOf(defaultErrorCode)));
    } else {
      // Send back default error code
      response().setStatusCode(defaultErrorCode);
       if (request().method() == HttpMethod.HEAD) {
         // HEAD responses don't have a body
         response().end();
       } else {
         response()
                 .putHeader(HttpHeaderNames.CONTENT_TYPE, "text/html; charset=utf-8")
                .end(String.format(DEFAULT_ERROR_MESSAGE, HttpResponseStatus.valueOf(defaultErrorCode).reasonPhrase()));
       }
     }

It's an example