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

Handling complex type bindings in query parameters #248

Open
fcamblor opened this Issue May 19, 2016 · 1 comment

Comments

Projects
None yet
1 participant
@fcamblor
Contributor

fcamblor commented May 19, 2016

This issue is a follow up of #177 (and concerns #194 as well)

The goal is to be able to declare a Criteria POJO as QUERY parameter (not BODY content which is already supported), something like this :

@RestxResource @Component
public class ParametersResource {
    @GET("/search")
    public Criteria mandatorySearchCriteria(Criteria criteria) {
...
    }
}

My initial thought on this was to simply convert our Map<String, String> of query parameters into the target type, using Jackson's T ObjectMapper.convertValue(Map, Class<T>) method (see http://stackoverflow.com/a/16430704)
However, it looked a bit more complicated than expected since query parameters should rather be represented with a MultiMap<String,String> than a Map<String,String>, meaning that Iterable|Array types in target POJO type may have to be handled differently : we cannot consider always using a 1-1 binding since some leaf nodes in the map should have to be arrays instead of simple values.

Moreover, it implies some topics I'd like to discuss here with you.
Some of these topics may be correlated with each other

1/ Should we systematically reuse the java parameter name (in my example, criteria) as a prefix for the query parameter names ?

Pros :

  • it allows to use easily 2 different criteria (not sure if it would be useful) like this :
    @GET("/search")
    public Criteria mandatorySearchCriteria(Criteria criteria, AnotherCriteria anotherCriteria ) { ... }

and call it like this : /search?criteria.name=foo&anotherCriteria.id=bar

Cons :

  • it requires to always prefix query parameter names (with dotted notation) with java parameter name, which may represent an issue
  • it doesn't allow to share same query parameter between 2 different criteria (not sure if it would be useful)

My POV on this : I think the case where we have 2 different POJOs mapped on query parameters won't happen very frequently at all.
Not sure if we should spend time to support this case and consider that parameter name prefix is not required.

2/ How should we handle data binding between query parameters (which is a MultiMap<String, String>) and our target POJO ?

I guess we have 3 different options here :

  • Either use reflection, which will not be in the general habit of restx, and will cost some performance cut off
  • Generate setter calls (through a Supplier implementation, for instance) when generating code, something like this (in Router's ParamDef section) :
new ParamDef[]{
    ParamDef.of(new TypeReference<foo.bar.Criteria>(){}, "criteria", new ComplexTypeSupplier<foo.bar.Criteria(){
        public foo.bar.Criteria fill(foo.bar.Criteria criteria, RestxRequest req, RestxRequestMatch match){
            criteria.setName(req.getQueryParam("name"));
            criteria.setDate(mapQueryObjectFromRequest(DateTime.class, "date", req, match, EndpointParameterKind.QUERY));
        }
    })
}
  • Create a SetterBuilder api which may describe previous option using java 8 method references, making it less verbose and allowing to handle Iterable|Array types (see question 4/ below) through a dedicated api method.
    Something like this :
new ParamDef[]{
    ParamDef.of(new TypeReference<foo.bar.Criteria>(){}, "criteria", ComplexTypeSupplier.of(foo.bar.Criteria.class)
        .withSetter(foo.bar.Criteria::setName, DateTime.class, "date", EndpointParameterKind.QUERY)
        .create()
    )
}

Anyway, it would require to upgrade restx java minimum version to java 8

My POV on this is not decided yet. I see pros & cons for every of those solutions.

3/ Should we allow complex (nested) types in our target POJO ?

That is to say, should we allow to nest complex type on our Criteria POJO ?
Calling it through dot notation like this : /search?id=1&company.name=foo (expecting having a Company nested property inside the Criteria POJO)

My POV on this is : I think we should try to handle this if possible, but if it implies some complexity, give up on this because :

  • Query parameter should be very limited (describing a "whole" object tree with query parameters should be discouraged I guess
  • At worst, we may ask the developper to put first level setter methods which would set target property (putting a setCompanyName() setter which, in turn, will call a this.company.name = companyName)

4/ Should we allow aggregates in our taget POJO ?

That is to say, should we allow to call such URLs : /search?ids=1&ids=2

My POV on this is : I guess we should allow it as it may happen frequently (I personally use it frequently).
However, I guess we should limit restrict this to Iterable|Array of "simple types" (don't nest complex iterable types which would involve indexed notation -/search?nested[0].id=0&nested[1].id=1- in query parameter names which looks weird to my POV)

What do you think ?

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Jul 13, 2016

Any advice here regarding these 4 topics ? (planning my holidays homework .. ;-))

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