HttpServerRequest supports query parameters.

[jchenga]

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]

@jchenga ,

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

[pderop]

@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

[violetagg]

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

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

@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). https://github.com/reactor/reactor-netty/blob/906de4bcd67c309640d45b71d162667fe1755ef3/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerRequest.java#L70

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? https://github.com/reactor/reactor-netty/blob/906de4bcd67c309640d45b71d162667fe1755ef3/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java#L194

[violetagg]

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

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]

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.

https://github.com/reactor/reactor-netty/blob/1e693a9b4619e98c8abebc0f277dcbeacff88919/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java#L467-L469

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]

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]

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]

I have created a request in netty https://github.com/netty/netty/issues/13239 to make decodeParams public.

[jchenga]

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

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

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

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

sure thing. let me get started on that.

[jchenga]

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

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

https://github.com/reactor/reactor-netty/blob/10c127a1bd09c9f22550e3423cd3ffbafa797d8e/reactor-netty-core/src/main/java/reactor/netty/transport/NameResolverProvider.java#L735

and this

https://github.com/reactor/reactor-netty/blob/10c127a1bd09c9f22550e3423cd3ffbafa797d8e/reactor-netty-core/src/main/java/reactor/netty/transport/NameResolverProvider.java#L717-L719

[jchenga]

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

@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()'
 	]
 }

[jchenga]

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

@jchenga Can you please rebase?

[jchenga]

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

[jchenga]

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]

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]

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.