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

Added query parameters getter inside RoutingContext #581

Merged
merged 5 commits into from Jul 3, 2017

Conversation

3 participants
@slinkydeveloper
Copy link
Member

slinkydeveloper commented Mar 31, 2017

To allow users to avoid conflicts between path parameters and query parameters, I created a query parameters getter.
Unfortunately i can't return from queryParams() a Map<String, List<String>> because of this: https://github.com/vert-x3/vertx-codegen/blob/master/src/main/java/io/vertx/codegen/ClassModel.java#L424
So:

  • queryParams()returns a Map<String, String>. If one query parameter is an array It will return as a comma separated list
  • queryParam(name) returns a List<String>, containing the query parameter value/values

I also wrote a small unit test to test this getter

@vietj vietj added the to review label Mar 31, 2017

*
* @return the map of query parameters (if a query parameter value is an array, it will be provided as comma separated string, so if you need to extract array from query use {@link #queryParam(String)} queryParam)
*/
Map<String, String> queryParams();

This comment has been minimized.

@pmlopes

pmlopes Apr 10, 2017

Member

In order to make it correct this should be a MultiMap http://vertx.io/docs/apidocs/io/vertx/core/MultiMap.html

This comment has been minimized.

@slinkydeveloper

slinkydeveloper Apr 15, 2017

Author Member

Working on it, I will commit as soon as possible

return getQueryParamsParsed().get(query);
}

protected void setQueryParams(Map<String, List<String>> q) {

This comment has been minimized.

@pmlopes

pmlopes Apr 10, 2017

Member

same, should be multimap

queryParams.put(e.getKey(), String.join(",", e.getValue()));
}

private Map<String, String> getQueryParams() {

This comment has been minimized.

@pmlopes

pmlopes Apr 10, 2017

Member

Multimap


private Map<String, String> getQueryParams() {
if (queryParams == null) {
queryParams = new HashMap<>();

This comment has been minimized.

@pmlopes

pmlopes Apr 10, 2017

Member

the initial state could be fetched from the HttpServerRequest.getParams() since it does the initial parsing for us

This comment has been minimized.

@slinkydeveloper

slinkydeveloper Apr 15, 2017

Author Member

The real trouble is that HttpServerRequest.params() don't return only query params, It returns an unique multimap with path params and query params. I've done a small test:
Router router = Router.router(vertx);

    router.route().handler(BodyHandler.create());
    router.route("/:id").handler(routingContext -> {
        System.out.println(Arrays.toString(routingContext.request().params().entries().toArray()));
       routingContext.response().end("Hello!");
    });

    vertx.createHttpServer().requestHandler(router::accept).listen(8080);

Doing a request with this URL:
http://localhost:8080/ciao?ei=ao

It prints in console this: [ei: ao, id: ciao]

return queryParams;
}

private Map<String, List<String>> getQueryParamsParsed() {

This comment has been minimized.

@pmlopes

pmlopes Apr 10, 2017

Member

This becomes not needed if we populate from the base object and parse on setPathParams right?

@pmlopes
Copy link
Member

pmlopes left a comment

Use the MultiMap type for Map<String, List<String>> kind of type and maybe we can remove the need for the second map parsedPathParams

@pmlopes

pmlopes approved these changes May 5, 2017

@pmlopes pmlopes added this to the 3.4.2 milestone May 5, 2017

@slinkydeveloper

This comment has been minimized.

Copy link
Member Author

slinkydeveloper commented May 14, 2017

Fixed a bug that duplicates entries in queryParams when more than one handler is set for a path

@pmlopes pmlopes modified the milestones: 3.5, 3.4.2 May 31, 2017

@pmlopes pmlopes merged commit c96bc21 into vert-x3:master Jul 3, 2017

@pmlopes pmlopes removed the to review label Jul 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment