diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java index 3807e6401..6e7915230 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java @@ -20,6 +20,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.TimeUnit; import feign.Capability; @@ -63,6 +64,7 @@ * @author Ilia Ilinykh * @author Marcin Grzejszczak * @author Jonatan Ivanov + * @author Benjamin Einaudi */ public class FeignClientFactoryBean implements FactoryBean, InitializingBean, ApplicationContextAware, BeanFactoryAware { @@ -322,18 +324,6 @@ protected Map getInheritedAwareInstances(FeignContext context, Cl } } - protected T loadBalance(Feign.Builder builder, FeignContext context, HardCodedTarget target) { - Client client = getOptional(context, Client.class); - if (client != null) { - builder.client(client); - Targeter targeter = get(context, Targeter.class); - return targeter.target(this, builder, context, target); - } - - throw new IllegalStateException( - "No Feign Client for loadBalancing defined. Did you forget to include spring-cloud-starter-loadbalancer?"); - } - @Override public Object getObject() { return getTarget(); @@ -344,27 +334,27 @@ public Object getObject() { * @return a {@link Feign} client created with the specified data and the context * information */ + @SuppressWarnings("unchecked") T getTarget() { FeignContext context = beanFactory != null ? beanFactory.getBean(FeignContext.class) : applicationContext.getBean(FeignContext.class); Feign.Builder builder = feign(context); + configureClient(context, builder); + String targetUrl = targetUrl(); + Targeter targeter = get(context, Targeter.class); + return (T) targeter.target(this, builder, context, new HardCodedTarget<>(type, name, targetUrl + cleanPath())); + } - if (!StringUtils.hasText(url)) { - if (!name.startsWith("http")) { - url = "http://" + name; - } - else { - url = name; + private void configureClient(FeignContext context, Feign.Builder builder) { + Client client = getOptional(context, Client.class); + if (client == null) { + if (loadBalanceRequired()) { + throw new IllegalStateException( + "No Feign Client for loadBalancing defined. Did you forget to include spring-cloud-starter-loadbalancer?"); } - url += cleanPath(); - return (T) loadBalance(builder, context, new HardCodedTarget<>(type, name, url)); - } - if (StringUtils.hasText(url) && !url.startsWith("http")) { - url = "http://" + url; + return; } - String url = this.url + cleanPath(); - Client client = getOptional(context, Client.class); - if (client != null) { + if (!loadBalanceRequired()) { if (client instanceof FeignBlockingLoadBalancerClient) { // not load balancing because we have a url, // but Spring Cloud LoadBalancer is on the classpath, so unwrap @@ -375,10 +365,21 @@ T getTarget() { // but Spring Cloud LoadBalancer is on the classpath, so unwrap client = ((RetryableFeignBlockingLoadBalancerClient) client).getDelegate(); } - builder.client(client); } - Targeter targeter = get(context, Targeter.class); - return (T) targeter.target(this, builder, context, new HardCodedTarget<>(type, name, url)); + builder.client(client); + } + + private boolean loadBalanceRequired() { + return !StringUtils.hasText(url) || url.matches("^lb[s]?://.*"); + } + + private String targetUrl() { + return normalizeProtocol(Optional.ofNullable(url).filter(StringUtils::hasText).orElse(name)); + } + + private String normalizeProtocol(String url) { + String unbalancedUrl = url.replaceAll("^lb([s]?)://", "http$1://"); + return unbalancedUrl.startsWith("http") ? unbalancedUrl : "http://" + unbalancedUrl; } private String cleanPath() { diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignBuilderCustomizerTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignBuilderCustomizerTests.java index b08038b3c..ad545422f 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignBuilderCustomizerTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignBuilderCustomizerTests.java @@ -78,7 +78,7 @@ private static FeignClientFactoryBean defaultFeignClientFactoryBean() { FeignClientFactoryBean feignClientFactoryBean = new FeignClientFactoryBean(); feignClientFactoryBean.setContextId("test"); feignClientFactoryBean.setName("test"); - feignClientFactoryBean.setType(FeignClientFactoryTests.TestType.class); + feignClientFactoryBean.setType(FeignClientFactoryBeanTests.TestType.class); feignClientFactoryBean.setPath(""); feignClientFactoryBean.setUrl("http://some.absolute.url"); return feignClientFactoryBean; diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryBeanTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryBeanTests.java new file mode 100644 index 000000000..f5b2eff96 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryBeanTests.java @@ -0,0 +1,214 @@ +/* + * 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; + +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Map; + +import feign.Client; +import feign.Client.Default; +import feign.InvocationHandlerFactory; +import feign.InvocationHandlerFactory.MethodHandler; +import feign.Target; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledForJreRange; +import org.junit.jupiter.api.condition.JRE; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import org.springframework.boot.test.context.assertj.AssertableApplicationContext; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.cloud.client.loadbalancer.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; +import org.springframework.cloud.loadbalancer.config.LoadBalancerAutoConfiguration; +import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; +import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.web.bind.annotation.RequestMapping; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.springframework.web.bind.annotation.RequestMethod.GET; + +/** + * @author Spencer Gibb + * @author Benjamin Einaudi + */ +public class FeignClientFactoryBeanTests { + + public static final String FACTORY_NAME = "test"; + + @ParameterizedTest + @ValueSource(strings = { "/some/path", "/some/path/", "some/path", "some/path/" }) + public void shouldNormalizePathWhenUrlNotProvided(String configurationPath) { + new ApplicationContextRunner().withUserConfiguration(LoadBalancerTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean("", configurationPath)) + .run(context -> assertThat(target(buildMethodHandler(context)).url()) + .isEqualTo("http://" + FACTORY_NAME + "/some/path")); + } + + @ParameterizedTest + @ValueSource(strings = { "/some/path", "/some/path/", "some/path", "some/path/" }) + public void shouldNormalizePathWhenUrlProvided(String configurationPath) { + new ApplicationContextRunner().withUserConfiguration(LoadBalancerTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean("domain.org", configurationPath)) + .run(context -> assertThat(target(buildMethodHandler(context)).url()) + .isEqualTo("http://domain.org/some/path")); + } + + @Test + @DisabledForJreRange(min = JRE.JAVA_16) + @SuppressWarnings({ "ConstantConditions" }) + public void shouldPrependProtocolToNameWhenUrlNotSetAndUseProvidedLoadBalanceClient() { + new ApplicationContextRunner().withUserConfiguration(LoadBalancerTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean("", "")).run(context -> { + MethodHandler methodHandler = buildMethodHandler(context); + assertThat(target(methodHandler).url()).isEqualTo("http://" + FACTORY_NAME); + assertThat(client(methodHandler)).isInstanceOf(FeignBlockingLoadBalancerClient.class); + }); + } + + @ParameterizedTest + @ValueSource(strings = { "", "lb://some-service-name", "lbs://some-secured-service-name" }) + @DisabledForJreRange(min = JRE.JAVA_16) + public void shouldFailWhenNoLoadBalanceClientProvidedAndLoadBalanceRequired(String configuredUrl) { + new ApplicationContextRunner().withUserConfiguration(NoClientTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean(configuredUrl, "")) + .run(context -> assertThatExceptionOfType(IllegalStateException.class) + .isThrownBy(() -> context.getBean(FeignClientFactoryBean.class).getTarget()) + .withMessageContaining("No Feign Client for loadBalancing defined")); + } + + @Test + @DisabledForJreRange(min = JRE.JAVA_16) + @SuppressWarnings({ "ConstantConditions" }) + public void shouldPrependProtocolToUrlWhenUrlSetWithoutProtocol() { + final String targetUrl = "some.absolute.url"; + new ApplicationContextRunner().withUserConfiguration(NoClientTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean(targetUrl, "")) + .run(context -> assertThat(target(buildMethodHandler(context)).url()).isEqualTo("http://" + targetUrl)); + } + + @Test + @DisabledForJreRange(min = JRE.JAVA_16) + public void shouldUseLoadBalanceClientWhenUrlUsesLoadBalanceProtocolAndOverrideUrl() { + new ApplicationContextRunner().withUserConfiguration(LoadBalancerTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean("lb://some-service-name", "")) + .run(context -> { + final MethodHandler methodHandler = buildMethodHandler(context); + assertThat(target(methodHandler).url()).isEqualTo("http://some-service-name"); + assertThat(client(methodHandler)).isInstanceOf(FeignBlockingLoadBalancerClient.class); + }); + } + + @Test + @DisabledForJreRange(min = JRE.JAVA_16) + public void shouldNotRequireLoadBalanceClientWhenUrlSetAndUseDefault() { + new ApplicationContextRunner().withUserConfiguration(NoClientTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean("http://some.absolute.url", "")) + .run(context -> assertThat(client(buildMethodHandler(context))).isInstanceOf(Client.Default.class)); + } + + @Test + @DisabledForJreRange(min = JRE.JAVA_16) + public void shouldRedirectToDelegateWhenUrlSet() { + new ApplicationContextRunner().withUserConfiguration(LoadBalancerTestConfig.class) + .withBean(FeignClientFactoryBean.class, () -> feignClientFactoryBean("http://some.absolute.url", "")) + .run(context -> assertThat(client(buildMethodHandler(context))).isInstanceOf(Client.Default.class)); + } + + @SuppressWarnings({ "unchecked", "ConstantConditions" }) + private MethodHandler buildMethodHandler(AssertableApplicationContext context) { + Proxy target = context.getBean(FeignClientFactoryBean.class).getTarget(); + Object invocationHandler = ReflectionTestUtils.getField(target, "h"); + Map dispatch = (Map) ReflectionTestUtils + .getField(invocationHandler, "dispatch"); + Method key = new ArrayList<>(dispatch.keySet()).get(0); + return dispatch.get(key); + } + + private Client client(MethodHandler methodHandler) { + return (Client) ReflectionTestUtils.getField(methodHandler, "client"); + } + + private Target target(MethodHandler methodHandler) { + return (Target) ReflectionTestUtils.getField(methodHandler, "target"); + } + + private FeignClientFactoryBean feignClientFactoryBean(String url, String path) { + FeignClientFactoryBean feignClientFactoryBean = new FeignClientFactoryBean(); + feignClientFactoryBean.setContextId("test"); + feignClientFactoryBean.setName(FeignClientFactoryBeanTests.FACTORY_NAME); + feignClientFactoryBean.setType(TestType.class); + feignClientFactoryBean.setPath(path); + feignClientFactoryBean.setUrl(url); + return feignClientFactoryBean; + } + + interface TestType { + + @RequestMapping(value = "/", method = GET) + String hello(); + + } + + static class AbstractTestConfig { + + @Bean + FeignContext feignContext() { + FeignContext feignContext = new FeignContext(); + feignContext.setConfigurations(Collections.singletonList( + new FeignClientSpecification("test", new Class[] { LoadBalancerAutoConfiguration.class }))); + return feignContext; + } + + @Bean + FeignClientProperties feignClientProperties() { + return new FeignClientProperties(); + } + + @Bean + Targeter targeter() { + return new DefaultTargeter(); + } + + } + + @Configuration + static class LoadBalancerTestConfig extends AbstractTestConfig { + + @Bean + Client loadBalancerClient() { + final BlockingLoadBalancerClient loadBalancerClient = new BlockingLoadBalancerClient( + new LoadBalancerClientFactory(), new LoadBalancerProperties()); + return new FeignBlockingLoadBalancerClient(new Default(null, null), loadBalancerClient, + new LoadBalancerProperties(), new LoadBalancerClientFactory()); + } + + } + + @Configuration + static class NoClientTestConfig extends AbstractTestConfig { + + } + +} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryTests.java deleted file mode 100644 index da4edd1ba..000000000 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryTests.java +++ /dev/null @@ -1,163 +0,0 @@ -/* - * 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; - -import java.lang.reflect.Method; -import java.lang.reflect.Proxy; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.Map; - -import feign.Client; -import feign.InvocationHandlerFactory; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.condition.DisabledForJreRange; -import org.junit.jupiter.api.condition.JRE; - -import org.springframework.boot.test.context.assertj.AssertableApplicationContext; -import org.springframework.boot.test.context.runner.ApplicationContextRunner; -import org.springframework.cloud.client.loadbalancer.LoadBalancerProperties; -import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; -import org.springframework.cloud.loadbalancer.config.LoadBalancerAutoConfiguration; -import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; -import org.springframework.context.annotation.AnnotationConfigApplicationContext; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.test.util.ReflectionTestUtils; -import org.springframework.web.bind.annotation.RequestMapping; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.springframework.web.bind.annotation.RequestMethod.GET; - -/** - * @author Spencer Gibb - */ -public class FeignClientFactoryTests { - - @Test - public void testChildContexts() { - AnnotationConfigApplicationContext parent = new AnnotationConfigApplicationContext(); - parent.refresh(); - FeignContext context = new FeignContext(); - context.setApplicationContext(parent); - context.setConfigurations(Arrays.asList(getSpec("foo", FooConfig.class), getSpec("bar", BarConfig.class))); - - Foo foo = context.getInstance("foo", Foo.class); - assertThat(foo).as("foo was null").isNotNull(); - - Bar bar = context.getInstance("bar", Bar.class); - assertThat(bar).as("bar was null").isNotNull(); - - Bar foobar = context.getInstance("foo", Bar.class); - assertThat(foobar).as("bar was not null").isNull(); - } - - @Test - @DisabledForJreRange(min = JRE.JAVA_16) - public void shouldRedirectToDelegateWhenUrlSet() { - new ApplicationContextRunner().withUserConfiguration(TestConfig.class).run(this::defaultClientUsed); - } - - @SuppressWarnings({ "unchecked", "ConstantConditions" }) - private void defaultClientUsed(AssertableApplicationContext context) { - Proxy target = context.getBean(FeignClientFactoryBean.class).getTarget(); - Object invocationHandler = ReflectionTestUtils.getField(target, "h"); - Map dispatch = (Map) ReflectionTestUtils - .getField(invocationHandler, "dispatch"); - Method key = new ArrayList<>(dispatch.keySet()).get(0); - Object client = ReflectionTestUtils.getField(dispatch.get(key), "client"); - assertThat(client).isInstanceOf(Client.Default.class); - } - - private FeignClientSpecification getSpec(String name, Class configClass) { - return new FeignClientSpecification(name, new Class[] { configClass }); - } - - interface TestType { - - @RequestMapping(value = "/", method = GET) - String hello(); - - } - - @Configuration - static class TestConfig { - - @Bean - BlockingLoadBalancerClient loadBalancerClient() { - return new BlockingLoadBalancerClient(new LoadBalancerClientFactory(), new LoadBalancerProperties()); - } - - @Bean - FeignContext feignContext() { - FeignContext feignContext = new FeignContext(); - feignContext.setConfigurations(Collections.singletonList( - new FeignClientSpecification("test", new Class[] { LoadBalancerAutoConfiguration.class }))); - return feignContext; - } - - @Bean - FeignClientProperties feignClientProperties() { - return new FeignClientProperties(); - } - - @Bean - Targeter targeter() { - return new DefaultTargeter(); - } - - @Bean - FeignClientFactoryBean feignClientFactoryBean() { - FeignClientFactoryBean feignClientFactoryBean = new FeignClientFactoryBean(); - feignClientFactoryBean.setContextId("test"); - feignClientFactoryBean.setName("test"); - feignClientFactoryBean.setType(TestType.class); - feignClientFactoryBean.setPath(""); - feignClientFactoryBean.setUrl("http://some.absolute.url"); - return feignClientFactoryBean; - } - - } - - static class FooConfig { - - @Bean - Foo foo() { - return new Foo(); - } - - } - - static class Foo { - - } - - static class BarConfig { - - @Bean - Bar bar() { - return new Bar(); - } - - } - - static class Bar { - - } - -} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignContextTest.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignContextTest.java index 072bfef73..ff7c5db23 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignContextTest.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignContextTest.java @@ -16,12 +16,13 @@ package org.springframework.cloud.openfeign; +import java.util.Arrays; import java.util.Collection; import feign.Logger; import feign.RequestInterceptor; import org.assertj.core.util.Lists; -import org.junit.Test; +import org.junit.jupiter.api.Test; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; @@ -32,6 +33,24 @@ public class FeignContextTest { + @Test + public void testChildContexts() { + AnnotationConfigApplicationContext parent = new AnnotationConfigApplicationContext(); + parent.refresh(); + FeignContext context = new FeignContext(); + context.setApplicationContext(parent); + context.setConfigurations(Arrays.asList(getSpec("foo", FooConfig.class), getSpec("bar", BarConfig.class))); + + Foo foo = context.getInstance("foo", Foo.class); + assertThat(foo).as("foo was null").isNotNull(); + + Bar bar = context.getInstance("bar", Bar.class); + assertThat(bar).as("bar was null").isNotNull(); + + Bar foobar = context.getInstance("foo", Bar.class); + assertThat(foobar).as("bar was not null").isNull(); + } + @Test public void getInstanceWithoutAncestors_verifyNullForMissing() { AnnotationConfigApplicationContext parent = new AnnotationConfigApplicationContext(); @@ -117,4 +136,30 @@ public RequestInterceptor requestInterceptor() { } + static class FooConfig { + + @Bean + Foo foo() { + return new Foo(); + } + + } + + static class Foo { + + } + + static class BarConfig { + + @Bean + Bar bar() { + return new Bar(); + } + + } + + static class Bar { + + } + }