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

Improve status code handling on WebFlux router predicates [SPR-17582] #22114

Closed
spring-issuemaster opened this issue Dec 8, 2018 · 2 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 8, 2018

rodolphocouto opened SPR-17582 and commented

The request predicates were designed to return a boolean to match the request, so the WebFlux router engine always returns the same status code (404) when the predicate returns false.

The problem is that some predicates need custom responses, for example:

RequestPredicates.accept() should return 406 (Not Acceptable)
RequestPredicates.contentType() should return 415 (Unsupported Media Type)
RequestPredicates.GET() or POST() or any other method should return 405 (Method Not Allowed)

My suggestion is to redesign the RequestPredicate to be able to return customized responses to properly handle each HTTP status code.

Does it make sense?


Affects: 5.1.3

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 17, 2018

Arjen Poutsma commented

I like the idea of allowing predicates to return customised status codes, but I am afraid that that alone will not be sufficient. Let me try and explain the problem.

Let's say we add a default method that allows a RequestPredicate to expose a custom status code. Something like:

default HttpStatus testForStatus(ServerRequest request) {
	if (test(request)) {
		return HttpStatus.OK;
	} else {
		return HttpStatus.NOT_FOUND;
	}
}

The predicates provided in RequestPredicates would override this method to give a status code, like in the example you provided above. This is not trivial, especially with composing predicates (AND, OR, NOT), but can be done.

Unfortunately, this would not be enough to return useful status codes. For example, consider the following router function:

RouterFunction<ServerResponse> router =
      route(GET("/foo"), handler::foo)
  .andRoute(POST("/bar"), handler::bar);

which is effectively the same as:

RouterFunction<ServerResponse> router =
      route(method(GET).and(path("/foo")), handler::foo)
  .andRoute(method(POST).and(path("/bar")), handler::bar);

If a POST "/foo" request comes in, then the desired response would be a 405 (Method Not Allowed), as the predicates mapping to foo are "closest". But for that to work, the predicate would also need to have a way to express a distance score, to indicate that the predicate mapping to foo is "closer" than that of mapping to bar. A simple boolean result does not suffice.

Implementing such a score is not trivial either, because path-based scores should weigh heavier than other scores. If they would not, then the above example would still make GET "/bar" return a 404 instead of a 405 (as both routes' predicates have a 50/50 hit-miss ratio).

Currently, routes are tested in order, and the first one that matches is used. But if we would have a way to calculate route distances, then a valid question is why we do not use that for route matching as well (i.e. the closest match wins; not the first). The only reason would be to maintain backward compatibility with current releases, which seems a lacking answer.

Hopefully, you can see that this relatively simple feature request would have major consequences for the rest of WebFlux.fn, and would significantly change some of the core assumptions of the framework. So I am going to put this issue on the backlog for now, and hope that we come across a more elegant solution to solve the issues described above.

@poutsma

This comment has been minimized.

Copy link
Contributor

@poutsma poutsma commented Jan 28, 2019

Closing as declined.

@poutsma poutsma closed this Jan 28, 2019
@snicoll snicoll removed this from the 5.x Backlog milestone Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.