diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java index 0023cf60..16089f0f 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java @@ -18,9 +18,14 @@ package org.springframework.cloud.openfeign.loadbalancer; import feign.Client; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; /** @@ -35,8 +40,21 @@ class DefaultFeignLoadBalancerConfiguration { @Bean @ConditionalOnMissingBean + @Conditional(OnRetryNotEnabledCondition.class) public Client feignClient(LoadBalancerClient loadBalancerClient) { return new FeignBlockingLoadBalancerClient(new Client.Default(null, null), loadBalancerClient); } + @Bean + @ConditionalOnMissingBean + @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") + @ConditionalOnBean(LoadBalancedRetryFactory.class) + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true", + matchIfMissing = true) + public Client feignRetryClient(LoadBalancerClient loadBalancerClient, + LoadBalancedRetryFactory loadBalancedRetryFactory) { + return new RetryableBlockingFeignLoadBalancerClient(new Client.Default(null, null), loadBalancerClient, + loadBalancedRetryFactory); + } + } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java index 352484c9..979b6057 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java @@ -24,9 +24,11 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; import org.springframework.cloud.openfeign.clientconfig.HttpClientFeignConfiguration; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; @@ -46,9 +48,22 @@ class HttpClientFeignLoadBalancerConfiguration { @Bean @ConditionalOnMissingBean + @Conditional(OnRetryNotEnabledCondition.class) public Client feignClient(LoadBalancerClient loadBalancerClient, HttpClient httpClient) { ApacheHttpClient delegate = new ApacheHttpClient(httpClient); return new FeignBlockingLoadBalancerClient(delegate, loadBalancerClient); } + @Bean + @ConditionalOnMissingBean + @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") + @ConditionalOnBean(LoadBalancedRetryFactory.class) + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true", + matchIfMissing = true) + public Client feignRetryClient(LoadBalancerClient loadBalancerClient, HttpClient httpClient, + LoadBalancedRetryFactory loadBalancedRetryFactory) { + ApacheHttpClient delegate = new ApacheHttpClient(httpClient); + return new RetryableBlockingFeignLoadBalancerClient(delegate, loadBalancerClient, loadBalancedRetryFactory); + } + } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java index a2efd30b..d55fe8d6 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java @@ -23,9 +23,11 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; import org.springframework.cloud.openfeign.clientconfig.OkHttpFeignConfiguration; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; @@ -45,9 +47,22 @@ class OkHttpFeignLoadBalancerConfiguration { @Bean @ConditionalOnMissingBean + @Conditional(OnRetryNotEnabledCondition.class) public Client feignClient(okhttp3.OkHttpClient okHttpClient, LoadBalancerClient loadBalancerClient) { OkHttpClient delegate = new OkHttpClient(okHttpClient); return new FeignBlockingLoadBalancerClient(delegate, loadBalancerClient); } + @Bean + @ConditionalOnMissingBean + @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") + @ConditionalOnBean(LoadBalancedRetryFactory.class) + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true", + matchIfMissing = true) + public Client feignRetryClient(LoadBalancerClient loadBalancerClient, okhttp3.OkHttpClient okHttpClient, + LoadBalancedRetryFactory loadBalancedRetryFactory) { + OkHttpClient delegate = new OkHttpClient(okHttpClient); + return new RetryableBlockingFeignLoadBalancerClient(delegate, loadBalancerClient, loadBalancedRetryFactory); + } + } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OnRetryNotEnabledCondition.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OnRetryNotEnabledCondition.java new file mode 100644 index 00000000..7a8e9a08 --- /dev/null +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OnRetryNotEnabledCondition.java @@ -0,0 +1,55 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.openfeign.loadbalancer; + +import org.springframework.boot.autoconfigure.condition.AnyNestedCondition; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; +import org.springframework.retry.support.RetryTemplate; + +/** + * A condition that verifies that {@link RetryTemplate} is on the classpath, a + * {@link LoadBalancedRetryFactory} bean is present and + * spring.cloud.loadbalancer.retry.enabled is not set to false. + * + * @author Olga Maciaszek-Sharma + * @since 2.2.6 + */ +public class OnRetryNotEnabledCondition extends AnyNestedCondition { + + public OnRetryNotEnabledCondition() { + super(ConfigurationPhase.REGISTER_BEAN); + } + + @ConditionalOnMissingClass("org.springframework.retry.support.RetryTemplate") + static class OnNoRetryTemplateCondition { + + } + + @ConditionalOnMissingBean(LoadBalancedRetryFactory.class) + static class OnRetryFactoryCondition { + + } + + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "false") + static class OnLoadBalancerRetryEnabledCondition { + + } + +} diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClient.java new file mode 100644 index 00000000..1e9c6046 --- /dev/null +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClient.java @@ -0,0 +1,191 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.openfeign.loadbalancer; + +import java.io.IOException; +import java.net.URI; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import feign.Client; +import feign.Request; +import feign.Response; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.DefaultRequest; +import org.springframework.cloud.client.loadbalancer.InterceptorRetryPolicy; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRecoveryCallback; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy; +import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; +import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; +import org.springframework.cloud.client.loadbalancer.RetryableStatusCodeException; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpRequest; +import org.springframework.retry.RetryListener; +import org.springframework.retry.backoff.BackOffPolicy; +import org.springframework.retry.backoff.NoBackOffPolicy; +import org.springframework.retry.policy.NeverRetryPolicy; +import org.springframework.retry.support.RetryTemplate; +import org.springframework.util.Assert; + +/** + * A {@link Client} implementation that provides Spring Retry support for requests + * load-balanced with Spring Cloud LoadBalancer. + * + * @author Olga Maciaszek-Sharma + * @since 2.2.6 + */ +public class RetryableBlockingFeignLoadBalancerClient implements Client { + + private static final Log LOG = LogFactory.getLog(FeignBlockingLoadBalancerClient.class); + + private final Client delegate; + + private final LoadBalancerClient loadBalancerClient; + + private final LoadBalancedRetryFactory loadBalancedRetryFactory; + + public RetryableBlockingFeignLoadBalancerClient(Client delegate, LoadBalancerClient loadBalancerClient, + LoadBalancedRetryFactory loadBalancedRetryFactory) { + this.delegate = delegate; + this.loadBalancerClient = loadBalancerClient; + this.loadBalancedRetryFactory = loadBalancedRetryFactory; + } + + @Override + public Response execute(Request request, Request.Options options) throws IOException { + final URI originalUri = URI.create(request.url()); + String serviceId = originalUri.getHost(); + Assert.state(serviceId != null, "Request URI does not contain a valid hostname: " + originalUri); + final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryFactory.createRetryPolicy(serviceId, + loadBalancerClient); + RetryTemplate retryTemplate = buildRetryTemplate(serviceId, request, retryPolicy); + return retryTemplate.execute(context -> { + Request feignRequest = null; + // On retries the policy will choose the server and set it in the context + // and extract the server and update the request being made + if (context instanceof LoadBalancedRetryContext) { + LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; + ServiceInstance serviceInstance = lbContext.getServiceInstance(); + if (serviceInstance == null) { + if (LOG.isDebugEnabled()) { + LOG.debug("Service instance retrieved from LoadBalancedRetryContext: was null. " + + "Reattempting service instance selection"); + } + ServiceInstance previousServiceInstance = lbContext.getPreviousServiceInstance(); + DefaultRequest lbRequest = new DefaultRequest<>( + new RetryableRequestContext(previousServiceInstance, request)); + serviceInstance = loadBalancerClient.choose(serviceId, lbRequest); + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("Selected service instance: %s", serviceInstance)); + } + lbContext.setServiceInstance(serviceInstance); + } + if (serviceInstance != null) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("Using service instance from LoadBalancedRetryContext: %s", + serviceInstance)); + } + String reconstructedUrl = loadBalancerClient.reconstructURI(serviceInstance, originalUri) + .toString(); + feignRequest = Request.create(request.httpMethod(), reconstructedUrl, request.headers(), + request.body(), request.charset(), request.requestTemplate()); + } + } + if (feignRequest == null) { + if (LOG.isWarnEnabled()) { + LOG.warn("Service instance was not resolved, executing the original request"); + } + feignRequest = request; + } + Response response = delegate.execute(feignRequest, options); + int responseStatus = response.status(); + if (retryPolicy != null && retryPolicy.retryableStatusCode(responseStatus)) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("Retrying on status code: %d", responseStatus)); + } + response.close(); + throw new RetryableStatusCodeException(serviceId, responseStatus, response, URI.create(request.url())); + } + return response; + }, new LoadBalancedRecoveryCallback() { + @Override + protected Response createResponse(Response response, URI uri) { + return response; + } + }); + } + + private RetryTemplate buildRetryTemplate(String serviceId, Request request, LoadBalancedRetryPolicy retryPolicy) { + RetryTemplate retryTemplate = new RetryTemplate(); + BackOffPolicy backOffPolicy = this.loadBalancedRetryFactory.createBackOffPolicy(serviceId); + retryTemplate.setBackOffPolicy(backOffPolicy == null ? new NoBackOffPolicy() : backOffPolicy); + RetryListener[] retryListeners = this.loadBalancedRetryFactory.createRetryListeners(serviceId); + if (retryListeners != null && retryListeners.length != 0) { + retryTemplate.setListeners(retryListeners); + } + + retryTemplate.setRetryPolicy(retryPolicy == null ? new NeverRetryPolicy() + : new InterceptorRetryPolicy(toHttpRequest(request), retryPolicy, loadBalancerClient, serviceId)); + return retryTemplate; + } + + // Visible for Sleuth instrumentation + public Client getDelegate() { + return delegate; + } + + private HttpRequest toHttpRequest(Request request) { + return new HttpRequest() { + @Override + public HttpMethod getMethod() { + return HttpMethod.resolve(request.httpMethod().name()); + } + + @Override + public String getMethodValue() { + return getMethod().name(); + } + + @Override + public URI getURI() { + return URI.create(request.url()); + } + + @Override + public HttpHeaders getHeaders() { + Map> headers = new HashMap<>(); + Map> feignHeaders = request.headers(); + for (String key : feignHeaders.keySet()) { + headers.put(key, new ArrayList<>(feignHeaders.get(key))); + } + HttpHeaders httpHeaders = new HttpHeaders(); + httpHeaders.putAll(headers); + return httpHeaders; + } + }; + } + +} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignHttpClientUrlTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignHttpClientUrlTests.java index 7862ad59..c4c5c008 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignHttpClientUrlTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignHttpClientUrlTests.java @@ -23,10 +23,9 @@ import feign.Client; import feign.Feign; import feign.Target; import feign.httpclient.ApacheHttpClient; -import org.junit.AfterClass; -import org.junit.BeforeClass; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; @@ -36,7 +35,6 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringRunner; import org.springframework.util.ReflectionUtils; import org.springframework.util.SocketUtils; import org.springframework.web.bind.annotation.RequestMapping; @@ -49,11 +47,11 @@ import static org.springframework.boot.test.context.SpringBootTest.WebEnvironmen /** * @author Spencer Gibb */ -@RunWith(SpringRunner.class) -@SpringBootTest(classes = FeignHttpClientUrlTests.TestConfig.class, webEnvironment = DEFINED_PORT, value = { - "spring.application.name=feignclienturltest", "feign.hystrix.enabled=false", "feign.okhttp.enabled=false" }) +@SpringBootTest(classes = FeignHttpClientUrlTests.TestConfig.class, webEnvironment = DEFINED_PORT, + value = { "spring.application.name=feignclienturltest", "feign.hystrix.enabled=false", + "feign.okhttp.enabled=false", "spring.cloud.loadbalancer.retry.enabled=false" }) @DirtiesContext -public class FeignHttpClientUrlTests { +class FeignHttpClientUrlTests { static int port; @@ -66,35 +64,35 @@ public class FeignHttpClientUrlTests { @Autowired private BeanUrlClient beanClient; - @BeforeClass + @BeforeAll public static void beforeClass() { port = SocketUtils.findAvailableTcpPort(); System.setProperty("server.port", String.valueOf(port)); } - @AfterClass + @AfterAll public static void afterClass() { System.clearProperty("server.port"); } @Test - public void testUrlHttpClient() { - assertThat(this.urlClient).as("UrlClient was null").isNotNull(); - Hello hello = this.urlClient.getHello(); + void testUrlHttpClient() { + assertThat(urlClient).as("UrlClient was null").isNotNull(); + Hello hello = urlClient.getHello(); assertThat(hello).as("hello was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); } @Test - public void testBeanUrl() { - Hello hello = this.beanClient.getHello(); + void testBeanUrl() { + Hello hello = beanClient.getHello(); assertThat(hello).as("hello was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); } @Test public void testBeanUrlNoProtocol() { - Hello hello = this.beanClientNoProtocol.getHello(); + Hello hello = beanClientNoProtocol.getHello(); assertThat(hello).as("hello was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); } @@ -174,15 +172,15 @@ public class FeignHttpClientUrlTests { private String message; - public Hello() { + Hello() { } - public Hello(String message) { + Hello(String message) { this.message = message; } public String getMessage() { - return this.message; + return message; } public void setMessage(String message) { @@ -198,12 +196,12 @@ public class FeignHttpClientUrlTests { return false; } Hello that = (Hello) o; - return Objects.equals(this.message, that.message); + return Objects.equals(message, that.message); } @Override public int hashCode() { - return Objects.hash(this.message); + return Objects.hash(message); } } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java index ce0c80c5..d3d1dc88 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java @@ -21,7 +21,7 @@ import java.util.Map; import feign.Client; import feign.httpclient.ApacheHttpClient; import feign.okhttp.OkHttpClient; -import org.junit.Test; +import org.junit.jupiter.api.Test; import org.springframework.boot.WebApplicationType; import org.springframework.boot.builder.SpringApplicationBuilder; @@ -36,30 +36,54 @@ import static org.assertj.core.api.Assertions.assertThat; /** * @author Olga Maciaszek-Sharma */ -public class FeignLoadBalancerAutoConfigurationTests { +class FeignLoadBalancerAutoConfigurationTests { @Test - public void shouldInstantiateDefaultFeignBlockingLoadBalancerClientWhenHttpClientDisabled() { - ConfigurableApplicationContext context = initContext("feign.httpclient.enabled=false"); + void shouldInstantiateDefaultFeignBlockingLoadBalancerClientWhenHttpClientDisabled() { + ConfigurableApplicationContext context = initContext("feign.httpclient.enabled=false", + "spring.cloud.loadbalancer.retry.enabled=false"); assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); assertLoadBalanced(context, Client.Default.class); } @Test - public void shouldInstantiateHttpFeignClientWhenEnabled() { - ConfigurableApplicationContext context = initContext("spring.cloud.loadbalancer.ribbon.enabled=false"); + void shouldInstantiateHttpFeignClientWhenEnabled() { + ConfigurableApplicationContext context = initContext("spring.cloud.loadbalancer.retry.enabled=false"); assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); assertLoadBalanced(context, ApacheHttpClient.class); } @Test - public void shouldInstantiateOkHttpFeignClientWhenEnabled() { + void shouldInstantiateOkHttpFeignClientWhenEnabled() { ConfigurableApplicationContext context = initContext("feign.httpclient.enabled=false", - "feign.okhttp.enabled=true"); + "feign.okhttp.enabled=true", "spring.cloud.loadbalancer.retry.enabled=false"); assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); assertLoadBalanced(context, OkHttpClient.class); } + @Test + void shouldInstantiateRetryableDefaultFeignBlockingLoadBalancerClientWhenHttpClientDisabled() { + ConfigurableApplicationContext context = initContext("spring.cloud.loadbalancer.ribbon.enabled=false", + "feign.httpclient.enabled=false"); + assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); + assertLoadBalancedWithRetries(context, Client.Default.class); + } + + @Test + void shouldInstantiateRetryableHttpFeignClientWhenEnabled() { + ConfigurableApplicationContext context = initContext("spring.cloud.loadbalancer.ribbon.enabled=false"); + assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); + assertLoadBalancedWithRetries(context, ApacheHttpClient.class); + } + + @Test + void shouldInstantiateRetryableOkHttpFeignClientWhenEnabled() { + ConfigurableApplicationContext context = initContext("spring.cloud.loadbalancer.ribbon.enabled=false", + "feign.httpclient.enabled=false", "feign.okhttp.enabled=true"); + assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); + assertLoadBalancedWithRetries(context, OkHttpClient.class); + } + private ConfigurableApplicationContext initContext(String... properties) { return new SpringApplicationBuilder().web(WebApplicationType.NONE).properties(properties) .sources(HttpClientConfiguration.class, LoadBalancerAutoConfiguration.class, @@ -79,4 +103,14 @@ public class FeignLoadBalancerAutoConfigurationTests { assertThat(beans.get("feignClient").getDelegate()).isInstanceOf(delegateClass); } + private void assertLoadBalancedWithRetries(ConfigurableApplicationContext context, Class delegateClass) { + Map retryableBeans = context + .getBeansOfType(RetryableBlockingFeignLoadBalancerClient.class); + assertThat(retryableBeans).hasSize(1); + Map beans = context + .getBeansOfType(FeignBlockingLoadBalancerClient.class); + assertThat(beans).isEmpty(); + assertThat(retryableBeans.get("feignRetryClient").getDelegate()).isInstanceOf(delegateClass); + } + } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClientTests.java new file mode 100644 index 00000000..28b90d62 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClientTests.java @@ -0,0 +1,184 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.openfeign.loadbalancer; + +import java.io.IOException; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import feign.Client; +import feign.Request; +import feign.Response; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.junit.jupiter.MockitoExtension; + +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; +import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; +import org.springframework.cloud.loadbalancer.blocking.retry.BlockingLoadBalancedRetryPolicy; +import org.springframework.http.HttpHeaders; +import org.springframework.http.MediaType; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link RetryableBlockingFeignLoadBalancerClient}. Note: the underlying + * {@link BlockingLoadBalancerClient} is already extensively tested in the Spring Cloud + * Commons project, so here we are only testing the interactions between + * {@link RetryableBlockingFeignLoadBalancerClient} and its delegates. + * + * @see BlockingLoadBalancerClientTests + * @author Olga Maciaszek-Sharma + */ +@ExtendWith(MockitoExtension.class) +class RetryableBlockingFeignLoadBalancerClientTests { + + private Client delegate = mock(Client.class); + + private LoadBalancedRetryFactory retryFactory = mock(LoadBalancedRetryFactory.class); + + private BlockingLoadBalancerClient loadBalancerClient = mock(BlockingLoadBalancerClient.class); + + private LoadBalancerProperties properties = new LoadBalancerProperties(); + + private RetryableBlockingFeignLoadBalancerClient feignBlockingLoadBalancerClient = new RetryableBlockingFeignLoadBalancerClient( + delegate, loadBalancerClient, retryFactory); + + private ServiceInstance serviceInstance = new DefaultServiceInstance("test-a", "test", "testhost", 80, false); + + @BeforeEach + void setUp() { + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy(properties)); + when(loadBalancerClient.choose(eq("test"), any())).thenReturn(serviceInstance); + } + + @Test + void shouldExtractServiceIdFromRequestUrl() throws IOException { + Request request = testRequest(); + Response response = testResponse(200); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy(properties)); + when(loadBalancerClient.reconstructURI(serviceInstance, URI.create("http://test/path"))) + .thenReturn(URI.create("http://testhost:80/path")); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(loadBalancerClient).choose(eq("test"), any()); + verify(loadBalancerClient).reconstructURI(serviceInstance, URI.create("http://test/path")); + + verify(delegate).execute( + argThat((Request actualRequest) -> actualRequest.url().equals("http://testhost:80/path")), any()); + } + + private Response testResponse(int status) { + return Response.builder().request(testRequest()).status(status).build(); + } + + @Test + void shouldExecuteOriginalRequestIfInstanceNotFound() throws IOException { + Request request = testRequest(); + Response response = testResponse(503); + when(loadBalancerClient.choose(eq("test"), any())).thenReturn(null); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy(properties)); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(delegate).execute(eq(request), any()); + } + + @Test + void shouldRetryOnRepeatableStatusCode() throws IOException { + properties.getRetry().getRetryableStatusCodes().add(503); + Request request = testRequest(); + Response response = testResponse(503); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy(properties)); + when(loadBalancerClient.reconstructURI(serviceInstance, URI.create("http://test/path"))) + .thenReturn(URI.create("http://testhost:80/path")); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(loadBalancerClient, times(2)).reconstructURI(serviceInstance, URI.create("http://test/path")); + verify(delegate, times(2)).execute(any(), any()); + } + + @Test + void shouldPassCorrectRequestToDelegate() throws IOException { + Request request = testRequest(); + Request.Options options = new Request.Options(); + String url = "http://127.0.0.1/path"; + ServiceInstance serviceInstance = new DefaultServiceInstance("test-1", "test", "test-host", 8888, false); + when(loadBalancerClient.choose(eq("test"), any())).thenReturn(serviceInstance); + when(loadBalancerClient.reconstructURI(serviceInstance, URI.create("http://test/path"))) + .thenReturn(URI.create(url)); + Response response = testResponse(200); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy(properties)); + + feignBlockingLoadBalancerClient.execute(request, options); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Request.class); + verify(delegate, times(1)).execute(captor.capture(), eq(options)); + Request actualRequest = captor.getValue(); + assertThat(actualRequest.httpMethod()).isEqualTo(Request.HttpMethod.GET); + assertThat(actualRequest.url()).isEqualTo(url); + assertThat(actualRequest.headers()).hasSize(1); + assertThat(actualRequest.headers()).containsEntry(HttpHeaders.CONTENT_TYPE, + Collections.singletonList(MediaType.APPLICATION_JSON_VALUE)); + assertThat(new String(actualRequest.body())).isEqualTo("hello"); + } + + private Request testRequest() { + return testRequest("test"); + } + + private Request testRequest(String host) { + return Request.create(Request.HttpMethod.GET, "http://" + host + "/path", testHeaders(), "hello".getBytes(), + StandardCharsets.UTF_8, null); + } + + private Map> testHeaders() { + Map> feignHeaders = new HashMap<>(); + feignHeaders.put(HttpHeaders.CONTENT_TYPE, Collections.singletonList(MediaType.APPLICATION_JSON_VALUE)); + return feignHeaders; + + } + +} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/ApacheHttpClientConfigurationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/ApacheHttpClientConfigurationTests.java index cc13b1e9..719670dc 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/ApacheHttpClientConfigurationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/ApacheHttpClientConfigurationTests.java @@ -33,8 +33,7 @@ import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; import org.apache.http.message.BasicHeader; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.Test; import org.mockito.MockingDetails; import org.mockito.Mockito; @@ -52,7 +51,6 @@ import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalance import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.util.ReflectionUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -63,11 +61,11 @@ import static org.mockito.Mockito.mockingDetails; /** * @author Ryan Baxter + * @author Olga Maciaszek-Sharma */ -@RunWith(SpringJUnit4ClassRunner.class) -@SpringBootTest(properties = { "feign.okhttp.enabled: false" }) +@SpringBootTest(properties = { "feign.okhttp.enabled: false", "spring.cloud.loadbalancer.retry.enabled=false" }) @DirtiesContext -public class ApacheHttpClientConfigurationTests { +class ApacheHttpClientConfigurationTests { @Autowired ApacheHttpClientConnectionManagerFactory connectionManagerFactory; @@ -79,18 +77,18 @@ public class ApacheHttpClientConfigurationTests { FeignBlockingLoadBalancerClient feignClient; @Test - public void testFactories() { - assertThat(this.connectionManagerFactory).isInstanceOf(ApacheHttpClientConnectionManagerFactory.class); - assertThat(this.connectionManagerFactory) + void testFactories() { + assertThat(connectionManagerFactory).isInstanceOf(ApacheHttpClientConnectionManagerFactory.class); + assertThat(connectionManagerFactory) .isInstanceOf(ApacheHttpClientConfigurationTestApp.MyApacheHttpClientConnectionManagerFactory.class); - assertThat(this.httpClientFactory).isInstanceOf(ApacheHttpClientFactory.class); - assertThat(this.httpClientFactory) + assertThat(httpClientFactory).isInstanceOf(ApacheHttpClientFactory.class); + assertThat(httpClientFactory) .isInstanceOf(ApacheHttpClientConfigurationTestApp.MyApacheHttpClientFactory.class); } @Test - public void testHttpClientWithFeign() { - Client delegate = this.feignClient.getDelegate(); + void testHttpClientWithFeign() { + Client delegate = feignClient.getDelegate(); assertThat(ApacheHttpClient.class.isInstance(delegate)).isTrue(); ApacheHttpClient apacheHttpClient = (ApacheHttpClient) delegate; HttpClient httpClient = getField(apacheHttpClient, "client"); diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/OkHttpClientConfigurationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/OkHttpClientConfigurationTests.java index 40993f5b..11adc9b9 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/OkHttpClientConfigurationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/test/OkHttpClientConfigurationTests.java @@ -21,8 +21,7 @@ import java.util.concurrent.TimeUnit; import feign.Client; import okhttp3.ConnectionPool; import okhttp3.OkHttpClient; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.Test; import org.mockito.MockingDetails; import org.springframework.beans.factory.annotation.Autowired; @@ -37,7 +36,6 @@ import org.springframework.cloud.openfeign.FeignClient; import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; import org.springframework.context.annotation.Bean; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringRunner; import org.springframework.test.util.ReflectionTestUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -46,13 +44,14 @@ import static org.mockito.Mockito.mockingDetails; /** * @author Ryan Baxter + * @author Olga Maciaszek-Sharma */ -@RunWith(SpringRunner.class) @SpringBootTest(properties = { "feign.okhttp.enabled: true", "spring.cloud.httpclientfactories.ok.enabled: true", "ribbon.eureka.enabled = false", "ribbon.okhttp.enabled: true", "feign.okhttp.enabled: true", - "ribbon.httpclient.enabled: false", "feign.httpclient.enabled: false" }) + "ribbon.httpclient.enabled: false", "feign.httpclient.enabled: false", + "spring.cloud.loadbalancer.retry.enabled=false" }) @DirtiesContext -public class OkHttpClientConfigurationTests { +class OkHttpClientConfigurationTests { @Autowired OkHttpClientFactory okHttpClientFactory; @@ -64,16 +63,16 @@ public class OkHttpClientConfigurationTests { FeignBlockingLoadBalancerClient feignClient; @Test - public void testFactories() { - assertThat(this.connectionPoolFactory).isInstanceOf(OkHttpClientConnectionPoolFactory.class); - assertThat(this.connectionPoolFactory).isInstanceOf(TestConfig.MyOkHttpClientConnectionPoolFactory.class); - assertThat(this.okHttpClientFactory).isInstanceOf(OkHttpClientFactory.class); - assertThat(this.okHttpClientFactory).isInstanceOf(TestConfig.MyOkHttpClientFactory.class); + void testFactories() { + assertThat(connectionPoolFactory).isInstanceOf(OkHttpClientConnectionPoolFactory.class); + assertThat(connectionPoolFactory).isInstanceOf(TestConfig.MyOkHttpClientConnectionPoolFactory.class); + assertThat(okHttpClientFactory).isInstanceOf(OkHttpClientFactory.class); + assertThat(okHttpClientFactory).isInstanceOf(TestConfig.MyOkHttpClientFactory.class); } @Test - public void testHttpClientWithFeign() { - Client delegate = this.feignClient.getDelegate(); + void testHttpClientWithFeign() { + Client delegate = feignClient.getDelegate(); assertThat(feign.okhttp.OkHttpClient.class.isInstance(delegate)).isTrue(); feign.okhttp.OkHttpClient okHttpClient = (feign.okhttp.OkHttpClient) delegate; OkHttpClient httpClient = getField(okHttpClient, "delegate"); diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignHttpClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignHttpClientTests.java index 6911afff..8c26b296 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignHttpClientTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignHttpClientTests.java @@ -19,8 +19,7 @@ package org.springframework.cloud.openfeign.valid; import java.util.Objects; import feign.Client; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; @@ -41,7 +40,6 @@ import org.springframework.core.env.Environment; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringRunner; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestHeader; @@ -53,15 +51,13 @@ import static org.assertj.core.api.Assertions.assertThat; /** * @author Spencer Gibb + * @author Olga Maciaszek-Sharma */ -@RunWith(SpringRunner.class) -@SpringBootTest(classes = FeignHttpClientTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, value = { - "spring.application.name=feignclienttest", "feign.hystrix.enabled=false", "feign.okhttp.enabled=false" }) +@SpringBootTest(classes = FeignHttpClientTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, + value = { "spring.application.name=feignclienttest", "feign.hystrix.enabled=false", + "feign.okhttp.enabled=false", "spring.cloud.loadbalancer.retry.enabled=false" }) @DirtiesContext -public class FeignHttpClientTests { - - @LocalServerPort - private int port = 0; +class FeignHttpClientTests { @Autowired private TestClient testClient; @@ -73,32 +69,32 @@ public class FeignHttpClientTests { private UserClient userClient; @Test - public void testSimpleType() { - Hello hello = this.testClient.getHello(); + void testSimpleType() { + Hello hello = testClient.getHello(); assertThat(hello).as("hello was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); } @Test - public void testPatch() { - ResponseEntity response = this.testClient.patchHello(new Hello("foo")); + void testPatch() { + ResponseEntity response = testClient.patchHello(new Hello("foo")); assertThat(response).isNotNull(); String header = response.getHeaders().getFirst("x-hello"); assertThat(header).isEqualTo("hello world patch"); } @Test - public void testFeignClientType() throws IllegalAccessException { - assertThat(this.feignClient).isInstanceOf(FeignBlockingLoadBalancerClient.class); - FeignBlockingLoadBalancerClient client = (FeignBlockingLoadBalancerClient) this.feignClient; + void testFeignClientType() { + assertThat(feignClient).isInstanceOf(FeignBlockingLoadBalancerClient.class); + FeignBlockingLoadBalancerClient client = (FeignBlockingLoadBalancerClient) feignClient; Client delegate = client.getDelegate(); assertThat(delegate).isInstanceOf(feign.httpclient.ApacheHttpClient.class); } @Test - public void testFeignInheritanceSupport() { - assertThat(this.userClient).as("UserClient was null").isNotNull(); - final User user = this.userClient.getUser(1); + void testFeignInheritanceSupport() { + assertThat(userClient).as("UserClient was null").isNotNull(); + final User user = userClient.getUser(1); assertThat(user).as("Returned user was null").isNotNull(); assertThat(new User("John Smith")).as("Users were different").isEqualTo(user); } @@ -167,15 +163,15 @@ public class FeignHttpClientTests { private String message; - public Hello() { + Hello() { } - public Hello(String message) { + Hello(String message) { this.message = message; } public String getMessage() { - return this.message; + return message; } public void setMessage(String message) { @@ -191,12 +187,12 @@ public class FeignHttpClientTests { return false; } Hello that = (Hello) o; - return Objects.equals(this.message, that.message); + return Objects.equals(message, that.message); } @Override public int hashCode() { - return Objects.hash(this.message); + return Objects.hash(message); } } @@ -205,15 +201,15 @@ public class FeignHttpClientTests { private String name; - public User() { + User() { } - public User(String name) { + User(String name) { this.name = name; } public String getName() { - return this.name; + return name; } public void setName(String name) { @@ -229,12 +225,12 @@ public class FeignHttpClientTests { return false; } User that = (User) o; - return Objects.equals(this.name, that.name); + return Objects.equals(name, that.name); } @Override public int hashCode() { - return Objects.hash(this.name); + return Objects.hash(name); } } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignOkHttpTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignOkHttpTests.java index 9c7aeeb4..6fad1bc4 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignOkHttpTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignOkHttpTests.java @@ -19,11 +19,9 @@ package org.springframework.cloud.openfeign.valid; import java.util.Objects; import feign.Client; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.SpringBootTest.WebEnvironment; @@ -41,7 +39,6 @@ import org.springframework.context.annotation.Import; import org.springframework.core.env.Environment; import org.springframework.http.ResponseEntity; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestHeader; @@ -53,17 +50,14 @@ import static org.assertj.core.api.Assertions.assertThat; /** * @author Spencer Gibb + * @author Olga Maciaszek-Sharma */ -@RunWith(SpringJUnit4ClassRunner.class) @SpringBootTest(classes = FeignOkHttpTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, value = { "spring.application.name=feignclienttest", "feign.hystrix.enabled=false", "feign.httpclient.enabled=false", "feign.okhttp.enabled=true", - "spring.cloud.httpclientfactories.ok.enabled=true" }) + "spring.cloud.httpclientfactories.ok.enabled=true", "spring.cloud.loadbalancer.retry.enabled=false" }) @DirtiesContext -public class FeignOkHttpTests { - - @Value("${local.server.port}") - private int port = 0; +class FeignOkHttpTests { @Autowired private TestClient testClient; @@ -75,32 +69,32 @@ public class FeignOkHttpTests { private UserClient userClient; @Test - public void testSimpleType() { - Hello hello = this.testClient.getHello(); + void testSimpleType() { + Hello hello = testClient.getHello(); assertThat(hello).as("hello was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); } @Test - public void testPatch() { - ResponseEntity response = this.testClient.patchHello(new Hello("foo")); + void testPatch() { + ResponseEntity response = testClient.patchHello(new Hello("foo")); assertThat(response).isNotNull(); String header = response.getHeaders().getFirst("x-hello"); assertThat(header).isEqualTo("hello world patch"); } @Test - public void testFeignClientType() throws IllegalAccessException { - assertThat(this.feignClient).isInstanceOf(FeignBlockingLoadBalancerClient.class); - FeignBlockingLoadBalancerClient client = (FeignBlockingLoadBalancerClient) this.feignClient; + void testFeignClientType() { + assertThat(feignClient).isInstanceOf(FeignBlockingLoadBalancerClient.class); + FeignBlockingLoadBalancerClient client = (FeignBlockingLoadBalancerClient) feignClient; Client delegate = client.getDelegate(); assertThat(delegate).isInstanceOf(feign.okhttp.OkHttpClient.class); } @Test - public void testFeignInheritanceSupport() { - assertThat(this.userClient).as("UserClient was null").isNotNull(); - final User user = this.userClient.getUser(1); + void testFeignInheritanceSupport() { + assertThat(userClient).as("UserClient was null").isNotNull(); + final User user = userClient.getUser(1); assertThat(user).as("Returned user was null").isNotNull(); assertThat(new User("John Smith")).as("Users were different").isEqualTo(user); } @@ -171,15 +165,15 @@ public class FeignOkHttpTests { private String message; - public Hello() { + Hello() { } - public Hello(String message) { + Hello(String message) { this.message = message; } public String getMessage() { - return this.message; + return message; } public void setMessage(String message) { @@ -195,12 +189,12 @@ public class FeignOkHttpTests { return false; } Hello that = (Hello) o; - return Objects.equals(this.message, that.message); + return Objects.equals(message, that.message); } @Override public int hashCode() { - return Objects.hash(this.message); + return Objects.hash(message); } } @@ -209,15 +203,15 @@ public class FeignOkHttpTests { private String name; - public User() { + User() { } - public User(String name) { + User(String name) { this.name = name; } public String getName() { - return this.name; + return name; } public void setName(String name) { @@ -233,12 +227,12 @@ public class FeignOkHttpTests { return false; } User that = (User) o; - return Objects.equals(this.name, that.name); + return Objects.equals(name, that.name); } @Override public int hashCode() { - return Objects.hash(this.name); + return Objects.hash(name); } } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/ValidFeignClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/ValidFeignClientTests.java index 00bbd988..d3c32308 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/ValidFeignClientTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/ValidFeignClientTests.java @@ -40,14 +40,10 @@ import feign.Logger; import feign.RequestInterceptor; import feign.RequestTemplate; import feign.codec.EncodeException; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.ExpectedException; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; -import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.builder.SpringApplicationBuilder; import org.springframework.boot.test.context.SpringBootTest; @@ -76,7 +72,6 @@ import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.mock.web.MockMultipartFile; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestMapping; @@ -87,7 +82,7 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.web.multipart.MultipartFile; import static org.assertj.core.api.Assertions.assertThat; -import static org.hamcrest.Matchers.instanceOf; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; /** * @author Spencer Gibb @@ -96,14 +91,15 @@ import static org.hamcrest.Matchers.instanceOf; * @author Halvdan Hoem Grelland * @author Aaron Whiteside * @author Darren Foong + * @author Olga Maciaszek-Sharma */ -@RunWith(SpringJUnit4ClassRunner.class) @SpringBootTest(classes = ValidFeignClientTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, value = { "spring.application.name=feignclienttest", "logging.level.org.springframework.cloud.openfeign.valid=DEBUG", "feign.httpclient.enabled=false", - "feign.okhttp.enabled=false", "feign.hystrix.enabled=true" }) + "feign.okhttp.enabled=false", "feign.hystrix.enabled=true", + "spring.cloud.loadbalancer.retry.enabled=false" }) @DirtiesContext -public class ValidFeignClientTests { +class ValidFeignClientTests { public static final String HELLO_WORLD_1 = "hello world 1"; @@ -113,12 +109,6 @@ public class ValidFeignClientTests { public static final String MYHEADER2 = "myheader2"; - @Rule - public ExpectedException expected = ExpectedException.none(); - - @Value("${local.server.port}") - private int port = 0; - @Autowired private TestClient testClient; @@ -146,104 +136,104 @@ public class ValidFeignClientTests { } @Test - public void testClient() { - assertThat(this.testClient).as("testClient was null").isNotNull(); - assertThat(Proxy.isProxyClass(this.testClient.getClass())).as("testClient is not a java Proxy").isTrue(); - InvocationHandler invocationHandler = Proxy.getInvocationHandler(this.testClient); + void testClient() { + assertThat(testClient).as("testClient was null").isNotNull(); + assertThat(Proxy.isProxyClass(testClient.getClass())).as("testClient is not a java Proxy").isTrue(); + InvocationHandler invocationHandler = Proxy.getInvocationHandler(testClient); assertThat(invocationHandler).as("invocationHandler was null").isNotNull(); } @Test - public void testRequestMappingClassLevelPropertyReplacement() { - Hello hello = this.testClient.getHelloUsingPropertyPlaceHolder(); + void testRequestMappingClassLevelPropertyReplacement() { + Hello hello = testClient.getHelloUsingPropertyPlaceHolder(); assertThat(hello).as("hello was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello(OI_TERRA_2)); } @Test public void testSimpleType() { - Hello hello = this.testClient.getHello(); + Hello hello = testClient.getHello(); assertThat(hello).as("hello was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello(HELLO_WORLD_1)); } @Test - public void testOptional() { - Optional hello = this.testClient.getOptionalHello(); + void testOptional() { + Optional hello = testClient.getOptionalHello(); assertThat(hello).isNotNull().isPresent().contains(new Hello(HELLO_WORLD_1)); } @Test - public void testGenericType() { - List hellos = this.testClient.getHellos(); + void testGenericType() { + List hellos = testClient.getHellos(); assertThat(hellos).as("hellos was null").isNotNull(); assertThat(getHelloList()).as("hellos didn't match").isEqualTo(hellos); } @Test - public void testRequestInterceptors() { - List headers = this.testClient.getHelloHeaders(); + void testRequestInterceptors() { + List headers = testClient.getHelloHeaders(); assertThat(headers).as("headers was null").isNotNull(); assertThat(headers.contains("myheader1value")).as("headers didn't contain myheader1value").isTrue(); assertThat(headers.contains("myheader2value")).as("headers didn't contain myheader2value").isTrue(); } @Test - public void testHeaderPlaceholders() { - String header = this.testClient.getHelloHeadersPlaceholders(); + void testHeaderPlaceholders() { + String header = testClient.getHelloHeadersPlaceholders(); assertThat(header).as("header was null").isNotNull(); assertThat(header).as("header was wrong").isEqualTo("myPlaceholderHeaderValue"); } @Test - public void testFeignClientType() throws IllegalAccessException { - assertThat(this.feignClient).isInstanceOf(FeignBlockingLoadBalancerClient.class); - FeignBlockingLoadBalancerClient client = (FeignBlockingLoadBalancerClient) this.feignClient; + void testFeignClientType() { + assertThat(feignClient).isInstanceOf(FeignBlockingLoadBalancerClient.class); + FeignBlockingLoadBalancerClient client = (FeignBlockingLoadBalancerClient) feignClient; Client delegate = client.getDelegate(); assertThat(delegate).isInstanceOf(Client.Default.class); } @Test - public void testServiceId() { - assertThat(this.testClientServiceId).as("testClientServiceId was null").isNotNull(); - final Hello hello = this.testClientServiceId.getHello(); + void testServiceId() { + assertThat(testClientServiceId).as("testClientServiceId was null").isNotNull(); + final Hello hello = testClientServiceId.getHello(); assertThat(hello).as("The hello response was null").isNotNull(); assertThat(hello).as("first hello didn't match").isEqualTo(new Hello(HELLO_WORLD_1)); } @Test - public void testParams() { + void testParams() { List list = Arrays.asList("a", "1", "test"); - List params = this.testClient.getParams(list); + List params = testClient.getParams(list); assertThat(params).as("params was null").isNotNull(); assertThat(params.size()).as("params size was wrong").isEqualTo(list.size()); } @Test - public void testFormattedParams() { + void testFormattedParams() { List list = Arrays.asList(LocalDate.of(2001, 1, 1), LocalDate.of(2018, 6, 10)); - List params = this.testClient.getFormattedParams(list); + List params = testClient.getFormattedParams(list); assertThat(params).as("params was null").isNotNull(); assertThat(params).as("params not converted correctly").isEqualTo(list); } @Test - public void testNoContentResponse() { - ResponseEntity response = this.testClient.noContent(); + void testNoContentResponse() { + ResponseEntity response = testClient.noContent(); assertThat(response).as("response was null").isNotNull(); assertThat(response.getStatusCode()).as("status code was wrong").isEqualTo(HttpStatus.NO_CONTENT); } @Test - public void testHeadResponse() { - ResponseEntity response = this.testClient.head(); + void testHeadResponse() { + ResponseEntity response = testClient.head(); assertThat(response).as("response was null").isNotNull(); assertThat(response.getStatusCode()).as("status code was wrong").isEqualTo(HttpStatus.OK); } @Test - public void testHttpEntity() { - HttpEntity entity = this.testClient.getHelloEntity(); + void testHttpEntity() { + HttpEntity entity = testClient.getHelloEntity(); assertThat(entity).as("entity was null").isNotNull(); Hello hello = entity.getBody(); assertThat(hello).as("hello was null").isNotNull(); @@ -251,129 +241,130 @@ public class ValidFeignClientTests { } @Test - public void testMoreComplexHeader() { - String response = this.testClient.moreComplexContentType("{\"value\":\"OK\"}"); + void testMoreComplexHeader() { + String response = testClient.moreComplexContentType("{\"value\":\"OK\"}"); assertThat(response).as("response was null").isNotNull(); assertThat(response).as("didn't respond with {\"value\":\"OK\"}").isEqualTo("{\"value\":\"OK\"}"); } @Test - public void testDecodeNotFound() { - ResponseEntity response = this.decodingTestClient.notFound(); + void testDecodeNotFound() { + ResponseEntity response = decodingTestClient.notFound(); assertThat(response).as("response was null").isNotNull(); assertThat(response.getStatusCode()).as("status code was wrong").isEqualTo(HttpStatus.NOT_FOUND); assertThat(response.getBody()).as("response body was not null").isNull(); } @Test - public void testOptionalNotFound() { - Optional s = this.decodingTestClient.optional(); + void testOptionalNotFound() { + Optional s = decodingTestClient.optional(); assertThat(s).isNotPresent(); } @Test - public void testConvertingExpander() { - assertThat(this.testClient.getToString(Arg.A)).isEqualTo(Arg.A.toString()); - assertThat(this.testClient.getToString(Arg.B)).isEqualTo(Arg.B.toString()); + void testConvertingExpander() { + assertThat(testClient.getToString(Arg.A)).isEqualTo(Arg.A.toString()); + assertThat(testClient.getToString(Arg.B)).isEqualTo(Arg.B.toString()); - assertThat(this.testClient.getToString(new OtherArg("foo"))).isEqualTo("bar"); + assertThat(testClient.getToString(new OtherArg("foo"))).isEqualTo("bar"); List args = new ArrayList<>(); args.add(new OtherArg("foo")); args.add(new OtherArg("goo")); List expectedResult = new ArrayList<>(); expectedResult.add("bar"); expectedResult.add("goo"); - assertThat(this.testClient.getToString(args)).isEqualTo(expectedResult); + assertThat(testClient.getToString(args)).isEqualTo(expectedResult); } @Test - public void namedFeignClientWorks() { - assertThat(this.namedFeignClient).as("namedFeignClient was null").isNotNull(); + void namedFeignClientWorks() { + assertThat(namedFeignClient).as("namedFeignClient was null").isNotNull(); } @Test - public void testSingleRequestPart() { - String response = this.multipartClient.singlePart("abc"); + void testSingleRequestPart() { + String response = multipartClient.singlePart("abc"); assertThat(response).isEqualTo("abc"); } @Test - public void testSinglePojoRequestPart() { - String response = this.multipartClient.singlePojoPart(new Hello(HELLO_WORLD_1)); + void testSinglePojoRequestPart() { + String response = multipartClient.singlePojoPart(new Hello(HELLO_WORLD_1)); assertThat(response).isEqualTo(HELLO_WORLD_1); } @Test - public void testMultipleRequestParts() { + void testMultipleRequestParts() { MockMultipartFile file = new MockMultipartFile("file", "hello.bin", null, "hello".getBytes()); - String response = this.multipartClient.multipart("abc", "123", file); + String response = multipartClient.multipart("abc", "123", file); assertThat(response).isEqualTo("abc123hello.bin"); } @Test - public void testMultiplePojoRequestParts() { + void testMultiplePojoRequestParts() { Hello pojo1 = new Hello(HELLO_WORLD_1); Hello pojo2 = new Hello(OI_TERRA_2); MockMultipartFile file = new MockMultipartFile("file", "hello.bin", null, "hello".getBytes()); - String response = this.multipartClient.multipartPojo("abc", "123", pojo1, pojo2, file); + String response = multipartClient.multipartPojo("abc", "123", pojo1, pojo2, file); assertThat(response).isEqualTo("abc123hello world 1oi terra 2hello.bin"); } @Test - public void testRequestPartWithListOfMultipartFiles() { + void testRequestPartWithListOfMultipartFiles() { List multipartFiles = Arrays.asList( new MockMultipartFile("file1", "hello1.bin", null, "hello".getBytes()), new MockMultipartFile("file2", "hello2.bin", null, "hello".getBytes())); - String partNames = this.multipartClient.requestPartListOfMultipartFilesReturnsPartNames(multipartFiles); + String partNames = multipartClient.requestPartListOfMultipartFilesReturnsPartNames(multipartFiles); assertThat(partNames).isEqualTo("files,files"); - String fileNames = this.multipartClient.requestPartListOfMultipartFilesReturnsFileNames(multipartFiles); + String fileNames = multipartClient.requestPartListOfMultipartFilesReturnsFileNames(multipartFiles); assertThat(fileNames).contains("hello1.bin", "hello2.bin"); } @Test - public void testRequestPartWithListOfPojosAndListOfMultipartFiles() { + void testRequestPartWithListOfPojosAndListOfMultipartFiles() { Hello pojo1 = new Hello(HELLO_WORLD_1); Hello pojo2 = new Hello(OI_TERRA_2); MockMultipartFile file1 = new MockMultipartFile("file1", "hello1.bin", null, "hello".getBytes()); MockMultipartFile file2 = new MockMultipartFile("file2", "hello2.bin", null, "hello".getBytes()); - String response = this.multipartClient.requestPartListOfPojosAndListOfMultipartFiles( - Arrays.asList(pojo1, pojo2), Arrays.asList(file1, file2)); + String response = multipartClient.requestPartListOfPojosAndListOfMultipartFiles(Arrays.asList(pojo1, pojo2), + Arrays.asList(file1, file2)); assertThat(response).isEqualTo("hello world 1oi terra 2hello1.binhello2.bin"); } @Test - public void testRequestBodyWithSingleMultipartFile() { + void testRequestBodyWithSingleMultipartFile() { String partName = UUID.randomUUID().toString(); MockMultipartFile file1 = new MockMultipartFile(partName, "hello1.bin", null, "hello".getBytes()); - String response = this.multipartClient.requestBodySingleMultipartFile(file1); + String response = multipartClient.requestBodySingleMultipartFile(file1); assertThat(response).isEqualTo(partName); } @Test - public void testRequestBodyWithListOfMultipartFiles() { + void testRequestBodyWithListOfMultipartFiles() { MockMultipartFile file1 = new MockMultipartFile("file1", "hello1.bin", null, "hello".getBytes()); MockMultipartFile file2 = new MockMultipartFile("file2", "hello2.bin", null, "hello".getBytes()); - String response = this.multipartClient.requestBodyListOfMultipartFiles(Arrays.asList(file1, file2)); + String response = multipartClient.requestBodyListOfMultipartFiles(Arrays.asList(file1, file2)); assertThat(response).contains("file1", "file2"); } @Test - public void testRequestBodyWithMap() { + void testRequestBodyWithMap() { MockMultipartFile file1 = new MockMultipartFile("file1", "hello1.bin", null, "hello".getBytes()); MockMultipartFile file2 = new MockMultipartFile("file2", "hello2.bin", null, "hello".getBytes()); Map form = new HashMap<>(); form.put("file1", file1); form.put("file2", file2); form.put("hello", "world"); - String response = this.multipartClient.requestBodyMap(form); + String response = multipartClient.requestBodyMap(form); assertThat(response).contains("file1", "file2", "hello"); } @Test - public void testInvalidMultipartFile() { - MockMultipartFile file = new MockMultipartFile("file1", "hello1.bin", null, "hello".getBytes()); - expected.expect(instanceOf(EncodeException.class)); - this.multipartClient.invalid(file); + void testInvalidMultipartFile() { + assertThatExceptionOfType(EncodeException.class).isThrownBy(() -> { + MockMultipartFile file = new MockMultipartFile("file1", "hello1.bin", null, "hello".getBytes()); + multipartClient.invalid(file); + }); } protected enum Arg { @@ -525,7 +516,7 @@ public class ValidFeignClientTests { @Override public String toString() { - return this.value; + return value; } } @@ -765,15 +756,15 @@ public class ValidFeignClientTests { private String message; - public Hello() { + Hello() { } - public Hello(String message) { + Hello(String message) { this.message = message; } public String getMessage() { - return this.message; + return message; } public void setMessage(String message) { @@ -789,12 +780,12 @@ public class ValidFeignClientTests { return false; } Hello that = (Hello) o; - return Objects.equals(this.message, that.message); + return Objects.equals(message, that.message); } @Override public int hashCode() { - return Objects.hash(this.message); + return Objects.hash(message); } }