search

graphql-java-kickstart / graphql-java-servlet

Servlet endpoint for GraphQL Java
https://www.graphql-java-kickstart.com/servlet/
Other
223 stars 114 forks source link

Fixes for OSGi, remove automatic creation of executor for async requests #538

Closed tdraier closed 7 months ago

tdraier commented 11 months ago

Hi,

Here's a PR with different fixes for OSGi, and a fix for https://github.com/graphql-java-kickstart/graphql-java-servlet/issues/537 . This has been done on the javax branch but should be portable on master.

On OSGi side, I've changed version ranges so that it can be deployed on all compatible versions of servlet (3+) /websocket (1.1+) and graphql (the current code works well with graphql 20 and 21). Also updated the gradle dependencies accordingly. I've changed GraphQLMutationProvider and GraphQLSubscriptionProvider that were both implementing the same getFields methods from GraphQLFieldsProvider (removed), preventing any provider to implement both interfaces. I'm not sure what was the reason to have a common getFields() method but the code seems more simple with the 2 different methods. Also exposed the getSubscriptionProtocolFactory() from websocket endpoint, as I need to be able to call it externally in my code ..

I've slightly changed the GraphQLConfiguration.Builder so that we can provide some base configuration from OSGi with a GraphQLConfigurationProvider. Also added some safeguard to prevent providing invocationInputFactory and invocationInputFactoryBuilderat the same time.

However, a big change in GraphqlConfiguration is the removal of the thread pool executor creation for async support, as explained in https://github.com/graphql-java-kickstart/graphql-java-servlet/issues/537 . In my opinion it cannot be created by the GraphQLConfiguration object or its builder, but should be created and managed by the servlet itself. The code creating a default thread pool (and destroying it) could be moved to AbstractGraphQLHttpServlet - but it should be possible to skip the creation of that default pool if we want to use our own executor. Tell me what do you think, I will update the PR if needed.

federicorispo commented 7 months ago

@tdraier The javax branch is no longer maintained since the artifact of the javax flavour is generated on the master branch during the build phase. Could you please reopen this PR pointing to the master branch?