Skip to content

Propagate errors to individual Subscription events #2610

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
bbakerman merged 2 commits into from
Dec 6, 2021
Merged

Propagate errors to individual Subscription events #2610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d52319
Propagate errors to individual Subscription events
jord1e Nov 13, 2021
d1b42fa
Propagate ExecutionStrategy errors in all circumstances
jord1e Nov 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/main/java/graphql/execution/ExecutionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ protected FieldValueInfo completeValue(ExecutionContext executionContext, Execut
// and validate the field is nullable, if non-nullable throw exception
parameters.getNonNullFieldValidator().validate(parameters.getPath(), null);
// complete the field as null
fieldValue = completedFuture(new ExecutionResultImpl(null, null));
fieldValue = completedFuture(new ExecutionResultImpl(null, executionContext.getErrors()));
}
return FieldValueInfo.newFieldValueInfo(OBJECT).fieldValue(fieldValue).build();
}
Expand All @@ -473,7 +473,7 @@ private void handleUnresolvedTypeProblem(ExecutionContext context, ExecutionStra
protected CompletableFuture<ExecutionResult> completeValueForNull(ExecutionContext executionContext, ExecutionStrategyParameters parameters) {
return Async.tryCatch(() -> {
Object nullValue = parameters.getNonNullFieldValidator().validate(parameters.getPath(), null);
return completedFuture(new ExecutionResultImpl(nullValue, null));
return completedFuture(new ExecutionResultImpl(nullValue, executionContext.getErrors()));
});
}

Expand All @@ -495,7 +495,7 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext,
return FieldValueInfo.newFieldValueInfo(LIST).fieldValue(exceptionallyCompletedFuture(e)).build();
}
if (resultIterable == null) {
return FieldValueInfo.newFieldValueInfo(LIST).fieldValue(completedFuture(new ExecutionResultImpl(null, null))).build();
return FieldValueInfo.newFieldValueInfo(LIST).fieldValue(completedFuture(new ExecutionResultImpl(null, executionContext.getErrors()))).build();
}
return completeValueForList(executionContext, parameters, resultIterable);
}
Expand Down Expand Up @@ -562,7 +562,7 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext,
for (ExecutionResult completedValue : results) {
completedResults.add(completedValue.getData());
}
ExecutionResultImpl executionResult = new ExecutionResultImpl(completedResults, null);
ExecutionResultImpl executionResult = new ExecutionResultImpl(completedResults, executionContext.getErrors());
overallResult.complete(executionResult);
});
overallResult.whenComplete(completeListCtx::onCompleted);
Expand Down Expand Up @@ -602,7 +602,7 @@ protected CompletableFuture<ExecutionResult> completeValueForScalar(ExecutionCon
} catch (NonNullableFieldWasNullException e) {
return exceptionallyCompletedFuture(e);
}
return completedFuture(new ExecutionResultImpl(serialized, null));
return completedFuture(new ExecutionResultImpl(serialized, executionContext.getErrors()));
}

