graphql-java · bbakerman · Jul 18, 2021 · Jul 13, 2021 · Jul 13, 2021 · Jul 13, 2021
diff --git a/src/main/java/graphql/schema/GraphQLCodeRegistry.java b/src/main/java/graphql/schema/GraphQLCodeRegistry.java
@@ -1,6 +1,7 @@
 package graphql.schema;
 
 import graphql.Assert;
+import graphql.Internal;
 import graphql.PublicApi;
 import graphql.schema.visibility.GraphqlFieldVisibility;
 
@@ -189,7 +190,7 @@ public static class Builder {
         private final Map<String, TypeResolver> typeResolverMap = new HashMap<>();
         private GraphqlFieldVisibility fieldVisibility = DEFAULT_FIELD_VISIBILITY;
         private DataFetcherFactory<?> defaultDataFetcherFactory = env -> PropertyDataFetcher.fetching(env.getFieldDefinition().getName());
-
+        private boolean changed = false;
 
         private Builder() {
         }
@@ -202,6 +203,38 @@ private Builder(GraphQLCodeRegistry codeRegistry) {
             this.defaultDataFetcherFactory = codeRegistry.defaultDataFetcherFactory;
         }
 
+        /**
+         * A helper method to track if the builder changes from the point
+         * at which this method was called.
+         *
+         * @return this builder for fluent code
+         */
+        @Internal
+        public Builder trackChanges() {
+            changed = false;
+            return this;
+        }
+
+        /**
+         * @return true if the builder has changed since {@link #trackChanges()} was called
+         */
+        @Internal
+        public boolean hasChanged() {
+            return changed;
+        }
+
+        private Builder markChanged() {
+            changed = true;
+            return this;
+        }
+
+        private Builder markChanged(boolean condition) {
+            if (condition) {
+                changed = true;
+            }
+            return this;
+        }
+
         /**
          * Returns a data fetcher associated with a field within a container type
          *
@@ -309,7 +342,7 @@ public Builder systemDataFetcher(FieldCoordinates coordinates, DataFetcher<?> da
             assertNotNull(coordinates);
             coordinates.assertValidNames();
             systemDataFetcherMap.put(coordinates.getFieldName(), DataFetcherFactories.useDataFetcher(dataFetcher));
-            return this;
+            return markChanged();
         }
 
         /**
@@ -329,7 +362,7 @@ public Builder dataFetcher(FieldCoordinates coordinates, DataFetcherFactory<?> d
             } else {
                 dataFetcherMap.put(coordinates, dataFetcherFactory);
             }
-            return this;
+            return markChanged();
         }
 
         /**
@@ -347,6 +380,7 @@ public Builder dataFetcherIfAbsent(FieldCoordinates coordinates, DataFetcher<?>
                 } else {
                     dataFetcher(coordinates, dataFetcher);
                 }
+                return markChanged();
             }
             return this;
         }
@@ -362,7 +396,7 @@ public Builder dataFetcherIfAbsent(FieldCoordinates coordinates, DataFetcher<?>
         public Builder dataFetchers(String parentTypeName, Map<String, DataFetcher<?>> fieldDataFetchers) {
             assertNotNull(fieldDataFetchers);
             fieldDataFetchers.forEach((fieldName, dataFetcher) -> dataFetcher(coordinates(parentTypeName, fieldName), dataFetcher));
-            return this;
+            return markChanged(!fieldDataFetchers.isEmpty());
         }
 
         /**
@@ -375,57 +409,63 @@ public Builder dataFetchers(String parentTypeName, Map<String, DataFetcher<?>> f
          */
         public Builder defaultDataFetcher(DataFetcherFactory<?> defaultDataFetcherFactory) {
             this.defaultDataFetcherFactory = Assert.assertNotNull(defaultDataFetcherFactory);
-            return this;
+            return markChanged();
         }
 
         public Builder dataFetchers(GraphQLCodeRegistry codeRegistry) {
             this.dataFetcherMap.putAll(codeRegistry.dataFetcherMap);
-            return this;
+            return markChanged(!codeRegistry.dataFetcherMap.isEmpty());
         }
 
         public Builder typeResolver(GraphQLInterfaceType interfaceType, TypeResolver typeResolver) {
             typeResolverMap.put(interfaceType.getName(), typeResolver);
-            return this;
+            return markChanged();
         }
 
         public Builder typeResolverIfAbsent(GraphQLInterfaceType interfaceType, TypeResolver typeResolver) {
-            typeResolverMap.putIfAbsent(interfaceType.getName(), typeResolver);
+            if (!typeResolverMap.containsKey(interfaceType.getName())) {
+                typeResolverMap.put(interfaceType.getName(), typeResolver);
+                return markChanged();
+            }
             return this;
         }
 
         public Builder typeResolver(GraphQLUnionType unionType, TypeResolver typeResolver) {
             typeResolverMap.put(unionType.getName(), typeResolver);
-            return this;
+            return markChanged();
         }
 
         public Builder typeResolverIfAbsent(GraphQLUnionType unionType, TypeResolver typeResolver) {
-            typeResolverMap.putIfAbsent(unionType.getName(), typeResolver);
-            return this;
+            if (!typeResolverMap.containsKey(unionType.getName())) {
+                typeResolverMap.put(unionType.getName(), typeResolver);
+                return markChanged();
+            }
+            return markChanged();
         }
 
         public Builder typeResolver(String typeName, TypeResolver typeResolver) {
             typeResolverMap.put(assertValidName(typeName), typeResolver);
-            return this;
+            return markChanged();
         }
 
         public Builder typeResolvers(GraphQLCodeRegistry codeRegistry) {
             this.typeResolverMap.putAll(codeRegistry.typeResolverMap);
-            return this;
+            return markChanged(!codeRegistry.typeResolverMap.isEmpty());
         }
 
         public Builder fieldVisibility(GraphqlFieldVisibility fieldVisibility) {
             this.fieldVisibility = assertNotNull(fieldVisibility);
-            return this;
+            return markChanged();
         }
 
         public Builder clearDataFetchers() {
             dataFetcherMap.clear();
-            return this;
+            return markChanged();
         }
 
         public Builder clearTypeResolvers() {
             typeResolverMap.clear();
-            return this;
+            return markChanged();
         }
 
         public GraphQLCodeRegistry build() {

diff --git a/src/main/java/graphql/schema/SchemaTransformer.java b/src/main/java/graphql/schema/SchemaTransformer.java
@@ -140,7 +140,7 @@ private Object transformImpl(final GraphQLSchema schema, GraphQLSchemaElement sc
         final Map<String, GraphQLTypeReference> typeReferences = new LinkedHashMap<>();
 
         // first pass - general transformation
-        traverseAndTransform(dummyRoot, changedTypes, typeReferences, visitor, codeRegistry);
+        boolean schemaChanged = traverseAndTransform(dummyRoot, changedTypes, typeReferences, visitor, codeRegistry);
 
         // if we have changed any named elements AND we have type references referring to them then
         // we need to make a second pass to replace these type references to the new names
@@ -152,9 +152,13 @@ private Object transformImpl(final GraphQLSchema schema, GraphQLSchemaElement sc
         }
 
         if (schema != null) {
-            GraphQLSchema graphQLSchema = dummyRoot.rebuildSchema(codeRegistry);
-            if (postTransformation != null) {
-                graphQLSchema = graphQLSchema.transform(postTransformation);
+
+            GraphQLSchema graphQLSchema = schema;
+            if (schemaChanged || codeRegistry.hasChanged()) {
+                graphQLSchema = dummyRoot.rebuildSchema(codeRegistry);
+                if (postTransformation != null) {
+                    graphQLSchema = graphQLSchema.transform(postTransformation);
+                }
             }
             return graphQLSchema;
         } else {
@@ -177,7 +181,7 @@ public TraversalControl visitGraphQLTypeReference(GraphQLTypeReference typeRef,
         traverseAndTransform(dummyRoot, new HashMap<>(), new HashMap<>(), typeRefVisitor, codeRegistry);
     }
 
-    private void traverseAndTransform(DummyRoot dummyRoot, Map<String, GraphQLNamedType> changedTypes, Map<String, GraphQLTypeReference> typeReferences, GraphQLTypeVisitor visitor, GraphQLCodeRegistry.Builder codeRegistry) {
+    private boolean traverseAndTransform(DummyRoot dummyRoot, Map<String, GraphQLNamedType> changedTypes, Map<String, GraphQLTypeReference> typeReferences, GraphQLTypeVisitor visitor, GraphQLCodeRegistry.Builder codeRegistry) {
         List<NodeZipper<GraphQLSchemaElement>> zippers = new LinkedList<>();
         Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> zipperByNodeAfterTraversing = new LinkedHashMap<>();
         Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> zipperByOriginalNode = new LinkedHashMap<>();
@@ -268,16 +272,16 @@ public TraversalControl backRef(TraverserContext<GraphQLSchemaElement> context)
 
         List<List<GraphQLSchemaElement>> stronglyConnectedTopologicallySorted = getStronglyConnectedComponentsTopologicallySorted(reverseDependencies, typeRefReverseDependencies);
 
-        zipUpToDummyRoot(zippers, stronglyConnectedTopologicallySorted, breadcrumbsByZipper, zipperByNodeAfterTraversing);
+        return zipUpToDummyRoot(zippers, stronglyConnectedTopologicallySorted, breadcrumbsByZipper, zipperByNodeAfterTraversing);
     }
 
 
-    private void zipUpToDummyRoot(List<NodeZipper<GraphQLSchemaElement>> zippers,
-                                  List<List<GraphQLSchemaElement>> stronglyConnectedTopologicallySorted,
-                                  Map<NodeZipper<GraphQLSchemaElement>, List<List<Breadcrumb<GraphQLSchemaElement>>>> breadcrumbsByZipper,
-                                  Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> nodeToZipper) {
+    private boolean zipUpToDummyRoot(List<NodeZipper<GraphQLSchemaElement>> zippers,
+                                     List<List<GraphQLSchemaElement>> stronglyConnectedTopologicallySorted,
+                                     Map<NodeZipper<GraphQLSchemaElement>, List<List<Breadcrumb<GraphQLSchemaElement>>>> breadcrumbsByZipper,
+                                     Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> nodeToZipper) {
         if (zippers.size() == 0) {
-            return;
+            return false;
         }
         Set<NodeZipper<GraphQLSchemaElement>> curZippers = new LinkedHashSet<>(zippers);
 
@@ -331,8 +335,8 @@ private void zipUpToDummyRoot(List<NodeZipper<GraphQLSchemaElement>> zippers,
                 breadcrumbsByZipper.put(newZipper, breadcrumbsForOriginalParent);
 
             }
-
         }
+        return true;
     }
 
     private Map<NodeZipper<GraphQLSchemaElement>, List<Breadcrumb<GraphQLSchemaElement>>> zipperWithSameParent(GraphQLSchemaElement parent,

diff --git a/src/main/java/graphql/schema/idl/SchemaDirectiveWiringSchemaGeneratorPostProcessing.java b/src/main/java/graphql/schema/idl/SchemaDirectiveWiringSchemaGeneratorPostProcessing.java
@@ -43,23 +43,35 @@ public SchemaDirectiveWiringSchemaGeneratorPostProcessing(TypeDefinitionRegistry
 
     @Override
     public GraphQLSchema process(GraphQLSchema originalSchema) {
-        GraphQLSchema newSchema = SchemaTransformer.transformSchema(originalSchema, new Visitor());
-        return newSchema.transform(builder -> {
-            // they could have changed the code registry so rebuild it
-            GraphQLCodeRegistry codeRegistry = this.codeRegistryBuilder.build();
-            builder.codeRegistry(codeRegistry);
-        });
+        codeRegistryBuilder.trackChanges();
+        Visitor visitor = new Visitor();
+        GraphQLSchema newSchema = SchemaTransformer.transformSchema(originalSchema, visitor);
+        if (visitor.schemaChanged() || codeRegistryBuilder.hasChanged()) {
+            return newSchema.transform(builder -> {
+                // they could have changed the code registry so rebuild it
+                GraphQLCodeRegistry codeRegistry = this.codeRegistryBuilder.build();
+                builder.codeRegistry(codeRegistry);
+            });
+        }
+        return newSchema;
     }
 
     public class Visitor extends GraphQLTypeVisitorStub {
 
+        private boolean schemaChanged = false;
+
+        public boolean schemaChanged() {
+            return schemaChanged;
+        }
+
         private SchemaGeneratorDirectiveHelper.Parameters mkBehaviourParams() {
             return new SchemaGeneratorDirectiveHelper.Parameters(typeRegistry, runtimeWiring, directiveBehaviourContext, codeRegistryBuilder);
         }
 
         private TraversalControl changOrContinue(GraphQLSchemaElement node, GraphQLSchemaElement newNode, TraverserContext<GraphQLSchemaElement> context) {
             if (node != newNode) {
                 TreeTransformerUtil.changeNode(context, newNode);
+                schemaChanged = true;
             }
             return CONTINUE;
         }

diff --git a/src/main/java/graphql/util/DefaultTraverserContext.java b/src/main/java/graphql/util/DefaultTraverserContext.java
@@ -56,10 +56,11 @@ public DefaultTraverserContext(T curNode,
         if (parent == null || parent.isRootContext()) {
             this.breadcrumbs = ImmutableKit.emptyList();
         } else {
-            List<Breadcrumb<T>> breadcrumbs = new ArrayList<>(parent.getBreadcrumbs().size() + 1);
-            breadcrumbs.add(new Breadcrumb<>(this.parent.thisNode(), this.location));
-            breadcrumbs.addAll(parent.getBreadcrumbs());
-            this.breadcrumbs = ImmutableList.copyOf(breadcrumbs);
+            this.breadcrumbs = ImmutableList.<Breadcrumb<T>>builderWithExpectedSize(parent.getBreadcrumbs().size() + 1)
+                    .add(new Breadcrumb<>(this.parent.thisNode(), this.location))
+                    .addAll(parent.getBreadcrumbs())
+                    .build();
+
         }
     }
 

diff --git a/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy b/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy
@@ -345,4 +345,50 @@ class GraphQLCodeRegistryTest extends Specification {
         codeRegistry.getDataFetcher(userCoords, userFieldDef) == dfUser
 
     }
+
+    def "builder can track changes - internal methods"() {
+        DataFetcher<?> dfSystem = { env -> "system" }
+        DataFetcher<?> dfUser = { env -> "user" }
+        def systemFieldDef = newFieldDefinition().name("__system").type(GraphQLInt).build()
+        def userFieldDef = newFieldDefinition().name("field").type(GraphQLInt).build()
+        def systemCoords = FieldCoordinates.systemCoordinates(systemFieldDef.name)
+        def userCoords = FieldCoordinates.coordinates("User", userFieldDef.name)
+
+        when:
+        def codeRegistry = GraphQLCodeRegistry.newCodeRegistry()
+        then:
+        !codeRegistry.hasChanged()
+
+        when:
+        codeRegistry.dataFetcher(systemCoords, dfSystem)
+        codeRegistry.dataFetcher(userCoords, dfUser)
+        then:
+        codeRegistry.hasChanged()
+
+        when:
+        codeRegistry.trackChanges()
+        then:
+        !codeRegistry.hasChanged()
+
+        when:
+        codeRegistry.clearDataFetchers()
+        then:
+        codeRegistry.hasChanged()
+
+        when:
+        def newCodeRegistry = GraphQLCodeRegistry.newCodeRegistry(codeRegistry.build())
+        then:
+        !newCodeRegistry.hasChanged()
+
+        when:
+        newCodeRegistry = GraphQLCodeRegistry.newCodeRegistry(codeRegistry.build())
+        then:
+        !newCodeRegistry.hasChanged()
+
+        when:
+        newCodeRegistry.dataFetcher(systemCoords, dfSystem as DataFetcher)
+        newCodeRegistry.dataFetcher(userCoords, dfUser as DataFetcher)
+        then:
+        newCodeRegistry.hasChanged()
+    }
 }
diff --git a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy
@@ -744,4 +744,23 @@ type Query {
 }
 '''
     }
+
+    def "if nothing changes in the schema transformer, we return the same object"() {
+
+        def schema = TestUtil.schema("type Query { f : String }")
+
+        def fieldChanger = new GraphQLTypeVisitorStub() {
+
+            @Override
+            TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node,
+                                                         TraverserContext<GraphQLSchemaElement> context) {
+                return TraversalControl.CONTINUE
+            }
+        }
+
+        when:
+        def newSchema = SchemaTransformer.transformSchema(schema, fieldChanger)
+        then:
+        newSchema === schema
+    }
 }
diff --git a/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy
@@ -52,7 +52,7 @@ class SchemaGeneratorDirectiveHelperTest extends Specification {
     })
             .build()
 
-    def assertCallHierarchy(elementHierarchy, astHierarchy, String name, List<String> l) {
+    static def assertCallHierarchy(elementHierarchy, astHierarchy, String name, List<String> l) {
         assert elementHierarchy[name] == l, "unexpected elementHierarchy"
         assert astHierarchy[name] == l, "unexpected astHierarchy"
         true