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

HttpServerRequest supports query parameters. #2696

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jchenga
Copy link
Contributor

@jchenga jchenga commented Feb 15, 2023

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.

@pderop pderop self-assigned this Feb 15, 2023
@pderop
Copy link
Contributor

pderop commented Feb 15, 2023

@jchenga ,

thanks for this PR ! I will look into it soon today.

@pderop pderop added the type/enhancement A general enhancement label Feb 15, 2023
@pderop pderop added this to the 1.1.4 milestone Feb 15, 2023
@pderop
Copy link
Contributor

pderop commented Feb 15, 2023

@violetagg ,

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 ?
thanks

Copy link
Contributor

@pderop pderop left a 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 new queryParams()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 !

Comment on lines 473 to 476
private Map<String, List<String>> parseQueryParams(HttpRequest request) {
QueryStringDecoder decoder = new QueryStringDecoder(request.uri());
return decoder.parameters();
}
Copy link
Contributor

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 ?

Comment on lines 150 to 152
* @return query parameters {@link Map&lt;String,List&lt;String&gt;&gt;}
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return query parameters {@link Map&lt;String,List&lt;String&gt;&gt;}
*/
* @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.

@violetagg
Copy link
Member

@jchenga Can we limit this feature only to route functionality similar to how it is done for parameters resolution (see reactor.netty.http.server.DefaultHttpServerRoutes.HttpRouteHandler and reactor.netty.http.server.HttpPredicate)? Frameworks typically use handle and they may want to use something else for query resolution.

@jchenga
Copy link
Contributor Author

jchenga commented Feb 16, 2023

@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?

@violetagg
Copy link
Member

@jchenga Take a look at the parameters resolver. You can provide a resolver so that if you use handle we can use this resolver to give you want you want. The point is that frameworks that use handle may not want to use the Netty's query resolver but something else (e.g. Spring Framework).

HttpServerRequest paramsResolver(Function<? super String, Map<String, String>> paramsResolver);

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?

@violetagg
Copy link
Member

@jchenga I see now...

Can we discuss with Netty maintainers to make decodeParams public similar to decodeComponent as basically we need just this utility or it is not the case?

@violetagg
Copy link
Member

I mean I don't see why we want to create this object every time, basically QueryStringDecoder holds mostly the same information as our HttpServerOperations, isn't it?

@jchenga
Copy link
Contributor Author

jchenga commented Feb 21, 2023

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.

if (queryParamsMap == null) {
queryParamsMap = Collections.unmodifiableMap(parseQueryParams(this.nettyRequest));
}

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 decodeParams public.

another option is to bring in decodeParams codes from Netty into HttpOperations. Since decodeParams is trivial relative to other logics, perhaps we can just bring in the function into this project? Or maybe reusing the similar function in spring is an option too?

@violetagg
Copy link
Member

another option is to bring in decodeParams codes from Netty into HttpOperations. Since decodeParams is trivial relative to other logics, perhaps we can just bring in the function into this project?

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?

@jchenga
Copy link
Contributor Author

jchenga commented Feb 22, 2023

another option is to bring in decodeParams codes from Netty into HttpOperations. Since decodeParams is trivial relative to other logics, perhaps we can just bring in the function into this project?

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.

@jchenga
Copy link
Contributor Author

jchenga commented Feb 22, 2023

I have created a request in netty netty/netty#13239 to make decodeParams public.

@violetagg violetagg modified the milestones: 1.1.4, 1.1.x Backlog Mar 2, 2023
@jchenga
Copy link
Contributor Author

jchenga commented Mar 15, 2023

@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?

@violetagg
Copy link
Member

@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 ...

@jchenga
Copy link
Contributor Author

jchenga commented Mar 26, 2023

@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.

@violetagg
Copy link
Member

@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?

@jchenga Let's go with bringing this code to Reactor Netty

@jchenga
Copy link
Contributor Author

jchenga commented Apr 1, 2023

sure thing. let me get started on that.

@jchenga
Copy link
Contributor Author

jchenga commented Apr 4, 2023

@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.

@violetagg
Copy link
Member

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

this.queryTimeout = Objects.requireNonNull(queryTimeout, "queryTimeout");

and this

if (maxQueriesPerResolve < 1) {
throw new IllegalArgumentException("maxQueriesPerResolve must be positive");
}

@violetagg violetagg modified the milestones: 1.1.x Backlog, 1.1.7 Apr 7, 2023
@violetagg violetagg modified the milestones: 1.1.7, 1.1.x Backlog May 2, 2023
@violetagg violetagg marked this pull request as draft June 29, 2023 05:53
@jchenga jchenga marked this pull request as ready for review August 27, 2023 03:10
@jchenga
Copy link
Contributor Author

jchenga commented Aug 27, 2023

@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?

@violetagg
Copy link
Member

@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?

diff --git a/reactor-netty-http/build.gradle b/reactor-netty-http/build.gradle
index 02b717a63..3272339ed 100644
--- a/reactor-netty-http/build.gradle
+++ b/reactor-netty-http/build.gradle
@@ -250,6 +250,7 @@ task japicmp(type: JapicmpTask) {
 
 	compatibilityChangeExcludes = [ "METHOD_NEW_DEFAULT" ]
 	methodExcludes = [
+			'reactor.netty.http.server.HttpServerRequest#queryParams()'
 	]
 }

@@ -0,0 +1,203 @@
/*
* Copyright (c) 2012-2023 VMware, Inc. or its affiliates, All Rights Reserved.
Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

@jchenga
Copy link
Contributor Author

jchenga commented Sep 4, 2023

@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?

diff --git a/reactor-netty-http/build.gradle b/reactor-netty-http/build.gradle
index 02b717a63..3272339ed 100644
--- a/reactor-netty-http/build.gradle
+++ b/reactor-netty-http/build.gradle
@@ -250,6 +250,7 @@ task japicmp(type: JapicmpTask) {
 
 	compatibilityChangeExcludes = [ "METHOD_NEW_DEFAULT" ]
 	methodExcludes = [
+			'reactor.netty.http.server.HttpServerRequest#queryParams()'
 	]
 }

done. that resolved the issue.

@violetagg
Copy link
Member

@jchenga Can you please rebase?

@jchenga jchenga force-pushed the 68-query-parameters-in-request branch 3 times, most recently from 6f73ec8 to ac78a0b Compare October 16, 2023 00:06
@jchenga
Copy link
Contributor Author

jchenga commented Oct 16, 2023

@violetagg I have rebased the branch. the windows 2019, nio build failed, but it does not seem to be related to this PR

@jchenga jchenga force-pushed the 68-query-parameters-in-request branch from ac78a0b to 80c2ee8 Compare November 8, 2023 19:10
@jchenga
Copy link
Contributor Author

jchenga commented Nov 8, 2023

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?

@violetagg
Copy link
Member

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

@jchenga
Copy link
Contributor Author

jchenga commented Nov 9, 2023

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.

@violetagg violetagg modified the milestones: 1.1.x Backlog, 1.2.x Backlog Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants