Skip to content

call Overlapping validation only once during validation #3835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andimarek merged 1 commit into from
Feb 26, 2025
Merged

call Overlapping validation only once during validation #3835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/main/java/graphql/validation/rules/OverlappingFieldsCanBeMerged.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import graphql.language.FragmentDefinition;
import graphql.language.FragmentSpread;
import graphql.language.InlineFragment;
import graphql.language.OperationDefinition;
import graphql.language.Selection;
import graphql.language.SelectionSet;
import graphql.schema.GraphQLFieldDefinition;
Expand Down Expand Up @@ -47,7 +48,6 @@
import static graphql.util.FpKit.filterSet;
import static graphql.util.FpKit.groupingBy;
import static graphql.validation.ValidationErrorType.FieldsConflict;
import static java.lang.String.format;

@Internal
public class OverlappingFieldsCanBeMerged extends AbstractRule {
Expand All @@ -62,10 +62,15 @@ public OverlappingFieldsCanBeMerged(ValidationContext validationContext, Validat
}

@Override
public void leaveSelectionSet(SelectionSet selectionSet) {
public void checkOperationDefinition(OperationDefinition operationDefinition) {
super.checkOperationDefinition(operationDefinition);
impl(operationDefinition.getSelectionSet(), getValidationContext().getOutputType());
}

public void impl(SelectionSet selectionSet, GraphQLOutputType graphQLOutputType) {
Map<String, Set<FieldAndType>> fieldMap = new LinkedHashMap<>();
Set<String> visitedFragmentSpreads = new LinkedHashSet<>();
collectFields(fieldMap, selectionSet, getValidationContext().getOutputType(), visitedFragmentSpreads);
collectFields(fieldMap, selectionSet, graphQLOutputType, visitedFragmentSpreads);
List<Conflict> conflicts = findConflicts(fieldMap);
for (Conflict conflict : conflicts) {
if (conflictsReported.contains(conflict.fields)) {
Expand Down
101 changes: 85 additions & 16 deletions src/test/groovy/graphql/validation/rules/OverlappingFieldsCanBeMergedTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
def "identical fields are ok"() {
given:
def query = """
{...f}
fragment f on Test{
name
name
Expand All @@ -59,9 +60,22 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
errorCollector.errors.isEmpty()
}

def "identical fields are ok 2"() {
given:
def query = """
{ name name name name: name}
"""
when:
traverse(query, null)

then:
errorCollector.errors.isEmpty()
}

def "two aliases with different targets"() {
given:
def query = """
{... f }
fragment f on Test{
myName : name
myName : nickname
Expand All @@ -72,8 +86,8 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[f]) : 'myName' : 'name' and 'nickname' are different fields"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 17), new SourceLocation(4, 17)]
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'myName' : 'name' and 'nickname' are different fields"
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 17), new SourceLocation(5, 17)]
}

static GraphQLSchema unionSchema() {
Expand Down Expand Up @@ -134,7 +148,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[boxUnion]) : 'scalar' : returns different types 'Int' and 'String'"
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'boxUnion/scalar' : returns different types 'Int' and 'String'"
}


Expand Down Expand Up @@ -182,7 +196,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[boxUnion]) : 'scalar' : fields have different nullability shapes"
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'boxUnion/scalar' : fields have different nullability shapes"
}

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

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[boxUnion]) : 'scalar' : fields have different list shapes"
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'boxUnion/scalar' : fields have different list shapes"
}


Expand Down Expand Up @@ -303,6 +317,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
def 'Same aliases with different field targets'() {
given:
def query = """
{dog{...sameAliasesWithDifferentFieldTargets}}
fragment sameAliasesWithDifferentFieldTargets on Dog {
fido: name
fido: nickname
Expand All @@ -322,13 +337,14 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[sameAliasesWithDifferentFieldTargets]) : 'fido' : 'name' and 'nickname' are different fields"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'dog/fido' : 'name' and 'nickname' are different fields"
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 13), new SourceLocation(5, 13)]
}

def 'Alias masking direct field access'() {
given:
def query = """
{dog{...aliasMaskingDirectFieldAccess}}
fragment aliasMaskingDirectFieldAccess on Dog {
name: nickname
name
Expand All @@ -346,8 +362,8 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[aliasMaskingDirectFieldAccess]) : 'name' : 'nickname' and 'name' are different fields"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'dog/name' : 'nickname' and 'name' are different fields"
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 13), new SourceLocation(5, 13)]
}

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

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

Expand Down Expand Up @@ -398,13 +414,14 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

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

def 'conflicting args'() {
given:
def query = """
{dog{...conflictingArgs}}
fragment conflictingArgs on Dog {
doesKnowCommand(dogCommand: SIT)
doesKnowCommand(dogCommand: HEEL)
Expand All @@ -424,8 +441,8 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[conflictingArgs]) : 'doesKnowCommand' : fields have different arguments"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'dog/doesKnowCommand' : fields have different arguments"
errorCollector.getErrors()[0].locations == [new SourceLocation(4, 13), new SourceLocation(5, 13)]
}

//
Expand Down Expand Up @@ -524,7 +541,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
then:
errorCollector.getErrors().size() == 1

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

Expand Down Expand Up @@ -672,7 +689,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[field]) : 'deepField/x' : 'a' and 'b' are different fields"
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'field/deepField/x' : 'a' and 'b' are different fields"
errorCollector.getErrors()[0].locations.size() == 2
}

Expand Down Expand Up @@ -892,7 +909,7 @@ class OverlappingFieldsCanBeMergedTest extends Specification {

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict@[pets]) : 'friends/conflict' : returns different types 'Int' and 'Float'"
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'pets/friends/conflict' : returns different types 'Int' and 'Float'"
}


Expand Down Expand Up @@ -948,5 +965,57 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
errorCollector.getErrors().size() == 0
}

def "overlapping fields on lower level"() {
given:
def schema = schema('''
type Query {
pets: [Pet]
}
interface Pet {
name: String
breed: String
friends: [Pet]
}
type Dog implements Pet {
name: String
age: Int
dogBreed: String
breed: String
friends: [Pet]

}
type Cat implements Pet {
catBreed: String
breed: String
height: Float
name : String
friends: [Pet]

}
''')
def query = '''
{
pets {
friends {
... on Dog {
x: name
}
... on Cat {
x: height
}
}
}
}
'''
when:
traverse(query, schema)


then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error (FieldsConflict) : 'pets/friends/x' : returns different types 'String' and 'Float'"

}


}
Loading