From 5a53ec4d339c206ce1ea1ec1f5b62ce6d131af6d Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 15 Apr 2024 22:24:25 +1000 Subject: [PATCH 1/2] strictMode for RuntimeWiring and TypeRuntimeWiring --- .../graphql/schema/idl/RuntimeWiring.java | 28 ++++++- .../graphql/schema/idl/TypeRuntimeWiring.java | 47 ++++++++++++ .../idl/errors/StrictModeWiringException.java | 17 +++++ .../schema/idl/RuntimeWiringTest.groovy | 75 +++++++++++++++---- .../schema/idl/TypeRuntimeWiringTest.groovy | 74 ++++++++++++++++++ 5 files changed, 225 insertions(+), 16 deletions(-) create mode 100644 src/main/java/graphql/schema/idl/errors/StrictModeWiringException.java create mode 100644 src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy diff --git a/src/main/java/graphql/schema/idl/RuntimeWiring.java b/src/main/java/graphql/schema/idl/RuntimeWiring.java index 9edc34a246..0a95adce08 100644 --- a/src/main/java/graphql/schema/idl/RuntimeWiring.java +++ b/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 registeredDirectiveWiring = new LinkedHashMap<>(); private final List 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 bu public Builder type(TypeRuntimeWiring typeRuntimeWiring) { String typeName = typeRuntimeWiring.getTypeName(); Map 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; diff --git a/src/main/java/graphql/schema/idl/TypeRuntimeWiring.java b/src/main/java/graphql/schema/idl/TypeRuntimeWiring.java index 60bfe6f60c..1626139175 100644 --- a/src/main/java/graphql/schema/idl/TypeRuntimeWiring.java +++ b/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 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 && fieldDataFetchers.containsKey(fieldName)) { + throw new StrictModeWiringException(format("The field %s already has a data fetcher defined", fieldName)); + } fieldDataFetchers.put(fieldName, dataFetcher); return this; } @@ -119,6 +159,13 @@ public Builder dataFetcher(String fieldName, DataFetcher dataFetcher) { */ public Builder dataFetchers(Map dataFetchersMap) { assertNotNull(dataFetchersMap, () -> "you must provide a data fetchers map"); + if (strictMode) { + dataFetchersMap.forEach((fieldName, df) -> { + if (fieldDataFetchers.containsKey(fieldName)) { + throw new StrictModeWiringException(format("The field %s already has a data fetcher defined", fieldName)); + } + }); + } fieldDataFetchers.putAll(dataFetchersMap); return this; } diff --git a/src/main/java/graphql/schema/idl/errors/StrictModeWiringException.java b/src/main/java/graphql/schema/idl/errors/StrictModeWiringException.java new file mode 100644 index 0000000000..6da6b637fc --- /dev/null +++ b/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); + } +} diff --git a/src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy b/src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy index a12b854fe7..bd3c225d2a 100644 --- a/src/test/groovy/graphql/schema/idl/RuntimeWiringTest.groovy +++ b/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) + 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) .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) + 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) .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" + + } } diff --git a/src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy b/src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy new file mode 100644 index 0000000000..49408b3d8f --- /dev/null +++ b/src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy @@ -0,0 +1,74 @@ +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: + thrown(StrictModeWiringException) + } + + 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 + thrown(StrictModeWiringException) + + when: + TypeRuntimeWiring.setStrictModeJvmWide(false) + inStrictMode = TypeRuntimeWiring.getStrictModeJvmWide() + + TypeRuntimeWiring.newTypeWiring("Foo") + .dataFetcher("foo", DF1) + .dataFetcher("foo", DF2) + .build() + then: + !inStrictMode + noExceptionThrown() + } +} From bb82efb811aec223448f312ef706b7c7856c35a2 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 15 Apr 2024 22:41:46 +1000 Subject: [PATCH 2/2] Better testing --- .../graphql/schema/idl/TypeRuntimeWiring.java | 14 +++++++++----- .../schema/idl/TypeRuntimeWiringTest.groovy | 18 ++++++++++++++++-- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/main/java/graphql/schema/idl/TypeRuntimeWiring.java b/src/main/java/graphql/schema/idl/TypeRuntimeWiring.java index 1626139175..3480a5d6b0 100644 --- a/src/main/java/graphql/schema/idl/TypeRuntimeWiring.java +++ b/src/main/java/graphql/schema/idl/TypeRuntimeWiring.java @@ -143,8 +143,8 @@ public Builder strictMode() { 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 && fieldDataFetchers.containsKey(fieldName)) { - throw new StrictModeWiringException(format("The field %s already has a data fetcher defined", fieldName)); + if (strictMode) { + assertFieldStrictly(fieldName); } fieldDataFetchers.put(fieldName, dataFetcher); return this; @@ -161,15 +161,19 @@ public Builder dataFetchers(Map dataFetchersMap) { assertNotNull(dataFetchersMap, () -> "you must provide a data fetchers map"); if (strictMode) { dataFetchersMap.forEach((fieldName, df) -> { - if (fieldDataFetchers.containsKey(fieldName)) { - throw new StrictModeWiringException(format("The field %s already has a data fetcher defined", fieldName)); - } + 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. diff --git a/src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy b/src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy index 49408b3d8f..2a833ace22 100644 --- a/src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy +++ b/src/test/groovy/graphql/schema/idl/TypeRuntimeWiringTest.groovy @@ -35,7 +35,20 @@ class TypeRuntimeWiringTest extends Specification { .dataFetcher("foo", DF2) .build() then: - thrown(StrictModeWiringException) + 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"() { @@ -57,7 +70,8 @@ class TypeRuntimeWiringTest extends Specification { .build() then: inStrictMode - thrown(StrictModeWiringException) + def e = thrown(StrictModeWiringException) + e.message == "The field foo already has a data fetcher defined" when: TypeRuntimeWiring.setStrictModeJvmWide(false)