spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.5k stars 297 forks source link

`InvocableHandlerMethodSupport#adaptCallable` does not unwrap `InvocationTargetException` #973

Closed making closed 1 month ago

making commented 1 month ago

After upgrading Spring Boot to 3.3.0-SNAPSHOT, I encountered an issue that seems related to #958 in Spring GraphQL, causing my tests to fail with the following stack trace. The tests pass with 3.3.0-RC1 and also pass if virtual threads are disabled.

    at org.springframework.graphql.data.method.InvocableHandlerMethodSupport.lambda$adaptCallable$1(InvocableHandlerMethodSupport.java:165)
    at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
    at java.base/java.lang.VirtualThread.run(VirtualThread.java:309)
Caused by: java.lang.reflect.InvocationTargetException: null
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:118)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.springframework.graphql.data.method.InvocableHandlerMethodSupport.lambda$doInvoke$0(InvocableHandlerMethodSupport.java:112)
    at io.micrometer.context.ContextSnapshot.lambda$wrap$1(ContextSnapshot.java:106)
    at org.springframework.graphql.data.method.InvocableHandlerMethodSupport.lambda$adaptCallable$1(InvocableHandlerMethodSupport.java:161)
    ... 2 common frames omitted
Caused by: org.springframework.security.authorization.AuthorizationDeniedException: Access Denied
    at org.springframework.security.authorization.method.ThrowingMethodAuthorizationDeniedHandler.handleDeniedInvocation(ThrowingMethodAuthorizationDeniedHandler.java:38)
    at org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor.handle(AuthorizationManagerBeforeMethodInterceptor.java:290)
    at org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor.attemptAuthorization(AuthorizationManagerBeforeMethodInterceptor.java:261)
    at org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor.invoke(AuthorizationManagerBeforeMethodInterceptor.java:197)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184)
    at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.proceed(CglibAopProxy.java:768)
    at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:720)
    at am.ik.blog.entry.AuthorizedEntryService$$SpringCGLIB$$0.findOne(<generated>)
    at am.ik.blog.entry.web.EntryGraphqlController.getEntry(EntryGraphqlController.java:34)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
    ... 6 common frames omitted

https://github.com/categolj/blog-api/actions/runs/9154720818/job/25165762034?pr=268#step:5:827

This test attempts to check for Unauthorized access to a GraphQL endpoint that calls an authorized service with @Authorized. From the stack trace, it seems micrometer might also be involved.

I was able to pass the test by modifying InvocableHandlerMethodSupport#adaptCallable as follows. This is probably unrelated to the support for virtual threads and is likely an existing issue that surfaced due to the recent changes.

private CompletableFuture<?> adaptCallable(GraphQLContext graphQLContext, Callable<?> result) {
        return CompletableFuture.supplyAsync(() -> {
            try {
                return ContextSnapshotFactoryHelper.captureFrom(graphQLContext).wrap(result).call();
            } catch (Exception ex) {
// <<---
                if (ex instanceof InvocationTargetException && ex.getCause() instanceof RuntimeException) {
                    throw (RuntimeException) ex.getCause();
                }
// --->
                String msg = "Failure in Callable returned from " + getBridgedMethod().toGenericString();
                throw new IllegalStateException(msg, ex);
            }
        }, this.executor);
    }
rstoyanchev commented 1 month ago

It looks like this is an existing issue that surfaced with the changes for #958. The exception handling in adaptCallable is lacking compared to what we have in doInvoke. In this case the controller method does not return Callable and therefore previously it would have been invoked directly with the exception handled in doInvoke. After #958, on Java 21+, we wrap blocking methods with Callable transparently, and that's what exposed the issue.

github-actions[bot] commented 1 month ago

Fixed via 33bdea95567e5b24b39bbdcc5b65adc193db98ed