Support returning DataFetcherResult from subscriptions

[smyrick]

Describe the bug
In graphql-java, a subscription must return a Publisher<T> from its data fetcher. This is enforced by the SubscriptionExecutionStrategy here:

graphql-java/src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

Lines 70 to 77 in 101dc6c

	CompletableFuture<FetchedValue> fieldFetched = fetchField(executionContext, newParameters);
	return fieldFetched.thenApply(fetchedValue -> {
	Object publisher = fetchedValue.getFetchedValue();
	if (publisher != null) {
	assertTrue(publisher instanceof Publisher, "You data fetcher must return a Publisher of events when using graphql subscriptions");
	}
	//noinspection unchecked
	return (Publisher<Object>) publisher;

Just before the check for a type of Publisher, graphql-java calls the base ExecutionStrategy.fetchField. In that method, it checks if the result is of type DataFetcherResult and if it is, unboxes it:

graphql-java/src/main/java/graphql/execution/ExecutionStrategy.java

Lines 300 to 302 in 101dc6c

	if (result instanceof DataFetcherResult) {
	//noinspection unchecked
	DataFetcherResult<?> dataFetcherResult = (DataFetcherResult) result;

That means however that the object passed to fetchField is a Publisher so it will never unbox a DataFetcherResult.

To support partial results, a second check in the SubscriptionExecutionStrategy after unboxing the Publisher, should be added to check for a DataFetcherResult again. With some refactoring of the base ExecutionStrategy some of the existing code could be used to do this.

The GraphQL spec does allow subscription to return data and errors in a single message: https://spec.graphql.org/June2018/#sec-Response-Stream

To Reproduce
Create a subscription that returns a Publisher<DataFetcherResult<T>> and it fails to parse the proper data and errors fields.

Related Issues
This was brought to our attention in graphql-kotlin ExpediaGroup/graphql-kotlin#608

[bbakerman]

I agree that DataFetcherResult should be supported from an unpacked DataFetcherResult

However I am a bit confused when you say ".. we check for a type of Publisher" Who is we - some kotlin library or the graphql-java library.

Also we want should support top level subscription fields returning Publisher<T> and DataFetcherResult<Publisher<T>> - and yes could itself be DataFetcherResults

You said

To Reproduce
Create a subscription that returns a Publisher<DataFetcherResult<T>> and it fails to parse the proper data and errors fields.

Did you mean

Create a subscription that returns a DataFetcherResult<Publisher<T>> and it fails to parse the proper data and errors fields.

?

[smyrick]

Sorry, the "we" in this case just means the graphql-java library. I will edit my above comment to make it more clear. However, when graphql-kotlin is running it's reflection, it also needs to unwrap the types so it can produce the proper schema information.

Here is the helper method in graphql-kotlin that unwraps the return types from functions: https://github.com/ExpediaGroup/graphql-kotlin/blob/b1292443d2cb563ab030cfc3a8d6dc216cdc499f/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/types/utils/functionReturnTypes.kt#L41

graphql-kotlin assumes that the valid types are:

• Valid type T
• Publisher<T>
• DataFetcherResult<T>
• CompletableFuture<T>
• CompletableFuture<DataFetcherResult<T>>

To support subscription partial results should the type supported be Publisher<DataFetcherResult<T>> or DataFetcherResult<Publisher<T>>?

I would assume it is Publisher<DataFetcherResult<T>>, since the DataFetcherResult can be thought of as the JSON object that has both data and errors and subscriptions returns a stream of those objects.

If it was DataFetcherResult<Publisher<T>> then there could only be one message with errors

[bbakerman]

The PR above supports with a DataFetcherResult wrapping the Publisher (that is on the top level field) and also each return "stream object" from the Publisher can also be a DataFetcherResult<T>

Because as you say the initial top level field (which gives of the reactive stream Publisher) can have that and errors and each "streamed" object can have errors

[smyrick]

Closed in #1804