/**
Expand All @@ -627,7 +627,7 @@ protected CompletableFuture<ExecutionResult> completeValueForEnum(ExecutionConte
} catch (NonNullableFieldWasNullException e) {
return exceptionallyCompletedFuture(e);
}
return completedFuture(new ExecutionResultImpl(serialized, null));
return completedFuture(new ExecutionResultImpl(serialized, executionContext.getErrors()));
}

/**
Expand Down
77 changes: 72 additions & 5 deletions src/test/groovy/graphql/execution/SubscriptionExecutionStrategyTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import graphql.GraphQL
import graphql.GraphQLError
import graphql.GraphqlErrorBuilder
import graphql.TestUtil
import graphql.TypeMismatchError
import graphql.execution.instrumentation.TestingInstrumentation
import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters
import graphql.execution.pubsub.CapturingSubscriber
Expand Down Expand Up @@ -43,20 +44,37 @@ class SubscriptionExecutionStrategyTest extends Specification {

type Subscription {
newMessage(roomId:Int) : Message
newListOfMessages(roomId:Int): [Message]
}
"""

GraphQL buildSubscriptionQL(DataFetcher newMessageDF) {
buildSubscriptionQL(newMessageDF, PropertyDataFetcher.fetching("sender"), PropertyDataFetcher.fetching("text"))
}

GraphQL buildSubscriptionQL(DataFetcher newMessageDF, DataFetcher senderDF, DataFetcher textDF) {
RuntimeWiring runtimeWiring = RuntimeWiring.newRuntimeWiring()
.type(newTypeWiring("Subscription").dataFetcher("newMessage", newMessageDF).build())
RuntimeWiring.Builder buildBaseSubscriptionWiring(DataFetcher senderDF, DataFetcher textDF) {
return RuntimeWiring.newRuntimeWiring()
.type(newTypeWiring("Message")
.dataFetcher("sender", senderDF)
.dataFetcher("text", textDF)
.build())
}

GraphQL buildSubscriptionListQL(DataFetcher newListOfMessagesDF) {
return buildSubscriptionListQL(newListOfMessagesDF, PropertyDataFetcher.fetching("sender"), PropertyDataFetcher.fetching("text"))
}

GraphQL buildSubscriptionListQL(DataFetcher newListOfMessagesDF, DataFetcher senderDF, DataFetcher textDF) {
RuntimeWiring runtimeWiring = buildBaseSubscriptionWiring(senderDF, textDF)
.type(newTypeWiring("Subscription").dataFetcher("newListOfMessages", newListOfMessagesDF).build())
.build()

return TestUtil.graphQL(idl, runtimeWiring).subscriptionExecutionStrategy(new SubscriptionExecutionStrategy()).build()
}

GraphQL buildSubscriptionQL(DataFetcher newMessageDF, DataFetcher senderDF, DataFetcher textDF) {
RuntimeWiring runtimeWiring = buildBaseSubscriptionWiring(senderDF, textDF)
.type(newTypeWiring("Subscription").dataFetcher("newMessage", newMessageDF).build())
.build()

return TestUtil.graphQL(idl, runtimeWiring).subscriptionExecutionStrategy(new SubscriptionExecutionStrategy()).build()
Expand All @@ -66,6 +84,54 @@ class SubscriptionExecutionStrategyTest extends Specification {
GraphqlErrorBuilder.newError().message(message).build()
}

@Unroll
def "#2609 when a GraphQLList is expected and a non iterable or non array is passed then it should yield a TypeMismatch error (spec October2021/6.2.3.2.ExecuteSubscriptionEvent(...).5)"() {
given:
DataFetcher newListOfMessagesDF = new DataFetcher() {
@Override
Object get(DataFetchingEnvironment environment) {
return new ReactiveStreamsMessagePublisher(5) {
@Override
protected Message examineMessage(Message message, Integer at) {
// Should actually be list of messages ([Message])
return new Message("aSender_" + at, "someText" + at)
}
}
}
}

GraphQL graphQL = buildSubscriptionListQL(newListOfMessagesDF)

def executionInput = ExecutionInput.newExecutionInput().query("""
subscription NewListOfMessages {
newListOfMessages(roomId: 123) {
sender
text
}
}
""").build()

when:
def executionResult = graphQL.execute(executionInput)

Publisher<ExecutionResult> msgStream = executionResult.getData()

def capturingSubscriber = new CapturingSubscriber<ExecutionResult>()
msgStream.subscribe(capturingSubscriber)

then:
Awaitility.await().untilTrue(capturingSubscriber.isDone())

def messages = capturingSubscriber.events
messages.size() == 5
for (int i = 0; i < messages.size(); i++) {
def message = messages[i]
assert message.errors.size() == 1
assert message.errors[0] instanceof TypeMismatchError
assert message.errors[0].message.contains("/newListOfMessages")
assert message.errors[0].message.contains("LIST")
}
}

@Unroll
def "subscription query sends out a stream of events using the '#why' implementation"() {
Expand Down Expand Up @@ -547,10 +613,10 @@ class SubscriptionExecutionStrategyTest extends Specification {

def subscribedFieldCalls = instrumentation.executionList.findAll { s -> s.contains(":subscribed-field-event") }
subscribedFieldCalls.size() == 20 // start and end calls
subscribedFieldCalls.findAll { s -> s.contains("newMessage")}.size() == 20 // all for our subscribed field
subscribedFieldCalls.findAll { s -> s.contains("newMessage") }.size() == 20 // all for our subscribed field

// this works because our calls are serial - if we truly had async values then the order would not work
subscribedFieldCalls.withIndex().collect({s,index ->
subscribedFieldCalls.withIndex().collect({ s, index ->
if (index % 2 == 0) {
assert s.startsWith("start:"), "expected start: on even indexes"
} else {
Expand All @@ -560,4 +626,5 @@ class SubscriptionExecutionStrategyTest extends Specification {

instrumentResultCalls.size() == 11 // one for the initial execution and then one for each stream event
}

}