Safely parse URL and respect http server status code error rule#5128
Conversation
6b43747 to
1df795e
Compare
BenchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases. See unchanged results
|
46ffec0 to
c8c6d7c
Compare
There was a problem hiding this comment.
Maybe I'm picky here, but for curiosity sake I wonder if it's really worth using lambdas here when the difference is at most just one line of code. Without passing a lambda it would be almost the same from the reading complexity perspective.
final String uri = request.getUri();
final URI parsed = URIUtils.safeParse(uri);
if (parsed != null) {
if ((uri.getHost() == null || uri.getHost().equals(""))
&& request.headers().contains(HOST)) {
return URIDataAdapterBase.fromURI(
"http://" + request.headers().get(HOST) + request.getUri(),
URIDefaultDataAdapter::new);
}
return new URIDefaultDataAdapter(uri);
}
return new UnparseableURIDataAdapter(uri);There was a problem hiding this comment.
It would open opportunity to remove the mapper parameter completely and just use URIDefaultDataAdapter's constructor within URIDataAdapterBase.fromURI for simple use-cases.
There was a problem hiding this comment.
Nice to have it covered with a test.
There was a problem hiding this comment.
As I mentioned before, I'm not completely sure if the mapper parameter is worth it. With or without it it's the same amount of boilerplate for the custom use-cases, and is more lightweight for simple cases.
There was a problem hiding this comment.
I agree with you. I leverage the mapping in the netty decorator, where the parsed uri is used, eventually, with host coming from the http headers to fix the url. So having a mapping function saves me some if and some other boilerplate to parse the URI. I may think about a better way to do it without having a mapper fuction and improve in a next PR
ygree
left a comment
There was a problem hiding this comment.
Looks good to me. Just not sure about worthiness of the mapper parameter that takes a lambda.
c8c6d7c to
ffb7916
Compare
What Does This Do
This PR is doing a couple of things:
Motivation
tldr; on recent tomcat versions backslashes are not allowed (it's configurable with
org.apache.catalina.connector.CoyoteAdapter.ALLOW_BACKSLASH=true) while before it was.This parsing error is captured because reported among the servlet response parameters. While we should log the stacktrace on the span, we do not should (unless configured) mark the span as error because we have a 4XX error
Additional Notes