Skip to content

Fixes #763 QueryTraversal problem #765

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 2 commits into from
Oct 17, 2017
Merged

Fixes #763 QueryTraversal problem #765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74e75b0
Added tests for #763 that currently fail
bbakerman Oct 10, 2017
9d84308
Fixed the Union / __typename problem and also made the common code co…
bbakerman Oct 10, 2017
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
23 changes: 2 additions & 21 deletions src/main/java/graphql/analysis/QueryTraversal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import graphql.Internal;
import graphql.execution.ConditionalNodes;
import graphql.execution.ValuesResolver;
import graphql.introspection.Introspection;
import graphql.language.Document;
import graphql.language.Field;
import graphql.language.FragmentDefinition;
Expand All @@ -15,7 +16,6 @@
import graphql.language.TypeName;
import graphql.schema.GraphQLCompositeType;
import graphql.schema.GraphQLFieldDefinition;
import graphql.schema.GraphQLFieldsContainer;
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLSchema;
import graphql.schema.GraphQLUnmodifiedType;
Expand All @@ -26,9 +26,6 @@

import static graphql.Assert.assertNotNull;
import static graphql.Assert.assertShouldNeverHappen;
import static graphql.introspection.Introspection.SchemaMetaFieldDef;
import static graphql.introspection.Introspection.TypeMetaFieldDef;
import static graphql.introspection.Introspection.TypeNameMetaFieldDef;

@Internal
public class QueryTraversal {
Expand Down Expand Up @@ -95,8 +92,7 @@ private void visitImpl(QueryVisitor visitor, SelectionSet selectionSet, GraphQLC

for (Selection selection : selectionSet.getSelections()) {
if (selection instanceof Field) {
GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) type;
GraphQLFieldDefinition fieldDefinition = getFieldDef(fieldsContainer, (Field) selection);
GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(schema, type, ((Field) selection).getName());
visitField(visitor, (Field) selection, fieldDefinition, type, parent, preOrder);
} else if (selection instanceof InlineFragment) {
visitInlineFragment(visitor, (InlineFragment) selection, type, parent, preOrder);
Expand All @@ -106,21 +102,6 @@ private void visitImpl(QueryVisitor visitor, SelectionSet selectionSet, GraphQLC
}
}

protected GraphQLFieldDefinition getFieldDef(GraphQLFieldsContainer parentType, Field field) {
if (schema.getQueryType() == parentType) {
if (field.getName().equals(SchemaMetaFieldDef.getName())) {
return SchemaMetaFieldDef;
}
if (field.getName().equals(TypeMetaFieldDef.getName())) {
return TypeMetaFieldDef;
}
}
if (field.getName().equals(TypeNameMetaFieldDef.getName())) {
return TypeNameMetaFieldDef;
}
return assertNotNull(parentType.getFieldDefinition(field.getName()), "should not happen: unknown field " + field.getName());
}

private void visitFragmentSpread(QueryVisitor visitor, FragmentSpread fragmentSpread, QueryVisitorEnvironment parent, boolean preOrder) {
if (!conditionalNodes.shouldInclude(this.variables, fragmentSpread.getDirectives())) {
return;
Expand Down
21 changes: 2 additions & 19 deletions src/main/java/graphql/execution/ExecutionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package graphql.execution;

import graphql.Assert;
import graphql.ExecutionResult;
import graphql.ExecutionResultImpl;
import graphql.PublicSpi;
Expand All @@ -10,6 +9,7 @@
import graphql.execution.instrumentation.InstrumentationContext;
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldParameters;
import graphql.introspection.Introspection;
import graphql.language.Field;
import graphql.schema.CoercingSerializeException;
import graphql.schema.DataFetcher;
Expand Down Expand Up @@ -40,9 +40,6 @@

import static graphql.execution.ExecutionTypeInfo.newTypeInfo;
import static graphql.execution.FieldCollectorParameters.newParameters;
import static graphql.introspection.Introspection.SchemaMetaFieldDef;
import static graphql.introspection.Introspection.TypeMetaFieldDef;
import static graphql.introspection.Introspection.TypeNameMetaFieldDef;
import static graphql.schema.DataFetchingEnvironmentBuilder.newDataFetchingEnvironment;
import static java.util.concurrent.CompletableFuture.completedFuture;
import static java.util.stream.Collectors.toList;
Expand Down Expand Up @@ -589,21 +586,7 @@ protected GraphQLFieldDefinition getFieldDef(ExecutionContext executionContext,
* @return a {@link GraphQLFieldDefinition}
*/
protected GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLObjectType parentType, Field field) {
if (schema.getQueryType() == parentType) {
if (field.getName().equals(SchemaMetaFieldDef.getName())) {
return SchemaMetaFieldDef;
}
if (field.getName().equals(TypeMetaFieldDef.getName())) {
return TypeMetaFieldDef;
}
}
if (field.getName().equals(TypeNameMetaFieldDef.getName())) {
return TypeNameMetaFieldDef;
}

GraphQLFieldDefinition fieldDefinition = schema.getFieldVisibility().getFieldDefinition(parentType, field.getName());
Assert.assertTrue(fieldDefinition != null, "Unknown field " + field.getName());
return fieldDefinition;
return Introspection.getFieldDef(schema, parentType, field.getName());
}

/**
Expand Down
35 changes: 34 additions & 1 deletion src/main/java/graphql/introspection/Introspection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import graphql.schema.DataFetcher;
import graphql.schema.DataFetchingEnvironment;
import graphql.schema.GraphQLArgument;
import graphql.schema.GraphQLCompositeType;
import graphql.schema.GraphQLDirective;
import graphql.schema.GraphQLEnumType;
import graphql.schema.GraphQLEnumValueDefinition;
Expand All @@ -28,6 +29,7 @@
import java.util.ArrayList;
import java.util.List;

import static graphql.Assert.assertTrue;
import static graphql.Scalars.GraphQLBoolean;
import static graphql.Scalars.GraphQLString;
import static graphql.schema.GraphQLArgument.newArgument;
Expand Down Expand Up @@ -461,11 +463,42 @@ public enum DirectiveLocation {
// make sure all TypeReferences are resolved
GraphQLSchema.newSchema()
.query(GraphQLObjectType.newObject()
.name("dummySchema")
.name("IntrospectionQuery")
.field(SchemaMetaFieldDef)
.field(TypeMetaFieldDef)
.field(TypeNameMetaFieldDef)
.build())
.build();
}

/**
Copy link
Member Author

@bbakerman bbakerman Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was used in at least 2 spots (again according to IDEA) so I moved it into a common place with better common logic and type handling

* This will look up a field definition by name, and understand that fields like __typename and __schema are special
* and take precedence in field resolution
*
* @param schema the schema to use
* @param parentType the type of the parent object
* @param fieldName the field to look up
*
* @return a field definition otherwise throws an assertion exception if its null
*/
public static GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLCompositeType parentType, String fieldName) {

if (schema.getQueryType() == parentType) {
if (fieldName.equals(SchemaMetaFieldDef.getName())) {
return SchemaMetaFieldDef;
}
if (fieldName.equals(TypeMetaFieldDef.getName())) {
return TypeMetaFieldDef;
}
}
if (fieldName.equals(TypeNameMetaFieldDef.getName())) {
return TypeNameMetaFieldDef;
}

assertTrue(parentType instanceof GraphQLFieldsContainer, "should not happen : parent type must be an object or interface : " + parentType);
GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) parentType;
GraphQLFieldDefinition fieldDefinition = schema.getFieldVisibility().getFieldDefinition(fieldsContainer, fieldName);
Assert.assertTrue(fieldDefinition != null, "Unknown field " + fieldName);
return fieldDefinition;
}
}
55 changes: 53 additions & 2 deletions src/test/groovy/graphql/analysis/QueryTraversalTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,6 @@ class QueryTraversalTest extends Specification {
subFoo: String
}
""")
def visitor = Mock(QueryVisitor)
def query = createQuery("""
{foo { subFoo} bar }
""")
Expand Down Expand Up @@ -582,7 +581,6 @@ class QueryTraversalTest extends Specification {
subFoo: String
}
""")
def visitor = Mock(QueryVisitor)
Copy link
Member Author

