Skip to content

Commit 009a39c

Browse files
bbakermanandimarek
bbakerman
authored and
andimarek
committed
Fixed a bug in multi level non null handling
This now uses a multi level exception to capture the hierarchy of type information up from parent to child
1 parent 1f26c32 commit 009a39c

4 files changed

Lines changed: 198 additions & 16 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,7 @@ protected GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLObject
552552
protected void assertNonNullFieldPrecondition(NonNullableFieldWasNullException e) throws NonNullableFieldWasNullException {
553553
ExecutionTypeInfo typeInfo = e.getTypeInfo();
554554
if (typeInfo.hasParentType() && typeInfo.getParentTypeInfo().isNonNullType()) {
555-
throw e;
555+
throw new NonNullableFieldWasNullException(e);
556556
}
557557
}
558558

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,18 @@ public CompletableFuture<ExecutionResult> execute(final ExecutionContext executi
6464
try {
6565
Map<String, Object> results = new LinkedHashMap<>();
6666
for (String fieldName : futures.keySet()) {
67-
ExecutionResult executionResult = futures.get(fieldName).get();
68-
67+
ExecutionResult executionResult;
68+
try {
69+
executionResult = futures.get(fieldName).get();
70+
} catch (ExecutionException e) {
71+
if (e.getCause() instanceof NonNullableFieldWasNullException) {
72+
assertNonNullFieldPrecondition((NonNullableFieldWasNullException) e.getCause());
73+
results = null;
74+
break;
75+
} else {
76+
throw e;
77+
}
78+
}
6979
results.put(fieldName, executionResult != null ? executionResult.getData() : null);
7080
}
7181
return CompletableFuture.completedFuture(new ExecutionResultImpl(results, executionContext.getErrors()));

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,27 @@ public class NonNullableFieldWasNullException extends RuntimeException implement
2020

2121

2222
public NonNullableFieldWasNullException(ExecutionTypeInfo typeInfo, ExecutionPath path) {
23-
super(buildMsg(assertNotNull(typeInfo), assertNotNull(path)));
23+
super(
24+
buildMsg(assertNotNull(typeInfo),
25+
assertNotNull(path))
26+
);
2427
this.typeInfo = typeInfo;
2528
this.path = path;
2629
}
2730

31+
public NonNullableFieldWasNullException(NonNullableFieldWasNullException previousException) {
32+
super(
33+
buildMsg(
34+
assertNotNull(previousException.typeInfo.getParentTypeInfo()),
35+
assertNotNull(previousException.typeInfo.getParentTypeInfo().getPath())
36+
),
37+
previousException
38+
);
39+
this.typeInfo = previousException.typeInfo.getParentTypeInfo();
40+
this.path = previousException.typeInfo.getParentTypeInfo().getPath();
41+
}
42+
43+
2844
private static String buildMsg(ExecutionTypeInfo typeInfo, ExecutionPath path) {
2945
if (typeInfo.hasParentType()) {
3046
return String.format("Cannot return null for non-nullable type: '%s' within parent '%s' (%s)", typeInfo.getType().getName(), typeInfo.getParentTypeInfo().getType().getName(), path);
@@ -60,8 +76,8 @@ public List<Object> getPath() {
6076
@Override
6177
public String toString() {
6278
return "NonNullableFieldWasNullException{" +
63-
"path=" + path +
64-
"typeInfo=" + typeInfo +
79+
" path=" + path +
80+
" typeInfo=" + typeInfo +
6581
'}';
6682
}
6783
}

‎src/test/groovy/graphql/NonNullHandlingTest.groovy‎

Lines changed: 166 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package graphql
22

3+
import graphql.execution.ExecutorServiceExecutionStrategy
4+
import graphql.execution.SimpleExecutionStrategy
35
import graphql.schema.GraphQLOutputType
46
import graphql.schema.GraphQLSchema
57
import spock.lang.Specification
@@ -9,18 +11,24 @@ import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition
911
import static graphql.schema.GraphQLNonNull.nonNull
1012
import static graphql.schema.GraphQLObjectType.newObject
1113
import static graphql.schema.GraphQLSchema.newSchema
14+
import static java.util.concurrent.ForkJoinPool.commonPool
1215

1316
/**
1417
* A set of tests to show how non null field handling correctly bubble up or not
1518
*/
19+
@SuppressWarnings("GroovyUnusedDeclaration")
1620
class NonNullHandlingTest extends Specification {
1721

18-
@SuppressWarnings("GroovyUnusedDeclaration")
19-
class ParentTypeImplementation {
22+
class SimpleObject {
2023
String nullChild = null
2124
String nonNullChild = "not null"
2225
}
2326

27+
class ContainingObject {
28+
SimpleObject nullParent = null
29+
SimpleObject nonNullParent = new SimpleObject()
30+
}
31+
2432
def executionInput(String query) {
2533
ExecutionInput.newExecutionInput().query(query).build()
2634
}
@@ -46,7 +54,7 @@ class NonNullHandlingTest extends Specification {
4654
.field(newFieldDefinition()
4755
.name("parent")
4856
.type(parentType) // nullable parent
49-
.dataFetcher({ env -> new ParentTypeImplementation() })
57+
.dataFetcher({ env -> new SimpleObject() })
5058

5159
))
5260
.build()
@@ -60,13 +68,22 @@ class NonNullHandlingTest extends Specification {
6068
}
6169
"""
6270

63-
when:
64-
def result = GraphQL.newGraphQL(schema).build().execute(executionInput(query))
71+
def result = GraphQL
72+
.newGraphQL(schema)
73+
.queryExecutionStrategy(executionStrategy)
74+
.build()
75+
.execute(executionInput(query))
6576

66-
then:
77+
expect:
6778

6879
result.errors.size() == 1
6980
result.data["parent"] == null
81+
82+
where:
83+
84+
strategyName | executionStrategy
85+
'executor' | new ExecutorServiceExecutionStrategy(commonPool())
86+
'simple' | new SimpleExecutionStrategy()
7087
}
7188

7289
def "#268 - null child field values are NOT allowed in non nullable parent types"() {
@@ -91,7 +108,7 @@ class NonNullHandlingTest extends Specification {
91108
newFieldDefinition()
92109
.name("parent")
93110
.type(nonNull(parentType)) // non nullable parent
94-
.dataFetcher({ env -> new ParentTypeImplementation() })
111+
.dataFetcher({ env -> new SimpleObject() })
95112

96113
))
97114
.build()
@@ -105,12 +122,151 @@ class NonNullHandlingTest extends Specification {
105122
}
106123
"""
107124

108-
when:
109-
def result = GraphQL.newGraphQL(schema).build().execute(executionInput(query))
125+
def result = GraphQL
126+
.newGraphQL(schema)
127+
.queryExecutionStrategy(executionStrategy)
128+
.build()
129+
.execute(executionInput(query))
110130

111-
then:
131+
expect:
112132

113133
result.errors.size() == 1
114134
result.data == null
135+
136+
where:
137+
138+
strategyName | executionStrategy
139+
'executor' | new ExecutorServiceExecutionStrategy(commonPool())
140+
'simple' | new SimpleExecutionStrategy()
115141
}
142+
143+
def "#581 - null child field values are allowed in nullable grand parent type"() {
144+
145+
given:
146+
147+
148+
GraphQLOutputType parentType = newObject()
149+
.name("parentType")
150+
.field(newFieldDefinition().name("nullChild")
151+
.type(nonNull(GraphQLString)))
152+
.field(newFieldDefinition().name("nonNullChild")
153+
.type(nonNull(GraphQLString)))
154+
.build()
155+
156+
GraphQLOutputType topType = newObject()
157+
.name("topType")
158+
.field(newFieldDefinition().name("nullParent")
159+
.type(nonNull(parentType)))
160+
.field(newFieldDefinition().name("nonNullParent")
161+
.type(nonNull(parentType)))
162+
.build()
163+
164+
GraphQLSchema schema = newSchema().query(
165+
newObject()
166+
.name("RootQueryType")
167+
.field(
168+
newFieldDefinition()
169+
.name("top")
170+
.type(topType) // nullable grand parent
171+
.dataFetcher({ env -> new ContainingObject() })
172+
173+
))
174+
.build()
175+
176+
def query = """
177+
query {
178+
top {
179+
nonNullParent {
180+
nonNullChild
181+
nullChild
182+
}
183+
}
184+
}
185+
"""
186+
187+
def result = GraphQL
188+
.newGraphQL(schema)
189+
.queryExecutionStrategy(executionStrategy)
190+
.build()
191+
.execute(executionInput(query))
192+
193+
expect:
194+
195+
result.errors.size() == 1
196+
result.data != null
197+
result.data["grandParent"] == null
198+
199+
where:
200+
201+
strategyName | executionStrategy
202+
'executor' | new ExecutorServiceExecutionStrategy(commonPool())
203+
'simple' | new SimpleExecutionStrategy()
204+
205+
}
206+
207+
def "#581 - null child field values are NOT allowed in non nullable grand parent types"() {
208+
209+
given:
210+
211+
212+
GraphQLOutputType parentType = newObject()
213+
.name("parentType")
214+
.field(newFieldDefinition().name("nullChild")
215+
.type(nonNull(GraphQLString)))
216+
.field(newFieldDefinition().name("nonNullChild")
217+
.type(nonNull(GraphQLString)))
218+
.build()
219+
220+
GraphQLOutputType topType = newObject()
221+
.name("topType")
222+
.field(newFieldDefinition().name("nullParent")
223+
.type(nonNull(parentType)))
224+
.field(newFieldDefinition().name("nonNullParent")
225+
.type(nonNull(parentType)))
226+
.build()
227+
228+
GraphQLSchema schema = newSchema().query(
229+
newObject()
230+
.name("RootQueryType")
231+
.field(
232+
newFieldDefinition()
233+
.name("top")
234+
.type(nonNull(topType)) // non nullable grand parent
235+
.dataFetcher({ env -> new ContainingObject() })
236+
237+
))
238+
.build()
239+
240+
def query = """
241+
query {
242+
top {
243+
nonNullParent {
244+
nonNullChild
245+
nullChild
246+
}
247+
}
248+
}
249+
"""
250+
251+
def result = GraphQL
252+
.newGraphQL(schema)
253+
.queryExecutionStrategy(executionStrategy)
254+
.build()
255+
.execute(executionInput(query))
256+
257+
expect:
258+
259+
result != null
260+
result.errors.size() == 1
261+
result.data == null
262+
263+
where:
264+
265+
strategyName | executionStrategy
266+
'executor' | new ExecutorServiceExecutionStrategy(commonPool())
267+
'simple' | new SimpleExecutionStrategy()
268+
269+
}
270+
271+
116272
}

0 commit comments

Comments
 (0)