-
Notifications
You must be signed in to change notification settings - Fork 651
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
HttpServerRequest supports query parameters. #2696
base: main
Are you sure you want to change the base?
Conversation
@jchenga , thanks for this PR ! I will look into it soon today. |
I have set the target milestone to 1.1.4 since it's an enhancement. Let me know if we should backport this to 1.0.x branch ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jchenga ,
So, I have taken a look, can you please check the following reviews ?
In addition, I have the following comments:
-
can you rebase your PR on top of the latest main branch, because for japicmp, you need to update the reactor-netty-http/build.gradle, which has been updated yesterday when we did the release ? thanks.
-
japicmp:
in order to avoid the japicmp error, can you please update the reactor-netty-http/build.gradle, and exclude the newqueryParams()
method, something like this line 244:
methodExcludes = [
'reactor.netty.http.server.HttpServerRequest#queryParams()'
]
(in the latest branch, the methodExcludes is now empty because we have made the release yesterday, so just add your new method).
- I'm not sure if there is more works to be done, such as injecting a querystring decoder ? Indeed the decoder seems to be immutable, and contains only few objects, so why would we have to inject a reusable decoder ? But maybe I missed your point ?
thank you !
private Map<String, List<String>> parseQueryParams(HttpRequest request) { | ||
QueryStringDecoder decoder = new QueryStringDecoder(request.uri()); | ||
return decoder.parameters(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in order to avoid mixing public and private methods, what do you think about moving the private parseQueryParams
method just before the static inner classes that are declared in the end of the class ?
* @return query parameters {@link Map<String,List<String>>} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @return query parameters {@link Map<String,List<String>>} | |
*/ | |
* @return query parameters {@link Map} | |
*/ |
To avoid some compilation warnings, I think it is safe to just remove the generic types in the javadoc link.
reactor-netty-http/src/test/java/reactor/netty/http/server/HttpServerTests.java
Show resolved
Hide resolved
reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerRequest.java
Show resolved
Hide resolved
@jchenga Can we limit this feature only to |
@violetagg thanks! i can look into limiting the feature in route. out of curiosity, why do we want to limit this to route only? wouldn't we want to make query params widely available? |
@jchenga Take a look at the parameters resolver. You can provide a resolver so that if you use reactor-netty/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerRequest.java Line 70 in 906de4b
I also need to take a look at the Netty's query resolver. It seems it resolves everything? if that's true we will have double parsing of the uri or not? reactor-netty/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java Line 194 in 906de4b
|
@jchenga I see now... Can we discuss with Netty maintainers to make |
I mean I don't see why we want to create this object every time, basically |
There is definitely no need to create an QueryStringDecoder object every time parseQueryParams method is called. I tried to eliminate the need to recreate objects by including the null check on the map. reactor-netty/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java Lines 467 to 469 in 1e693a9
That does seem a bit hacky and I see your point about having to create an object every time we need to parse query params. I can reach out to Netty's maintainers to see if they are willing to make another option is to bring in |
My opinion is that forking/copying comes with a drawback that we need always to check whether there are some changes to the original code. So my proposal is to ask Netty maintainers. If this suggestion is not ok from their point of view, we can discuss the other two options. Wdyt? |
Sounds good to me. I'll create a request over at the Netty's repo for this. |
I have created a request in netty netty/netty#13239 to make decodeParams public. |
@violetagg it looks like Netty folks are not keen on exposing decodeParams as public API. Let's go with bringing the codes into this project's codebase? what do you think? |
I will need to take a closer look at this one ... |
Let me know once you know what you would like to do and I'll jump in to help. |
@jchenga Let's go with bringing this code to Reactor Netty |
sure thing. let me get started on that. |
@violetagg here is what i am thinking. I'll bring in the QueryStringDecoder from Netty, remove logics not related to decoding parameters, and add two public methods below: public static Map<String, List<String>> decodeParams(final String uri) {
return decodeParams(uri, HttpConstants.DEFAULT_CHARSET,
DEFAULT_MAX_PARAMS, true);
}
public static Map<String, List<String>> decodeParams(final String uri, final Charset charset, final int maxParams, final boolean semicolonIsNormalChar) {
checkNotNull(uri, "uri");
checkNotNull(charset, "charset");
checkPositive(maxParams, "maxParams");
int from = findPathEndIndex(uri);
return decodeParams(uri, from, charset,
maxParams, semicolonIsNormalChar);
} checkNotNull and checkPositive are Netty utility methods. Do we have equivalent methods? I'll also bring in the unit test and modify it to suit our needs. |
Something like this reactor-netty/reactor-netty-core/src/main/java/reactor/netty/transport/NameResolverProvider.java Line 735 in 10c127a
and this reactor-netty/reactor-netty-core/src/main/java/reactor/netty/transport/NameResolverProvider.java Lines 717 to 719 in 10c127a
|
@violetagg I've finally go time to pick this up again. the PR failed the API compatibility check because one new method was added to the HttpServerRequest interface. What should I do to resolve this failure? |
Can you try adding this?
|
@@ -0,0 +1,203 @@ | |||
/* | |||
* Copyright (c) 2012-2023 VMware, Inc. or its affiliates, All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the codes came from Netty, should the license statement be replaced with the one from Netty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@violetagg could you have a look and see if the PR is good to go?
done. that resolved the issue. |
@jchenga Can you please rebase? |
6f73ec8
to
ac78a0b
Compare
@violetagg I have rebased the branch. the windows 2019, nio build failed, but it does not seem to be related to this PR |
ac78a0b
to
80c2ee8
Compare
Hi @violetagg I rebased the feature branch on main and some checks failed. Looking at the errors, I don't think the issue is related to the change in the PR. the failures seem to OS specific as "Check Matrix / build (ubuntu-20.04, native) (pull_request) " and "Check Matrix / build (ubuntu-20.04, nio) (pull_request) " were successful. I am stumped. would you be able to help? |
I ran again the tests and now they are ok |
Thank you for the help. This PR is ready to be merged if everything looks ok. |
This PR is for issue #68. I have done an implementation to add support for query parameters in HttpServerRequest using Netty's QueryStringDecoder.
I think more needs to be done to fully flesh out the feature, such as injecting a querystring decoder instead of newing one up. @violetagg and @pderop any suggestion is appreciated.