From 59bbe9f584b50ef7f12909e460c7c9415e8618c9 Mon Sep 17 00:00:00 2001 From: dmgcodevil Date: Tue, 15 Apr 2014 14:47:53 +0300 Subject: [PATCH] Fix for issue #241 (leaner error propagation in hystrix javanica) --- hystrix-contrib/hystrix-javanica/build.gradle | 1 + .../aop/aspectj/HystrixCommandAspect.java | 23 +++-- .../spring/error/ErrorPropagationTest.java | 83 ++++++++++++++----- 3 files changed, 82 insertions(+), 25 deletions(-) diff --git a/hystrix-contrib/hystrix-javanica/build.gradle b/hystrix-contrib/hystrix-javanica/build.gradle index 337d90308..26086ae9a 100644 --- a/hystrix-contrib/hystrix-javanica/build.gradle +++ b/hystrix-contrib/hystrix-javanica/build.gradle @@ -19,6 +19,7 @@ dependencies { testCompile 'org.springframework:spring-aop:4.0.0.RELEASE' testCompile 'org.springframework:spring-test:4.0.0.RELEASE' testCompile 'cglib:cglib:3.1' + testCompile 'org.mockito:mockito-all:1.9.5' } eclipse { diff --git a/hystrix-contrib/hystrix-javanica/src/main/java/com/netflix/hystrix/contrib/javanica/aop/aspectj/HystrixCommandAspect.java b/hystrix-contrib/hystrix-javanica/src/main/java/com/netflix/hystrix/contrib/javanica/aop/aspectj/HystrixCommandAspect.java index 4d9b82112..dab692193 100644 --- a/hystrix-contrib/hystrix-javanica/src/main/java/com/netflix/hystrix/contrib/javanica/aop/aspectj/HystrixCommandAspect.java +++ b/hystrix-contrib/hystrix-javanica/src/main/java/com/netflix/hystrix/contrib/javanica/aop/aspectj/HystrixCommandAspect.java @@ -15,10 +15,15 @@ */ package com.netflix.hystrix.contrib.javanica.aop.aspectj; +import com.netflix.hystrix.HystrixExecutable; import com.netflix.hystrix.contrib.javanica.annotation.HystrixCollapser; import com.netflix.hystrix.contrib.javanica.annotation.HystrixCommand; import com.netflix.hystrix.contrib.javanica.collapser.CommandCollapser; -import com.netflix.hystrix.contrib.javanica.command.*; +import com.netflix.hystrix.contrib.javanica.command.CommandExecutor; +import com.netflix.hystrix.contrib.javanica.command.ExecutionType; +import com.netflix.hystrix.contrib.javanica.command.GenericHystrixCommandFactory; +import com.netflix.hystrix.contrib.javanica.command.MetaHolder; +import com.netflix.hystrix.exception.HystrixBadRequestException; import org.apache.commons.lang3.Validate; import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.Around; @@ -57,13 +62,21 @@ public Object methodsAnnotatedWithHystrixCommand(final ProceedingJoinPoint joinP .defaultCommandKey(method.getName()) .defaultCollapserKey(method.getName()) .defaultGroupKey(obj.getClass().getSimpleName()).build(); + HystrixExecutable executable; if (hystrixCollapser != null) { - CommandCollapser commandCollapser = new CommandCollapser(metaHolder); - return CommandExecutor.execute(commandCollapser, executionType); + executable = new CommandCollapser(metaHolder); } else { - GenericCommand genericCommand = GenericHystrixCommandFactory.getInstance().create(metaHolder, null); - return CommandExecutor.execute(genericCommand, executionType); + executable = GenericHystrixCommandFactory.getInstance().create(metaHolder, null); } + Object result; + try { + result = CommandExecutor.execute(executable, executionType); + } catch (HystrixBadRequestException e) { + throw e.getCause(); + } catch (Throwable throwable){ + throw throwable; + } + return result; } } diff --git a/hystrix-contrib/hystrix-javanica/src/test/java/com/netflix/hystrix/contrib/javanica/test/spring/error/ErrorPropagationTest.java b/hystrix-contrib/hystrix-javanica/src/test/java/com/netflix/hystrix/contrib/javanica/test/spring/error/ErrorPropagationTest.java index a7e0fdcc5..67cd8c912 100644 --- a/hystrix-contrib/hystrix-javanica/src/test/java/com/netflix/hystrix/contrib/javanica/test/spring/error/ErrorPropagationTest.java +++ b/hystrix-contrib/hystrix-javanica/src/test/java/com/netflix/hystrix/contrib/javanica/test/spring/error/ErrorPropagationTest.java @@ -6,11 +6,11 @@ import com.netflix.hystrix.contrib.javanica.annotation.HystrixCommand; import com.netflix.hystrix.contrib.javanica.test.spring.conf.AopCglibConfig; import com.netflix.hystrix.contrib.javanica.test.spring.domain.User; -import com.netflix.hystrix.exception.HystrixBadRequestException; import com.netflix.hystrix.strategy.concurrency.HystrixRequestContext; -import org.apache.commons.lang3.Validate; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.MockitoAnnotations; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Configurable; import org.springframework.context.annotation.Bean; @@ -19,7 +19,9 @@ import static com.netflix.hystrix.contrib.javanica.CommonUtils.getHystrixCommandByKey; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; /** * Test covers "Error Propagation" functionality. @@ -29,43 +31,78 @@ @ContextConfiguration(classes = {AopCglibConfig.class, ErrorPropagationTest.ErrorPropagationTestConfig.class}) public class ErrorPropagationTest { + private static final String COMMAND_KEY = "getUserById"; @Autowired private UserService userService; - @Test(expected = HystrixBadRequestException.class) - public void testGetUser() { + @MockitoAnnotations.Mock + private FailoverService failoverService; + + @Before + public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); + userService.setFailoverService(failoverService); + } + + @Test(expected = IllegalArgumentException.class) + public void testGetUserByEmptyId() { HystrixRequestContext context = HystrixRequestContext.initializeContext(); try { - userService.getUser("", ""); - assertEquals(1, HystrixRequestLog.getCurrentRequest().getAllExecutedCommands().size()); - com.netflix.hystrix.HystrixCommand getUserCommand = getHystrixCommandByKey("getUser"); - assertTrue(getUserCommand.getExecutionEvents().contains(HystrixEventType.FAILURE)); + userService.getUserById(""); } finally { + assertEquals(1, HystrixRequestLog.getCurrentRequest().getAllExecutedCommands().size()); + com.netflix.hystrix.HystrixCommand getUserCommand = getHystrixCommandByKey(COMMAND_KEY); + // will not affect metrics + assertFalse(getUserCommand.getExecutionEvents().contains(HystrixEventType.FAILURE)); + // and will not trigger fallback logic + verify(failoverService, never()).getDefUser(); context.shutdown(); } } + @Test(expected = NullPointerException.class) + public void testGetUserByNullId() { + HystrixRequestContext context = HystrixRequestContext.initializeContext(); + try { + userService.getUserById(null); + } finally { + assertEquals(1, HystrixRequestLog.getCurrentRequest().getAllExecutedCommands().size()); + com.netflix.hystrix.HystrixCommand getUserCommand = getHystrixCommandByKey(COMMAND_KEY); + // will not affect metrics + assertFalse(getUserCommand.getExecutionEvents().contains(HystrixEventType.FAILURE)); + // and will not trigger fallback logic + verify(failoverService, never()).getDefUser(); + context.shutdown(); + } + } public static class UserService { - @HystrixCommand(cacheKeyMethod = "getUserIdCacheKey", - ignoreExceptions = {NullPointerException.class, IllegalArgumentException.class}) - public User getUser(String id, String name) { - validate(id, name); - return new User(id, name + id); // it should be network call + private FailoverService failoverService; + + public void setFailoverService(FailoverService failoverService) { + this.failoverService = failoverService; } - @HystrixCommand - private String getUserIdCacheKey(String id, String name) { - return id + name; + @HystrixCommand(commandKey = COMMAND_KEY, ignoreExceptions = {NullPointerException.class, IllegalArgumentException.class}, + fallbackMethod = "fallback") + public User getUserById(String id) { + validate(id); + return new User(id, "name" + id); // it should be network call } - private void validate(String id, String name) throws NullPointerException, IllegalArgumentException { - Validate.notBlank(id); - Validate.notBlank(name); + private User fallback(String id) { + return failoverService.getDefUser(); } + private void validate(String val) throws NullPointerException, IllegalArgumentException { + if (val == null) { + throw new NullPointerException("parameter cannot be null"); + } else if (val.length() == 0) { + throw new IllegalArgumentException("parameter cannot be empty"); + } + } } @Configurable @@ -77,4 +114,10 @@ public UserService userService() { } } + private class FailoverService { + public User getDefUser() { + return new User("def", "def"); + } + } + }