Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Builder with clear/addConverterFactory wrong behavior #4240

Open
mtrakal opened this issue Oct 14, 2024 · 0 comments
Open

Builder with clear/addConverterFactory wrong behavior #4240

mtrakal opened this issue Oct 14, 2024 · 0 comments

Comments

@mtrakal
Copy link

mtrakal commented Oct 14, 2024

Retrofit.Builder or retrofit.newBuilder shares same instance for converters. So you are not able to add / remove converter and don't affect original instance. There is missing some copy function in newBuilder.

For example, we need two different converters, due to external library (which need GSON converter), but for app we uses KotlinX Serialization.
In that case, we expect solution like:

fun replaceConverters(
        retrofit: Retrofit,
        converterFactory: Converter.Factory
    ): Retrofit{
        val builder = retrofit.newBuilder()
        builder.converterFactories().clear() // remove GSON converter
        builder.addConverterFactory(converterFactory) //add new converter KotlinX.serialization
        return builder.build()
    } 

But it affects the origin instance as well! Because newBuilder didn't copy instance but just reuse origin one. So converterFactories().clear() remove from origin retrofit existing converter and add new one addConverterFactory(converterFactory). So new request to external library start failing due to wrong converters.

Same solution is, if you create singleton instance without converters and try to add later:

    @Provides
    @Singleton
    @RetrofitQualifier
    internal fun provideRetrofit(
        baseUrl: HttpUrl,
        okHttpClient: OkHttpClient,
        pinningStrategy: PinningStrategy,
    ): Retrofit = Retrofit
        .Builder()
        .baseUrl(baseUrl)
        .client(
            okHttpClient
                .newBuilder()
                .setPinningStrategy(pinningStrategy)
                .build(),
        )
        .build()

@Provides
    @GsonRetrofitQualifier
    @Singleton
    fun provideGsonRetrofit(
        @GsonQualifier converterFactory: Converter.Factory,
        @RetrofitQualifier retrofit: Retrofit,
    ): Retrofit = retrofit.newBuilder()
        .also { Log.d("Retro GSON", retrofitBuilder.converterFactories().count().toString()) } // Expecting all the time 0.
        .addConverterFactory(converterFactory)
        .also { Log.d("Retro GSON", retrofitBuilder.converterFactories().count().toString()) } // Expecting all the time 1. 
        .build()

    @Provides
    @KotlinxSerializationRetrofitQualifier
    @Singleton
    fun provideKotlinxSerializationRetrofit(
        @KotlinxSerializationQualifier converterFactory: Converter.Factory,
        @RetrofitQualifier retrofit: Retrofit,
    ): Retrofit = retrofit.newBuilder()
        .also { Log.d("Retro KS", retrofitBuilder.converterFactories().count().toString()) } // Expecting all the time 0.
        .addConverterFactory(converterFactory)
        .also { Log.d("Retro KS", retrofitBuilder.converterFactories().count().toString()) } // Expecting all the time 1.
        .build()

   @Qualifier
    annotation class RetrofitQualifier

   @Qualifier
    annotation class GsonRetrofitQualifier

    @Qualifier
    annotation class KotlinxSerializationRetrofitQualifier

But if you will call second instance with different converter, it will not print to console 1 but 2 for converters. So builder again reuses same instance.


Workaround is, not to use newBuilder and not use @Singleton for @RetrofitQualifier. So we create 2 (or more) instances of base retrofit. But it's not much clear and we spend a lot of time checking what's wrong with requests.

Expected result:

  • newBuilder can remove converters and will not affect origin instance.
  • creating new instances from retrofit without factories will add only proper converter, and will not share same inner instance with different instances.

How to test it:
Create two APIs which uses different converter + serialization with name conversion:

import kotlinx.serialization.SerialName

data class ResponseKotlinXDto(
    @SerialName("data") myData: String
)

vs

import com.google.gson.annotations.SerializedName

data class ResponseGsonDto(
    @SerializedName("data") myData: String
)

If you will try to use converter for names data > myData it will fails when wrong convertor is used (because it will fry to parse myData entity instead of data entity).

@mtrakal mtrakal changed the title Singleton Builder with addConverterFactory wrong behavior Builder with clear/addConverterFactory wrong behavior Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant