PlaytikaOSS / feign-reactive

Reactive Feign client inspired by https://github.com/OpenFeign project
Apache License 2.0
600 stars 119 forks source link

Allow parameter expansion into multiple query parameters #625

Open lucianoRM opened 9 months ago

lucianoRM commented 9 months ago

I am still doubting if I should post this issue here or in https://github.com/OpenFeign , since I think this is actually a Feign limitation. However, Feign allows the configuration of Encoders, which ReactiveFeign does not. So, this solution: https://github.com/OpenFeign/feign/issues/254 is not possible here.

Issue

I need to be able to expand a single parameter into multiple query parameters. Consider the following interface method:

@GetMapping(value = "/things")
Mono<List<String>> getPaginated(final @PageRequestParam PageRequest pageRequest);

where

public class PageRequest {
 private int offset; 
 private int limit;
}

Attempted solutions

Solution 1: AnnotatedParameterProcessor

I can create a new AnnotatedParameterProcessor that knows how to handle @PageRequestParam annotations. Here, I can modify the RequestTemplate and include the required query parameters. The issue is that I can only use templating since the AnnotatedParameterProcessor does not know about the actual values provided when making the request.

On top of that, parameter templating only let's me use the name of the parameter to resolve. It does not allow for attribute accessing or any expression language.

I could do something like:

public class PageRequestParameterProcessor implements AnnotatedParameterProcessor {

    @Override
    public boolean processArgument(AnnotatedParameterContext context, Annotation annotation, Method method) {
        ...
        date.template().query("offset", "{pagerRequest.offset}")
        data.template().query("limit", "{pageRequest.limit}");
    }

Solution 2: AnnotatedParameterProcessor + ParameterExpander

I could also configure a parameter expander to transform from PageRequest into the values I need. The issue here is that the RequestTemplate only allows to configure one expander per method index. Then, my PageRequest can only be expanded once into an String. It would be great if the Expander is called once every time the template has to be resolved and providing additional context for the resolution.

Here: reactivefeign.methodhandler.PublisherClientMethodHandler#buildRequest. Instead of building an static Substitutions object from the provided arguments, call an Expander every time but with additional context like that lets the expander know what the object is being expanded for (query params, body, header, etc).

Solution 3: AnnotatedParameterProcessor + ParameterExpander into multiple query params

Since the parameter Expander will only be called once for my PageRequest, I can "hack" the String response and do something like:

public class PageRequestParameterProcessor implements AnnotatedParameterProcessor {

    @Override
    public boolean processArgument(AnnotatedParameterContext context, Annotation annotation, Method method) {
        ...
         data.indexToExpander().put(parameterIndex, value -> {
            final PageRequest pageRequest = (PageRequest) value;
            "%d&offset=%d".formatted(pageRequest.getLimit(), pageRequest.getOffset());
        });
        date.template().query("offset", "{pageRequest}")
    }

But then the value gets encoded and the special chars like & and = are replaced.

Solution 4: AnnotatedParameterProcessor + mark the parameter as QueryMap

I could also mark the parameter as a query map within my processor by calling:

data.queryMapIndex(parameterIndex)

And that will cause Feign to expand it when creating the query parameters. But this also can only be done once. If I have an actual @QueryMap in my method they will conflict with each other.


I don't know. I am kind of lost here. Is this something that can be done?

Thanks