Skip to content

Commit 806ece0

Browse files
fix(web): remove Retrofit2EncodeCorrectionInterceptor from OkHttpClient client meant for retrofit1 client (#1229) (#1232)
* test(web): add a test to demonstrate the issue of Retrofit2EncodeCorrectionInterceptor being attached to retrofit2 client * test(web): add assertions, comments to Retrofit1ImpactTest * fix(web): remove Retrofit2EncodeCorrectionInterceptor from OkHttpClient.Builder which is meant for retrofit1 client * doc(web): add comments to OkHttp3ClientConfiguration --------- Co-authored-by: David Byron <dbyron@salesforce.com> (cherry picked from commit af2ec39) Co-authored-by: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com>
1 parent 2830bde commit 806ece0

File tree

3 files changed

+119
-6
lines changed

3 files changed

+119
-6
lines changed

kork-web/kork-web.gradle

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,5 @@ dependencies {
5454
testImplementation testFixtures(project(":kork-crypto"))
5555
testRuntimeOnly "cglib:cglib-nodep"
5656
testRuntimeOnly "org.objenesis:objenesis"
57+
testImplementation "com.jakewharton.retrofit:retrofit1-okhttp3-client"
5758
}

kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttp3ClientConfiguration.groovy

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ class OkHttp3ClientConfiguration {
9292
}
9393

9494
/**
95-
* @return OkHttpClient w/ <optional> key and trust stores
95+
* @return OkHttpClient w/ <optional> key and trust stores. For use with retrofit1. Do not use with retrofit2.
9696
*/
9797
OkHttpClient.Builder create() {
9898
if (okHttpClientConfigurationProperties.refreshableKeys.enabled) {
@@ -106,10 +106,6 @@ class OkHttp3ClientConfiguration {
106106
okHttpClientBuilder.addInterceptor(okHttp3MetricsInterceptor)
107107
}
108108

109-
if (retrofit2EncodeCorrectionInterceptor != null) {
110-
okHttpClientBuilder.addInterceptor(retrofit2EncodeCorrectionInterceptor)
111-
}
112-
113109
if (!okHttpClientConfigurationProperties.keyStore && !okHttpClientConfigurationProperties.trustStore) {
114110
return okHttpClientBuilder
115111
}
@@ -118,7 +114,8 @@ class OkHttp3ClientConfiguration {
118114
}
119115

120116
/**
121-
* @return OkHttpClient with SpinnakerRequestHeaderInterceptor as initial interceptor w/ <optional> key and trust stores
117+
* @return OkHttpClient with SpinnakerRequestHeaderInterceptor and Retrofit2EncodeCorrectionInterceptor
118+
* as initial interceptors w/ <optional> key and trust stores
122119
*/
123120
OkHttpClient.Builder createForRetrofit2() {
124121
if (okHttpClientConfigurationProperties.refreshableKeys.enabled) {
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
/*
2+
* Copyright 2025 OpsMx, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.netflix.spinnaker.okhttp;
18+
19+
import static com.github.tomakehurst.wiremock.client.WireMock.get;
20+
import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
21+
import static com.github.tomakehurst.wiremock.client.WireMock.ok;
22+
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
23+
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
24+
25+
import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
26+
import com.jakewharton.retrofit.Ok3Client;
27+
import com.netflix.spectator.api.NoopRegistry;
28+
import com.netflix.spinnaker.config.OkHttp3ClientConfiguration;
29+
import com.netflix.spinnaker.config.OkHttpMetricsInterceptorProperties;
30+
import com.netflix.spinnaker.config.okhttp3.DefaultOkHttpClientBuilderProvider;
31+
import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
32+
import java.net.URLEncoder;
33+
import java.nio.charset.StandardCharsets;
34+
import okhttp3.OkHttpClient;
35+
import okhttp3.logging.HttpLoggingInterceptor;
36+
import org.junit.jupiter.api.Test;
37+
import org.junit.jupiter.api.extension.RegisterExtension;
38+
import org.springframework.beans.factory.annotation.Autowired;
39+
import org.springframework.boot.test.context.SpringBootTest;
40+
import org.springframework.context.annotation.Bean;
41+
import org.springframework.context.annotation.Configuration;
42+
import retrofit.http.GET;
43+
import retrofit.http.Query;
44+
45+
@SpringBootTest(
46+
webEnvironment = SpringBootTest.WebEnvironment.NONE,
47+
classes = {
48+
OkHttpClient.class,
49+
OkHttpClientConfigurationProperties.class,
50+
OkHttpClientProvider.class,
51+
DefaultOkHttpClientBuilderProvider.class,
52+
Retrofit2EncodeCorrectionInterceptor.class,
53+
Retrofit1ImpactTest.TestConfiguration.class,
54+
OkHttpMetricsInterceptorProperties.class,
55+
OkHttp3MetricsInterceptor.class,
56+
OkHttp3ClientConfiguration.class,
57+
NoopRegistry.class
58+
})
59+
public class Retrofit1ImpactTest {
60+
61+
private static final String QUERY_PARAM_VAL = "qry_with space";
62+
private static final String ENCODED_QUERY_PARAM_VAL =
63+
encodedString(QUERY_PARAM_VAL); // qry_with+space
64+
65+
@RegisterExtension
66+
static WireMockExtension wireMock =
67+
WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build();
68+
69+
@Autowired OkHttp3ClientConfiguration clientConfiguration;
70+
71+
@Test
72+
public void test_Retrofit2Interceptors_Impact_On_Retrofit1() {
73+
wireMock.stubFor(get("/test?qry=" + ENCODED_QUERY_PARAM_VAL).willReturn(ok()));
74+
Retrofit1Service retrofit1Service = getRetrofit1Service(wireMock.baseUrl());
75+
76+
retrofit1Service.get(QUERY_PARAM_VAL);
77+
78+
// proves no presence of Retrofit2EncodeCorrectionInterceptor in retrofit1 client
79+
wireMock.verify(1, getRequestedFor(urlEqualTo("/test?qry=" + ENCODED_QUERY_PARAM_VAL)));
80+
}
81+
82+
private static String encodedString(String input) {
83+
return URLEncoder.encode(input, StandardCharsets.UTF_8);
84+
}
85+
86+
private Retrofit1Service getRetrofit1Service(String baseUrl) {
87+
// clientConfiguration.create() is for use with retrofit1. The point of
88+
// this test is to verify that clientConfiguration.create doesn't include
89+
// the Retrofit2EncodeCorrectionInterceptor since that's only for retrofit2.
90+
return new retrofit.RestAdapter.Builder()
91+
.setEndpoint(baseUrl)
92+
.setClient(new Ok3Client(clientConfiguration.create().build()))
93+
.build()
94+
.create(Retrofit1Service.class);
95+
}
96+
97+
@Configuration
98+
public static class TestConfiguration {
99+
100+
@Bean
101+
public HttpLoggingInterceptor.Level logLevel() {
102+
return HttpLoggingInterceptor.Level.BASIC;
103+
}
104+
105+
@Bean
106+
public SpinnakerRequestHeaderInterceptor spinnakerRequestHeaderInterceptor() {
107+
return new SpinnakerRequestHeaderInterceptor(true);
108+
}
109+
}
110+
111+
interface Retrofit1Service {
112+
@GET("/test")
113+
Void get(@Query(value = "qry") String qry);
114+
}
115+
}

0 commit comments

Comments
 (0)