Merge pull request #242 from dmgcodevil/1.3.x

Fix for issue 241 (Cleaner error propagation in hystrix javanica)
Netflix · Apr 15, 2014 · 257426e · 257426e
2 parents 7df30d3 + 144df1e
commit 257426e
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 25 deletions.
diff --git 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/.../src/main/java/com/netflix/hystrix/contrib/javanica/aop/aspectj/HystrixCommandAspect.java b/.../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/...est/java/com/netflix/hystrix/contrib/javanica/test/spring/error/ErrorPropagationTest.java b/...est/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().getExecutedCommands().size());
-            com.netflix.hystrix.HystrixCommand getUserCommand = getHystrixCommandByKey("getUser");
-            assertTrue(getUserCommand.getExecutionEvents().contains(HystrixEventType.FAILURE));
+            userService.getUserById("");
         } finally {
+            assertEquals(1, HystrixRequestLog.getCurrentRequest().getExecutedCommands().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().getExecutedCommands().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");
+        }
+    }
+
 }