Skip to content

Commit 2f880f3

Browse files
andimarekandimarek
committed
call Overlapping validation only once during validation
1 parent 6ba6fd8 commit 2f880f3

2 files changed

Lines changed: 93 additions & 19 deletions

File tree

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import graphql.language.FragmentDefinition;
1111
import graphql.language.FragmentSpread;
1212
import graphql.language.InlineFragment;
13+
import graphql.language.OperationDefinition;
1314
import graphql.language.Selection;
1415
import graphql.language.SelectionSet;
1516
import graphql.schema.GraphQLFieldDefinition;
@@ -47,7 +48,6 @@
4748
import static graphql.util.FpKit.filterSet;
4849
import static graphql.util.FpKit.groupingBy;
4950
import static graphql.validation.ValidationErrorType.FieldsConflict;
50-
import static java.lang.String.format;
5151

5252
@Internal
5353
public class OverlappingFieldsCanBeMerged extends AbstractRule {
@@ -62,10 +62,15 @@ public OverlappingFieldsCanBeMerged(ValidationContext validationContext, Validat
6262
}
6363

6464
@Override
65-
public void leaveSelectionSet(SelectionSet selectionSet) {
65+
public void checkOperationDefinition(OperationDefinition operationDefinition) {
66+
super.checkOperationDefinition(operationDefinition);
67+
impl(operationDefinition.getSelectionSet(), getValidationContext().getOutputType());
68+
}
69+
70+
public void impl(SelectionSet selectionSet, GraphQLOutputType graphQLOutputType) {
6671
Map<String, Set<FieldAndType>> fieldMap = new LinkedHashMap<>();
6772
Set<String> visitedFragmentSpreads = new LinkedHashSet<>();
68-
collectFields(fieldMap, selectionSet, getValidationContext().getOutputType(), visitedFragmentSpreads);
73+
collectFields(fieldMap, selectionSet, graphQLOutputType, visitedFragmentSpreads);
6974
List<Conflict> conflicts = findConflicts(fieldMap);
7075
for (Conflict conflict : conflicts) {
7176
if (conflictsReported.contains(conflict.fields)) {

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

Lines changed: 85 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
4747
def "identical fields are ok"() {
4848
given:
4949
def query = """
50+
{...f}
5051
fragment f on Test{
5152
name
5253
name
@@ -59,9 +60,22 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
5960
errorCollector.errors.isEmpty()
6061
}
6162

63+
def "identical fields are ok 2"() {
64+
given:
65+
def query = """
66+
{ name name name name: name}
67+
"""
68+
when:
69+
traverse(query, null)
70+
71+
then:
72+
errorCollector.errors.isEmpty()
73+
}
74+
6275
def "two aliases with different targets"() {
6376
given:
6477
def query = """
78+
{... f }
6579
fragment f on Test{
6680
myName : name
6781
myName : nickname
@@ -72,8 +86,8 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
7286

7387
then:
7488
errorCollector.getErrors().size() == 1
75-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[f]) : 'myName' : 'name' and 'nickname' are different fields"
76-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 17), new SourceLocation(4, 17)]
89+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'myName' : 'name' and 'nickname' are different fields"
90+
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 17), new SourceLocation(5, 17)]
7791
}
7892

7993
static GraphQLSchema unionSchema() {
@@ -134,7 +148,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
134148

135149
then:
136150
errorCollector.getErrors().size() == 1
137-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[boxUnion]) : 'scalar' : returns different types 'Int' and 'String'"
151+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'boxUnion/scalar' : returns different types 'Int' and 'String'"
138152
}
139153

140154

@@ -182,7 +196,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
182196

183197
then:
184198
errorCollector.getErrors().size() == 1
185-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[boxUnion]) : 'scalar' : fields have different nullability shapes"
199+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'boxUnion/scalar' : fields have different nullability shapes"
186200
}
187201

188202
def 'not the same list return types'() {
@@ -206,7 +220,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
206220

207221
then:
208222
errorCollector.getErrors().size() == 1
209-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[boxUnion]) : 'scalar' : fields have different list shapes"
223+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'boxUnion/scalar' : fields have different list shapes"
210224
}
211225

212226

