Fix bug with error handling in the defer execution code #3640
Conversation
| public void onFetchingException(ResultPath path, SourceLocation sourceLocation, Throwable throwable) { | ||
| ExceptionWhileDataFetching error = new ExceptionWhileDataFetching(path, throwable, sourceLocation); | ||
| onError(error); | ||
| public void addErrors(List<GraphQLError> errors) { |
There was a problem hiding this comment.
keep naming consistent with ExecutionContext
20bc193 to
037e06b
Compare
| @@ -187,9 +218,6 @@ public Builder deferredCallContext(DeferredCallContext deferredCallContext) { | |||
| } | |||
|
|
|||
| public ExecutionStrategyParameters build() { | |||
| if (deferredCallContext == null) { | |||
| deferredCallContext = new DeferredCallContext(); | |||
There was a problem hiding this comment.
There was no point in creating a new instance of DeferredCallContext by default.
With the current change, the presence of an instance of DeferredCallContext signifies that the execution is part of a deferred call.
| parameters.getDeferredCallContext().addError(error); | ||
| } else { | ||
| executionContext.addError(error, path); | ||
| } |
There was a problem hiding this comment.
I wonder if there's a better way to do this, because this is fairly error prone.
i.e. if you're adding an error you have to remember to check for the presence of a deferred context first.
Could we clone the ExecutionContext for a defer and have a separate error array?
ofc this is only a problem for framework code, and end users should be ok… unless they invoke ExecutionContext.addError themselves? Seems like it's public API?
There was a problem hiding this comment.
I totally agree about it being high error-prone, but I couldn't figure out a simple solution. As long as ExecutionContext#addError exists, any code can always add errors directly to ExecutionContext without checking for defer...
Could we clone the ExecutionContext for a defer and have a separate error array?
hm... DeferredCallContext is sort of that. Is a context but only with the state that is relevant for a defer block, which for now is just a list of errors.
Can you expand on your suggestion? :-)
Improvements in the error handling for deferred code.
DataFetcherResultwhere not being added to deferred payloadsDeferredCallContextonly, no longer being duplicated inExecutionContext