pojo request param mapping

[mxab]

Hi,

currently only annotated parameters with RequestParam/RequestBody/PathVar are taken into account.

In my usecase I have a request param pojo

public class PersonQuery extends PageRequest {
    private String q = "";
    public PersonQuery() {   
        super(0, 20, new Sort("lastName"));
    }
//...getter/setter
}

and a RestController like this:

@GetMapping
public List<Person> query(PersonQuery query) {
   return lookup(query);
}

As I know you cannot simply use every param in the method signature, maybe allow to list the request param pojos in the build script

alpina {
     ...
     requestParamPojos = [
            'foo.bar.PersonQuery'
    ]
}

[komu]

Hi!

Does your example actually work with Spring if you make a manual request without Apina? Apina does absolutely nothing on the server side, it simply figures out what Spring and Jackson are doing and generates client code based on that.

That said, you are not supposed to annotate your data classes, but the request method. So your example would become:

@GetMapping public List<Person> query(@RequestParam PersonQuery query) {
   ...
}

Hope this helps. You can consult the Spring documentation for details.

[mxab]

Yes my request usually works, if there is no RequestParam annotation spring tries to map the query parameters to the pojo.

if you add the RequestParam annotation it expects a request param called "query" in the request which is not available

[komu]

Ah, of course you're right and I've used that over and over again when binding forms. Somehow I failed to think of that because of different context.

