OpenFeign / feign

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

@QueryMap not working with POJOs as specified in documentation #691

Closed doublemc closed 5 years ago

doublemc commented 6 years ago

According to: https://github.com/OpenFeign/feign#dynamic-query-parameters it should be possible to write: @RequestLine("GET /api/v1/historicalTrades") fun getHistoricalTrades(@QueryMap orderBookQuery: OrderBookQuery): List<TradeEntry>

where OrderBookQuery looks like that: data class OrderBookQuery(val symbol: String, val limit: Int? = 100)

Feign should generate: /api/v1/historicalTrades?symbol={symobl}&limit={limit} but I'm getting:

Exception in thread "main" java.lang.IllegalStateException: QueryMap parameter must be a Map: class com.binance.api.feign.marketdata.model.OrderBookQuery
    at feign.Util.checkState(Util.java:128)
    at feign.Contract$BaseContract.parseAndValidateMetadata(Contract.java:126)
    at feign.Contract$BaseContract.parseAndValidatateMetadata(Contract.java:64)
    at feign.ReflectiveFeign$ParseHandlersByName.apply(ReflectiveFeign.java:146)
    at feign.ReflectiveFeign.newInstance(ReflectiveFeign.java:53)
    at feign.Feign$Builder.target(Feign.java:198)
    at feign.Feign$Builder.target(Feign.java:194)

Here is my config and usage that produces above exception:

val marketDataRestClient =
        Feign.builder().encoder(GsonEncoder())
            .decoder(GsonDecoder())
            .logger(Logger.ErrorLogger())
            .logLevel(Logger.Level.FULL)
            .target(MarketDataRestClient::class.java, "https://api.binance.com")
    val s = marketDataRestClient.getHistoricalTrades(OrderBookQuery("ETHBTC"))

Link to my SO if someone wants to get them free karma points ;) : https://stackoverflow.com/questions/50044283/feign-querymap-usage-with-pojo

kdavisk6 commented 6 years ago

I don't believe this feature is available in the released versions. It's been accepted to master and will be part of 9.7. If you want to use this now, you will need to build the library from master.

doublemc commented 6 years ago

DO you know when 9.7 is going live?

velo commented 6 years ago

I think, by the end of the week

On Sun, Apr 29, 2018, 03:39 Michal notifications@github.com wrote:

DO you know when 9.7 is going live?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/OpenFeign/feign/issues/691#issuecomment-385184975, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIVjtHgZ1SFEGfS0JUq-zgwAb-G7Lhkks5ttI0cgaJpZM4Tp4jl .

kdavisk6 commented 6 years ago

9.7 was released.

kdavisk6 commented 6 years ago

@doublemc Are you still experiencing this issue with 9.7?

3p5i10n commented 6 years ago

Bug is still present, neither 9.7 nor 10.0.1 solve the Issue.

java.lang.IllegalStateException: QueryMap parameter must be a Map: class org.foo.QueryParameters

No QueryMapEncoder is used, as mentioned in the docs.

@RequestLine("GET /v1/foo/") public Foo getFoo(@QueryMap QueryParameters queryParameters);

rage-shadowman commented 6 years ago

Do you have a small failing test case example?

kdavisk6 commented 6 years ago

I'm looking at the latest code and line 126 in Contract does not call checkState anymore, it does a direct check for the map:

https://github.com/OpenFeign/feign/blob/17a515e073e8dc6e3d06a8404ad9742274ce0bb0/core/src/main/java/feign/Contract.java#L125-L128

Are you sure you are using the right feign jar file and version?

3p5i10n commented 6 years ago

While trying to reproduce it again in a separate sample project, i was not able to. Now 9.7 and 10.0.1 are working fine ...have to check my base project again.

Sorry for the inconvenience and the late response, been busy the last weeks.