Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InvocableHandlerMethodSupport#adaptCallable does not unwrap InvocationTargetException #973

Closed
making opened this issue May 20, 2024 · 2 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@making
Copy link
Member

making commented May 20, 2024

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);
	}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 20, 2024
@rstoyanchev rstoyanchev self-assigned this May 20, 2024
@rstoyanchev rstoyanchev added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels May 20, 2024
@rstoyanchev rstoyanchev added this to the 1.3.0 milestone May 20, 2024
@rstoyanchev rstoyanchev added type: bug A general bug and removed type: task A general task labels May 20, 2024
@rstoyanchev
Copy link
Contributor

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.

@rstoyanchev rstoyanchev changed the title The exception handling in InvocableHandlerMethodSupport#adaptCallable is lacking InvocableHandlerMethodSupport#adaptCallable does not unwrap InvocationTargetException May 20, 2024
@rstoyanchev rstoyanchev modified the milestones: 1.3.0, 1.2.7 May 20, 2024
Copy link
Contributor

Fixed via 33bdea9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants