Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

Allow errorHandlerSupplier to re-init #305

Conversation

solidline
Copy link

We noticed @ExceptionHandler annotated methods are not getting picked up in our application. After debugging this is because GraphQLErrorStartupListener only initializes the errorHandlerSupplier on the first ContextRefreshedEvent event. This is an issue because the application has not registered all beans. By removing the if statement we now see all factories inside GraphQLErrorFromExceptionHandler

@solidline solidline changed the title allow errorHandlerSupplier to re-init Allow errorHandlerSupplier to re-init Dec 5, 2019
@solidline solidline closed this Dec 5, 2019
@solidline solidline reopened this Dec 5, 2019
@@ -18,12 +18,10 @@ public GraphQLErrorStartupListener(ErrorHandlerSupplier errorHandlerSupplier, bo

@Override
public void onApplicationEvent(@NonNull ContextRefreshedEvent event) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked myself and this is called about 10+ times during startup.
As I wrote in #309, this is basically going to call ReflectiveMethodValidator#isGraphQLExceptionHandler for each event.getApplicationContext().getBeanDefinitionNames()

The first couple of times it has around 20 beans in my case and the last call has over 1k beans returned. Each this bean is traversed using reflection, so I think that's not the best approach here.

I tried debugging for ApplicationReadyEvent and that was only called once with all the 1k beans. So, perhaps, changing the event to that one would do the trick?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks to work as expected with the ApplicationReadyEvent i've updated the PR to reflect the change.

@Sam-Kruglov Sam-Kruglov mentioned this pull request Jan 2, 2020
@oliemansm oliemansm merged commit 3d9f9c7 into graphql-java-kickstart:master Feb 6, 2020
@oliemansm oliemansm added this to the 7.0.0 milestone Feb 6, 2020
@oliemansm
Copy link
Member

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants