fix(web): Update OkHttpClientProvider to maintain compatibility with existing retrofit1 clients (#1233) (#1234)

* test(retrofit1): add a test to demonstrate the issue of Retrofit2EncodeCorrectionInterceptor being added to retrofit1 clients

* fix(web): Update OkHttpClientProvider to maintain compatibility with existing retrofit1 clients

* fix(web): add javadoc for clarity

(cherry picked from commit a7b6fde)

Co-authored-by: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com>

Author: mergify[bot]

kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/RetrofitServiceFactoryAutoConfiguration.java

@@ -17,8 +17,10 @@
 
 package com.netflix.spinnaker.kork.retrofit;
 
+import com.netflix.spinnaker.config.okhttp3.OkHttpClientBuilderProvider;
 import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
 import com.netflix.spinnaker.kork.client.ServiceClientFactory;
+import java.util.List;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
@@ -31,6 +33,15 @@
 @ConditionalOnProperty(value = "retrofit.enabled", havingValue = "true", matchIfMissing = true)
 public class RetrofitServiceFactoryAutoConfiguration {
 
+  /**
+   * Creates OkHttpClientProvider bean for use with RetrofitServiceFactory i.e. for retrofit1
+   * clients
+   */
+  @Bean
+  public OkHttpClientProvider okHttpClientProvider(List<OkHttpClientBuilderProvider> providers) {
+    return new OkHttpClientProvider(providers);
+  }
+
   @Bean
   @Order(Ordered.LOWEST_PRECEDENCE)
   ServiceClientFactory serviceClientFactory(

kork-retrofit/src/test/kotlin/com/netflix/spinnaker/kork/retrofit/RetrofitServiceProviderTest.kt

@@ -56,6 +56,7 @@ class RetrofitServiceProviderTest  : JUnit5Minutests {
           .withConfiguration(AutoConfigurations.of(
             RetrofitServiceFactoryAutoConfiguration::class.java,
             TaskExecutionAutoConfiguration::class.java,
+            DefaultOkHttpClientBuilderProvider::class.java,
             OkHttpClientComponents::class.java,
             RetrofitConfiguration::class.java,
             TestConfiguration::class.java
@@ -79,6 +80,19 @@ class RetrofitServiceProviderTest  : JUnit5Minutests {
         }
       }
 
+      test("check if Retrofit2EncodeCorrectionInterceptor is added to OkHttpClientProvider") {
+        run { ctx: AssertableApplicationContext ->
+          expect {
+            that(
+              ctx.getBean(OkHttpClientProvider::class.java)
+                .getClient(DefaultServiceEndpoint("retrofit1", "https://www.test.com"))
+                .interceptors
+                .count { it is Retrofit2EncodeCorrectionInterceptor }
+            ).isEqualTo(0)
+          }
+        }
+      }
+
     }
   }
 
@@ -96,11 +110,6 @@ private open class TestConfiguration {
     return RawOkHttpClientFactory().create(OkHttpClientConfigurationProperties(), emptyList(), httpTracing)
   }
 
-  @Bean
-  open fun okHttpClientProvider(okHttpClient: OkHttpClient): OkHttpClientProvider {
-    return OkHttpClientProvider(listOf(DefaultOkHttpClientBuilderProvider(okHttpClient, OkHttpClientConfigurationProperties())), Retrofit2EncodeCorrectionInterceptor())
-  }
-
   @Bean
   open fun objectMapper(): ObjectMapper {
     return  ObjectMapper()

kork-web/src/main/java/com/netflix/spinnaker/config/okhttp3/OkHttpClientProvider.java

@@ -22,22 +22,48 @@
 import com.netflix.spinnaker.kork.exceptions.SystemException;
 import com.netflix.spinnaker.okhttp.Retrofit2EncodeCorrectionInterceptor;
 import java.util.List;
+import java.util.Optional;
 import okhttp3.Interceptor;
 import okhttp3.OkHttpClient;
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
 
 @Component
 public class OkHttpClientProvider {
 
   private final List<OkHttpClientBuilderProvider> providers;
 
-  private final Retrofit2EncodeCorrectionInterceptor retrofit2EncodeCorrectionInterceptor;
+  private final Optional<Retrofit2EncodeCorrectionInterceptor> retrofit2EncodeCorrectionInterceptor;
 
+  /**
+   * Constructor used for retrofit2 clients through Retrofit2ServiceFactory.
+   * Retrofit2EncodeCorrectionInterceptor is required for retrofit2 clients for correcting the
+   * partial encoding done by retrofit2
+   *
+   * @param providers list of {@link OkHttpClientBuilderProvider} that can build a client for the
+   *     given service config
+   * @param retrofit2EncodeCorrectionInterceptor interceptor that corrects the partial encoding done
+   *     by retrofit2
+   */
+  @Autowired
   public OkHttpClientProvider(
       List<OkHttpClientBuilderProvider> providers,
       Retrofit2EncodeCorrectionInterceptor retrofit2EncodeCorrectionInterceptor) {
     this.providers = providers;
-    this.retrofit2EncodeCorrectionInterceptor = retrofit2EncodeCorrectionInterceptor;
+    this.retrofit2EncodeCorrectionInterceptor = Optional.of(retrofit2EncodeCorrectionInterceptor);
+  }
+
+  /**
+   * Constructor used for retrofit1 clients through RetrofitServiceFactory.
+   * Retrofit2EncodeCorrectionInterceptor is not required for retrofit1 clients hence set to empty.
+   *
+   * @param providers list of {@link OkHttpClientBuilderProvider} that can build a client for the
+   *     given service config
+   */
+  public OkHttpClientProvider(List<OkHttpClientBuilderProvider> providers) {
+    this.providers = providers;
+    // to maintain backward compatibility with existing Retrofit1 clients
+    this.retrofit2EncodeCorrectionInterceptor = Optional.empty();
   }
 
   /**
@@ -64,11 +90,11 @@ public OkHttpClient getClient(ServiceEndpoint service, List<Interceptor> interce
   public OkHttpClient getClient(
       ServiceEndpoint service, List<Interceptor> interceptors, boolean skipEncodeCorrection) {
     OkHttpClient.Builder builder = findProvider(service).get(service);
-    if (!skipEncodeCorrection) {
+    if (!skipEncodeCorrection && retrofit2EncodeCorrectionInterceptor.isPresent()) {
       boolean encodeCorrectionExist =
           interceptors.stream().anyMatch(i -> i instanceof Retrofit2EncodeCorrectionInterceptor);
       if (!encodeCorrectionExist) {
-        builder.addInterceptor(retrofit2EncodeCorrectionInterceptor);
+        builder.addInterceptor(retrofit2EncodeCorrectionInterceptor.get());
       }
     }
     interceptors.forEach(builder::addInterceptor);