square / retrofit

A type-safe HTTP client for Android and the JVM
https://square.github.io/retrofit/
Apache License 2.0
43.11k stars 7.3k forks source link

ScalarsConverterFactory is applied regardless Content-Type #4213

Closed snedbalek-paylocity closed 1 month ago

snedbalek-paylocity commented 2 months ago

By the documentation of ScalarsConverterFactory, I'd expect the converter to be applied only on text/plain:

A {@linkplain Converter.Factory converter} for strings and both primitives and their boxed types to {@code text/plain} bodies.

But that is ignored and it consumes even application/json as shown in this test case:

import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
import org.junit.Assert
import org.junit.Rule
import org.junit.Test
import retrofit2.Call
import retrofit2.Retrofit
import retrofit2.converter.moshi.MoshiConverterFactory
import retrofit2.converter.scalars.ScalarsConverterFactory
import retrofit2.http.GET

class RetrofitBug {
    @JvmField
    @Rule
    val server: MockWebServer = MockWebServer()

    internal interface Service {
        @GET("/path")
        fun foo(): Call<String>
    }

    @Test
    @Throws(Exception::class)
    fun test() {
        val retrofit = Retrofit.Builder()
            .baseUrl(server.url("/"))
            .addConverterFactory(ScalarsConverterFactory.create())
            .addConverterFactory(MoshiConverterFactory.create())
            .build()
        val example = retrofit.create(
            Service::class.java
        )

        server.enqueue(
            MockResponse()
                .addHeader("Content-Type", "application/json")
                .setBody("\"Hi\"")
        )

        val call = example.foo()
        val response = call.execute()
        Assert.assertEquals("application/json", response.headers()["Content-Type"])
        Assert.assertEquals("Hi", response.body())
    }
}

The shared case won't fail if the factories are reversed:

            .addConverterFactory(MoshiConverterFactory.create())
            .addConverterFactory(ScalarsConverterFactory.create())

But that has implications on body serialization. Is there a way how to restrict converter only to certain content type? Is this a bug? Should the docu be updated?

JakeWharton commented 1 month ago

We do not filter based on content type. The assumption is that your API returns a fixed content type based on the request, and if you have multiple you can overload the methods in your interface and add headers to force the content type one way or the other.

You can, however, filter on content type by writing a custom Converter.Factory. Once the Converter is created, it's handed a ResponseBody instance which does contain the MediaType that you can use to choose between two other real converters.

snedbalek-paylocity commented 1 month ago

Thanks for the confirmation!

We had the issue partially on the input side as well. Since we were passing serialized json into @Body. So we decided to go raw with ResponseBody and have a full control over it without use of the ScalarsConverterFactory completely.

But in case we will need to add Scalars in the future, I'll remember your idea. Thanks!

JakeWharton commented 1 month ago

For request body serialization you can use an annotation to disambiguate between two converters that otherwise support the same type. We have a sample showing JSON and XML selection: https://github.com/square/retrofit/blob/trunk/samples/src/main/java/com/example/retrofit/JsonAndXmlConverters.java

snedbalek-paylocity commented 1 month ago

Yeah, I saw that one. It's just that I need it one way for serialization of request body, another way for serializing of response body. E.g. We have

@POST
fun requestJson(jsonBody: String): Unit

@GET
fun responseString(): String

So I'd still need to mix both of the approaches and creating a bit of a complexity. But thanks for suggestion anyway :)

JakeWharton commented 1 month ago

Well I guess it ultimately depends on what you want.

If the choice is symmetric (i.e., the request body format always matches the response body format) then you can use the annotation exclusively to choose both the request and response converters.

If the choice is asymmetric (i.e., caller can choose arbitrary request format and server and choose arbitrary response format) then you are forced to go with the two mechanisms.

You can also use Retrofit's own annotations to choose, such as by looking at @Headers for the Accept which should dictate what format the server will respond with. You can also overwrite the Content-Type in @Headers which a delegating factory could look at to determine the request body format.