Merge pull request #3565 from graphql-java/strict-mode-runtime-wiring

strictMode for RuntimeWiring and TypeRuntimeWiring

src/main/java/graphql/schema/idl/RuntimeWiring.java

@@ -7,10 +7,10 @@
 import graphql.schema.GraphQLSchema;
 import graphql.schema.GraphqlTypeComparatorRegistry;
 import graphql.schema.TypeResolver;
+import graphql.schema.idl.errors.StrictModeWiringException;
 import graphql.schema.visibility.GraphqlFieldVisibility;
 
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -19,6 +19,7 @@
 
 import static graphql.Assert.assertNotNull;
 import static graphql.schema.visibility.DefaultGraphqlFieldVisibility.DEFAULT_FIELD_VISIBILITY;
+import static java.lang.String.format;
 
 /**
  * A runtime wiring is a specification of data fetchers, type resolvers and custom scalars that are needed
@@ -161,6 +162,7 @@ public static class Builder {
         private final Map<String, SchemaDirectiveWiring> registeredDirectiveWiring = new LinkedHashMap<>();
         private final List<SchemaDirectiveWiring> directiveWiring = new ArrayList<>();
         private WiringFactory wiringFactory = new NoopWiringFactory();
+        private boolean strictMode = false;
         private GraphqlFieldVisibility fieldVisibility = DEFAULT_FIELD_VISIBILITY;
         private GraphQLCodeRegistry codeRegistry = GraphQLCodeRegistry.newCodeRegistry().build();
         private GraphqlTypeComparatorRegistry comparatorRegistry = GraphqlTypeComparatorRegistry.AS_IS_REGISTRY;
@@ -169,6 +171,16 @@ private Builder() {
             ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS.forEach(this::scalar);
         }
 
+        /**
+         * This puts the builder into strict mode, so if things get defined twice, for example, it will throw a {@link StrictModeWiringException}.
+         *
+         * @return this builder
+         */
+        public Builder strictMode() {
+            this.strictMode = true;
+            return this;
+        }
+
         /**
          * Adds a wiring factory into the runtime wiring
          *
@@ -214,6 +226,9 @@ public Builder codeRegistry(GraphQLCodeRegistry.Builder codeRegistry) {
          * @return the runtime wiring builder
          */
         public Builder scalar(GraphQLScalarType scalarType) {
+            if (strictMode && scalars.containsKey(scalarType.getName())) {
+                throw new StrictModeWiringException(format("The scalar %s is already defined", scalarType.getName()));
+            }
             scalars.put(scalarType.getName(), scalarType);
             return this;
         }
