Allow to skip parsing the request's url into URI
#10178
Conversation
http-client/src/main/java/io/micronaut/http/client/netty/NettyClientHttpRequest.java
Outdated
Show resolved
Hide resolved
| @@ -200,34 +197,6 @@ public NettyHttpRequest(io.netty.handler.codec.http.HttpRequest nettyRequest, | |||
| this.channelHandlerContext = ctx; | |||
| this.headers = new NettyHttpHeaders(nettyRequest.headers(), conversionService); | |||
| this.body = ByteBody.of(nettyRequest); | |||
| this.contentLength = headers.contentLength().orElse(-1); | |||
There was a problem hiding this comment.
are these three actually worthwhile? i would guess they are always accessed anyway.
There was a problem hiding this comment.
After thinking about it more, I will move the caching to headers and avoid nullifying it. Keep it this way for now. Routing should avoid checking the headers in cases when it will always match for any content-type/accepts.
There was a problem hiding this comment.
i think always matching is rare, it's not the default
| @@ -59,8 +59,6 @@ final class NettyRequestLifecycle extends RequestLifecycle { | |||
| super(rib.routeExecutor); | |||
| this.rib = rib; | |||
| this.outboundAccess = outboundAccess; | |||
|
|
|||
| multipartEnabled(rib.multipartEnabled); | |||
There was a problem hiding this comment.
Configuration is extracted from the route executor in RequestLifecycle.
There was a problem hiding this comment.
did you test this with servlet? i think this check was only present in RoutingInBoundHandler which is why i only added it to NettyRequestLifecycle and not the RequestLifecycle
There was a problem hiding this comment.
The check is coming from the shared config so I expect it's valid in the servlet
.../test/groovy/io/micronaut/http/server/netty/errors/MalformedUriDisabledValidationSpec.groovy
Show resolved
Hide resolved
|
@yawkat I'm trying to remove the new |
|
ok i'll do my best, still on the train though so it might take a while :) |
|
Hopefully, it's not a Czech train and Wi-Fi :D |
|
worse. german train wifi |
|
I added a check to prevent an exception from propagating and removed all |
|
the leak happens because RequestHandler.accept throws an exception, which it shouldnt. but PipeliningServerHandler needs to handle that better. so i will make a note to improve that |
http-netty/src/main/java/io/micronaut/http/netty/AbstractNettyHttpRequest.java
Outdated
Show resolved
Hide resolved
|
what did the force push change, just rebase? i dont want to review everything again |
|
Had to rewrite it a bit, moved some of the cached fields into the headers implementation. |
| return this; | ||
| } | ||
|
|
||
| private void onModify(CharSequence header) { | ||
| if (contentType != null && header.equals(HttpHeaders.CONTENT_TYPE)) { |
There was a problem hiding this comment.
can't use equals here, it's CharSequence. Is this called on the hot path? maybe it's fine to just unset all the caches and be done with it
There was a problem hiding this comment.
Removed, looks like the the modifications are mostly for the response anyway
http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java
Show resolved
Hide resolved
|
SonarCloud Quality Gate failed.
|
|
|
||
| return runWithFilters(request, (filteredRequest, propagatedContext) -> executeRoute(filteredRequest, propagatedContext, routeMatch)); | ||
| return runWithFilters(request, (filteredRequest, propagatedContext) -> { | ||
| if (validateUrl) { |
There was a problem hiding this comment.
@dstepanov why does this happen inside the filter execution? i think it should go before. if it's placed here, it's possible a filter will see and work with an invalid path, which might be a security issue.
There was a problem hiding this comment.
We can move it somewere else, the filter cannot see the invalid path because it will throw an exception
Based on #10177