From 9d0ed7710e607430a8a2d0e788e6978169c3f80b Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Sanchez Date: Wed, 24 Nov 2021 15:11:02 +0000 Subject: [PATCH 1/7] add gRPC exception advice support for factory beans --- .../OnMissingErrorHandlerCondition.java | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java index fa1e3569..ab289465 100644 --- a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java +++ b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java @@ -3,12 +3,15 @@ import org.lognet.springboot.grpc.recovery.GRpcExceptionHandler; import org.lognet.springboot.grpc.recovery.GRpcServiceAdvice; import org.lognet.springboot.grpc.recovery.HandlerMethod; +import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition; +import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.boot.autoconfigure.condition.ConditionOutcome; import org.springframework.boot.autoconfigure.condition.SpringBootCondition; import org.springframework.context.annotation.ConditionContext; import org.springframework.core.MethodIntrospector; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.type.AnnotatedTypeMetadata; +import org.springframework.core.type.MethodMetadata; import org.springframework.util.ReflectionUtils; import java.lang.reflect.Method; @@ -24,26 +27,33 @@ public ConditionOutcome getMatchOutcome(ConditionContext context, AnnotatedTypeM .get("value"); ReflectionUtils.MethodFilter f = method -> AnnotatedElementUtils.hasAnnotation(method, GRpcExceptionHandler.class); - for(String adviceBeanName:context.getBeanFactory().getBeanNamesForAnnotation(GRpcServiceAdvice.class)){ - final String beanClassName = context.getBeanFactory().getBeanDefinition(adviceBeanName) - .getBeanClassName(); - + for (String adviceBeanName : context.getBeanFactory().getBeanNamesForAnnotation(GRpcServiceAdvice.class)) { + final String beanClassName = getBeanClassName(context.getBeanFactory().getBeanDefinition(adviceBeanName)); try { for (Method method : MethodIntrospector.selectMethods(Class.forName(beanClassName), f)) { final Optional> handledException = HandlerMethod.getHandledException(method, false); - if(handledException.isPresent() && handledException.get().isAssignableFrom(exc)){ + if (handledException.isPresent() && handledException.get().isAssignableFrom(exc)) { return ConditionOutcome.noMatch(String.format("Found %s handler at %s.%s", - handledException.get().getName(), - beanClassName, - method.getName() - )); + handledException.get().getName(), + beanClassName, + method.getName() + )); } } } catch (ClassNotFoundException e) { - throw new IllegalStateException(e); + throw new IllegalStateException(e); } - }; + } return ConditionOutcome.match(); } + + private String getBeanClassName(BeanDefinition beanDefinition) { + if (beanDefinition instanceof AnnotatedBeanDefinition) { // definition with @Bean Annotation cause this issue + MethodMetadata factoryMethodMetadata = ((AnnotatedBeanDefinition) beanDefinition).getFactoryMethodMetadata(); + return factoryMethodMetadata.getReturnTypeName(); + } else { + return beanDefinition.getBeanClassName(); + } + } } From c4b40eb659ae78fa85fcc3e7b7770d65c384df8f Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Sanchez Date: Wed, 24 Nov 2021 15:18:19 +0000 Subject: [PATCH 2/7] cleanup code --- .../grpc/autoconfigure/OnMissingErrorHandlerCondition.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java index ab289465..e719d4e4 100644 --- a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java +++ b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java @@ -28,7 +28,8 @@ public ConditionOutcome getMatchOutcome(ConditionContext context, AnnotatedTypeM ReflectionUtils.MethodFilter f = method -> AnnotatedElementUtils.hasAnnotation(method, GRpcExceptionHandler.class); for (String adviceBeanName : context.getBeanFactory().getBeanNamesForAnnotation(GRpcServiceAdvice.class)) { - final String beanClassName = getBeanClassName(context.getBeanFactory().getBeanDefinition(adviceBeanName)); + BeanDefinition beanDefinition = context.getBeanFactory().getBeanDefinition(adviceBeanName); + String beanClassName = getBeanClassName(beanDefinition); try { for (Method method : MethodIntrospector.selectMethods(Class.forName(beanClassName), f)) { final Optional> handledException = HandlerMethod.getHandledException(method, false); @@ -49,7 +50,7 @@ public ConditionOutcome getMatchOutcome(ConditionContext context, AnnotatedTypeM } private String getBeanClassName(BeanDefinition beanDefinition) { - if (beanDefinition instanceof AnnotatedBeanDefinition) { // definition with @Bean Annotation cause this issue + if (beanDefinition instanceof AnnotatedBeanDefinition) { MethodMetadata factoryMethodMetadata = ((AnnotatedBeanDefinition) beanDefinition).getFactoryMethodMetadata(); return factoryMethodMetadata.getReturnTypeName(); } else { From a4bf9eb8a07fdabd3ae1762f0dfdae026713b12d Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Sanchez Date: Wed, 24 Nov 2021 15:37:51 +0000 Subject: [PATCH 3/7] address NPE --- .../grpc/autoconfigure/OnMissingErrorHandlerCondition.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java index e719d4e4..6d07e2e9 100644 --- a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java +++ b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java @@ -52,9 +52,10 @@ public ConditionOutcome getMatchOutcome(ConditionContext context, AnnotatedTypeM private String getBeanClassName(BeanDefinition beanDefinition) { if (beanDefinition instanceof AnnotatedBeanDefinition) { MethodMetadata factoryMethodMetadata = ((AnnotatedBeanDefinition) beanDefinition).getFactoryMethodMetadata(); - return factoryMethodMetadata.getReturnTypeName(); - } else { - return beanDefinition.getBeanClassName(); + if (factoryMethodMetadata != null) { + return factoryMethodMetadata.getReturnTypeName(); + } } + return beanDefinition.getBeanClassName(); } } From c69b19309e1b43d1c3f3dbd442e1feff7ba18308 Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Sanchez Date: Wed, 24 Nov 2021 15:52:46 +0000 Subject: [PATCH 4/7] fix formatting --- .../OnMissingErrorHandlerCondition.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java index 6d07e2e9..13407597 100644 --- a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java +++ b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java @@ -27,17 +27,18 @@ public ConditionOutcome getMatchOutcome(ConditionContext context, AnnotatedTypeM .get("value"); ReflectionUtils.MethodFilter f = method -> AnnotatedElementUtils.hasAnnotation(method, GRpcExceptionHandler.class); - for (String adviceBeanName : context.getBeanFactory().getBeanNamesForAnnotation(GRpcServiceAdvice.class)) { + for(String adviceBeanName : context.getBeanFactory().getBeanNamesForAnnotation(GRpcServiceAdvice.class)){ BeanDefinition beanDefinition = context.getBeanFactory().getBeanDefinition(adviceBeanName); String beanClassName = getBeanClassName(beanDefinition); try { for (Method method : MethodIntrospector.selectMethods(Class.forName(beanClassName), f)) { final Optional> handledException = HandlerMethod.getHandledException(method, false); - if (handledException.isPresent() && handledException.get().isAssignableFrom(exc)) { + if(handledException.isPresent() && handledException.get().isAssignableFrom(exc)) { return ConditionOutcome.noMatch(String.format("Found %s handler at %s.%s", - handledException.get().getName(), - beanClassName, - method.getName() + handledException.get().getName(), + beanClassName, + method.getName() + )); )); } } @@ -50,9 +51,9 @@ public ConditionOutcome getMatchOutcome(ConditionContext context, AnnotatedTypeM } private String getBeanClassName(BeanDefinition beanDefinition) { - if (beanDefinition instanceof AnnotatedBeanDefinition) { + if(beanDefinition instanceof AnnotatedBeanDefinition){ MethodMetadata factoryMethodMetadata = ((AnnotatedBeanDefinition) beanDefinition).getFactoryMethodMetadata(); - if (factoryMethodMetadata != null) { + if(factoryMethodMetadata != null){ return factoryMethodMetadata.getReturnTypeName(); } } From 01f6d42e5053d542ff22d406b41ed2c5b86241d8 Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Sanchez Date: Wed, 24 Nov 2021 15:54:00 +0000 Subject: [PATCH 5/7] more formatting fixes --- .../grpc/autoconfigure/OnMissingErrorHandlerCondition.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java index 13407597..367ddc66 100644 --- a/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java +++ b/grpc-spring-boot-starter/src/main/java/org/lognet/springboot/grpc/autoconfigure/OnMissingErrorHandlerCondition.java @@ -33,19 +33,18 @@ public ConditionOutcome getMatchOutcome(ConditionContext context, AnnotatedTypeM try { for (Method method : MethodIntrospector.selectMethods(Class.forName(beanClassName), f)) { final Optional> handledException = HandlerMethod.getHandledException(method, false); - if(handledException.isPresent() && handledException.get().isAssignableFrom(exc)) { + if(handledException.isPresent() && handledException.get().isAssignableFrom(exc)){ return ConditionOutcome.noMatch(String.format("Found %s handler at %s.%s", handledException.get().getName(), beanClassName, method.getName() )); - )); } } } catch (ClassNotFoundException e) { throw new IllegalStateException(e); } - } + }; return ConditionOutcome.match(); } From fc4785239fe1c9f482547ec6efaf7248b521013d Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Sanchez Date: Mon, 29 Nov 2021 09:28:31 +0000 Subject: [PATCH 6/7] add tests --- .../grpc/recovery/GRpcRecoveryTest.java | 64 +++++++++++-------- .../main/resources/grpc-spring-boot.gradle | 16 ++++- 2 files changed, 50 insertions(+), 30 deletions(-) diff --git a/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java b/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java index 19586a58..fc948429 100644 --- a/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java +++ b/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java @@ -1,5 +1,20 @@ package org.lognet.springboot.grpc.recovery; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.isA; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.NONE; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + import io.grpc.Metadata; import io.grpc.Status; import io.grpc.StatusRuntimeException; @@ -15,25 +30,11 @@ import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.TestConfiguration; import org.springframework.boot.test.mock.mockito.SpyBean; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Import; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.isA; -import static org.hamcrest.Matchers.notNullValue; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; -import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.NONE; - @RunWith(SpringRunner.class) @SpringBootTest(classes = {DemoApp.class}, webEnvironment = NONE) @ActiveProfiles({"disable-security"}) @@ -43,6 +44,7 @@ public class GRpcRecoveryTest extends GrpcServerTestBase { static class CheckedException extends Exception { } + static class CheckedException1 extends Exception { } @@ -59,16 +61,25 @@ static class Exception1 extends RuntimeException { } + @GRpcServiceAdvice + static class BeanCustomErrorHandler { + + @GRpcExceptionHandler + public Status handleA(ExceptionA e, GRpcExceptionScope scope) { + return Status.NOT_FOUND; + } + } + @TestConfiguration static class Cfg { + @Bean + public BeanCustomErrorHandler otherErrorHandler() { + return new BeanCustomErrorHandler(); + } @GRpcServiceAdvice static class CustomErrorHandler { - @GRpcExceptionHandler - public Status handleA(ExceptionA e, GRpcExceptionScope scope) { - return Status.NOT_FOUND; - } @GRpcExceptionHandler public Status handleB(ExceptionB e, GRpcExceptionScope scope) { @@ -87,7 +98,6 @@ public Status handle(Exception e, GRpcExceptionScope scope) { } } - @GRpcService static class CustomService extends CustomServiceGrpc.CustomServiceImplBase { @@ -150,12 +160,14 @@ public Status handleB(ExceptionB e, GRpcExceptionScope scope) { @SpyBean private Cfg.CustomErrorHandler handler; + @SpyBean + private BeanCustomErrorHandler beanErrorHandler; + @Test public void streamingServiceErrorHandlerTest() throws ExecutionException, InterruptedException, TimeoutException { - final CompletableFuture errorFuture = new CompletableFuture<>(); final StreamObserver reply = new StreamObserver() { @@ -180,15 +192,12 @@ public void onCompleted() { requests.onCompleted(); - - - final Throwable actual = errorFuture.get(20, TimeUnit.SECONDS); assertThat(actual, notNullValue()); assertThat(actual, isA(StatusRuntimeException.class)); - assertThat(((StatusRuntimeException)actual).getStatus(), is(Status.RESOURCE_EXHAUSTED)); + assertThat(((StatusRuntimeException) actual).getStatus(), is(Status.RESOURCE_EXHAUSTED)); - Mockito.verify(srv,times(1)).handle(any(CheckedException1.class),any()); + Mockito.verify(srv, times(1)).handle(any(CheckedException1.class), any()); } @@ -205,9 +214,8 @@ public void checkedExceptionHandlerTest() { Mockito.verify(srv, never()).handleB(any(), any()); - + Mockito.verify(beanErrorHandler, never()).handleA(any(), any()); Mockito.verify(handler, never()).handle(any(), any()); - Mockito.verify(handler, never()).handleA(any(), any()); Mockito.verify(handler, times(1)).handleCheckedException(any(CheckedException.class), any()); Mockito.verify(handler, never()).handleB(any(), any()); diff --git a/grpc-spring-boot-starter-gradle-plugin/src/main/resources/grpc-spring-boot.gradle b/grpc-spring-boot-starter-gradle-plugin/src/main/resources/grpc-spring-boot.gradle index 543d74c8..f5019f3f 100644 --- a/grpc-spring-boot-starter-gradle-plugin/src/main/resources/grpc-spring-boot.gradle +++ b/grpc-spring-boot-starter-gradle-plugin/src/main/resources/grpc-spring-boot.gradle @@ -40,11 +40,23 @@ sourceSets.configureEach{s-> protobuf { protoc { - artifact = "com.google.protobuf:protoc:${grpcSpringBoot.protocVersion.get()}" + + // for apple m1, please add protoc_platform=osx-x86_64 in $HOME/.gradle/gradle.properties + if (project.hasProperty('protoc_platform')) { + artifact = "com.google.protobuf:protoc:${grpcSpringBoot.protocVersion.get()}:${protoc_platform}" + } else { + artifact = "com.google.protobuf:protoc:${grpcSpringBoot.protocVersion.get()}" + } } + plugins { grpc { - artifact = "io.grpc:protoc-gen-grpc-java:${grpcSpringBoot.grpcVersion.get()}" + // for apple m1, please add protoc_platform=osx-x86_64 in $HOME/.gradle/gradle.properties + if (project.hasProperty('protoc_platform')) { + artifact = "io.grpc:protoc-gen-grpc-java:${grpcSpringBoot.grpcVersion.get()}:${protoc_platform}" + } else { + artifact = "io.grpc:protoc-gen-grpc-java:${grpcSpringBoot.grpcVersion.get()}" + } } } From 860950d7f261f41fe733f419d13ec7b3f7b529b0 Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Sanchez Date: Mon, 29 Nov 2021 09:35:09 +0000 Subject: [PATCH 7/7] more fixes --- .../lognet/springboot/grpc/recovery/GRpcRecoveryTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java b/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java index fc948429..4db377d4 100644 --- a/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java +++ b/grpc-spring-boot-starter-demo/src/test/java/org/lognet/springboot/grpc/recovery/GRpcRecoveryTest.java @@ -237,7 +237,7 @@ public void globalHandlerTest() { Mockito.verify(handler, never()).handle(any(), any()); - Mockito.verify(handler, times(1)).handleA(any(), any()); + Mockito.verify(beanErrorHandler, times(1)).handleA(any(), any()); Mockito.verify(handler, never()).handleB(any(), any()); @@ -257,7 +257,7 @@ public void globalHandlerWithExceptionHierarchyTest() { Mockito.verify(srv, never()).handleB(any(), any()); - Mockito.verify(handler, never()).handleA(any(), any()); + Mockito.verify(beanErrorHandler, never()).handleA(any(), any()); Mockito.verify(handler, never()).handleB(any(), any()); Mockito.verify(handler, times(1)).handle(any(), any()); @@ -280,9 +280,9 @@ public void privateHandlerHasHigherPrecedence() { final String testTrailer = statusRuntimeException.getTrailers().get(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER)); assertThat(testTrailer, is("5")); - + + Mockito.verify(beanErrorHandler, never()).handleA(any(), any()); Mockito.verify(handler, never()).handle(any(), any()); - Mockito.verify(handler, never()).handleA(any(), any()); Mockito.verify(handler, never()).handleB(any(), any()); }