@@ -264,17 +279,26 @@ public Builder type(String typeName, UnaryOperator<TypeRuntimeWiring.Builder> bu
         public Builder type(TypeRuntimeWiring typeRuntimeWiring) {
             String typeName = typeRuntimeWiring.getTypeName();
             Map<String, DataFetcher> typeDataFetchers = dataFetchers.computeIfAbsent(typeName, k -> new LinkedHashMap<>());
-            typeRuntimeWiring.getFieldDataFetchers().forEach(typeDataFetchers::put);
+            if (strictMode && !typeDataFetchers.isEmpty()) {
+                throw new StrictModeWiringException(format("The type %s has already been defined", typeName));
+            }
+            typeDataFetchers.putAll(typeRuntimeWiring.getFieldDataFetchers());
 
             defaultDataFetchers.put(typeName, typeRuntimeWiring.getDefaultDataFetcher());
 
             TypeResolver typeResolver = typeRuntimeWiring.getTypeResolver();
             if (typeResolver != null) {
+                if (strictMode && this.typeResolvers.containsKey(typeName)) {
+                    throw new StrictModeWiringException(format("The type %s already has a type resolver defined", typeName));
+                }
                 this.typeResolvers.put(typeName, typeResolver);
             }
 
             EnumValuesProvider enumValuesProvider = typeRuntimeWiring.getEnumValuesProvider();
             if (enumValuesProvider != null) {
+                if (strictMode && this.enumValuesProviders.containsKey(typeName)) {
+                    throw new StrictModeWiringException(format("The type %s already has a enum provider defined", typeName));
+                }
                 this.enumValuesProviders.put(typeName, enumValuesProvider);
             }
             return this;

src/main/java/graphql/schema/idl/TypeRuntimeWiring.java

@@ -4,12 +4,15 @@
 import graphql.schema.DataFetcher;
 import graphql.schema.GraphQLSchema;
 import graphql.schema.TypeResolver;
+import graphql.schema.idl.errors.StrictModeWiringException;
 
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.UnaryOperator;
 
 import static graphql.Assert.assertNotNull;
+import static java.lang.String.format;
 
 /**
  * A type runtime wiring is a specification of the data fetchers and possible type resolver for a given type name.
@@ -18,6 +21,28 @@
  */
 @PublicApi
 public class TypeRuntimeWiring {
+
+    private final static AtomicBoolean DEFAULT_STRICT_MODE = new AtomicBoolean(false);
+
+    /**
+     * By default {@link TypeRuntimeWiring} builders are not in strict mode, but you can set a JVM wide value
+     * so that any created will be.
+     *
+     * @param strictMode the desired strict mode state
+     *
+     * @see Builder#strictMode()
+     */
+    public static void setStrictModeJvmWide(boolean strictMode) {
+        DEFAULT_STRICT_MODE.set(strictMode);
+    }
+
+    /**
+     * @return the current JVM wide state of strict mode
+     */
+    public static boolean getStrictModeJvmWide() {
+        return DEFAULT_STRICT_MODE.get();
+    }
+
     private final String typeName;
     private final DataFetcher defaultDataFetcher;
     private final Map<String, DataFetcher> fieldDataFetchers;
@@ -82,6 +107,7 @@ public static class Builder {
         private DataFetcher defaultDataFetcher;
         private TypeResolver typeResolver;
         private EnumValuesProvider enumValuesProvider;
+        private boolean strictMode = DEFAULT_STRICT_MODE.get();
 
         /**
          * Sets the type name for this type wiring.  You MUST set this.
@@ -95,6 +121,17 @@ public Builder typeName(String typeName) {
             return this;
         }
 
+        /**
+         * This puts the builder into strict mode, so if things get defined twice, for example, it
+         * will throw a {@link StrictModeWiringException}.
+         *
+         * @return this builder
+         */
+        public Builder strictMode() {
+            this.strictMode = true;
+            return this;
+        }
+
         /**
          * Adds a data fetcher for the current type to the specified field
          *
@@ -106,6 +143,9 @@ public Builder typeName(String typeName) {
         public Builder dataFetcher(String fieldName, DataFetcher dataFetcher) {
             assertNotNull(dataFetcher, () -> "you must provide a data fetcher");
             assertNotNull(fieldName, () -> "you must tell us what field");
+            if (strictMode) {
+                assertFieldStrictly(fieldName);
+            }
             fieldDataFetchers.put(fieldName, dataFetcher);
             return this;
         }
@@ -119,10 +159,21 @@ public Builder dataFetcher(String fieldName, DataFetcher dataFetcher) {
          */
         public Builder dataFetchers(Map<String, DataFetcher> dataFetchersMap) {
             assertNotNull(dataFetchersMap, () -> "you must provide a data fetchers map");
+            if (strictMode) {
+                dataFetchersMap.forEach((fieldName, df) -> {
+                    assertFieldStrictly(fieldName);
+                });
+            }
             fieldDataFetchers.putAll(dataFetchersMap);
             return this;
         }
 
+        private void assertFieldStrictly(String fieldName) {
+            if (fieldDataFetchers.containsKey(fieldName)) {
+                throw new StrictModeWiringException(format("The field %s already has a data fetcher defined", fieldName));
+            }
+        }
+
         /**
          * All fields in a type need a data fetcher of some sort and this method is called to provide the default data fetcher
          * that will be used for this type if no specific one has been provided per field.

src/main/java/graphql/schema/idl/errors/StrictModeWiringException.java

@@ -0,0 +1,17 @@
+package graphql.schema.idl.errors;
+
+import graphql.GraphQLException;
+import graphql.PublicApi;
+import graphql.schema.idl.RuntimeWiring;
+import graphql.schema.idl.TypeRuntimeWiring;
+
+/**
+ * An exception that is throw when {@link RuntimeWiring.Builder#strictMode()} or {@link TypeRuntimeWiring.Builder#strictMode()} is true and
+ * something gets redefined.
+ */
+@PublicApi
+public class StrictModeWiringException extends GraphQLException {
+    public StrictModeWiringException(String msg) {
+        super(msg);
+    }
+}

src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy

@@ -1,5 +1,6 @@
 package graphql.schema.idl
 
+import graphql.Scalars
 import graphql.TypeResolutionEnvironment
 import graphql.schema.Coercing
 import graphql.schema.DataFetcher
@@ -9,6 +10,7 @@ import graphql.schema.GraphQLFieldsContainer
 import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLScalarType
 import graphql.schema.TypeResolver
+import graphql.schema.idl.errors.StrictModeWiringException
 import graphql.schema.visibility.GraphqlFieldVisibility
 import spock.lang.Specification
 
@@ -62,22 +64,22 @@ class RuntimeWiringTest extends Specification {
     def "basic call structure"() {
         def wiring = RuntimeWiring.newRuntimeWiring()
                 .type("Query", { type ->
-            type
-                    .dataFetcher("fieldX", new NamedDF("fieldX"))
-                    .dataFetcher("fieldY", new NamedDF("fieldY"))
-                    .dataFetcher("fieldZ", new NamedDF("fieldZ"))
-                    .defaultDataFetcher(new NamedDF("defaultQueryDF"))
-                    .typeResolver(new NamedTR("typeResolver4Query"))
-        } as UnaryOperator<TypeRuntimeWiring.Builder>)
+                    type
+                            .dataFetcher("fieldX", new NamedDF("fieldX"))
+                            .dataFetcher("fieldY", new NamedDF("fieldY"))
+                            .dataFetcher("fieldZ", new NamedDF("fieldZ"))
+                            .defaultDataFetcher(new NamedDF("defaultQueryDF"))
+                            .typeResolver(new NamedTR("typeResolver4Query"))
+                } as UnaryOperator<TypeRuntimeWiring.Builder>)
 
                 .type("Mutation", { type ->
-            type
-                    .dataFetcher("fieldX", new NamedDF("mfieldX"))
-                    .dataFetcher("fieldY", new NamedDF("mfieldY"))
-                    .dataFetcher("fieldZ", new NamedDF("mfieldZ"))
-                    .defaultDataFetcher(new NamedDF("defaultMutationDF"))
-                    .typeResolver(new NamedTR("typeResolver4Mutation"))
-        } as UnaryOperator<TypeRuntimeWiring.Builder>)
+                    type
+                            .dataFetcher("fieldX", new NamedDF("mfieldX"))
+                            .dataFetcher("fieldY", new NamedDF("mfieldY"))
+                            .dataFetcher("fieldZ", new NamedDF("mfieldZ"))
+                            .defaultDataFetcher(new NamedDF("defaultMutationDF"))
+                            .typeResolver(new NamedTR("typeResolver4Mutation"))
+                } as UnaryOperator<TypeRuntimeWiring.Builder>)
                 .build()
 
 
@@ -190,4 +192,49 @@ class RuntimeWiringTest extends Specification {
         newWiring.scalars["Custom2"] == customScalar2
         newWiring.fieldVisibility == fieldVisibility
     }
+
+    def "strict mode can stop certain redefinitions"() {
+        DataFetcher DF1 = env -> "x"
+        TypeResolver TR1 = env -> null
+        EnumValuesProvider EVP1 = name -> null
+
+        when:
+        RuntimeWiring.newRuntimeWiring()
+                .strictMode()
+                .type(TypeRuntimeWiring.newTypeWiring("Foo").dataFetcher("foo", DF1))
+                .type(TypeRuntimeWiring.newTypeWiring("Foo").dataFetcher("bar", DF1))
+
+
+        then:
+        def e1 = thrown(StrictModeWiringException)
+        e1.message == "The type Foo has already been defined"
+
+        when:
+        RuntimeWiring.newRuntimeWiring()
+                .strictMode()
+                .type(TypeRuntimeWiring.newTypeWiring("Foo").typeResolver(TR1))
+                .type(TypeRuntimeWiring.newTypeWiring("Foo").typeResolver(TR1))
+
+        then:
+        def e2 = thrown(StrictModeWiringException)
+        e2.message == "The type Foo already has a type resolver defined"
+
+        when:
+        RuntimeWiring.newRuntimeWiring()
+                .strictMode()
+                .type(TypeRuntimeWiring.newTypeWiring("Foo").enumValues(EVP1))
+                .type(TypeRuntimeWiring.newTypeWiring("Foo").enumValues(EVP1))
+        then:
+        def e3 = thrown(StrictModeWiringException)
+        e3.message == "The type Foo already has a enum provider defined"
+
+        when:
+        RuntimeWiring.newRuntimeWiring()
+                .strictMode()
+                .scalar(Scalars.GraphQLString)
+        then:
+        def e4 = thrown(StrictModeWiringException)
+        e4.message == "The scalar String is already defined"
+
+    }
 }

src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy

@@ -0,0 +1,88 @@
+package graphql.schema.idl
+
+import graphql.schema.DataFetcher
+import graphql.schema.idl.errors.StrictModeWiringException
+import spock.lang.Specification
+
+class TypeRuntimeWiringTest extends Specification {
+
+    void setup() {
+        TypeRuntimeWiring.setStrictModeJvmWide(false)
+    }
+
+    void cleanup() {
+        TypeRuntimeWiring.setStrictModeJvmWide(false)
+    }
+
+    DataFetcher DF1 = env -> "x"
+    DataFetcher DF2 = env -> "y"
+
+    def "strict mode is off by default"() {
+        when:
+        def typeRuntimeWiring = TypeRuntimeWiring.newTypeWiring("Foo")
+                .dataFetcher("foo", DF1)
+                .dataFetcher("foo", DF2)
+                .build()
+        then:
+        typeRuntimeWiring.getFieldDataFetchers().get("foo") == DF2
+    }
+
+    def "strict mode can be turned on"() {
+        when:
+        TypeRuntimeWiring.newTypeWiring("Foo")
+                .strictMode()
+                .dataFetcher("foo", DF1)
+                .dataFetcher("foo", DF2)
+                .build()
+        then:
+        def e = thrown(StrictModeWiringException)
+        e.message == "The field foo already has a data fetcher defined"
+    }
+
+    def "strict mode can be turned on for maps of fields"() {
+        when:
+        TypeRuntimeWiring.newTypeWiring("Foo")
+                .strictMode()
+                .dataFetcher("foo", DF1)
+                .dataFetchers(["foo": DF2])
+                .build()
+        then:
+        def e = thrown(StrictModeWiringException)
+        e.message == "The field foo already has a data fetcher defined"
+    }
+
+    def "strict mode can be turned on JVM wide"() {
+
+
+        when:
+        def inStrictMode = TypeRuntimeWiring.getStrictModeJvmWide()
+        then:
+        !inStrictMode
+
+
+        when:
+        TypeRuntimeWiring.setStrictModeJvmWide(true)
+        inStrictMode = TypeRuntimeWiring.getStrictModeJvmWide()
+
+        TypeRuntimeWiring.newTypeWiring("Foo")
+                .dataFetcher("foo", DF1)
+                .dataFetcher("foo", DF2)
+                .build()
+        then:
+        inStrictMode
+        def e = thrown(StrictModeWiringException)
+        e.message == "The field foo already has a data fetcher defined"
+
+        when:
+        TypeRuntimeWiring.setStrictModeJvmWide(false)
+        inStrictMode = TypeRuntimeWiring.getStrictModeJvmWide()
+
+        TypeRuntimeWiring.newTypeWiring("Foo")
+                .dataFetcher("foo", DF1)
+                .dataFetcher("foo", DF2)
+                .build()
+        then:
+        !inStrictMode
+        noExceptionThrown()
+    }
+}