Builder with clear/addConverterFactory wrong behavior

[mtrakal]

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).

[JakeWharton]

I cannot reproduce this.

I wrote this test:

  @Test public void issue4240() {
    Converter.Factory factory1 = new Converter.Factory() {
      @Override
      public String toString() {
        return "factory1";
      }
    };
    Converter.Factory factory2 = new Converter.Factory() {
      @Override
      public String toString() {
        return "factory2";
      }
    };
    Converter.Factory factory3 = new Converter.Factory() {
      @Override
      public String toString() {
        return "factory3";
      }
    };

    Retrofit original = new Retrofit.Builder()
      .baseUrl("http://example.com")
      .addConverterFactory(factory1)
      .build();
    System.out.println("Original: " + original.converterFactories());

    Retrofit.Builder builder1 = original.newBuilder();
    Retrofit.Builder builder2 = original.newBuilder();
    System.out.println("Builder 1 initial: " + builder1.converterFactories());
    System.out.println("Builder 2 initial: " + builder2.converterFactories());

    builder1.converterFactories().clear();
    builder2.converterFactories().clear();
    System.out.println("Builder 1 cleared: " + builder1.converterFactories());
    System.out.println("Builder 2 cleared: " + builder2.converterFactories());

    builder1.addConverterFactory(factory2);
    builder2.addConverterFactory(factory3);
    System.out.println("Builder 1 added: " + builder1.converterFactories());
    System.out.println("Builder 2 added: " + builder2.converterFactories());

    Retrofit new1 = builder1.build();
    Retrofit new2 = builder2.build();
    System.out.println("Original: " + original.converterFactories());
    System.out.println("New 1: " + new1.converterFactories());
    System.out.println("New 2: " + new2.converterFactories());
  }

which produces:

Original: [retrofit2.BuiltInConverters@3de8f619, factory1, retrofit2.OptionalConverterFactory@2ab4bc72]
Builder 1 initial: [factory1]
Builder 2 initial: [factory1]
Builder 1 cleared: []
Builder 2 cleared: []
Builder 1 added: [factory2]
Builder 2 added: [factory3]
Original: [retrofit2.BuiltInConverters@3de8f619, factory1, retrofit2.OptionalConverterFactory@2ab4bc72]
New 1: [retrofit2.BuiltInConverters@5b38c1ec, factory2, retrofit2.OptionalConverterFactory@338fc1d8]
New 2: [retrofit2.BuiltInConverters@4722ef0c, factory3, retrofit2.OptionalConverterFactory@48e1f6c7]

If you can modify my test to produce the errant behavior, I'll re-open.

However, you can also see this is impossible by looking at the source.

When we create the Retrofit instance from the builder, we create a copy of all the supplied factories, and then wrap it in an unmodifiable list:

retrofit/retrofit/src/main/java/retrofit2/Retrofit.java

Lines 686 to 713 in 54dd2f5

	// Make a defensive copy of the adapters and add the default Call adapter.
	List<CallAdapter.Factory> callAdapterFactories = new ArrayList<>(this.callAdapterFactories);
	List<? extends CallAdapter.Factory> defaultCallAdapterFactories =
	builtInFactories.createDefaultCallAdapterFactories(callbackExecutor);
	callAdapterFactories.addAll(defaultCallAdapterFactories);
	// Make a defensive copy of the converters.
	List<? extends Converter.Factory> defaultConverterFactories =
	builtInFactories.createDefaultConverterFactories();
	int defaultConverterFactoriesSize = defaultConverterFactories.size();
	List<Converter.Factory> converterFactories =
	new ArrayList<>(1 + this.converterFactories.size() + defaultConverterFactoriesSize);
	// Add the built-in converter factory first. This prevents overriding its behavior but also
	// ensures correct behavior when using converters that consume all types.
	converterFactories.add(new BuiltInConverters());
	converterFactories.addAll(this.converterFactories);
	converterFactories.addAll(defaultConverterFactories);
	return new Retrofit(
	callFactory,
	baseUrl,
	unmodifiableList(converterFactories),
	defaultConverterFactoriesSize,
	unmodifiableList(callAdapterFactories),
	defaultCallAdapterFactories.size(),
	callbackExecutor,
	validateEagerly);

When a new builder is created from an existing instance, all of its factories are copied into brand new lists which are mutable:

retrofit/retrofit/src/main/java/retrofit2/Retrofit.java

Lines 486 to 512 in 54dd2f5

	private final List<Converter.Factory> converterFactories = new ArrayList<>();
	private final List<CallAdapter.Factory> callAdapterFactories = new ArrayList<>();
	private @Nullable Executor callbackExecutor;
	private boolean validateEagerly;
	public Builder() {}
	Builder(Retrofit retrofit) {
	callFactory = retrofit.callFactory;
	baseUrl = retrofit.baseUrl;
	// Do not add the default BuiltIntConverters and platform-aware converters added by build().
	for (int i = 1,
	size = retrofit.converterFactories.size() - retrofit.defaultConverterFactoriesSize;
	i < size;
	i++) {
	converterFactories.add(retrofit.converterFactories.get(i));
	}
	// Do not add the default, platform-aware call adapters added by build().
	for (int i = 0,
	size =
	retrofit.callAdapterFactories.size() - retrofit.defaultCallAdapterFactoriesSize;
	i < size;
	i++) {
	callAdapterFactories.add(retrofit.callAdapterFactories.get(i));
	}