Skip to content

Subscription data fetcher return type should be checked #334

New issue

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

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

Already on GitHub? # to your account

Closed
vojtapol opened this issue Nov 5, 2019 · 4 comments · Fixed by #756
Closed

Subscription data fetcher return type should be checked #334

vojtapol opened this issue Nov 5, 2019 · 4 comments · Fixed by #756

Comments

@vojtapol
Copy link
Member

vojtapol commented Nov 5, 2019

When you add a subscription resolver:

public class ImportantEventSubscription implements GraphQLSubscriptionResolver {

    public String importantEvent(String id, DataFetchingEnvironment environment) {

        return "hi";
    }

}
extend type Subscription {
  importantEvent(id: String!): String
}

The schema is parsed and built by graphql-java-tools without errors but then it fails on subscribe with:

graphql.AssertException: You data fetcher must return a Publisher of events when using graphql subscriptions
	at graphql.Assert.assertTrue(Assert.java:76) ~[graphql-java-13.0.jar:na]
	at graphql.execution.SubscriptionExecutionStrategy.lambda$createSourceEventStream$2(SubscriptionExecutionStrategy.java:74) ~[graphql-java-13.0.jar:na]
	at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:602) ~[na:1.8.0_152-ea]
	at java.util.concurrent.CompletableFuture.uniApplyStage(CompletableFuture.java:614) ~[na:1.8.0_152-ea]
	at java.util.concurrent.CompletableFuture.thenApply(CompletableFuture.java:1983) ~[na:1.8.0_152-ea]
	at graphql.execution.SubscriptionExecutionStrategy.createSourceEventStream(SubscriptionExecutionStrategy.java:71) ~[graphql-java-13.0.jar:na]
	at graphql.execution.SubscriptionExecutionStrategy.execute(SubscriptionExecutionStrategy.java:37) ~[graphql-java-13.0.jar:na]
	at graphql.execution.Execution.executeOperation(Execution.java:161) ~[graphql-java-13.0.jar:na]
	at graphql.execution.Execution.execute(Execution.java:102) ~[graphql-java-13.0.jar:na]
	at graphql.GraphQL.execute(GraphQL.java:605) ~[graphql-java-13.0.jar:na]
	at graphql.GraphQL.parseValidateAndExecute(GraphQL.java:538) ~[graphql-java-13.0.jar:na]

I think we could be checking that subscription data fetchers return Publisher during the schema validation step.

@edudar-chwy
Copy link

@oryan-block, I think this was a little bit overdone. SubscriptionExecutionStrategy supports CompletionStage<Publisher<X>> or Publisher<X> return types.

protected Object /*CompletableFuture<FetchedValue> | FetchedValue>*/
    fetchField(ExecutionContext executionContext, ExecutionStrategyParameters parameters) {

This change prevents any kind of CompletionStage from being used as a return type, as it must be either Publisher or ReceiveChannel

@oryan-block
Copy link
Collaborator

@edudar-chwy is it good enough to allow for CompletableFuture or does it have to be all CompletionStage implementations? It's a bit of a problem because of generics..

@edudar-chwy
Copy link

I bet CompletableFuture would be enough

@oryan-block
Copy link
Collaborator

@edudar-chwy fix for that issue is here: #797
Sorry for the late response.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants