Support binding RouteMatch in server filter methods #10961
Conversation
This PR adds support for RouteMatch parameters to server filter methods. Unfortunately, binding was previously fixed for the server and client. This code had no access to RouteMatch (http does not depend on router). I refactored the code to move the arg binding into BaseFilterProcessor. This is then overridden in the server implementation to add RouteMatch binding support. I also moved ServerHttpRequest binding there while I was at it. I've tried to keep the move limited. Most of the arg binding code is still in MethodFilter. This avoids having to expose a dozen private classes. Fixes #10942
|
Looks overcomplicated. We can simple remove restriction: |
|
@dstepanov it's not possible to skip filter execution for non-routematch requests that way. it'd have to involve some changes to the binding logic that i'd rather avoid |
| } | ||
| if (argument.isAssignableFrom(RouteMatch.class)) { | ||
| if (!argument.isNullable()) { | ||
| builder.addFilterCondition(ctx -> ctx.request().getAttribute(HttpAttributes.ROUTE_MATCH).isPresent()); |
There was a problem hiding this comment.
shouldn't this always be conditional? I mean why would you want an error from the filter if there is not RouteMatch? Also this seems to indicate that null will never be passed.
There was a problem hiding this comment.
nullable -> no condition -> filter is executed even for static resources (with null as the arg)
not nullable -> condition -> filter is not executed for static resources (orElse is never hit)
| if (!argument.isNullable()) { | ||
| builder.addFilterCondition(ctx -> ctx.request().getAttribute(HttpAttributes.ROUTE_MATCH).isPresent()); | ||
| } | ||
| return (FilterArgBinder) ctx -> ctx.request().getAttribute(HttpAttributes.ROUTE_MATCH).orElse(null); |
There was a problem hiding this comment.
should probably be orElseThrow() and some internal server error or something
| return null; | ||
| }; | ||
| } | ||
| } else if (argumentType == FilterContinuation.class) { |
There was a problem hiding this comment.
might be good to cleanup all of this if/else into individual binders
http-server-tck/src/main/java/io/micronaut/http/server/tck/tests/filter/RequestFilterTest.java
Show resolved
Hide resolved
|
@yawkat Just create a new interface interface FilterPredicate {
boolean test(MutablePropagatedContext mutablePropagatedContext,
HttpRequest<?> request,
@Nullable HttpResponse<?> response,
@Nullable Throwable failure)
}If |
|
got a bit more ugly because i have to check for nullability as well, but it works |
|
|
@yawkat can you add documentation? |
|
@graemerocher it's documented in RequestFilter and ResponseFilter where we also document all the other supported argument types |
This PR adds support for RouteMatch parameters to server filter methods.
Unfortunately, binding was previously fixed for the server and client. This code had no access to RouteMatch (http does not depend on router). I refactored the code to move the arg binding into BaseFilterProcessor. This is then overridden in the server implementation to add RouteMatch binding support.
I also moved ServerHttpRequest binding there while I was at it.
I've tried to keep the move limited. Most of the arg binding code is still in MethodFilter. This avoids having to expose a dozen private classes.
Fixes #10942