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

Preparing fields for better query parameters handling #177

Merged
merged 31 commits into from Jan 19, 2017

Conversation

@fcamblor
Copy link
Contributor

@fcamblor fcamblor commented Mar 16, 2015

This PR makes some refactorings on the codebase to better handle
query & path parameters in the future.

I wanted to implement the issue #146 in multiple phases in order to gather feedback at different checkpoints during the issue resolution.

New things introduced in this PR :

  • NamedType which represents a named parameter alongside its type definition (either a primitive type or a TypeReference)
  • Endpoint class made of a method + pathPattern.
    Used it in StdRestxRequestMatcher in place of the method + pathPatterns fields (kept old constructor for easing / backward compatibility)
    Not sure if the Endpoint naming is nice here (I might rename it to something like EndpointDescriptor which is longer, WDYT ?)
  • EndpointParameter which aggregates both a NamedType with an Endpoint to represent a endpoint parameter definition
  • Body content representation might be Optional (you can have absentvalues even if I don't really see any workaround for this... this will allow to use a common behaviour between body / query params / path params)
  • Query and Path parameters are now bean validated. It seems useless for now since we only handle raw jackson-handled types (String, primitives, joda times and so on..) thanks to MainStringConverter. But later, when we'll support more complex types, it will be useful.

Some notes on my work :

  • I kept the different handling of body content on one hand, and path/query parameter on the other hand because otherwise, it was pulling too much changes (some were backward incompatibles, some others were tricky, like the call to lifecycleListener.onEntityInput() after body content resolution) :
    • Body content is parsed / converted to object (thanks to MainStringConverter) prior to calling StdEntityRoute.doRoute() (this object is passed as a doRoute argument)
    • Query / Path parameters are resolved / parsed / converted in the doRoute() implementation
  • I decided to extract the Optional type checks/initialization out of the scope of the EndpointParameterMapper since it would uselessly complexify the code at runtime whereas we have all the Optional things available at annotation processing (compile) time.
  • After having implemented all of this, I'm wondering if the EndpointParameterMapper / EndpointParameterMapperFactory thing is not overkill because :
    • The "pluggability" feature we were planning to settle for joda time types (for instance) is useless since for this case, I'll rely on MainStringConverter (thus, if a Jackson deserializer is provided for the target type, it will work out of the box)
    • I'm planning to handle types such as List, Array (etc..) by building some sort of simple AST Map<String, Either<String,Map,Collection<String>,Collection<Map>> with provided query / path parameters, then pass this Map to Jackson deserializer (which will handle every special cases for me, with nice performances included). In that case I will only have 2 mappers : EndpointRawParameterMapper (=the current one) and EndpointObjectParameterMapper (the new one). All I'll have to do is to identify which mapper to use depending on my target type (and here, ok, I'll need to put some pluggability mechanism to say "this type is or is not a raw type")
  • We'll see once I will have provided EndpointObjectParameterMapper implementation, but I'm wondering if we shouldn't simplify EndpointParameterMapper.mapRequest(EndpointParameter, RestxRequest, RestxRequestMatch, EndpointParameterKind) prototype (lots of its arguments will be useless)

WDYT ?

FYI, following generated Router code below :

        new StdEntityRoute<Void, java.lang.String>("default#ParametersResource#queryparams2",
                readerRegistry.<Void>build(Void.class, Optional.<String>absent()),
                writerRegistry.<java.lang.String>build(java.lang.String.class, Optional.<String>absent()),
                new StdRestxRequestMatcher("GET", "/params/query/2"),
                HttpStatus.OK, RestxLogLevel.DEFAULT) {
            @Override
            protected Optional<java.lang.String> doRoute(RestxRequest request, RestxRequestMatch match, Void body) throws IOException {
                securityManager.check(request, isAuthenticated());
                return Optional.of(resource.queryparams2(
                        /* [QUERY] a */ converter.convert(checkPresent(request.getQueryParam("a"), "query param a is required"), org.joda.time.DateTime.class),
                        /* [QUERY] b */ converter.convert(request.getQueryParam("b"), org.joda.time.DateTime.class)
                ));
            }

            @Override
            protected void describeOperation(OperationDescription operation) {
            // Same as before...

will look like this after this refactoring :

        new StdEntityRoute<Void, java.lang.String>("default#ParametersResource#queryparams2",
                readerRegistry.<Void>build(Void.class, Optional.<String>absent()),
                writerRegistry.<java.lang.String>build(java.lang.String.class, Optional.<String>absent()),
                Endpoint.of("GET", "/params/query/2"), // <- Here, using Endpoint.of() instead of new StdRestxRequestMatcher()
                HttpStatus.OK, RestxLogLevel.DEFAULT,
                paramMapperRegistry, new NamedType[]{ // <- Here, providing description for every endpoint parameter in order to search for adequate endpoint parameter mappers at route contruction time (which will speed up doRoute())
                    NamedType.of(new TypeReference<org.joda.time.DateTime>(){}, "a"),
                    NamedType.of(new TypeReference<org.joda.time.DateTime>(){}, "b")
                }) {
            @Override
            protected Optional<java.lang.String> doRoute(RestxRequest request, RestxRequestMatch match, Void body) throws IOException {
                securityManager.check(request, isAuthenticated());
                return Optional.of(resource.queryparams2(
                        // Here, things have been more complex... converter.convert() is now called inside the mapQueryObjectFromRequest() call, a new call to checkValid(), a check for non-nullity if not
                        /* [QUERY] a */ checkValid(validator, checkNotNull(mapQueryObjectFromRequest(NamedType.of(new TypeReference<org.joda.time.DateTime>(){}, "a"), request, match, EndpointParameterKind.QUERY), "QUERY param <a> is required")),
                        /* [QUERY] b */ Optional.fromNullable(checkValid(validator, mapQueryObjectFromRequest(NamedType.of(new TypeReference<org.joda.time.DateTime>(){}, "b"), request, match, EndpointParameterKind.QUERY)))
                ));
            }

            @Override
            protected void describeOperation(OperationDescription operation) {
            // Same as before...
@xhanin
Copy link
Contributor

@xhanin xhanin commented Mar 16, 2015

I've not yet looked at all the commits but it sounds good overall.

A few comments/feedback:

  1. I think NamedType does not convey what it really is. First I thought it was a type which is named, but it's not the type that that is named, it's the parameter. But I don't find very good names, maybe simply NamedParameter or just Parameter: the problem with these names is that they don't bring anything about the content (a Name + a Type) and are less generic, but I think they're more explicit. What is still missing is the fact that it's a descriptor and not the parameter itself. So maybe ParameterDescriptor or ParameterDefinition or just ParameterDef or ParamDef.

  2. Endpoint name sounds good to me, I don't think we need the longer version.

  3. EndpointParameter coud inherit ParamDef instead of aggregating. It's not that I favour inheritance but I think with the name ParamDef we really have the notion that a EndpointParameter "is a" ParamDef. We could even rename it EndpointParamDef (or whatever name we come up with for NamedType). But I realize that it will make it more difficult to write the route as an inner class, since we would need to reuse the Endpoint object when building constructor parameters, so maybe it's just not a good idea, I have to think more about it.

  4. in generated code, I wouldn't repeat the NamedType.of when calling mapQueryObjectFromRequest: you can call it only with parameters defined in the constructors, so you can avoid constructing the object again, giving the name is enough. Actually at first I was more thinking that when initializing the route we would obtain a kind of specialized mapper for each parameter, and then only call it. But maybe it's using an inner class which doesn't help for that. And I have to think more about your remark saying that the pluggable factory may not be useful.

I'll come back to that later.

@a-peyrard
Copy link
Contributor

@a-peyrard a-peyrard commented Mar 16, 2015

I'm not a big expert on the subject, so I will just go with some small questions about implementation:

  • Is there a reason to prefer com.google.common.base.Objects than the java.util.Objects (which is available is SE7) ?
  • Couldn't we store in NamedType the raw type and the type, in order to avoid raw type calculation (useful for example in LegacyEndpointParameterMapper, where you made a comment about raw type calculation) ? And couldn't we add a constructor for NamedType which would use the Types.newParameterizedType helper method ? Like this we will be able to generate the type and to store the rawtype, without any calculations.
  • Couldn't we simplify this: https://github.com/restx/restx/pull/177/files#diff-bf81b90e11d5cbfd090cf143dc234c38R35 by something like:
if(String.class == endpointParameter.getParameter().getType()) {

my 2c :)

@fcamblor
Copy link
Contributor Author

@fcamblor fcamblor commented Mar 16, 2015

Great, will take all these pertinent feedbacks into consideration :-)

About String.class comparison, you're totally right ... I just made so much refactorings with git rebases that I let this (too) complex test pass :-)

@fcamblor
Copy link
Contributor Author

@fcamblor fcamblor commented Mar 17, 2015

Took into consideration your remarks

fcamblor added 23 commits Feb 7, 2015
…ty method to identify if we're on guava/java8/no optional type and retrieve underlying type

Thus, simplified some RestxAnnotationProcessor methods
…oint because later, we will need to build parameter coordinates based on endpoint props (and not matcher one).

Note that RestxRequestMatcher is still used in the high level API (on the StdRoute superclass)
…registry).

Its purpose is to map request info to a given target type, from both path and query parameters
… when creating new StdEntityRoute.

Note that current code will fail since we didn't provided any mapper implementation yet
…pes on body parameters

Main purpose here is to prepare fields to have an homogeneous parameters construction
between body content/query param/path param
…y improving checks made :

- Path parameters can now be Optionals
- Request & Path parameters are now bean validated, like body parameters
…n itself without throwing constraint validation exception

It will be useful when checking parameters for validity, even when they are null
…g path & query parameters

values, thus allowing to plug more complex mappers in the future
Provided simple legacy mapper allowing to map simple String to object using MainStringConverter
(CoyoteStream.available() was returning 0 on buffered streams)
Thus, prefering to catch JsonMappingException to detect null body content
…turns a parameterized type :

using MyType<ParameterizedType>.class doesn't work : we must transform it to MyType.class and casting it back to MyType<ParameterizedType> after mapQueryObjectFromRequest()
…data is missing and target type is an iterable
fcamblor added 7 commits May 13, 2016
… (specific to annotation processor) to AggregateType in restx-common (shared amongst both restx-core and annotation processor)
…hod allowing to know if underlying object mapper knows how to deserialize a given type
…edValuesFor() allowing to retrieve multiple values for a given param name
…tQueryParams(name) when no query param matches given name
…erMapperFactory implementation allowing to bind simple and simple iterable types on query/path parameters.

using the same jacksone deserializer as the ones used for BODY deserialization.
Note that complex types (=POJOs) are not handled yet
@fcamblor fcamblor force-pushed the fcamblor:better-query-params branch from e61f0f7 to f486835 May 19, 2016
@fcamblor
Copy link
Contributor Author

@fcamblor fcamblor commented May 19, 2016

Just rebased my work on master in order to allow to merge the PR easily

I added a bunch of new commits (starting from 36329af) allowing to handle conversions from :

  • "Simple" parameter types (I consider as "simple type" types handles by MainStringConverter component today, which in turns rely on Jackson ObjectMapper for types like Date or Joda Dates)
  • Iterables, Collections, Sets & Arrays of those "simple" parameter types

It means that "complex" types (such as POJOs like SearchCriteria) are not handled yet.
I guess I'll create a separate PR for this purpose since :

  • Current PR starts to be huge
  • I have concerns/discussions about the ways to convert query/path parameters to POJOs (using reflections, or Supplier, or whatever ... but let's discuss it in dedicated #248 issue)

To my POV, I consider this PR as finished, but I would be very happy to have some code review from both @xhanin & @a-peyrard to ensure I didn't missed something important.

Regarding generated code, only 1 thing was changed compared to my initial PR description : I added an additional Supplier parameter to mapQueryObjectFromRequest() in the cases where parameter is of type of Iterable or Array (this supplier is intended to be called when no parameter is found in query parameters, and in order to instanciate an "empty" aggregate of target objects)
For Iterable<DateTime>, we will generate following code :

new StdEntityRoute<Void, java.lang.Iterable<org.joda.time.DateTime>>("default#ParametersResource#iterableJodaDatesParams",
        readerRegistry.<Void>build(Void.class, Optional.<String>absent()),
        writerRegistry.<java.lang.Iterable<org.joda.time.DateTime>>build(Types.newParameterizedType(java.lang.Iterable.class, org.joda.time.DateTime.class), Optional.<String>absent()),
        Endpoint.of("GET", "/params/iterableJodaDatesParams"),
        HttpStatus.OK, RestxLogLevel.DEFAULT, pf,
        paramMapperRegistry, new ParamDef[]{
            ParamDef.of(new TypeReference<java.lang.Iterable<org.joda.time.DateTime>>(){}, "params"),
            ParamDef.of(new TypeReference<java.lang.Iterable<org.joda.time.DateTime>>(){}, "otherParams")
        }) {
    @Override
    protected Optional<java.lang.Iterable<org.joda.time.DateTime>> doRoute(RestxRequest request, RestxRequestMatch match, Void body) throws IOException {
        securityManager.check(request, match, open());
        return Optional.of(resource.iterableJodaDatesParams(
                /* [QUERY] params */ checkValid(validator, (java.lang.Iterable<org.joda.time.DateTime>)mapQueryObjectFromRequest(java.lang.Iterable.class, "params", request, match, EndpointParameterKind.QUERY, EMPTY_ITERABLE_SUPPLIER)),
                /* [QUERY] otherParams */ checkValid(validator, (java.lang.Iterable<org.joda.time.DateTime>)mapQueryObjectFromRequest(java.lang.Iterable.class, "otherParams", request, match, EndpointParameterKind.QUERY, EMPTY_ITERABLE_SUPPLIER))
        ));
    }
...

And for DateTime[], it will generate following code :

new StdEntityRoute<Void, org.joda.time.DateTime[]>("default#ParametersResource#arrayedJodaDatesParams",
        readerRegistry.<Void>build(Void.class, Optional.<String>absent()),
        writerRegistry.<org.joda.time.DateTime[]>build(org.joda.time.DateTime[].class, Optional.<String>absent()),
        Endpoint.of("GET", "/params/arrayedJodaDatesParams"),
        HttpStatus.OK, RestxLogLevel.DEFAULT, pf,
        paramMapperRegistry, new ParamDef[]{
            ParamDef.of(new TypeReference<org.joda.time.DateTime[]>(){}, "params"),
            ParamDef.of(new TypeReference<org.joda.time.DateTime[]>(){}, "otherParams")
        }) {
    @Override
    protected Optional<org.joda.time.DateTime[]> doRoute(RestxRequest request, RestxRequestMatch match, Void body) throws IOException {
        securityManager.check(request, match, open());
        return Optional.of(resource.arrayedJodaDatesParams(
                /* [QUERY] params */ checkValid(validator, mapQueryObjectFromRequest(org.joda.time.DateTime[].class, "params", request, match, EndpointParameterKind.QUERY, Suppliers.ofInstance(new org.joda.time.DateTime[]{}))),
                /* [QUERY] otherParams */ checkValid(validator, mapQueryObjectFromRequest(org.joda.time.DateTime[].class, "otherParams", request, match, EndpointParameterKind.QUERY, Suppliers.ofInstance(new org.joda.time.DateTime[]{})))
        ));
    }
...
@fcamblor fcamblor force-pushed the fcamblor:better-query-params branch from f486835 to 248a30f May 19, 2016
@fcamblor
Copy link
Contributor Author

@fcamblor fcamblor commented Jul 13, 2016

Any input for merging this ? :-)

@fcamblor fcamblor mentioned this pull request Sep 7, 2016
@fcamblor
Copy link
Contributor Author

@fcamblor fcamblor commented Jan 15, 2017

@xhanin @a-peyrard Except strong disagreement until Wednesday (january 17th), I'm going to merge this PR to integrate it in upcoming release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants