refactoring, better test

andimarek · andimarek · commit 9c47bacbb194 · 2017-08-29T08:53:17.000+02:00
diff --git a/src/main/java/graphql/visitor/QueryTraversal.java b/src/main/java/graphql/visitor/QueryTraversal.java
@@ -15,9 +15,7 @@
 import graphql.schema.GraphQLFieldsContainer;
 import graphql.schema.GraphQLSchema;
 
-import java.util.ArrayList;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 
 public class QueryTraversal {
@@ -48,44 +46,40 @@ public QueryTraversal(Document document,
     }
 
     public void traverse() {
-        visit(operationDefinition.getSelectionSet(), new ArrayList<>(), schema.getQueryType());
+        visit(operationDefinition.getSelectionSet(), schema.getQueryType());
     }
 
 
-    private void visit(SelectionSet selectionSet, List<String> visitedFragments, GraphQLCompositeType type) {
+    private void visit(SelectionSet selectionSet, GraphQLCompositeType type) {
 
         for (Selection selection : selectionSet.getSelections()) {
             if (selection instanceof Field) {
                 GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) type;
                 GraphQLFieldDefinition fieldDefinition = fieldsContainer.getFieldDefinition(((Field) selection).getName());
-                visitField((Field) selection, visitedFragments, fieldDefinition);
+                visitField((Field) selection, fieldDefinition);
             } else if (selection instanceof InlineFragment) {
-                visitInlineFragment(visitedFragments, (InlineFragment) selection, type);
+                visitInlineFragment((InlineFragment) selection, type);
             } else if (selection instanceof FragmentSpread) {
-                visitFragmentSpread(visitedFragments, (FragmentSpread) selection);
+                visitFragmentSpread((FragmentSpread) selection);
             }
         }
     }
 
-    private void visitFragmentSpread(List<String> visitedFragments, FragmentSpread fragmentSpread) {
-        if (visitedFragments.contains(fragmentSpread.getName())) {
-            return;
-        }
+    private void visitFragmentSpread(FragmentSpread fragmentSpread) {
         if (!conditionalNodes.shouldInclude(this.variables, fragmentSpread.getDirectives())) {
             return;
         }
-//        visitedFragments.add(fragmentSpread.getName());
         FragmentDefinition fragmentDefinition = fragmentsByName.get(fragmentSpread.getName());
 
         if (!conditionalNodes.shouldInclude(variables, fragmentDefinition.getDirectives())) {
             return;
         }
         GraphQLCompositeType typeCondition = (GraphQLCompositeType) schema.getType(fragmentDefinition.getTypeCondition().getName());
-        visit(fragmentDefinition.getSelectionSet(), visitedFragments, typeCondition);
+        visit(fragmentDefinition.getSelectionSet(), typeCondition);
     }
 
 
-    private void visitInlineFragment(List<String> visitedFragments, InlineFragment inlineFragment, GraphQLCompositeType parentType) {
+    private void visitInlineFragment(InlineFragment inlineFragment, GraphQLCompositeType parentType) {
         if (!conditionalNodes.shouldInclude(variables, inlineFragment.getDirectives())) {
             return;
         }
@@ -98,16 +92,16 @@ private void visitInlineFragment(List<String> visitedFragments, InlineFragment i
             fragmentCondition = parentType;
         }
         // for unions we only have other fragments inside
-        visit(inlineFragment.getSelectionSet(), visitedFragments, fragmentCondition);
+        visit(inlineFragment.getSelectionSet(), fragmentCondition);
     }
 
-    private void visitField(Field field, List<String> visitedFragments, GraphQLFieldDefinition fieldDefinition) {
+    private void visitField(Field field, GraphQLFieldDefinition fieldDefinition) {
         if (!conditionalNodes.shouldInclude(variables, field.getDirectives())) {
             return;
         }
         visitor.visitField(field, fieldDefinition);
         if (fieldDefinition.getType() instanceof GraphQLCompositeType) {
-            visit(field.getSelectionSet(), visitedFragments, (GraphQLCompositeType) fieldDefinition.getType());
+            visit(field.getSelectionSet(), (GraphQLCompositeType) fieldDefinition.getType());
         }
     }
 
diff --git a/src/test/groovy/graphql/visitor/QueryTraversalTest.groovy b/src/test/groovy/graphql/visitor/QueryTraversalTest.groovy
@@ -28,7 +28,8 @@ class QueryTraversalTest extends Specification {
         return queryTraversal
     }
 
-    def "test "() {
+
+    def "simple query"() {
         given:
         def schema = TestUtil.schema("""
             type Query{
@@ -48,7 +49,9 @@ class QueryTraversalTest extends Specification {
         queryTraversal.traverse()
 
         then:
-        3 * visitor.visitField(_, _)
-
+        1 * visitor.visitField({ it.name == "foo" }, { it.type.name == "Foo" })
+        1 * visitor.visitField({ it.name == "bar" }, { it.type.name == "String" })
+        1 * visitor.visitField({ it.name == "subFoo" }, { it.type.name == "String" })
     }
+
 }

Original file line number	Diff line number	Diff line change
`@@ -15,9 +15,7 @@`
`15`	`15`	`import graphql.schema.GraphQLFieldsContainer;`
`16`	`16`	`import graphql.schema.GraphQLSchema;`
`17`	`17`
`18`		`-import java.util.ArrayList;`
`19`	`18`	`import java.util.LinkedHashMap;`
`20`		`-import java.util.List;`
`21`	`19`	`import java.util.Map;`
`22`	`20`
`23`	`21`	`public class QueryTraversal {`
`@@ -48,44 +46,40 @@ public QueryTraversal(Document document,`
`48`	`46`	`}`
`49`	`47`
`50`	`48`	`public void traverse() {`
`51`		`- visit(operationDefinition.getSelectionSet(), new ArrayList<>(), schema.getQueryType());`
	`49`	`+ visit(operationDefinition.getSelectionSet(), schema.getQueryType());`
`52`	`50`	`}`
`53`	`51`
`54`	`52`
`55`		`- private void visit(SelectionSet selectionSet, List<String> visitedFragments, GraphQLCompositeType type) {`
	`53`	`+ private void visit(SelectionSet selectionSet, GraphQLCompositeType type) {`
`56`	`54`
`57`	`55`	`for (Selection selection : selectionSet.getSelections()) {`
`58`	`56`	`if (selection instanceof Field) {`
`59`	`57`	`GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) type;`
`60`	`58`	`GraphQLFieldDefinition fieldDefinition = fieldsContainer.getFieldDefinition(((Field) selection).getName());`
`61`		`- visitField((Field) selection, visitedFragments, fieldDefinition);`
	`59`	`+ visitField((Field) selection, fieldDefinition);`
`62`	`60`	`} else if (selection instanceof InlineFragment) {`
`63`		`- visitInlineFragment(visitedFragments, (InlineFragment) selection, type);`
	`61`	`+ visitInlineFragment((InlineFragment) selection, type);`
`64`	`62`	`} else if (selection instanceof FragmentSpread) {`
`65`		`- visitFragmentSpread(visitedFragments, (FragmentSpread) selection);`
	`63`	`+ visitFragmentSpread((FragmentSpread) selection);`
`66`	`64`	`}`
`67`	`65`	`}`
`68`	`66`	`}`
`69`	`67`
`70`		`- private void visitFragmentSpread(List<String> visitedFragments, FragmentSpread fragmentSpread) {`
`71`		`- if (visitedFragments.contains(fragmentSpread.getName())) {`
`72`		`- return;`
`73`		`- }`
	`68`	`+ private void visitFragmentSpread(FragmentSpread fragmentSpread) {`
`74`	`69`	`if (!conditionalNodes.shouldInclude(this.variables, fragmentSpread.getDirectives())) {`
`75`	`70`	`return;`
`76`	`71`	`}`
`77`		`-// visitedFragments.add(fragmentSpread.getName());`
`78`	`72`	`FragmentDefinition fragmentDefinition = fragmentsByName.get(fragmentSpread.getName());`
`79`	`73`
`80`	`74`	`if (!conditionalNodes.shouldInclude(variables, fragmentDefinition.getDirectives())) {`
`81`	`75`	`return;`
`82`	`76`	`}`
`83`	`77`	`GraphQLCompositeType typeCondition = (GraphQLCompositeType) schema.getType(fragmentDefinition.getTypeCondition().getName());`
`84`		`- visit(fragmentDefinition.getSelectionSet(), visitedFragments, typeCondition);`
	`78`	`+ visit(fragmentDefinition.getSelectionSet(), typeCondition);`
`85`	`79`	`}`
`86`	`80`
`87`	`81`
`88`		`- private void visitInlineFragment(List<String> visitedFragments, InlineFragment inlineFragment, GraphQLCompositeType parentType) {`
	`82`	`+ private void visitInlineFragment(InlineFragment inlineFragment, GraphQLCompositeType parentType) {`
`89`	`83`	`if (!conditionalNodes.shouldInclude(variables, inlineFragment.getDirectives())) {`
`90`	`84`	`return;`
`91`	`85`	`}`
`@@ -98,16 +92,16 @@ private void visitInlineFragment(List<String> visitedFragments, InlineFragment i`
`98`	`92`	`fragmentCondition = parentType;`
`99`	`93`	`}`
`100`	`94`	`// for unions we only have other fragments inside`
`101`		`- visit(inlineFragment.getSelectionSet(), visitedFragments, fragmentCondition);`
	`95`	`+ visit(inlineFragment.getSelectionSet(), fragmentCondition);`
`102`	`96`	`}`
`103`	`97`
`104`		`- private void visitField(Field field, List<String> visitedFragments, GraphQLFieldDefinition fieldDefinition) {`
	`98`	`+ private void visitField(Field field, GraphQLFieldDefinition fieldDefinition) {`
`105`	`99`	`if (!conditionalNodes.shouldInclude(variables, field.getDirectives())) {`
`106`	`100`	`return;`
`107`	`101`	`}`
`108`	`102`	`visitor.visitField(field, fieldDefinition);`
`109`	`103`	`if (fieldDefinition.getType() instanceof GraphQLCompositeType) {`
`110`		`- visit(field.getSelectionSet(), visitedFragments, (GraphQLCompositeType) fieldDefinition.getType());`
	`104`	`+ visit(field.getSelectionSet(), (GraphQLCompositeType) fieldDefinition.getType());`
`111`	`105`	`}`
`112`	`106`	`}`
`113`	`107`
Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,8 @@ class QueryTraversalTest extends Specification {`
`28`	`28`	`return queryTraversal`
`29`	`29`	`}`
`30`	`30`
`31`		`- def "test "() {`
	`31`	`+`
	`32`	`+ def "simple query"() {`
`32`	`33`	`given:`
`33`	`34`	`def schema = TestUtil.schema("""`
`34`	`35`	`type Query{`
`@@ -48,7 +49,9 @@ class QueryTraversalTest extends Specification {`
`48`	`49`	`queryTraversal.traverse()`
`49`	`50`
`50`	`51`	`then:`
`51`		`- 3 * visitor.visitField(_, _)`
`52`		`-`
	`52`	`+ 1 * visitor.visitField({ it.name == "foo" }, { it.type.name == "Foo" })`
	`53`	`+ 1 * visitor.visitField({ it.name == "bar" }, { it.type.name == "String" })`
	`54`	`+ 1 * visitor.visitField({ it.name == "subFoo" }, { it.type.name == "String" })`
`53`	`55`	`}`
	`56`	`+`
`54`	`57`	`}`