review feedback

Author: anjeyy

docs/en/server/exception-handling.md

@@ -22,6 +22,7 @@ This section describes how you can handle exceptions inside GrpcService layer wi
 - [Security](security.md)
 
 ## Proper exception handling
+
 If you are already familiar with springs [error handling](https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#boot-features-error-handling),
 you should see some similarity with intended exception handling for gRPC.
 
@@ -33,13 +34,13 @@ public class GrpcExceptionAdvice {
 
 
     @GrpcExceptionHandler
-    public Status handleIllegalArgumentException(IllegalArgumentException e) {
-        return Status.INVALID_ARGUMENT.withDescription(e.getMessage());
+    public Status handleInvalidArgument(IllegalArgumentException e) {
+        return Status.INVALID_ARGUMENT.withDescription("Your description");
     }
 
     @GrpcExceptionHandler(ResourceNotFoundException.class)
     public StatusException handleResourceNotFoundException(ResourceNotFoundException e) {
-        Status status = Status.NOT_FOUND.withDescription(e.getMessage());
+        Status status = Status.NOT_FOUND.withDescription("Your description");
         Metadata metadata = ...
         return status.asException(metadata);
     }
@@ -49,14 +50,15 @@ public class GrpcExceptionAdvice {
 
 - `@GrpcAdvice` marks a class to be picked up for exception handling
 - `@GrpcExceptionHandler` maps given method to be executed, in case of _specified_ thrown exception
-    - f.e. if your application throws `IllegalArgumentException`, method `handleIllegalArgumentException(..)` is executed
+    - f.e. if your application throws `IllegalArgumentException`, then the `handleInvalidArgument(IllegalArgumentException e)` method will be is executed
 - `io.grpc.Status` is specified and returned response status
 
 > **Note:** Cause is not transmitted from server to client - as stated in [official docs](https://grpc.github.io/grpc-java/javadoc/io/grpc/Status.html#withCause-java.lang.Throwable-)
 
 ## Detailed explanation
 
 ### Priority of mapped exceptions
+
 Given method with specified Exception in Annotation *and* as method argument
 
 ```java
@@ -73,26 +75,31 @@ _(Matching means: Exception type in annotation is superclass of listed method pa
 If no annotation type is provided in the annotation, listed method parameter are being picked up.
 
 ### Sending Metadata in response
+
 In case you want to send metadata in your exception response, let's have a look at the following example.
 
 ```java
 @GrpcExceptionHandler
 public StatusRuntimeException handleResourceNotFoundException(IllegalArgumentException e) {
-Status status = Status.INVALID_ARGUMENT.withDescription(e.getMessage()).withCause(e);
-Metadata metadata = ...
-return status.asRuntimeException(metadata);
+    Status status = Status.INVALID_ARGUMENT.withDescription("Your description");
+    Metadata metadata = ...
+    return status.asRuntimeException(metadata);
 }
 ```
 
 As you do not need `Metadata` in your response, just return your specified `Status`.
 
 ### Overview of returnable types
+
 Here is a small overview of possible mapped return types with `@GrpcExceptionHandler` and if
 custom Metadata can be returned.
 
-| return type | Status | StatusException | StatusRuntimeException | Throwable |
-|-----------------|---------|----------|----------|---------|
-| custom Metadata | ✗ | ✔ | ✔ | ✗ |
+| Return Type | Custom Metadata |
+| ----------- | --------------- |
+| Status | ✗ |
+| StatusException | ✔ |
+| StatusRuntimeException | ✔ |
+| Throwable | ✗ |
 
 ## Additional Topics <!-- omit in toc -->
 

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

@@ -35,7 +35,7 @@
  * @author Andjelko Perisic ([email protected])
  * @see GrpcExceptionHandler
  */
-@Target(ElementType.TYPE)
+@Target({ElementType.TYPE, ElementType.METHOD})
 @Retention(RetentionPolicy.RUNTIME)
 @Documented
 @Component

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

@@ -23,7 +23,6 @@
 import java.lang.reflect.Type;
 import java.util.Map.Entry;
 
-import org.aspectj.lang.annotation.Aspect;
 import org.springframework.lang.Nullable;
 
 import lombok.extern.slf4j.Slf4j;
@@ -42,7 +41,6 @@
  * @see GrpcAdviceExceptionInterceptor
  */
 @Slf4j
-@Aspect
 public class GrpcAdviceExceptionHandler {
 
     private final GrpcExceptionHandlerMethodResolver grpcExceptionHandlerMethodResolver;
@@ -64,6 +62,7 @@ public GrpcAdviceExceptionHandler(
      */
     @Nullable
     public <E extends Throwable> Object handleThrownException(E exception) throws Throwable {
+        log.debug("Exception caught during gRPC execution: ", exception);
 
         final Class<? extends Throwable> exceptionClass = exception.getClass();
         boolean exceptionIsMapped =

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

@@ -45,8 +45,23 @@ public <ReqT, RespT> Listener<ReqT> interceptCall(
             ServerCall<ReqT, RespT> call,
             Metadata headers,
             ServerCallHandler<ReqT, RespT> next) {
-        Listener<ReqT> delegate = next.startCall(call, headers);
-        return new GrpcAdviceExceptionListener<>(delegate, call, grpcAdviceExceptionHandler);
+        try {
+            Listener<ReqT> delegate = next.startCall(call, headers);
+            return new GrpcAdviceExceptionListener<>(delegate, call, grpcAdviceExceptionHandler);
+        } catch (Throwable throwable) {
+            return noOpCallListener();
+        }
+    }
+
+    /**
+     * Creates a new no-op call listener because you can neither return null nor throw an exception in
+     * {@link #interceptCall(ServerCall, Metadata, ServerCallHandler)}.
+     *
+     * @param <ReqT> The type of the request.
+     * @return The newly created dummy listener.
+     */
+    protected <ReqT> Listener<ReqT> noOpCallListener() {
+        return new Listener<ReqT>() {};
     }
 
 }

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

@@ -57,21 +57,21 @@ protected GrpcAdviceExceptionListener(
     public void onHalfClose() {
         try {
             super.onHalfClose();
-        } catch (Exception exception) {
-            log.error("Exception caught during gRPC execution: ", exception);
-            handleCaughtException(exception);
+
+        } catch (Throwable throwable) {
+            handleCaughtException(throwable);
         }
     }
 
-    private void handleCaughtException(Exception exception) {
+    private void handleCaughtException(Throwable throwable) {
         try {
-            Object mappedReturnType = exceptionHandler.handleThrownException(exception);
-            Status status = resolveStatus(mappedReturnType).withCause(exception);
+            Object mappedReturnType = exceptionHandler.handleThrownException(throwable);
+            Status status = resolveStatus(mappedReturnType).withCause(throwable);
             Metadata metadata = resolveMetadata(mappedReturnType);
 
             serverCall.close(status, metadata);
-        } catch (Throwable throwable) {
-            handleThrownExceptionByImplementation(throwable);
+        } catch (Throwable throwableWhileResolving) {
+            handleThrownExceptionByImplementation(throwableWhileResolving);
         }
     }
 

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

@@ -26,8 +26,7 @@
 
 /**
  * Condition to check if {@link GrpcAdvice @GrpcAdvice} is present. Mainly checking if {@link GrpcAdviceDiscoverer}
- * should be a instantiated.<br>
- * <br>
+ * should be a instantiated.
  * 
  * @author Andjelko Perisic ([email protected])
  * @see GrpcAdviceDiscoverer

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

@@ -38,6 +38,7 @@
 import io.grpc.Metadata;
 import io.grpc.Status;
 import lombok.extern.slf4j.Slf4j;
+import net.devh.boot.grpc.server.advice.GrpcAdviceExceptionHandler;
 import net.devh.boot.grpc.server.advice.GrpcAdviceExceptionListener;
 import net.devh.boot.grpc.server.autoconfigure.GrpcAdviceAutoConfiguration;
 import net.devh.boot.grpc.test.config.BaseAutoConfiguration;
@@ -73,7 +74,9 @@ class AdviceExceptionHandlingTest extends AbstractSimpleServerClientTest {
 
     @BeforeEach
     void setup() {
-        loggingEventListAppender = LoggerTestUtil.getListAppenderForClasses(GrpcAdviceExceptionListener.class);
+        loggingEventListAppender = LoggerTestUtil.getListAppenderForClasses(
+                GrpcAdviceExceptionListener.class,
+                GrpcAdviceExceptionHandler.class);
     }
 
     @Test
@@ -107,7 +110,7 @@ void testThrownClassCastException_IsMappedAsStatusRuntimeExceptionAndWithMetadat
         ClassCastException exceptionToMap = new ClassCastException("Casting with classes failed.");
         testGrpcAdviceService.setExceptionToSimulate(exceptionToMap);
         Status expectedStatus = Status.FAILED_PRECONDITION.withDescription(exceptionToMap.getMessage());
-        Metadata metadata = GrpcMetdaDataUtils.createExpectedAsciiHeader();
+        Metadata metadata = GrpcMetaDataUtils.createExpectedAsciiHeader();
 
         testGrpcCallAndVerifyMappedException(expectedStatus, metadata);
     }
@@ -125,7 +128,7 @@ void testThrownMyRootRuntimeException_IsNotMappedAndResultsInInvocationException
         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 caught during gRPC execution: ", Level.DEBUG))
                 .contains(Tuple.tuple(
                         "Exception thrown during invocation of annotated @GrpcExceptionHandler method: ",
                         Level.ERROR));
@@ -138,7 +141,7 @@ void testThrownFirstLevelException_IsMappedAsStatusExceptionWithMetadata() {
         FirstLevelException exceptionToMap = new FirstLevelException("Trigger Advice");
         testGrpcAdviceService.setExceptionToSimulate(exceptionToMap);
         Status expectedStatus = Status.NOT_FOUND.withDescription(exceptionToMap.getMessage());
-        Metadata metadata = GrpcMetdaDataUtils.createExpectedAsciiHeader();
+        Metadata metadata = GrpcMetaDataUtils.createExpectedAsciiHeader();
 
         testGrpcCallAndVerifyMappedException(expectedStatus, metadata);
     }
@@ -157,7 +160,7 @@ void testThrownStatusMappingException_IsResolvedAsInternalServerError() {
 
         assertThat(loggingEventListAppender.list)
                 .extracting(ILoggingEvent::getMessage, ILoggingEvent::getLevel)
-                .contains(Tuple.tuple("Exception caught during gRPC execution: ", Level.ERROR))
+                .contains(Tuple.tuple("Exception caught during gRPC execution: ", Level.DEBUG))
                 .contains(Tuple.tuple(
                         "Exception thrown during invocation of annotated @GrpcExceptionHandler method: ",
                         Level.ERROR));

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

@@ -81,7 +81,7 @@ void testAdviceIsPresentWithExceptionMapping() {
         log.info("--- Starting tests with advice auto discovery ---");
 
         Map<String, Object> expectedAdviceBeans = new HashMap<>();
-        expectedAdviceBeans.put(TestAdviceWithOutMetadata.class.getName(), testAdviceWithOutMetadata);
+        expectedAdviceBeans.put("grpcAdviceWithBean", testAdviceWithOutMetadata);
         expectedAdviceBeans.put(TestAdviceWithMetadata.class.getName(), testAdviceWithMetadata);
         Set<Method> expectedAdviceMethods = expectedMethods();
 

tests/src/test/java/net/devh/boot/grpc/test/advice/GrpcMetdaDataUtils.java renamed to tests/src/test/java/net/devh/boot/grpc/test/advice/GrpcMetaDataUtils.java

@@ -19,9 +19,9 @@
 
 import io.grpc.Metadata;
 
-public class GrpcMetdaDataUtils {
+public class GrpcMetaDataUtils {
 
-    private GrpcMetdaDataUtils() {
+    private GrpcMetaDataUtils() {
         throw new UnsupportedOperationException("Util class not to be instantiated.");
     }
 

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

@@ -20,6 +20,7 @@
 import java.security.AccessControlException;
 
 import org.assertj.core.api.Assertions;
+import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.core.convert.ConversionFailedException;
 
@@ -33,7 +34,7 @@
 import net.devh.boot.grpc.server.advice.GrpcAdvice;
 import net.devh.boot.grpc.server.advice.GrpcExceptionHandler;
 import net.devh.boot.grpc.server.service.GrpcService;
-import net.devh.boot.grpc.test.advice.GrpcMetdaDataUtils;
+import net.devh.boot.grpc.test.advice.GrpcMetaDataUtils;
 import net.devh.boot.grpc.test.proto.SomeType;
 import net.devh.boot.grpc.test.proto.TestServiceGrpc;
 
@@ -58,6 +59,11 @@ public <E extends RuntimeException> void setExceptionToSimulate(E exception) {
     }
 
     @GrpcAdvice
+    @Bean
+    public TestAdviceWithOutMetadata grpcAdviceWithBean() {
+        return new TestAdviceWithOutMetadata();
+    }
+
     public static class TestAdviceWithOutMetadata {
 
         @GrpcExceptionHandler
@@ -85,15 +91,15 @@ public static class TestAdviceWithMetadata {
         public StatusException handleFirstLevelException(MyRootRuntimeException e) {
 
             Status status = Status.NOT_FOUND.withCause(e).withDescription(e.getMessage());
-            Metadata metadata = GrpcMetdaDataUtils.createExpectedAsciiHeader();
+            Metadata metadata = GrpcMetaDataUtils.createExpectedAsciiHeader();
             return status.asException(metadata);
         }
 
         @GrpcExceptionHandler(ClassCastException.class)
         public StatusRuntimeException handleClassCastException() {
 
             Status status = Status.FAILED_PRECONDITION.withDescription("Casting with classes failed.");
-            Metadata metadata = GrpcMetdaDataUtils.createExpectedAsciiHeader();
+            Metadata metadata = GrpcMetaDataUtils.createExpectedAsciiHeader();
             return status.asRuntimeException(metadata);
         }