From 5ffca33ec693766ff3498dfff1ca442eeccf7472 Mon Sep 17 00:00:00 2001 From: GitHub Date: Tue, 31 Jan 2023 07:01:50 +0100 Subject: [PATCH] fix: HttpClient.Factory instances can be prioritized Introduce a scoring system so that HttpClient.Factory instances provided by the SPI can be prioritized. The higher the priority, the higher the chances that the factory instance will be selected automatically to create HttpClient instances. Signed-off-by: GitHub --- .../kubernetes/client/http/HttpClient.java | 9 ++ .../client/utils/HttpClientUtils.java | 3 +- .../client/utils/HttpClientUtilsTest.java | 110 ++++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletion(-) diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/HttpClient.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/HttpClient.java index 7eba63cb0d6..cccf61e8968 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/HttpClient.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/HttpClient.java @@ -58,6 +58,15 @@ default boolean isDefault() { return false; } + /** + * The priority of the implementation. The higher the priority the more likely it will be used. + * + * @return the priority. + */ + default int priority() { + return 0; + } + } interface DerivedClientBuilder { diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/HttpClientUtils.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/HttpClientUtils.java index 03bc1a8c017..cd93779b296 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/HttpClientUtils.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/HttpClientUtils.java @@ -166,7 +166,8 @@ private static HttpClient.Factory getFactory(ServiceLoader l + "choosing the first non-default implementation. " + "You should exclude dependencies that aren't needed or use an explicit association of the HttpClient.Factory."); } - if (factory == null || (factory.isDefault() && !possible.isDefault())) { + if (factory == null || (factory.isDefault() && !possible.isDefault()) + || (!factory.isDefault()) && factory.priority() < possible.priority()) { factory = possible; } } diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/HttpClientUtilsTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/HttpClientUtilsTest.java index 0bc571b77d0..3cbb7e27fcc 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/HttpClientUtilsTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/HttpClientUtilsTest.java @@ -17,7 +17,10 @@ import io.fabric8.kubernetes.client.Config; import io.fabric8.kubernetes.client.ConfigBuilder; +import io.fabric8.kubernetes.client.KubernetesClientException; +import io.fabric8.kubernetes.client.http.HttpClient; import io.fabric8.kubernetes.client.http.Interceptor; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; @@ -27,17 +30,28 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.MockedStatic; import java.net.MalformedURLException; import java.net.URL; +import java.util.ArrayList; import java.util.Collection; +import java.util.List; +import java.util.ServiceLoader; import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; class HttpClientUtilsTest { @@ -182,4 +196,100 @@ void withNoHttpProxyProvidedReturnsNull() throws MalformedURLException { assertThat(url).isNull(); } } + + @Nested + @DisplayName("getHttpClientFactory") + class GetHttpClientFactory { + + private List factories; + @SuppressWarnings("rawtypes") + private MockedStatic mockedStaticServiceLoader; + + @BeforeEach + void setUp() { + // n.b. Since ServiceLoader is mocked for this test, it's not possible to use AssertJ assertions. + factories = new ArrayList<>(); + mockedStaticServiceLoader = mockStatic(ServiceLoader.class); + @SuppressWarnings("unchecked") + final ServiceLoader mockServiceLoader = mock(ServiceLoader.class); + when(mockServiceLoader.iterator()).thenAnswer(i -> new ArrayList<>(factories).iterator()); + when(ServiceLoader.load(eq(HttpClient.Factory.class), any())).thenReturn(mockServiceLoader); + mockedStaticServiceLoader.when(() -> ServiceLoader.load(eq(HttpClient.Factory.class))) + .thenReturn(mockServiceLoader); + } + + @AfterEach + void tearDown() { + factories.clear(); + mockedStaticServiceLoader.close(); + } + + @Test + @DisplayName("With no implementations, should throw exception") + void withNoImplementations() { + final KubernetesClientException result = assertThrows(KubernetesClientException.class, + HttpClientUtils::getHttpClientFactory); + assertEquals( + "No httpclient implementations found on the context classloader, please ensure your classpath includes an implementation jar", + result.getMessage()); + } + + @Test + @DisplayName("With default implementation, should return default") + void withDefault() { + factories.add(new DefaultHttpClientFactory()); + assertEquals(DefaultHttpClientFactory.class, HttpClientUtils.getHttpClientFactory().getClass()); + } + + @Test + @DisplayName("With default and other implementation, should return other") + void withDefaultAndOther() { + factories.add(new OtherHttpClientFactory()); + factories.add(new DefaultHttpClientFactory()); + assertEquals(OtherHttpClientFactory.class, HttpClientUtils.getHttpClientFactory().getClass()); + } + + @Test + @DisplayName("With default and other implementation, should return Priority") + void withDefaultAndPriorityAndOther() { + factories.add(new OtherHttpClientFactory()); + factories.add(new PriorityHttpClientFactory()); + factories.add(new DefaultHttpClientFactory()); + assertEquals(PriorityHttpClientFactory.class, HttpClientUtils.getHttpClientFactory().getClass()); + } + + private final class DefaultHttpClientFactory implements HttpClient.Factory { + + @Override + public HttpClient.Builder newBuilder() { + return null; + } + + @Override + public boolean isDefault() { + return true; + } + } + + private final class OtherHttpClientFactory implements HttpClient.Factory { + + @Override + public HttpClient.Builder newBuilder() { + return null; + } + } + + private final class PriorityHttpClientFactory implements HttpClient.Factory { + + @Override + public HttpClient.Builder newBuilder() { + return null; + } + + @Override + public int priority() { + return Integer.MAX_VALUE; + } + } + } }