This would, indeed, require some additional configuration to figure out which classes should be processed this way. Either in the build-file or using some Apina-specific annotations. (I've considered adding an optional annotation library for customization of class names etc. anyway.)

Anyway, seems like a nice improvement, but it's not something that I personally need, so it's not really high on my list of thing to do. Pull request would be appreciated, though.

Thanks for the suggestion!

[javier-vilares]

I had this problem since I started using apina. With Spring MVC we can map urls like this:

GET contests?page=1&size=10&sort=creationDate,desc&tags=tag1,tag2&name=foo

to a really clean method:

@GetMapping("/contests")
public Page<ContestDTO> getPage(ContestFilter filter, Pageable page) {
   ....
}

Probably lots of crud apps with tables and filters have similar queries. To make it work with apina the best way I found was to use a Post instead of a Get and to do something like this:

@PostMapping("/contests")
public Page<ContestDTO> getPage(@RequestParam Integer page, @RequesParam Integer size,
   String[] sort, @RequestBody filter) {
   ... Ugly transformations ..
}

Workaround

One workaround is to change the behaviour of the Spring resolver chain and then use @RequestParam in Pageable and filter params.
This is a really bad idea because even if the changes didnt break anything important, maybe next Spring version does...

/**
 * Forces spring to handle @RequestParam params that are not simple types (int, Integer...) as if they didn't have any annotation.
 */
@Configuration
@RequiredArgsConstructor
public class RequestParamAdapterConfigurer {

    private final RequestMappingHandlerAdapter adapter;

    @PostConstruct
    public void init() {
        /* Here we are deleting two resolvers of the chain and inserting our special one,
        probably breaking a lot of default useful behaviour! ... */
        var customResolvers =
                adapter.getArgumentResolvers().stream()
                        .filter(resolver -> !(resolver instanceof RequestParamMethodArgumentResolver))
                        .collect(toList());

        customResolvers.add(new CustomRequestParamMethodArgumentResolver());

        adapter.setArgumentResolvers(customResolvers);
    }
}

public class CustomRequestParamMethodArgumentResolver extends RequestParamMethodArgumentResolver {

    public CustomRequestParamMethodArgumentResolver() {
        super(true);
    }

    @Override
    public boolean supportsParameter(MethodParameter parameter) {

        boolean hasRequestParam = parameter.hasParameterAnnotation(RequestParam.class);
        boolean isSimpleType = BeanUtils.isSimpleProperty(parameter.getNestedParameterType());

        boolean canBeHandledByThisResolver = isSimpleType || !hasRequestParam;
        return canBeHandledByThisResolver ? super.supportsParameter(parameter) : false;
    }

}

With this I can use getPage(@RequesParam ContestFilter filter, @RequesParam Pageable page) and it has worked for two years as expected, but is obviously terrible ... 😓

Real solution

@komu I think that the regex solution in the build file would fit perfectly and solve this problem the right and consistent way. Something like this:

extraParamTypes=['Pageable','.+Filter']

What do you think? Tomorrow I could try to make a pull request. Thanks for your time!

[javier-vilares]

I made it work, making it right to merge in the repo is above my skill level, but I think that it would be helpful for a lot of people to have something like this in the library.

javier-vilares@3b1426c

[komu]

Thanks for your contribution!

The commit above changes the generation of the client side interface, but it doesn't affect the runtime behavior of the client, right? So the calls won't actually do anything, am I correct or am I missing something? Furthermore, isn't the extra parameter wrong?

So if we have something like:

@GetMapping("/foo")
fun foo(@RequestParam bar: String, extraParams: MyExtraParams) { ... }

class MyExtraParams(
    val baz: String,
    val quux: Int
)

The code in the commit seems to create a frontend code that would send request parameters bar and extraParams, whereas it should send bar, baz and quux.

Well, it would be relatively easy to add support for this simple case, but it gets more complicated. What if MyExtraParams was a nested structure:

class MyExtraParams(val second: SecondClass)
class SecondClass(val third: ThirdClass)
class ThirdClass(var x: Int)

If I remember correctly, this would mean that we have a request parameter second.third.x which is mapped to the field. Suddenly we have another model similar to Apina's JSON-model, but with subtly different rules. These classes would not to be generated on the client side, along with another reflective model for serializing these. What if same classes are used as JSON classes and extra request parameter? We'd need to create potentially different TypeScript interfaces for different usages (think @JsonIgnore and alike). How should we name these different classes?

So it's a real can of worms: once you try to support what Spring does, there are a lot of complexities because Spring does quite a lot. Of course we could support just a limited subset of what Spring does, but even small subsets seem to bring nasty corner cases to worry about.

Here's one proposal:

When there's an extra-param class, read the immediate members of the class and add them as parameters. So my original produces the same client side as if I'd just written:

@GetMapping("/foo")
fun foo(@RequestParam bar: String,
        @RequestParam baz: String,
        @RequestParam quux: Int) { ... }

There are two problems that spring into mind:

1. The order of class members is not deterministic in the class file. Perhaps you upgrade your compiler and end up with different order of the members, breaking the client code. Perhaps the members could be sorted by name to force a consistent ordering.
2. Nested classes. The example above was simple because members of MyExtraParams were "primitives", but what to do if it refers to SecondClass? An idea: if SecondClass has been registered as a "black box class", allow it and assume that client registers an appropriate type-converter on the client side. Otherwise, fail the translation with an error saying that nested classes are not (at least yet) supported by Apina.

I think this would bring most of the benefits without inducing a huge amount of extra complexity. I'll have to think about this a bit more, but it seems feasible.

[javier-vilares]

You are right, I forgot that to make my approach work I also had to intercept and flatten the request params in the client.

class CustomEndpointContext extends ApinaEndpointContext {

    request(data: RequestData): Observable<any> {
         const params = new HttpParams({fromObject: flatten(data.requestParams)});
         return this.httpClient.request(..., options: {params});
    }

}

The flatten method detects if each param is an object with properties, and in that case it flattens it and drops the param name:

So in your example

requestParams: {bar: 'foo', extraParams: {second: {third: {x: 1} }

get translated to

requestParams: {bar: 'foo', 'second.third.x': 1 }

This works for me because I am pretty sure that I don't ever want to send an object as a param, I always want it to get flattened this way. And if I wanted I could always make it a string and bypass the flattening. But well, I don't really know what a I am doing so probably it breaks lots of other usecases and it doesn fit as default behaviour 😅

About your proposal one little problem I see is that at least for my usecases the generated methods would have a veeery long list of params. In fact for the common case of "Pageable" (the same of the op) it wouldn't work, is a nested object with lots of properties that you don't want generated as params, and with this behaviour you couldn't override the class in the "imports" config of the gradle file.

[komu]

Right, it would be really nice to have the usability of distinct types on the client side as well, but it would entail quite a lot of complexity to support properly. 😦

Well, I'll have to think about this a bit more and perhaps try a spike implementation and see how hard it would be to handle the general case.