@bbakerman bbakerman Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used according to idea

def query = createQuery("""
{foo { subFoo} bar }
""")
Expand Down Expand Up @@ -712,4 +710,57 @@ class QueryTraversalTest extends Specification {

}

def "#763 handles union types"() {
given:
def schema = TestUtil.schema("""
type Query{
someObject: SomeObject
}
type SomeObject {
someUnionType: SomeUnionType
}

union SomeUnionType = TypeX | TypeY

type TypeX {
field1 : String
}

type TypeY {
field2 : String
}
""")
def visitor = Mock(QueryVisitor)
def query = createQuery("""
{
someObject {
someUnionType {
__typename
... on TypeX {
field1
}
... on TypeY {
field2
}
}
}
}
""")
QueryTraversal queryTraversal = createQueryTraversal(query, schema)
when:
queryTraversal."$visitFn"(visitor)

then:
1 * visitor.visitField({ QueryVisitorEnvironment it -> it.field.name == "someObject" && it.fieldDefinition.type.name == "SomeObject" && it.parentType.name == "Query" })
1 * visitor.visitField({ QueryVisitorEnvironment it -> it.field.name == "someUnionType" && it.fieldDefinition.type.name == "SomeUnionType" && it.parentType.name == "SomeObject" })
1 * visitor.visitField({ QueryVisitorEnvironment it -> it.field.name == "__typename" && it.fieldDefinition.type.wrappedType.name == "String"})
1 * visitor.visitField({ QueryVisitorEnvironment it -> it.field.name == "field1" && it.fieldDefinition.type.name == "String" && it.parentType.name == "TypeX" })
1 * visitor.visitField({ QueryVisitorEnvironment it -> it.field.name == "field2" && it.fieldDefinition.type.name == "String" && it.parentType.name == "TypeY" })

where:
order | visitFn
'postOrder' | 'visitPostOrder'
'preOrder' | 'visitPreOrder'

}
}