Skip to content

Commit d4b9f34

Browse files
bbakermanbbakerman
committed
Retaining the per field map was a bad idea. Now loop around to find errors since its unlikely to be a problem really
1 parent 2765892 commit d4b9f34

6 files changed

Lines changed: 38 additions & 48 deletions

File tree

‎src/main/java/graphql/SerializationError.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public List<Object> getPath() {
5353

5454
@Override
5555
public String toString() {
56-
return "ExceptionWhileDataFetching{" +
56+
return "SerializationError{" +
5757
"path=" + path +
5858
"exception=" + exception +
5959
'}';

‎src/main/java/graphql/TypeMismatchError.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ public List<Object> getPath() {
8080

8181
@Override
8282
public String toString() {
83-
return "ExceptionWhileDataFetching{" +
83+
return "TypeMismatchError{" +
8484
"path=" + path +
8585
"expectedType=" + expectedType +
8686
'}';

‎src/main/java/graphql/execution/ExecutionContext.java‎

Lines changed: 20 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212

1313
import java.util.ArrayList;
1414
import java.util.Collections;
15-
import java.util.LinkedHashMap;
1615
import java.util.List;
1716
import java.util.Map;
1817
import java.util.function.Consumer;
@@ -33,14 +32,13 @@ public class ExecutionContext {
3332
private final Object root;
3433
private final Object context;
3534
private final Instrumentation instrumentation;
36-
private final Map<String, GraphQLError> perFieldErrors = new LinkedHashMap<>();
37-
private final List<GraphQLError> otherErrors = new ArrayList<>();
35+
private final List<GraphQLError> errors = new ArrayList<>();
3836

3937
public ExecutionContext(Instrumentation instrumentation, ExecutionId executionId, GraphQLSchema graphQLSchema, InstrumentationState instrumentationState, ExecutionStrategy queryStrategy, ExecutionStrategy mutationStrategy, ExecutionStrategy subscriptionStrategy, Map<String, FragmentDefinition> fragmentsByName, Document document, OperationDefinition operationDefinition, Map<String, Object> variables, Object context, Object root) {
40-
this(instrumentation, executionId, graphQLSchema, instrumentationState, queryStrategy, mutationStrategy, subscriptionStrategy, fragmentsByName, document, operationDefinition, variables, context, root, Collections.emptyMap(), Collections.emptyList());
38+
this(instrumentation, executionId, graphQLSchema, instrumentationState, queryStrategy, mutationStrategy, subscriptionStrategy, fragmentsByName, document, operationDefinition, variables, context, root, Collections.emptyList());
4139
}
4240

43-
ExecutionContext(Instrumentation instrumentation, ExecutionId executionId, GraphQLSchema graphQLSchema, InstrumentationState instrumentationState, ExecutionStrategy queryStrategy, ExecutionStrategy mutationStrategy, ExecutionStrategy subscriptionStrategy, Map<String, FragmentDefinition> fragmentsByName, Document document, OperationDefinition operationDefinition, Map<String, Object> variables, Object context, Object root, Map<String, GraphQLError> startingFieldErrors, List<GraphQLError> startingErrors) {
41+
ExecutionContext(Instrumentation instrumentation, ExecutionId executionId, GraphQLSchema graphQLSchema, InstrumentationState instrumentationState, ExecutionStrategy queryStrategy, ExecutionStrategy mutationStrategy, ExecutionStrategy subscriptionStrategy, Map<String, FragmentDefinition> fragmentsByName, Document document, OperationDefinition operationDefinition, Map<String, Object> variables, Object context, Object root, List<GraphQLError> startingErrors) {
4442
this.graphQLSchema = graphQLSchema;
4543
this.executionId = executionId;
4644
this.instrumentationState = instrumentationState;
@@ -54,8 +52,7 @@ public ExecutionContext(Instrumentation instrumentation, ExecutionId executionId
5452
this.context = context;
5553
this.root = root;
5654
this.instrumentation = instrumentation;
57-
this.perFieldErrors.putAll(startingFieldErrors);
58-
this.otherErrors.addAll(startingErrors);
55+
this.errors.addAll(startingErrors);
5956
}
6057

6158

@@ -111,13 +108,21 @@ public FragmentDefinition getFragment(String name) {
111108
* @param fieldPath the field path to put it under
112109
*/
113110
public void addError(GraphQLError error, ExecutionPath fieldPath) {
111+
//
114112
// see http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability about how per
115-
// field errors should be handled - ie only once per field
113+
// field errors should be handled - ie only once per field if its already there for nullability
114+
// but unclear if its not that error path
115+
//
116116
synchronized (this) {
117-
String key = fieldPath.toString();
118-
if (!perFieldErrors.containsKey(key)) {
119-
this.perFieldErrors.put(key, error);
117+
for (GraphQLError graphQLError : errors) {
118+
List<Object> path = graphQLError.getPath();
119+
if (path != null) {
120+
if (fieldPath.equals(ExecutionPath.fromList(path))) {
121+
return;
122+
}
123+
}
120124
}
125+
this.errors.add(error);
121126
}
122127
}
123128

@@ -129,36 +134,19 @@ public void addError(GraphQLError error, ExecutionPath fieldPath) {
129134
*/
130135
public void addError(GraphQLError error) {
131136
// see https://github.com/graphql-java/graphql-java/issues/888 on how the spec is unclear
132-
// on how multiple errors should be handled - ie only once per field or not outside nullability
137+
// on how exactly multiple errors should be handled - ie only once per field or not outside the nullability
138+
// aspect.
133139
synchronized (this) {
134-
this.otherErrors.add(error);
140+
this.errors.add(error);
135141
}
136142
}
137143

138-
// access for builder only
139-
Map<String, GraphQLError> getPerFieldErrorMap() {
140-
synchronized (this) {
141-
return perFieldErrors;
142-
}
143-
}
144-
145-
// access for builder only
146-
List<GraphQLError> getOtherErrors() {
147-
synchronized (this) {
148-
return otherErrors;
149-
}
150-
}
151-
152-
153144
/**
154145
* @return the total list of errors for this execution context
155146
*/
156147
public List<GraphQLError> getErrors() {
157148
synchronized (this) {
158-
ArrayList<GraphQLError> errList = new ArrayList<>();
159-
errList.addAll(perFieldErrors.values());
160-
errList.addAll(otherErrors);
161-
return errList;
149+
return Collections.unmodifiableList(errors);
162150
}
163151
}
164152

‎src/main/java/graphql/execution/ExecutionContextBuilder.java‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212

1313
import java.util.ArrayList;
1414
import java.util.HashMap;
15-
import java.util.LinkedHashMap;
1615
import java.util.List;
1716
import java.util.Map;
1817

@@ -34,8 +33,7 @@ public class ExecutionContextBuilder {
3433
private OperationDefinition operationDefinition;
3534
private Map<String, Object> variables = new HashMap<>();
3635
private Map<String, FragmentDefinition> fragmentsByName = new HashMap<>();
37-
private Map<String, GraphQLError> perFieldErrors = new LinkedHashMap<>();
38-
private List<GraphQLError> otherErrors = new ArrayList<>();
36+
private List<GraphQLError> errors = new ArrayList<>();
3937

4038
/**
4139
* @return a new builder of {@link graphql.execution.ExecutionContext}s
@@ -74,8 +72,7 @@ public ExecutionContextBuilder() {
7472
operationDefinition = other.getOperationDefinition();
7573
variables = new HashMap<>(other.getVariables());
7674
fragmentsByName = new HashMap<>(other.getFragmentsByName());
77-
perFieldErrors = new LinkedHashMap<>(other.getPerFieldErrorMap());
78-
otherErrors = new ArrayList<>(other.getOtherErrors());
75+
errors = new ArrayList<>(other.getErrors());
7976
}
8077

8178
public ExecutionContextBuilder instrumentation(Instrumentation instrumentation) {
@@ -143,10 +140,6 @@ public ExecutionContextBuilder operationDefinition(OperationDefinition operation
143140
return this;
144141
}
145142

146-
public ExecutionContextBuilder errors(Map<String, GraphQLError> errors) {
147-
this.perFieldErrors = errors;
148-
return this;
149-
}
150143

151144
public ExecutionContext build() {
152145
// preconditions
@@ -166,7 +159,6 @@ public ExecutionContext build() {
166159
variables,
167160
context,
168161
root,
169-
perFieldErrors,
170-
otherErrors);
162+
errors);
171163
}
172164
}

‎src/main/java/graphql/execution/NonNullableFieldValidator.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,17 @@ public <T> T validate(ExecutionPath path, T result) throws NonNullableFieldWasNu
3535
if (result == null) {
3636
if (typeInfo.isNonNullType()) {
3737
// see http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability
38+
//
39+
// > If the field returns null because of an error which has already been added to the "errors" list in the response,
40+
// > the "errors" list must not be further affected. That is, only one error should be added to the errors list per field.
41+
//
42+
// We interpret this to cover the null field path only. So here we use the variant of addError() that checks
43+
// for the current path already.
44+
//
45+
// Other places in the code base use the addError() that does not care about previous errors on that path being there.
46+
//
47+
// We will do this until the spec makes this more explicit.
48+
//
3849
NonNullableFieldWasNullException nonNullException = new NonNullableFieldWasNullException(typeInfo, path);
3950
executionContext.addError(new NonNullableFieldWasNullError(nonNullException), path);
4051
throw nonNullException;

‎src/test/groovy/graphql/execution/batched/BatchedExecutionStrategyTest.groovy‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -473,9 +473,8 @@ class BatchedExecutionStrategyTest extends Specification {
473473
ExecutionResult result = graphQL.execute("query { user { id } }")
474474

475475
expect:
476-
result.getErrors().size() == 2
477-
result.getErrors()[0] instanceof NonNullableFieldWasNullError
478-
result.getErrors()[1] instanceof ExceptionWhileDataFetching
476+
result.getErrors().size() == 1
477+
result.getErrors()[0] instanceof ExceptionWhileDataFetching
479478
}
480479

481480
}

0 commit comments

Comments
 (0)