Skip to content

Now follows graphql-js and its more lenient field merging #708

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
bbakerman merged 1 commit into from
Sep 30, 2017
Merged

Now follows graphql-js and its more lenient field merging #708

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
57 changes: 27 additions & 30 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 @@ -2,16 +2,32 @@


import graphql.execution.TypeFromAST;
import graphql.language.*;
import graphql.schema.*;
import graphql.language.Argument;
import graphql.language.AstComparator;
import graphql.language.Field;
import graphql.language.FragmentDefinition;
import graphql.language.FragmentSpread;
import graphql.language.InlineFragment;
import graphql.language.Selection;
import graphql.language.SelectionSet;
import graphql.language.Value;
import graphql.schema.GraphQLFieldDefinition;
import graphql.schema.GraphQLFieldsContainer;
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLOutputType;
import graphql.schema.GraphQLType;
import graphql.validation.AbstractRule;
import graphql.validation.ErrorFactory;
import graphql.validation.ValidationContext;
import graphql.validation.ValidationErrorCollector;

import java.util.*;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static graphql.language.NodeUtil.directivesByName;
import static graphql.validation.ValidationErrorType.FieldsConflict;

public class OverlappingFieldsCanBeMerged extends AbstractRule {
Expand Down Expand Up @@ -66,6 +82,7 @@ private boolean isAlreadyChecked(Field field1, Field field2) {
return false;
}

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

Field field1 = fieldAndType1.field;
Expand Down Expand Up @@ -114,10 +131,6 @@ private Conflict findConflict(String responseName, FieldAndType fieldAndType1, F
String reason = String.format("%s: they have differing arguments", responseName);
return new Conflict(responseName, reason, field1, field2);
}
if (!sameDirectives(field1.getDirectives(), field2.getDirectives())) {
String reason = String.format("%s: they have differing directives", responseName);
return new Conflict(responseName, reason, field1, field2);
}
SelectionSet selectionSet1 = field1.getSelectionSet();
SelectionSet selectionSet2 = field2.getSelectionSet();
if (selectionSet1 != null && selectionSet2 != null) {
Expand Down Expand Up @@ -160,11 +173,13 @@ private String joinReasons(List<Conflict> conflicts) {
return result.toString();
}

@SuppressWarnings("SimplifiableIfStatement")
private boolean sameType(GraphQLType type1, GraphQLType type2) {
if (type1 == null || type2 == null) return true;
return type1.equals(type2);
}

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

private boolean sameDirectives(List<Directive> directives1, List<Directive> directives2) {
if (directives1.size() != directives2.size()) return false;
for (Directive directive : directives1) {
Directive matchedDirective = getDirectiveByName(directive.getName(), directives2);
if (matchedDirective == null) return false;
if (!sameArguments(directive.getArguments(), matchedDirective.getArguments())) return false;
}
return true;
}

private Directive getDirectiveByName(String name, List<Directive> directives) {
return directivesByName(directives).get(name);
}


private void collectFields(Map<String, List<FieldAndType>> fieldMap, SelectionSet selectionSet, GraphQLType parentType, Set<String> visitedFragmentSpreads) {

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

}

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

private void collectFieldsForInlineFragment(Map<String, List<FieldAndType>> fieldMap, Set<String> visitedFragmentSpreads, GraphQLType parentType, InlineFragment selection) {
InlineFragment inlineFragment = selection;
private void collectFieldsForInlineFragment(Map<String, List<FieldAndType>> fieldMap, Set<String> visitedFragmentSpreads, GraphQLType parentType, InlineFragment inlineFragment) {
GraphQLType graphQLType = inlineFragment.getTypeCondition() != null
? (GraphQLOutputType) TypeFromAST.getTypeFromAST(getValidationContext().getSchema(), inlineFragment.getTypeCondition())
: parentType;
collectFields(fieldMap, inlineFragment.getSelectionSet(), graphQLType, visitedFragmentSpreads);
}

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

private GraphQLFieldDefinition getVisibleFieldDefinition(GraphQLFieldsContainer fieldsContainer, Field field) {
return getValidationContext().getSchema().getFieldVisibility().getFieldDefinition(fieldsContainer,field.getName());
return getValidationContext().getSchema().getFieldVisibility().getFieldDefinition(fieldsContainer, field.getName());
}

private static class FieldPair {
Expand Down
80 changes: 17 additions & 63 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 @@ -95,11 +95,11 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
.name("BoxUnion")
.possibleTypes(StringBox, IntBox, NonNullStringBox1, NonNullStringBox2)
.typeResolver(new TypeResolver() {
@Override
GraphQLObjectType getType(TypeResolutionEnvironment env) {
return null
}
})
@Override
GraphQLObjectType getType(TypeResolutionEnvironment env) {
return null
}
})
.build()
def QueryRoot = newObject()
.name("QueryRoot")
Expand Down Expand Up @@ -297,71 +297,25 @@ class OverlappingFieldsCanBeMergedTest extends Specification {
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
}

def 'conflicting directives'() {
//
// The rules have been relaxed regarding fragment uniqueness.
//
// see https://github.com/facebook/graphql/pull/120/files
// and https://github.com/graphql/graphql-js/pull/230/files
//
def "different skip/include directives accepted"() {
given:
def query = """
fragment conflictingDirectiveArgs on Dog {
name @include(if: true)
name @skip(if: false)
}
"""
when:
traverse(query, null)

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: name: they have differing directives"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
}

def 'conflicting directive args'() {
given:
def query = """
fragment conflictingDirectiveArgs on Dog {
name @include(if: true)
name @include(if: false)
}
"""
when:
traverse(query, null)

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: name: they have differing directives"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
}

def 'conflicting args with matching directives'() {
given:
def query = """
fragment conflictingArgsWithMatchingDirectiveArgs on Dog {
doesKnowCommand(dogCommand: SIT) @include(if: true)
doesKnowCommand(dogCommand: HEEL) @include(if: true)
}
"""
when:
traverse(query, null)

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: doesKnowCommand: they have differing arguments"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
}

def 'conflicting directives with matching args'() {
def query = """
fragment conflictingDirectiveArgsWithMatchingArgs on Dog {
doesKnowCommand(dogCommand: SIT) @include(if: true)
doesKnowCommand(dogCommand: SIT) @skip(if: false)
}
fragment differentDirectivesWithDifferentAliases on Dog {
name @include(if: true)
name @include(if: false)
}
"""
when:
traverse(query, null)

then:
errorCollector.getErrors().size() == 1
errorCollector.getErrors()[0].message == "Validation error of type FieldsConflict: doesKnowCommand: they have differing directives"
errorCollector.getErrors()[0].locations == [new SourceLocation(3, 13), new SourceLocation(4, 13)]
errorCollector.getErrors().isEmpty()
}

def 'encounters conflict in fragments'() {
Expand Down