Skip to content

Commit 47fc09b

Browse files
bbakermanbbakerman
authored
Now follows graphql-js and its more lenient field merging (graphql-java#708)
1 parent 636f8d3 commit 47fc09b

File tree

2 files changed

+44
-93
lines changed

2 files changed

+44
-93
lines changed

‎src/main/java/graphql/validation/rules/OverlappingFieldsCanBeMerged.java‎

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,32 @@
22

33

44
import graphql.execution.TypeFromAST;
5-
import graphql.language.*;
6-
import graphql.schema.*;
5+
import graphql.language.Argument;
6+
import graphql.language.AstComparator;
7+
import graphql.language.Field;
8+
import graphql.language.FragmentDefinition;
9+
import graphql.language.FragmentSpread;
10+
import graphql.language.InlineFragment;
11+
import graphql.language.Selection;
12+
import graphql.language.SelectionSet;
13+
import graphql.language.Value;
14+
import graphql.schema.GraphQLFieldDefinition;
15+
import graphql.schema.GraphQLFieldsContainer;
16+
import graphql.schema.GraphQLObjectType;
17+
import graphql.schema.GraphQLOutputType;
18+
import graphql.schema.GraphQLType;
719
import graphql.validation.AbstractRule;
820
import graphql.validation.ErrorFactory;
921
import graphql.validation.ValidationContext;
1022
import graphql.validation.ValidationErrorCollector;
1123

12-
import java.util.*;
24+
import java.util.ArrayList;
25+
import java.util.LinkedHashMap;
26+
import java.util.LinkedHashSet;
27+
import java.util.List;
28+
import java.util.Map;
29+
import java.util.Set;
1330

14-
import static graphql.language.NodeUtil.directivesByName;
1531
import static graphql.validation.ValidationErrorType.FieldsConflict;
1632

1733
public class OverlappingFieldsCanBeMerged extends AbstractRule {
@@ -66,6 +82,7 @@ private boolean isAlreadyChecked(Field field1, Field field2) {
6682
return false;
6783
}
6884

85+
@SuppressWarnings("ConstantConditions")
6986
private Conflict findConflict(String responseName, FieldAndType fieldAndType1, FieldAndType fieldAndType2) {
7087

7188
Field field1 = fieldAndType1.field;
@@ -114,10 +131,6 @@ private Conflict findConflict(String responseName, FieldAndType fieldAndType1, F
114131
String reason = String.format("%s: they have differing arguments", responseName);
115132
return new Conflict(responseName, reason, field1, field2);
116133
}
117-
if (!sameDirectives(field1.getDirectives(), field2.getDirectives())) {
118-
String reason = String.format("%s: they have differing directives", responseName);
119-
return new Conflict(responseName, reason, field1, field2);
120-
}
121134
SelectionSet selectionSet1 = field1.getSelectionSet();
122135
SelectionSet selectionSet2 = field2.getSelectionSet();
123136
if (selectionSet1 != null && selectionSet2 != null) {
@@ -160,11 +173,13 @@ private String joinReasons(List<Conflict> conflicts) {
160173
return result.toString();
161174
}
162175

176+
@SuppressWarnings("SimplifiableIfStatement")
163177
private boolean sameType(GraphQLType type1, GraphQLType type2) {
164178
if (type1 == null || type2 == null) return true;
165179
return type1.equals(type2);
166180
}
167181

182+
@SuppressWarnings("SimplifiableIfStatement")
168183
private boolean sameValue(Value value1, Value value2) {
169184
if (value1 == null && value2 == null) return true;
170185
if (value1 == null) return false;
@@ -189,21 +204,6 @@ private Argument findArgumentByName(String name, List<Argument> arguments) {
189204
return null;
190205
}
191206

192-
private boolean sameDirectives(List<Directive> directives1, List<Directive> directives2) {
193-
if (directives1.size() != directives2.size()) return false;
194-
for (Directive directive : directives1) {
195-
Directive matchedDirective = getDirectiveByName(directive.getName(), directives2);
196-
if (matchedDirective == null) return false;
197-
if (!sameArguments(directive.getArguments(), matchedDirective.getArguments())) return false;
198-
}
199-
return true;
200-
}
201-
202-
private Directive getDirectiveByName(String name, List<Directive> directives) {
203-
return directivesByName(directives).get(name);
204-
}
205-
206-
207207
private void collectFields(Map<String, List<FieldAndType>> fieldMap, SelectionSet selectionSet, GraphQLType parentType, Set<String> visitedFragmentSpreads) {
208208

209209
for (Selection selection : selectionSet.getSelections()) {
@@ -220,8 +220,7 @@ private void collectFields(Map<String, List<FieldAndType>> fieldMap, SelectionSe
220220

221221
}
222222

223-
private void collectFieldsForFragmentSpread(Map<String, List<FieldAndType>> fieldMap, Set<String> visitedFragmentSpreads, FragmentSpread selection) {
224-
FragmentSpread fragmentSpread = selection;
223+
private void collectFieldsForFragmentSpread(Map<String, List<FieldAndType>> fieldMap, Set<String> visitedFragmentSpreads, FragmentSpread fragmentSpread) {
225224
FragmentDefinition fragment = getValidationContext().getFragment(fragmentSpread.getName());
226225
if (fragment == null) return;
227226
if (visitedFragmentSpreads.contains(fragment.getName())) {
@@ -233,16 +232,14 @@ private void collectFieldsForFragmentSpread(Map<String, List<FieldAndType>> fiel
233232
collectFields(fieldMap, fragment.getSelectionSet(), graphQLType, visitedFragmentSpreads);
234233
}
235234

236-
private void collectFieldsForInlineFragment(Map<String, List<FieldAndType>> fieldMap, Set<String> visitedFragmentSpreads, GraphQLType parentType, InlineFragment selection) {
237-
InlineFragment inlineFragment = selection;
235+
private void collectFieldsForInlineFragment(Map<String, List<FieldAndType>> fieldMap, Set<String> visitedFragmentSpreads, GraphQLType parentType, InlineFragment inlineFragment) {
238236
GraphQLType graphQLType = inlineFragment.getTypeCondition() != null
239237
? (GraphQLOutputType) TypeFromAST.getTypeFromAST(getValidationContext().getSchema(), inlineFragment.getTypeCondition())
240238
: parentType;
241239
collectFields(fieldMap, inlineFragment.getSelectionSet(), graphQLType, visitedFragmentSpreads);
242240
}
243241

244-
private void collectFieldsForField(Map<String, List<FieldAndType>> fieldMap, GraphQLType parentType, Field selection) {
245-
Field field = selection;
242+
private void collectFieldsForField(Map<String, List<FieldAndType>> fieldMap, GraphQLType parentType, Field field) {
246243
String responseName = field.getAlias() != null ? field.getAlias() : field.getName();
247244
if (!fieldMap.containsKey(responseName)) {
248245
fieldMap.put(responseName, new ArrayList<>());
@@ -257,7 +254,7 @@ private void collectFieldsForField(Map<String, List<FieldAndType>> fieldMap, Gra
257254
}
258255

259256
private GraphQLFieldDefinition getVisibleFieldDefinition(GraphQLFieldsContainer fieldsContainer, Field field) {
260-
return getValidationContext().getSchema().getFieldVisibility().getFieldDefinition(fieldsContainer,field.getName());
257+
return getValidationContext().getSchema().getFieldVisibility().getFieldDefinition(fieldsContainer, field.getName());
261258
}
262259

263260
private static class FieldPair {

‎src/test/groovy/graphql/validation/rules/OverlappingFieldsCanBeMergedTest.groovy‎

Lines changed: 17 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -95,11 +95,11 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
9595
.name("BoxUnion")
9696
.possibleTypes(StringBox, IntBox, NonNullStringBox1, NonNullStringBox2)
9797
.typeResolver(new TypeResolver() {
98-
@Override
99-
GraphQLObjectType getType(TypeResolutionEnvironment env) {
100-
return null
101-
}
102-
})
98+
@Override
99+
GraphQLObjectType getType(TypeResolutionEnvironment env) {
100+
return null
101+
}
102+
})
103103
.build()
104104
def QueryRoot = newObject()
105105
.name("QueryRoot")
@@ -297,71 +297,25 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
297297
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
298298
}
299299

300-
def 'conflicting directives'() {
300+
//
301+
// The rules have been relaxed regarding fragment uniqueness.
302+
//
303+
// see https://github.com/facebook/graphql/pull/120/files
304+
// and https://github.com/graphql/graphql-js/pull/230/files
305+
//
306+
def "different skip/include directives accepted"() {
301307
given:
302308
def query = """
303-
fragment conflictingDirectiveArgs on Dog {
304-
name @include(if: true)
305-
name @skip(if: false)
306-
}
307-
"""
308-
when:
309-
traverse(query, null)
310-
311-
then:
312-
errorCollector.getErrors().size() == 1
313-
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: name: they have differing directives"
314-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
315-
}
316-
317-
def 'conflicting directive args'() {
318-
given:
319-
def query = """
320-
fragment conflictingDirectiveArgs on Dog {
321-
name @include(if: true)
322-
name @include(if: false)
323-
}
324-
"""
325-
when:
326-
traverse(query, null)
327-
328-
then:
329-
errorCollector.getErrors().size() == 1
330-
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: name: they have differing directives"
331-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
332-
}
333-
334-
def 'conflicting args with matching directives'() {
335-
given:
336-
def query = """
337-
fragment conflictingArgsWithMatchingDirectiveArgs on Dog {
338-
doesKnowCommand(dogCommand: SIT) @include(if: true)
339-
doesKnowCommand(dogCommand: HEEL) @include(if: true)
340-
}
341-
"""
342-
when:
343-
traverse(query, null)
344-
345-
then:
346-
errorCollector.getErrors().size() == 1
347-
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: doesKnowCommand: they have differing arguments"
348-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
349-
}
350-
351-
def 'conflicting directives with matching args'() {
352-
def query = """
353-
fragment conflictingDirectiveArgsWithMatchingArgs on Dog {
354-
doesKnowCommand(dogCommand: SIT) @include(if: true)
355-
doesKnowCommand(dogCommand: SIT) @skip(if: false)
356-
}
309+
fragment differentDirectivesWithDifferentAliases on Dog {
310+
name @include(if: true)
311+
name @include(if: false)
312+
}
357313
"""
358314
when:
359315
traverse(query, null)
360316

361317
then:
362-
errorCollector.getErrors().size() == 1
363-
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: doesKnowCommand: they have differing directives"
364-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
318+
errorCollector.getErrors().isEmpty()
365319
}
366320

367321
def 'encounters conflict in fragments'() {

0 commit comments

Comments
 (0)