review feedback

Author: anjeyy

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

@@ -108,13 +108,11 @@ private Object invokeMappedMethodSafely(
             Method mappedMethod,
             Object instanceOfMappedMethod,
             Object[] instancedParams) throws Throwable {
-        Object statusThrowable = null;
         try {
-            statusThrowable = mappedMethod.invoke(instanceOfMappedMethod, instancedParams);
+            return mappedMethod.invoke(instanceOfMappedMethod, instancedParams);
         } catch (InvocationTargetException | IllegalAccessException e) {
             throw e.getCause(); // throw the exception thrown by implementation
         }
-        return statusThrowable;
     }
 
 }

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

@@ -39,7 +39,7 @@
  * @see GrpcAdviceExceptionHandler
  */
 @Slf4j
-class GrpcAdviceExceptionListener<ReqT, RespT> extends SimpleForwardingServerCallListener<ReqT> {
+public class GrpcAdviceExceptionListener<ReqT, RespT> extends SimpleForwardingServerCallListener<ReqT> {
 
     private final GrpcAdviceExceptionHandler exceptionHandler;
     private final ServerCall<ReqT, RespT> serverCall;
@@ -66,7 +66,7 @@ public void onHalfClose() {
     private void handleCaughtException(Exception exception) {
         try {
             Object mappedReturnType = exceptionHandler.handleThrownException(exception);
-            Status status = resolveStatus(mappedReturnType);
+            Status status = resolveStatus(mappedReturnType).withCause(exception);
             Metadata metadata = resolveMetadata(mappedReturnType);
 
             serverCall.close(status, metadata);
@@ -100,8 +100,9 @@ private Metadata resolveMetadata(Object mappedReturnType) {
     }
 
     private void handleThrownExceptionByImplementation(Throwable throwable) {
+        log.error("Exception thrown during invocation of annotated @GrpcExceptionHandler method: ", throwable);
         serverCall.close(Status.INTERNAL.withCause(throwable)
-                .withDescription(throwable.getMessage()), new Metadata());
+                .withDescription("There was a server error trying to handle an exception"), new Metadata());
     }
 
 }

grpc-server-spring-boot-autoconfigure/src/main/resources/META-INF/spring.factories

@@ -8,4 +8,4 @@ net.devh.boot.grpc.server.autoconfigure.GrpcServerFactoryAutoConfiguration,\
 net.devh.boot.grpc.server.autoconfigure.GrpcServerSecurityAutoConfiguration,\
 net.devh.boot.grpc.server.autoconfigure.GrpcServerMetricAutoConfiguration,\
 net.devh.boot.grpc.server.autoconfigure.GrpcServerTraceAutoConfiguration,\
-net.devh.boot.grpc.server.autoconfigure.GrpcAdviceAutoConfiguration
+net.devh.boot.grpc.server.autoconfigure.GrpcAdviceAutoConfiguration

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

@@ -17,27 +17,37 @@
 
 package net.devh.boot.grpc.test.advice;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 import java.security.AccessControlException;
 
+import org.assertj.core.groups.Tuple;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
 
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
 import io.grpc.Metadata;
 import io.grpc.Status;
 import lombok.extern.slf4j.Slf4j;
+import net.devh.boot.grpc.server.advice.GrpcAdviceExceptionListener;
 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.TestAdviceWithMetadata.FirstLevelException;
 import net.devh.boot.grpc.test.config.GrpcAdviceConfig.TestAdviceWithMetadata.MyRootRuntimeException;
+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;
+import net.devh.boot.grpc.test.util.LoggerTestUtil;
 
 /**
  * A test checking that the server and client can start and connect to each other with minimal config and a exception
@@ -56,10 +66,16 @@
 @DirtiesContext
 class AdviceExceptionHandlingTest extends AbstractSimpleServerClientTest {
 
+    private ListAppender<ILoggingEvent> loggingEventListAppender;
 
     @Autowired
     private TestGrpcAdviceService testGrpcAdviceService;
 
+    @BeforeEach
+    void setup() {
+        loggingEventListAppender = LoggerTestUtil.getListAppenderForClasses(GrpcAdviceExceptionListener.class);
+    }
+
     @Test
     @DirtiesContext
     void testThrownIllegalArgumentException_IsMappedAsStatus() {
@@ -98,14 +114,21 @@ void testThrownClassCastException_IsMappedAsStatusRuntimeExceptionAndWithMetadat
 
     @Test
     @DirtiesContext
-    void testThrownMyRootRuntimeException_IsNotMapped() {
+    void testThrownMyRootRuntimeException_IsNotMappedAndResultsInInvocationException() {
 
         MyRootRuntimeException exceptionToMap = new MyRootRuntimeException("Trigger Advice");
         testGrpcAdviceService.setExceptionToSimulate(exceptionToMap);
-        Status expectedStatus = Status.INTERNAL.withDescription(exceptionToMap.getMessage());
+        Status expectedStatus =
+                Status.INTERNAL.withDescription("There was a server error trying to handle an exception");
         Metadata metadata = new Metadata();
 
         testGrpcCallAndVerifyMappedException(expectedStatus, metadata);
+        assertThat(loggingEventListAppender.list)
+                .extracting(ILoggingEvent::getMessage, ILoggingEvent::getLevel)
+                .contains(Tuple.tuple("Exception caught during gRPC execution: ", Level.ERROR))
+                .contains(Tuple.tuple(
+                        "Exception thrown during invocation of annotated @GrpcExceptionHandler method: ",
+                        Level.ERROR));
     }
 
     @Test
@@ -120,6 +143,26 @@ void testThrownFirstLevelException_IsMappedAsStatusExceptionWithMetadata() {
         testGrpcCallAndVerifyMappedException(expectedStatus, metadata);
     }
 
+    @Test
+    @DirtiesContext
+    void testThrownStatusMappingException_IsResolvedAsInternalServerError() {
+
+        StatusMappingException exceptionToMap = new StatusMappingException("Trigger Advice");
+        testGrpcAdviceService.setExceptionToSimulate(exceptionToMap);
+        Status expectedStatus =
+                Status.INTERNAL.withDescription("There was a server error trying to handle an exception");
+        Metadata metadata = new Metadata();
+
+        testGrpcCallAndVerifyMappedException(expectedStatus, metadata);
+
+        assertThat(loggingEventListAppender.list)
+                .extracting(ILoggingEvent::getMessage, ILoggingEvent::getLevel)
+                .contains(Tuple.tuple("Exception caught during gRPC execution: ", Level.ERROR))
+                .contains(Tuple.tuple(
+                        "Exception thrown during invocation of annotated @GrpcExceptionHandler method: ",
+                        Level.ERROR));
+    }
+
 
 
     @BeforeAll

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

@@ -17,6 +17,8 @@
 
 package net.devh.boot.grpc.test.advice;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.Collection;
@@ -27,9 +29,9 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
@@ -59,7 +61,8 @@
 class AdviceIsPresentAutoConfigurationTest {
 
     private static final int ADVICE_CLASSES = 2;
-    private static final int ADVICE_METHODS = 4;
+    private static final int ADVICE_METHODS = 5;
+
 
     @Autowired
     private GrpcAdviceDiscoverer grpcAdviceDiscoverer;
@@ -69,27 +72,26 @@ class AdviceIsPresentAutoConfigurationTest {
     @Autowired
     private TestAdviceWithMetadata testAdviceWithMetadata;
 
+    @BeforeEach
+    void setup() {}
+
     @Test
     @DirtiesContext
     void testAdviceIsPresentWithExceptionMapping() {
         log.info("--- Starting tests with advice auto discovery ---");
 
         Map<String, Object> expectedAdviceBeans = new HashMap<>();
-        expectedAdviceBeans.put(
-                "net.devh.boot.grpc.test.config.GrpcAdviceConfig$TestAdviceWithOutMetadata",
-                testAdviceWithOutMetadata);
-        expectedAdviceBeans.put(
-                "net.devh.boot.grpc.test.config.GrpcAdviceConfig$TestAdviceWithMetadata",
-                testAdviceWithMetadata);
+        expectedAdviceBeans.put(TestAdviceWithOutMetadata.class.getName(), testAdviceWithOutMetadata);
+        expectedAdviceBeans.put(TestAdviceWithMetadata.class.getName(), testAdviceWithMetadata);
         Set<Method> expectedAdviceMethods = expectedMethods();
 
         Map<String, Object> actualAdviceBeans = grpcAdviceDiscoverer.getAnnotatedBeans();
         Set<Method> actualAdviceMethods = grpcAdviceDiscoverer.getAnnotatedMethods();
 
-        Assertions.assertThat(actualAdviceBeans)
+        assertThat(actualAdviceBeans)
                 .hasSize(ADVICE_CLASSES)
                 .containsExactlyInAnyOrderEntriesOf(expectedAdviceBeans);
-        Assertions.assertThat(actualAdviceMethods)
+        assertThat(actualAdviceMethods)
                 .hasSize(ADVICE_METHODS)
                 .containsExactlyInAnyOrderElementsOf(expectedAdviceMethods);
     }

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

@@ -97,6 +97,12 @@ public StatusRuntimeException handleClassCastException() {
             return status.asRuntimeException(metadata);
         }
 
+        @GrpcExceptionHandler
+        public StatusRuntimeException handleStatusMappingException(StatusMappingException e) {
+
+            throw new NullPointerException("Simulate developer error");
+        }
+
 
         public static class MyRootRuntimeException extends RuntimeException {
 
@@ -112,6 +118,13 @@ public FirstLevelException(String msg) {
             }
         }
 
+        public static class StatusMappingException extends RuntimeException {
+
+            public StatusMappingException(String msg) {
+                super(msg);
+            }
+        }
+
     }
 
 

tests/src/test/java/net/devh/boot/grpc/test/util/LoggerTestUtil.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2016-2020 Michael Zhang <[email protected]>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
+ * documentation files (the "Software"), to deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+ * WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+package net.devh.boot.grpc.test.util;
+
+import java.util.Arrays;
+
+import org.slf4j.LoggerFactory;
+import org.springframework.lang.Nullable;
+
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+
+/**
+ * Class to test proper @Slf4j logging.
+ * 
+ * @author Andjelko Perisic ([email protected])
+ */
+public class LoggerTestUtil {
+
+    private LoggerTestUtil() {
+        throw new UnsupportedOperationException("Util class not to be instantiated.");
+    }
+
+
+    public static ListAppender<ILoggingEvent> getListAppenderForClasses(@Nullable Class<?>... classList) {
+
+        ListAppender<ILoggingEvent> loggingEventListAppender = new ListAppender<>();
+        loggingEventListAppender.start();
+
+        if (classList == null) {
+            return loggingEventListAppender;
+        }
+
+        Arrays.stream(classList)
+                .map(clazz -> (Logger) LoggerFactory.getLogger(clazz))
+                .forEach(log -> log.addAppender(loggingEventListAppender));
+
+        return loggingEventListAppender;
+    }
+
+}