From 06ccb9d1e596de7c7ba6b5ae7ebd32a05575a3fe Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Fri, 17 Aug 2018 10:23:37 -0400 Subject: [PATCH] Remove code which was calling FallbackFactory with RuntimeException. Fixes #41 (#58) --- .../cloud/openfeign/HystrixTargeter.java | 13 ---- .../invalid/FeignClientValidationTests.java | 40 ------------ .../openfeign/valid/FeignClientTests.java | 64 ++++++++++++++++++- 3 files changed, 62 insertions(+), 55 deletions(-) diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/HystrixTargeter.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/HystrixTargeter.java index d9925043..c2a488f5 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/HystrixTargeter.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/HystrixTargeter.java @@ -62,19 +62,6 @@ class HystrixTargeter implements Targeter { Class fallbackFactoryClass) { FallbackFactory fallbackFactory = (FallbackFactory) getFromContext("fallbackFactory", feignClientName, context, fallbackFactoryClass, FallbackFactory.class); - /* We take a sample fallback from the fallback factory to check if it returns a fallback - that is compatible with the annotated feign interface. */ - Object exampleFallback = fallbackFactory.create(new RuntimeException()); - Assert.notNull(exampleFallback, - String.format( - "Incompatible fallbackFactory instance for feign client %s. Factory may not produce null!", - feignClientName)); - if (!target.type().isAssignableFrom(exampleFallback.getClass())) { - throw new IllegalStateException( - String.format( - "Incompatible fallbackFactory instance for feign client %s. Factory produces instances of '%s', but should produce instances of '%s'", - feignClientName, exampleFallback.getClass(), target.type())); - } return builder.target(target, fallbackFactory); } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/invalid/FeignClientValidationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/invalid/FeignClientValidationTests.java index 9d4ca565..84dfe1c2 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/invalid/FeignClientValidationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/invalid/FeignClientValidationTests.java @@ -240,44 +240,4 @@ public class FeignClientValidationTests { } } - - @Test - public void testWrongFallbackFactoryGenericType() { - try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( - WrongFallbackFactoryGenericTypeConfiguration.class)) { - this.expected.expectMessage("Incompatible fallbackFactory instance"); - assertNotNull(context.getBean(WrongFallbackFactoryGenericTypeConfiguration.Client.class)); - } - } - - @Configuration - @Import(FeignAutoConfiguration.class) - @EnableFeignClients(clients = WrongFallbackFactoryGenericTypeConfiguration.Client.class) - protected static class WrongFallbackFactoryGenericTypeConfiguration { - - @FeignClient(name = "foobar", url = "http://localhost", fallbackFactory = ClientFallback.class) - interface Client { - @RequestMapping(method = RequestMethod.GET, value = "/") - String get(); - } - - @Bean - ClientFallback dummy() { - return new ClientFallback(); - } - - class ClientFallback implements FallbackFactory { - - @Override - public String create(Throwable cause) { - return "tryinToTrickYa"; - } - } - - @Bean - public Feign.Builder feignBuilder() { - return HystrixFeign.builder(); - } - - } } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java index d4c55b2b..d5b76832 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java @@ -66,6 +66,7 @@ import org.springframework.web.bind.annotation.RestController; import com.netflix.hystrix.HystrixCommand; import com.netflix.hystrix.HystrixCommandGroupKey; import com.netflix.hystrix.HystrixCommandKey; +import com.netflix.hystrix.exception.HystrixRuntimeException; import com.netflix.loadbalancer.Server; import com.netflix.loadbalancer.ServerList; @@ -129,6 +130,12 @@ public class FeignClientTests { @Autowired private HystrixClientWithFallBackFactory hystrixClientWithFallBackFactory; + @Autowired + private InvalidTypeHystrixClientWithFallBackFactory invalidTypeHystrixClientWithFallBackFactory; + + @Autowired + private NullHystrixClientWithFallBackFactory nullHystrixClientWithFallBackFactory; + @Autowired @Qualifier("localapp3FeignClient") HystrixClient namedHystrixClient; @@ -275,6 +282,20 @@ public class FeignClientTests { Hello fail(); } + @FeignClient(name = "localapp6", fallbackFactory = InvalidTypeHystrixClientFallbackFactory.class) + protected interface InvalidTypeHystrixClientWithFallBackFactory { + + @RequestMapping(method = RequestMethod.GET, path = "/fail") + Hello fail(); + } + + @FeignClient(name = "localapp7", fallbackFactory = NullHystrixClientFallbackFactory.class) + protected interface NullHystrixClientWithFallBackFactory { + + @RequestMapping(method = RequestMethod.GET, path = "/fail") + Hello fail(); + } + static class HystrixClientFallbackFactory implements FallbackFactory { @Override @@ -289,6 +310,22 @@ public class FeignClientTests { } } + static class InvalidTypeHystrixClientFallbackFactory implements FallbackFactory { + + @Override + public String create(final Throwable cause) { + return "hello"; + } + } + + static class NullHystrixClientFallbackFactory implements FallbackFactory { + + @Override + public String create(final Throwable cause) { + return null; + } + } + static class HystrixClientFallback implements HystrixClient { @Override public Hello fail() { @@ -348,7 +385,8 @@ public class FeignClientTests { @RestController @EnableFeignClients(clients = { TestClientServiceId.class, TestClient.class, DecodingTestClient.class, HystrixClient.class, HystrixClientWithFallBackFactory.class, - HystrixSetterFactoryClient.class}, + HystrixSetterFactoryClient.class, InvalidTypeHystrixClientWithFallBackFactory.class, + NullHystrixClientWithFallBackFactory.class}, defaultConfiguration = TestDefaultFeignConfig.class) @RibbonClients({ @RibbonClient(name = "localapp", configuration = LocalRibbonClientConfiguration.class), @@ -356,7 +394,9 @@ public class FeignClientTests { @RibbonClient(name = "localapp2", configuration = LocalRibbonClientConfiguration.class), @RibbonClient(name = "localapp3", configuration = LocalRibbonClientConfiguration.class), @RibbonClient(name = "localapp4", configuration = LocalRibbonClientConfiguration.class), - @RibbonClient(name = "localapp5", configuration = LocalRibbonClientConfiguration.class) + @RibbonClient(name = "localapp5", configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp6", configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp7", configuration = LocalRibbonClientConfiguration.class) }) protected static class Application { @@ -371,6 +411,16 @@ public class FeignClientTests { return new HystrixClientFallbackFactory(); } + @Bean + public InvalidTypeHystrixClientFallbackFactory invalidTypeHystrixClientFallbackFactory() { + return new InvalidTypeHystrixClientFallbackFactory(); + } + + @Bean + public NullHystrixClientFallbackFactory nullHystrixClientFallbackFactory() { + return new NullHystrixClientFallbackFactory(); + } + @Bean FeignFormatterRegistrar feignFormatterRegistrar() { return new FeignFormatterRegistrar() { @@ -722,6 +772,16 @@ public class FeignClientTests { hello.getMessage().contains("500")); } + @Test(expected = HystrixRuntimeException.class) + public void testInvalidTypeHystrixFallbackFactory() throws Exception { + invalidTypeHystrixClientWithFallBackFactory.fail(); + } + + @Test(expected = HystrixRuntimeException.class) + public void testNullHystrixFallbackFactory() throws Exception { + nullHystrixClientWithFallBackFactory.fail(); + } + @Test public void namedFeignClientWorks() { assertNotNull("namedHystrixClient was null", this.namedHystrixClient);