@@ -303,6 +317,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
303317
def 'Same aliases with different field targets'() {
304318
given:
305319
def query = """
320+
{dog{...sameAliasesWithDifferentFieldTargets}}
306321
fragment sameAliasesWithDifferentFieldTargets on Dog {
307322
fido: name
308323
fido: nickname
@@ -322,13 +337,14 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
322337

323338
then:
324339
errorCollector.getErrors().size() == 1
325-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[sameAliasesWithDifferentFieldTargets]) : 'fido' : 'name' and 'nickname' are different fields"
326-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
340+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'dog/fido' : 'name' and 'nickname' are different fields"
341+
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 13), new SourceLocation(5, 13)]
327342
}
328343

329344
def 'Alias masking direct field access'() {
330345
given:
331346
def query = """
347+
{dog{...aliasMaskingDirectFieldAccess}}
332348
fragment aliasMaskingDirectFieldAccess on Dog {
333349
name: nickname
334350
name
@@ -346,8 +362,8 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
346362

347363
then:
348364
errorCollector.getErrors().size() == 1
349-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[aliasMaskingDirectFieldAccess]) : 'name' : 'nickname' and 'name' are different fields"
350-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
365+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'dog/name' : 'nickname' and 'name' are different fields"
366+
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 13), new SourceLocation(5, 13)]
351367
}
352368

353369
def 'issue 3332 - Alias masking direct field access non fragment'() {
@@ -370,7 +386,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
370386

371387
then:
372388
errorCollector.getErrors().size() == 1
373-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[dog]) : 'name' : 'nickname' and 'name' are different fields"
389+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'dog/name' : 'nickname' and 'name' are different fields"
374390
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
375391
}
376392

@@ -398,13 +414,14 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
398414

399415
then:
400416
errorCollector.getErrors().size() == 1
401-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[cat]) : 'foo1' : 'foo1' and 'foo2' are different fields"
417+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'cat/foo1' : 'foo1' and 'foo2' are different fields"
402418
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 17), new SourceLocation(5, 17)]
403419
}
404420

405421
def 'conflicting args'() {
406422
given:
407423
def query = """
424+
{dog{...conflictingArgs}}
408425
fragment conflictingArgs on Dog {
409426
doesKnowCommand(dogCommand: SIT)
410427
doesKnowCommand(dogCommand: HEEL)
@@ -424,8 +441,8 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
424441

425442
then:
426443
errorCollector.getErrors().size() == 1
427-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[conflictingArgs]) : 'doesKnowCommand' : fields have different arguments"
428-
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
444+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'dog/doesKnowCommand' : fields have different arguments"
445+
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 13), new SourceLocation(5, 13)]
429446
}
430447

431448
//
@@ -524,7 +541,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
524541
then:
525542
errorCollector.getErrors().size() == 1
526543

527-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[f1]) : 'x' : 'a' and 'b' are different fields"
544+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'f1/x' : 'a' and 'b' are different fields"
528545
errorCollector.getErrors()[0].locations == [new SourceLocation(18, 13), new SourceLocation(21, 13)]
529546
}
530547

@@ -672,7 +689,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
672689

673690
then:
674691
errorCollector.getErrors().size() == 1
675-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[field]) : 'deepField/x' : 'a' and 'b' are different fields"
692+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'field/deepField/x' : 'a' and 'b' are different fields"
676693
errorCollector.getErrors()[0].locations.size() == 2
677694
}
678695

@@ -892,7 +909,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
892909

893910
then:
894911
errorCollector.getErrors().size() == 1
895-
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[pets]) : 'friends/conflict' : returns different types 'Int' and 'Float'"
912+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'pets/friends/conflict' : returns different types 'Int' and 'Float'"
896913
}
897914

898915

@@ -948,5 +965,57 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
948965
errorCollector.getErrors().size() == 0
949966
}
950967

968+
def "overlapping fields on lower level"() {
969+
given:
970+
def schema = schema('''
971+
type Query {
972+
pets: [Pet]
973+
}
974+
interface Pet {
975+
name: String
976+
breed: String
977+
friends: [Pet]
978+
}
979+
type Dog implements Pet {
980+
name: String
981+
age: Int
982+
dogBreed: String
983+
breed: String
984+
friends: [Pet]
985+
986+
}
987+
type Cat implements Pet {
988+
catBreed: String
989+
breed: String
990+
height: Float
991+
name : String
992+
friends: [Pet]
993+
994+
}
995+
''')
996+
def query = '''
997+
{
998+
pets {
999+
friends {
1000+
... on Dog {
1001+
x: name
1002+
}
1003+
... on Cat {
1004+
x: height
1005+
}
1006+
}
1007+
}
1008+
}
1009+
'''
1010+
when:
1011+
traverse(query, schema)
1012+
1013+
1014+
then:
1015+
errorCollector.getErrors().size() == 1
1016+
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'pets/friends/x' : returns different types 'String' and 'Float'"
1017+
1018+
}
1019+
9511020

9521021
}

0 commit comments

Comments
 (0)