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

Feature: Param Aggregation Draft #11

Open
ghost opened this issue Oct 26, 2018 · 3 comments
Open

Feature: Param Aggregation Draft #11

ghost opened this issue Oct 26, 2018 · 3 comments
Labels
enhancement New feature or request

Comments

@ghost
Copy link

ghost commented Oct 26, 2018

#10 shares the problem when there's a meaningful amount of parameters on the handler function, this feature aims to reduce the number of parameters aggregating them into new objects.

Current:

get("/user/{userId}")
    .param(String.class)
    .query("page", Integer.class)
    .query("limit", Integer.class)
    .query("order", String.class)
    .handle((userId, page, limit, order) -> { /* */ });

with param aggregation:

get("/user/{userId}")
    .param(String.class)
    .aggregate().query("page", Integer.class).query("limit", Integer.class).query("order", String.class).as(Pagination::new)
    .handle((userId, pagination) -> { /* */ });
@ghost ghost added the enhancement New feature or request label Oct 26, 2018
@ghost ghost self-assigned this Oct 26, 2018
@ghost ghost changed the title Param Aggregation Draft Feature: Param Aggregation Draft Nov 5, 2018
@Florian-Schoenherr
Copy link
Contributor

aggregate seems like it overlays with DDD terminology, I would propose "tuple" if we really use this API.

But instead, we could have queryObject and paramObject.

get("/user/{userId}")
    .param(String.class)
    .queryObject("page limit order", Pagination.class)
    .handle((userId, pagination) -> { /* */ });

I don't have experience with fluent interfaces and the above example is highly optimistic, so this might not even work.
Mainly I would want the API to be pretty close to the current one, so delimiting the strings by a space would be easiest. We don't need to specify the types of the query params again, because they're already defined inside Pagination.class.

@VaughnVernon
Copy link
Contributor

VaughnVernon commented Apr 10, 2021

I like your idea to have a queryObject() but I think that queryParameters() is better. That is better than aggregate(). So maybe like this:

  • queryParameters("page;limit;order", Pagination.class) or queryParameters("page:limit:order", Pagination.class)
  • The media type separator is ; so that is probably the better choice
  • I think having a delimiter is better than spaces because it makes it more obvious what the words mean

You are welcome to give it a go when you can in a month or so. This is almost 2.5 years old so it is obviously not urgent.

@Florian-Schoenherr
Copy link
Contributor

@VaughnVernon delimiter ✔️
Semicolon is probably best, I had ? in mind (because queries), but these would naturally be more like ?...&.
& would make more sense than ?.
Now I also just read that ; gets used instead of & sometimes, so let's just go for those.

Will see, I think we have more urgent stuff to do 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants