Fixes for OSGi, remove automatic creation of executor for async requests - #538
Fixes for OSGi, remove automatic creation of executor for async requests#538tdraier wants to merge 5 commits into
Conversation
Sorry, something went wrong.
federicorispo
left a comment
There was a problem hiding this comment.
Could you create some tests for the automatic creation of the executor?
Sorry, something went wrong.
| package graphql.kickstart.servlet; | ||
|
|
||
| import graphql.schema.GraphQLSchema; | ||
| import java.util.concurrent.ThreadPoolExecutor; |
There was a problem hiding this comment.
Remove unused import
Sorry, something went wrong.
| import java.util.Collection; | ||
|
|
||
| public interface GraphQLMutationProvider extends GraphQLFieldProvider { | ||
| public interface GraphQLMutationProvider extends GraphQLProvider { |
There was a problem hiding this comment.
I would not touch so easily these interfaces since they are the contract to the outside and I think there are projects that could rely on them.
Same thing for the deletion of the GraphQLFieldProvider and the update of the GraphQlSubscriptionProvider
Sorry, something went wrong.
|
|
||
| @Setter | ||
| class OsgiSchemaBuilder { | ||
| public class OsgiSchemaBuilder { |
There was a problem hiding this comment.
Could you please create some unit tests for this builder since it has some login in it?
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
Hi,
Here's a PR with different fixes for OSGi, and a fix for #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 #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.