OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.51k stars 1.93k forks source link

@QueryMap does not work with POJOs when used after @Param #852

Open inad9300 opened 5 years ago

inad9300 commented 5 years ago

This kind of code (from https://github.com/OpenFeign/feign/pull/335/files#diff-13815c678552a275075ab18b17c67784R616):

@RequestLine("GET /?name={name}")
void queryMapWithQueryParams(@Param("name") String name, @QueryMap Map<String, Object> queryMap);

Does not work when @QueryMap is used on a POJO.

In my case, I was trying to use Spring's Pageable like so:

@RequestLine("GET /things?x={x}&y={y}&z={z}")
CustomPageImpl<ThingDto> getElements(
    @Param("x") ZonedDateTime x,
    @Param("y") String y,
    @Param("z") String z,
    @QueryMap Pageable pageable);

By the way... Is there any simpler way to declare query parameters without having to refer four times to their names?

kdavisk6 commented 5 years ago

This is expected. We do not support mixing @Param and @QueryMap using the Bean specification currently. The models are effectively incompatible. In the meantime, my suggestion is to use a custom Encoder for the parameters you want to control. You can find more information on that in the README - Custom Parameter Expansion

When using the @Param annotation, you must annotate each method parameter. The alternative is to use a @QueryMap, using the Map specification. In your particular case, you could refactor your interface to accomplish what you are asking:

@RequestLine("GET /things")
CustomPageImpl<ThingDto> getElements(
    @QueryMap Map<String, Object> parameters,
    @Param(encoder = "PageableEncoder.class") Pageable pageRequest);

Add all of your parameters to the map and they will be expanded onto the query string. Using the custom encoder, you can control how the Pageable is expanded as well.

Let me know if this helps or if you have more questions.

inad9300 commented 5 years ago

I disagree the two models are incompatible, but this should be explicitly stated in the documentation, since as it is written now, what I was trying to do initially should be expected to work.

Thanks for your proposed workaround.

jebeaudet commented 4 years ago

Just ran into this, IMO it should be clear in the documentation or even better, the Contract should throw an exception when it encounters such a case.

kdavisk6 commented 4 years ago

Time is a friend here. I’ll revisit this.

velo commented 4 years ago

Just ran into this, IMO it should be clear in the documentation or even better, the Contract should throw an exception when it encounters such a case.

Sounds like a good plan @jebeaudet, can you help on making it happen?

DzianisH commented 3 years ago

Hi there, is there is any progress on issue?