Merge pull request #485 from anjeyy/grpc-exception-best-match

Author: ST-DDT

grpc-server-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/server/advice/GrpcExceptionHandlerMethodResolver.java

@@ -29,7 +29,9 @@
 import java.util.function.Function;
 
 import org.springframework.beans.factory.InitializingBean;
+import org.springframework.core.ExceptionDepthComparator;
 import org.springframework.lang.NonNull;
+import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 
 /**
@@ -175,11 +177,13 @@ public <E extends Throwable> boolean isMethodMappedForException(Class<E> excepti
         return extractExtendedThrowable(exception) != null;
     }
 
-    private <E extends Throwable> Method extractExtendedThrowable(Class<E> exception) {
+    @Nullable
+    private <E extends Throwable> Method extractExtendedThrowable(Class<E> exceptionType) {
+
         return mappedMethods.keySet()
                 .stream()
-                .filter(clazz -> clazz.isAssignableFrom(exception))
-                .findAny()
+                .filter(ex -> ex.isAssignableFrom(exceptionType))
+                .min(new ExceptionDepthComparator(exceptionType))
                 .map(mappedMethods::get)
                 .orElse(null);
     }

tests/src/test/java/net/devh/boot/grpc/test/advice/AdviceExceptionHandlingTest.java

@@ -29,6 +29,7 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.security.authentication.AccountExpiredException;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
 
@@ -45,6 +46,7 @@
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata.FirstLevelException;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata.MyRootRuntimeException;
+import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata.SecondLevelException;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata.StatusMappingException;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestGrpcAdviceService;
 import net.devh.boot.grpc.test.config.InProcessConfiguration;
@@ -117,9 +119,10 @@ void testThrownClassCastException_IsMappedAsStatusRuntimeExceptionAndWithMetadat
 
     @Test
     @DirtiesContext
-    void testThrownMyRootRuntimeException_IsNotMappedAndResultsInInvocationException() {
+    void testThrownAccountExpiredException_IsNotMappedAndResultsInInvocationException() {
 
-        MyRootRuntimeException exceptionToMap = new MyRootRuntimeException("Trigger Advice");
+        AccountExpiredException exceptionToMap =
+                new AccountExpiredException("Trigger Advice"); // not mapped in GrpcAdviceConfig
         testGrpcAdviceService.setExceptionToSimulate(exceptionToMap);
         Status expectedStatus =
                 Status.INTERNAL.withDescription("There was a server error trying to handle an exception");
@@ -166,6 +169,33 @@ void testThrownStatusMappingException_IsResolvedAsInternalServerError() {
                         Level.ERROR));
     }
 
+    @Test
+    @DirtiesContext
+    void testThrownRootDepth_IsMappedCorrectlyWithRootException() {
+
+        MyRootRuntimeException rootRuntimeException = new MyRootRuntimeException("root exception triggered.");
+
+        testGrpcAdviceService.setExceptionToSimulate(rootRuntimeException);
+        Status expectedStatus = Status.DEADLINE_EXCEEDED.withDescription(rootRuntimeException.getMessage());
+        Metadata metadata = new Metadata();
+
+        testGrpcCallAndVerifyMappedException(expectedStatus, metadata);
+    }
+
+    @Test
+    @DirtiesContext
+    void testThrownSecondLevenDepth_IsMappedCorrectlyWithSecondLevelException() {
+
+        SecondLevelException secondLevelException =
+                new SecondLevelException("level under first level and second level under root triggered.");
+
+        testGrpcAdviceService.setExceptionToSimulate(secondLevelException);
+        Status expectedStatus = Status.ABORTED.withDescription(secondLevelException.getMessage());
+        Metadata metadata = new Metadata();
+
+        testGrpcCallAndVerifyMappedException(expectedStatus, metadata);
+    }
+
 
 
     @BeforeAll

tests/src/test/java/net/devh/boot/grpc/test/advice/AdviceIsPresentAutoConfigurationTest.java

@@ -44,6 +44,7 @@
 import net.devh.boot.grpc.server.autoconfigure.GrpcAdviceAutoConfiguration;
 import net.devh.boot.grpc.test.config.BaseAutoConfiguration;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig;
+import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceForInheritedExceptions;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithOutMetadata;
 
@@ -59,8 +60,8 @@
 @DirtiesContext
 class AdviceIsPresentAutoConfigurationTest {
 
-    private static final int ADVICE_CLASSES = 2;
-    private static final int ADVICE_METHODS = 5;
+    private static final int ADVICE_CLASSES = 3;
+    private static final int ADVICE_METHODS = 7;
 
 
     @Autowired
@@ -70,6 +71,8 @@ class AdviceIsPresentAutoConfigurationTest {
     private TestAdviceWithOutMetadata testAdviceWithOutMetadata;
     @Autowired
     private TestAdviceWithMetadata testAdviceWithMetadata;
+    @Autowired
+    private TestAdviceForInheritedExceptions testAdviceForInheritedExceptions;
 
 
     @Test
@@ -80,6 +83,7 @@ void testAdviceIsPresentWithExceptionMapping() {
         Map<String, Object> expectedAdviceBeans = new HashMap<>();
         expectedAdviceBeans.put("grpcAdviceWithBean", testAdviceWithOutMetadata);
         expectedAdviceBeans.put(TestAdviceWithMetadata.class.getName(), testAdviceWithMetadata);
+        expectedAdviceBeans.put(TestAdviceForInheritedExceptions.class.getName(), testAdviceForInheritedExceptions);
         Set<Method> expectedAdviceMethods = expectedMethods();
 
         Map<String, Object> actualAdviceBeans = grpcAdviceDiscoverer.getAnnotatedBeans();
@@ -103,8 +107,11 @@ private Set<Method> expectedMethods() {
                 Arrays.stream(testAdviceWithMetadata.getClass().getDeclaredMethods()).collect(Collectors.toSet());
         Set<Method> methodsWithOutMetadata =
                 Arrays.stream(testAdviceWithOutMetadata.getClass().getDeclaredMethods()).collect(Collectors.toSet());
+        Set<Method> methodsForInheritedExceptions =
+                Arrays.stream(testAdviceForInheritedExceptions.getClass().getDeclaredMethods())
+                        .collect(Collectors.toSet());
 
-        return Stream.of(methodsWithMetadata, methodsWithOutMetadata)
+        return Stream.of(methodsWithMetadata, methodsWithOutMetadata, methodsForInheritedExceptions)
                 .flatMap(Collection::stream)
                 .filter(method -> method.isAnnotationPresent(GrpcExceptionHandler.class))
                 .collect(Collectors.toSet());

tests/src/test/java/net/devh/boot/grpc/test/config/GrpcAdviceConfig.java

@@ -23,6 +23,7 @@
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.core.convert.ConversionFailedException;
+import org.springframework.security.authentication.AccountExpiredException;
 
 import com.google.protobuf.Empty;
 
@@ -35,6 +36,8 @@
 import net.devh.boot.grpc.server.advice.GrpcExceptionHandler;
 import net.devh.boot.grpc.server.service.GrpcService;
 import net.devh.boot.grpc.test.advice.GrpcMetaDataUtils;
+import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata.MyRootRuntimeException;
+import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata.SecondLevelException;
 import net.devh.boot.grpc.test.proto.SomeType;
 import net.devh.boot.grpc.test.proto.TestServiceGrpc;
 
@@ -78,7 +81,7 @@ public Throwable handleConversionFailedExceptionAndAccessControlException(
             return (e1 != null) ? e1 : ((e2 != null) ? e2 : new RuntimeException("Should not happen."));
         }
 
-        public Status methodNotToBePickup(IllegalArgumentException e) {
+        public Status methodNotToBePickup(AccountExpiredException e) {
             Assertions.fail("Not supposed to be picked up.");
             return Status.FAILED_PRECONDITION;
         }
@@ -124,6 +127,13 @@ public FirstLevelException(String msg) {
             }
         }
 
+        public static class SecondLevelException extends FirstLevelException {
+
+            public SecondLevelException(String msg) {
+                super(msg);
+            }
+        }
+
         public static class StatusMappingException extends RuntimeException {
 
             public StatusMappingException(String msg) {
@@ -134,4 +144,23 @@ public StatusMappingException(String msg) {
     }
 
 
+    @GrpcAdvice
+    public static class TestAdviceForInheritedExceptions {
+
+
+        @GrpcExceptionHandler(SecondLevelException.class)
+        public Status handleSecondLevelException(SecondLevelException e) {
+
+            return Status.ABORTED.withCause(e).withDescription(e.getMessage());
+        }
+
+        @GrpcExceptionHandler
+        public Status handleMyRootRuntimeException(MyRootRuntimeException e) {
+
+            return Status.DEADLINE_EXCEEDED.withCause(e).withDescription(e.getMessage());
+        }
+
+    }
+
+
 }