OpenFeign / feign

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

FieldQueryMapEncoder uses Param.value but ignores Param.expander #1312

Open shaunparker opened 4 years ago

shaunparker commented 4 years ago

The FieldQueryMapEncoder implementation uses the Param annotation to set the field names:

https://github.com/OpenFeign/feign/blob/68f79847449c6df36010e9c4118c4244854ea67c/core/src/main/java/feign/querymap/FieldQueryMapEncoder.java#L44-L45

but it does not use its expander to set the value:

https://github.com/OpenFeign/feign/blob/68f79847449c6df36010e9c4118c4244854ea67c/core/src/main/java/feign/querymap/FieldQueryMapEncoder.java#L42-L46

Was this done intentionally or could it be considered a bug?

I was hoping to use the @Param annotation on a POJO of optional query parameters with a field that needs custom encoding, e.g.:

package com.example.opts;

import com.example.expander.CustomExpander;
import feign.Param;
import lombok.Builder;
import lombok.Value;

import java.time.Instant;

@Builder
@Value
public class ExampleOptions {
    @Param(value = "customValue", expander = CustomExpander.class)
    Instant custom;
    Long normal;
}

In this example the parameter is named "customValue", but the value is just the .toString() value of the java.time.Instant instead of the value CustomExpander would have returned.

Please feel free to close this if FieldQueryMapEncoder is meant to work this way. If that's the case, I think it'd make sense to have a custom QueryMapEncoder that uses the expander.

I'd be happy to create a pull request that uses the Param.expander if you think it makes sense.

Thanks, Shaun

kdavisk6 commented 4 years ago

More than likely, no one looked for this feature. It's makes a lot of sense to me. Feel free to submit a PR to add this capability.