diff --git a/src/main/java/graphql/schema/DataFetcherFactories.java b/src/main/java/graphql/schema/DataFetcherFactories.java index e9f6a88bb5..2345535d56 100644 --- a/src/main/java/graphql/schema/DataFetcherFactories.java +++ b/src/main/java/graphql/schema/DataFetcherFactories.java @@ -20,7 +20,18 @@ public class DataFetcherFactories { * @return a data fetcher factory that always returns the provided data fetcher */ public static DataFetcherFactory useDataFetcher(DataFetcher dataFetcher) { - return fieldDefinition -> dataFetcher; + //noinspection deprecation + return new DataFetcherFactory<>() { + @Override + public DataFetcher get(DataFetcherFactoryEnvironment environment) { + return dataFetcher; + } + + @Override + public DataFetcher get(GraphQLFieldDefinition fieldDefinition) { + return dataFetcher; + } + }; } /** @@ -32,7 +43,7 @@ public static DataFetcherFactory useDataFetcher(DataFetcher dataFetche * * @return a new data fetcher that wraps the provided data fetcher */ - public static DataFetcher wrapDataFetcher(DataFetcher delegateDataFetcher, BiFunction mapFunction) { + public static DataFetcher wrapDataFetcher(DataFetcher delegateDataFetcher, BiFunction mapFunction) { return environment -> { Object value = delegateDataFetcher.get(environment); if (value instanceof CompletionStage) { diff --git a/src/main/java/graphql/schema/DataFetcherFactory.java b/src/main/java/graphql/schema/DataFetcherFactory.java index ece0dcb6ea..9e7eafa872 100644 --- a/src/main/java/graphql/schema/DataFetcherFactory.java +++ b/src/main/java/graphql/schema/DataFetcherFactory.java @@ -19,7 +19,23 @@ public interface DataFetcherFactory { * @param environment the environment that needs the data fetcher * * @return a data fetcher + * + * @deprecated This method will go away at some point and {@link DataFetcherFactory#get(GraphQLFieldDefinition)} will be used */ + @Deprecated(since = "2024-11-26") DataFetcher get(DataFetcherFactoryEnvironment environment); + /** + * Returns a {@link graphql.schema.DataFetcher} given the field definition + * which is cheaper in object allocation terms. + * + * @param fieldDefinition the field that needs the data fetcher + * + * @return a data fetcher + */ + + default DataFetcher get(GraphQLFieldDefinition fieldDefinition) { + return null; + } + } diff --git a/src/main/java/graphql/schema/DataFetcherFactoryEnvironment.java b/src/main/java/graphql/schema/DataFetcherFactoryEnvironment.java index 318b5f2bd3..7ff5aefb9a 100644 --- a/src/main/java/graphql/schema/DataFetcherFactoryEnvironment.java +++ b/src/main/java/graphql/schema/DataFetcherFactoryEnvironment.java @@ -5,8 +5,12 @@ /** * This is passed to a {@link graphql.schema.DataFetcherFactory} when it is invoked to * get a {@link graphql.schema.DataFetcher} + * + * @deprecated This class will go away at some point in the future since its pointless wrapper + * of a {@link GraphQLFieldDefinition} */ @PublicApi +@Deprecated(since = "2024-11-26") public class DataFetcherFactoryEnvironment { private final GraphQLFieldDefinition fieldDefinition; diff --git a/src/main/java/graphql/schema/GraphQLCodeRegistry.java b/src/main/java/graphql/schema/GraphQLCodeRegistry.java index 768d13897e..26574b819c 100644 --- a/src/main/java/graphql/schema/GraphQLCodeRegistry.java +++ b/src/main/java/graphql/schema/GraphQLCodeRegistry.java @@ -84,6 +84,7 @@ public boolean hasDataFetcher(FieldCoordinates coordinates) { return hasDataFetcherImpl(coordinates, dataFetcherMap, systemDataFetcherMap); } + @SuppressWarnings("deprecation") private static DataFetcher getDataFetcherImpl(FieldCoordinates coordinates, GraphQLFieldDefinition fieldDefinition, Map> dataFetcherMap, Map> systemDataFetcherMap, DataFetcherFactory defaultDataFetcherFactory) { assertNotNull(coordinates); assertNotNull(fieldDefinition); @@ -95,9 +96,15 @@ private static DataFetcher getDataFetcherImpl(FieldCoordinates coordinates, G dataFetcherFactory = defaultDataFetcherFactory; } } - return dataFetcherFactory.get(newDataFetchingFactoryEnvironment() - .fieldDefinition(fieldDefinition) - .build()); + // call direct from the field - cheaper to not make a new environment object + DataFetcher dataFetcher = dataFetcherFactory.get(fieldDefinition); + if (dataFetcher == null) { + DataFetcherFactoryEnvironment factoryEnvironment = newDataFetchingFactoryEnvironment() + .fieldDefinition(fieldDefinition) + .build(); + dataFetcher = dataFetcherFactory.get(factoryEnvironment); + } + return dataFetcher; } private static boolean hasDataFetcherImpl(FieldCoordinates coords, Map> dataFetcherMap, Map> systemDataFetcherMap) { @@ -149,7 +156,7 @@ private static TypeResolver getTypeResolverForUnion(GraphQLUnionType parentType, if (typeResolver == null) { typeResolver = parentType.getTypeResolver(); } - return assertNotNull(typeResolver, "There must be a type resolver for union %s",parentType.getName()); + return assertNotNull(typeResolver, "There must be a type resolver for union %s", parentType.getName()); } /** @@ -189,7 +196,7 @@ public static class Builder { private final Map> systemDataFetcherMap = new LinkedHashMap<>(); private final Map typeResolverMap = new HashMap<>(); private GraphqlFieldVisibility fieldVisibility = DEFAULT_FIELD_VISIBILITY; - private DataFetcherFactory defaultDataFetcherFactory = env -> PropertyDataFetcher.fetching(env.getFieldDefinition().getName()); + private DataFetcherFactory defaultDataFetcherFactory = SingletonPropertyDataFetcher.singletonFactory(); private boolean changed = false; private Builder() { diff --git a/src/main/java/graphql/schema/SingletonPropertyDataFetcher.java b/src/main/java/graphql/schema/SingletonPropertyDataFetcher.java new file mode 100644 index 0000000000..45af96c843 --- /dev/null +++ b/src/main/java/graphql/schema/SingletonPropertyDataFetcher.java @@ -0,0 +1,60 @@ +package graphql.schema; + +import java.util.function.Supplier; + +/** + * The {@link SingletonPropertyDataFetcher} is much like the {@link PropertyDataFetcher} except + * that it is designed to only ever fetch properties via the name of the field passed in. + *

+ * This uses the same code as {@link PropertyDataFetcher} and hence is also controlled + * by static methods such as {@link PropertyDataFetcher#setUseNegativeCache(boolean)} + * + * @param for two + */ +public class SingletonPropertyDataFetcher implements LightDataFetcher { + + private static final SingletonPropertyDataFetcher SINGLETON_FETCHER = new SingletonPropertyDataFetcher<>(); + + private static final DataFetcherFactory SINGLETON_FETCHER_FACTORY = environment -> SINGLETON_FETCHER; + + /** + * This returns the same singleton {@link LightDataFetcher} that fetches property values + * based on the name of the field that iis passed into it. + * + * @return a singleton property data fetcher + */ + public static LightDataFetcher singleton() { + return SINGLETON_FETCHER; + } + + /** + * This returns the same singleton {@link DataFetcherFactory} that returns the value of {@link #singleton()} + * + * @return a singleton data fetcher factory + */ + public static DataFetcherFactory singletonFactory() { + return SINGLETON_FETCHER_FACTORY; + } + + private SingletonPropertyDataFetcher() { + } + + @Override + public T get(GraphQLFieldDefinition fieldDefinition, Object sourceObject, Supplier environmentSupplier) throws Exception { + return fetchImpl(fieldDefinition, sourceObject, environmentSupplier); + } + + @Override + public T get(DataFetchingEnvironment environment) throws Exception { + return fetchImpl(environment.getFieldDefinition(), environment.getSource(), () -> environment); + } + + private T fetchImpl(GraphQLFieldDefinition fieldDefinition, Object source, Supplier environmentSupplier) { + if (source == null) { + return null; + } + // this is the same code that PropertyDataFetcher uses and hence unit tests for it include this one + //noinspection unchecked + return (T) PropertyDataFetcherHelper.getPropertyValue(fieldDefinition.getName(), source, fieldDefinition.getType(), environmentSupplier); + } +} diff --git a/src/main/java/graphql/schema/idl/MockedWiringFactory.java b/src/main/java/graphql/schema/idl/MockedWiringFactory.java index 3dc4b1ca2b..fcfca1f474 100644 --- a/src/main/java/graphql/schema/idl/MockedWiringFactory.java +++ b/src/main/java/graphql/schema/idl/MockedWiringFactory.java @@ -8,6 +8,7 @@ import graphql.schema.DataFetcher; import graphql.schema.GraphQLScalarType; import graphql.schema.PropertyDataFetcher; +import graphql.schema.SingletonPropertyDataFetcher; import graphql.schema.TypeResolver; @PublicApi @@ -44,7 +45,7 @@ public boolean providesDataFetcher(FieldWiringEnvironment environment) { @Override public DataFetcher getDataFetcher(FieldWiringEnvironment environment) { - return new PropertyDataFetcher(environment.getFieldDefinition().getName()); + return SingletonPropertyDataFetcher.singleton(); } @Override diff --git a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java index 0b4e74c45b..9fa7ea286f 100644 --- a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java +++ b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java @@ -57,6 +57,7 @@ import graphql.schema.GraphQLUnionType; import graphql.schema.GraphqlTypeComparatorRegistry; import graphql.schema.PropertyDataFetcher; +import graphql.schema.SingletonPropertyDataFetcher; import graphql.schema.TypeResolver; import graphql.schema.TypeResolverProxy; import graphql.schema.idl.errors.NotAnInputTypeError; @@ -801,23 +802,27 @@ GraphQLFieldDefinition buildField(BuildContext buildCtx, TypeDefinition paren // if they have already wired in a fetcher - then leave it alone FieldCoordinates coordinates = FieldCoordinates.coordinates(parentType.getName(), fieldDefinition.getName()); if (!buildCtx.getCodeRegistry().hasDataFetcher(coordinates)) { - DataFetcherFactory dataFetcherFactory = buildDataFetcherFactory(buildCtx, + Optional> dataFetcherFactory = buildDataFetcherFactory(buildCtx, parentType, fieldDef, fieldType, appliedDirectives.first, appliedDirectives.second); - buildCtx.getCodeRegistry().dataFetcher(coordinates, dataFetcherFactory); + + // if the dataFetcherFactory is empty, then it must have been the code registry default one + // and hence we don't need to make a "map entry" in the code registry since it will be defaulted + // anyway + dataFetcherFactory.ifPresent(fetcherFactory -> buildCtx.getCodeRegistry().dataFetcher(coordinates, fetcherFactory)); } return directivesObserve(buildCtx, fieldDefinition); } - private DataFetcherFactory buildDataFetcherFactory(BuildContext buildCtx, - TypeDefinition parentType, - FieldDefinition fieldDef, - GraphQLOutputType fieldType, - List directives, - List appliedDirectives) { + private Optional> buildDataFetcherFactory(BuildContext buildCtx, + TypeDefinition parentType, + FieldDefinition fieldDef, + GraphQLOutputType fieldType, + List directives, + List appliedDirectives) { String fieldName = fieldDef.getName(); String parentTypeName = parentType.getName(); TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry(); @@ -847,16 +852,18 @@ private DataFetcherFactory buildDataFetcherFactory(BuildContext buildCtx, if (dataFetcher == null) { DataFetcherFactory codeRegistryDFF = codeRegistry.getDefaultDataFetcherFactory(); if (codeRegistryDFF != null) { - return codeRegistryDFF; + // this will use the default of the code registry when its + // asked for at runtime + return Optional.empty(); } - dataFetcher = dataFetcherOfLastResort(wiringEnvironment); + dataFetcher = dataFetcherOfLastResort(); } } } } dataFetcherFactory = DataFetcherFactories.useDataFetcher(dataFetcher); } - return dataFetcherFactory; + return Optional.of(dataFetcherFactory); } GraphQLArgument buildArgument(BuildContext buildCtx, InputValueDefinition valueDefinition) { @@ -1087,9 +1094,8 @@ private Optional getOperationNamed(String name, Map dataFetcherOfLastResort(FieldWiringEnvironment environment) { - String fieldName = environment.getFieldDefinition().getName(); - return new PropertyDataFetcher(fieldName); + private DataFetcher dataFetcherOfLastResort() { + return SingletonPropertyDataFetcher.singleton(); } private List directivesOf(List> typeDefinitions) { diff --git a/src/test/groovy/graphql/DataFetcherTest.groovy b/src/test/groovy/graphql/DataFetcherTest.groovy index aa15db46dd..2f57c41efd 100644 --- a/src/test/groovy/graphql/DataFetcherTest.groovy +++ b/src/test/groovy/graphql/DataFetcherTest.groovy @@ -1,8 +1,10 @@ package graphql +import graphql.schema.GraphQLFieldDefinition import graphql.schema.GraphQLOutputType import graphql.schema.PropertyDataFetcher +import graphql.schema.SingletonPropertyDataFetcher import spock.lang.Specification import static graphql.Scalars.GraphQLBoolean @@ -55,43 +57,95 @@ class DataFetcherTest extends Specification { } - def env(GraphQLOutputType type) { - newDataFetchingEnvironment().source(dataHolder).fieldType(type).build() + def env(String propertyName, GraphQLOutputType type) { + GraphQLFieldDefinition fieldDefinition = mkField(propertyName, type) + newDataFetchingEnvironment().source(dataHolder).fieldType(type).fieldDefinition(fieldDefinition).build() + } + + def mkField(String propertyName, GraphQLOutputType type) { + GraphQLFieldDefinition.newFieldDefinition().name(propertyName).type(type).build() } def "get property value"() { given: - def environment = env(GraphQLString) + def environment = env("property", GraphQLString) + def field = mkField("property", GraphQLString) when: - def result = new PropertyDataFetcher("property").get(environment) + def result = fetcher.get(environment) then: result == "propertyValue" + + when: + result = fetcher.get(field, dataHolder, { environment }) + then: + result == "propertyValue" + + where: + fetcher | _ + new PropertyDataFetcher("property") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "get Boolean property value"() { given: - def environment = env(GraphQLBoolean) + def environment = env("booleanField", GraphQLBoolean) + def field = mkField("booleanField", GraphQLBoolean) + + when: + def result = fetcher.get(environment) + then: + result == true + when: - def result = new PropertyDataFetcher("booleanField").get(environment) + result = fetcher.get(field, dataHolder, { environment }) then: result == true + + where: + fetcher | _ + new PropertyDataFetcher("booleanField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "get Boolean property value with get"() { given: - def environment = env(GraphQLBoolean) + def environment = env("booleanFieldWithGet", GraphQLBoolean) + def field = mkField("booleanFieldWithGet", GraphQLBoolean) + + when: + def result = fetcher.get(environment) + then: + result == false + when: - def result = new PropertyDataFetcher("booleanFieldWithGet").get(environment) + result = fetcher.get(field, dataHolder, { environment }) then: result == false + + where: + fetcher | _ + new PropertyDataFetcher("booleanFieldWithGet") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "get public field value as property"() { given: - def environment = env(GraphQLString) + def environment = env("publicField", GraphQLString) + def field = mkField("publicField", GraphQLString) + + when: + def result = fetcher.get(environment) + then: + result == "publicValue" + when: - def result = new PropertyDataFetcher("publicField").get(environment) + result = fetcher.get(field, dataHolder, { environment }) then: result == "publicValue" + + where: + fetcher | _ + new PropertyDataFetcher("publicField") | _ + SingletonPropertyDataFetcher.singleton() | _ } } diff --git a/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy b/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy new file mode 100644 index 0000000000..1aa971c6e7 --- /dev/null +++ b/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy @@ -0,0 +1,43 @@ +package graphql + + +import graphql.schema.FieldCoordinates +import graphql.schema.GraphQLFieldDefinition +import graphql.schema.PropertyDataFetcher +import graphql.schema.SingletonPropertyDataFetcher +import spock.lang.Specification + +class LargeSchemaDataFetcherTest extends Specification { + + def howManyFields = 100_000 + + def "large schema with lots of fields has the same property data fetcher by default"() { + def sdl = """ + type Query { + ${mkFields()} + } + """ + + when: + def schema = TestUtil.schema(sdl) + def codeRegistry = schema.getCodeRegistry() + def singletonDF = SingletonPropertyDataFetcher.singleton() + + then: + + for (int i = 0; i < howManyFields; i++) { + def fieldName = "f$i" + def fieldDef = GraphQLFieldDefinition.newFieldDefinition().name(fieldName).type(Scalars.GraphQLString).build() + def df = codeRegistry.getDataFetcher(FieldCoordinates.coordinates("Query", fieldName), fieldDef) + assert df == singletonDF + } + } + + def mkFields() { + StringBuilder sb = new StringBuilder() + for (int i = 0; i < howManyFields; i++) { + sb.append("f$i : String\n") + } + return sb.toString() + } +} diff --git a/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy b/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy index a92cf94518..a98cf833e2 100644 --- a/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy @@ -13,7 +13,9 @@ import graphql.language.AstPrinter import graphql.parser.Parser import graphql.schema.DataFetcher import graphql.schema.DataFetchingEnvironment +import graphql.schema.LightDataFetcher import graphql.schema.PropertyDataFetcher +import graphql.schema.SingletonPropertyDataFetcher import graphql.schema.StaticDataFetcher import org.awaitility.Awaitility import org.jetbrains.annotations.NotNull @@ -99,7 +101,7 @@ class InstrumentationTest extends Specification { instrumentation.dfClasses.size() == 2 instrumentation.dfClasses[0] == StaticDataFetcher.class - instrumentation.dfClasses[1] == PropertyDataFetcher.class + instrumentation.dfClasses[1] == SingletonPropertyDataFetcher.class instrumentation.dfInvocations.size() == 2 diff --git a/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy b/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy index 5050ba0ef1..3fb58ed1e7 100644 --- a/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy +++ b/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy @@ -32,7 +32,17 @@ class DataFetcherFactoriesTest extends Specification { def fetcherFactory = DataFetcherFactories.useDataFetcher(pojoDF) when: - def value = fetcherFactory.get(null).get(null) + def value = fetcherFactory.get((GraphQLFieldDefinition)null).get(null) + + then: + value == "goodbye" + } + + def "will use given df via field"() { + def fetcherFactory = DataFetcherFactories.useDataFetcher(pojoDF) + + when: + def value = fetcherFactory.get((GraphQLFieldDefinition) null).get(null) then: value == "goodbye" diff --git a/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy b/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy index eef53c9794..e528f5619f 100644 --- a/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy @@ -5,6 +5,7 @@ import graphql.GraphQL import graphql.Scalars import graphql.StarWarsSchema import graphql.TestUtil +import graphql.TrivialDataFetcher import graphql.TypeResolutionEnvironment import graphql.schema.visibility.GraphqlFieldVisibility import spock.lang.Specification @@ -98,7 +99,7 @@ class GraphQLCodeRegistryTest extends Specification { (codeRegistryBuilder.getDataFetcher(objectType("parentType3"), field("fieldD")) as NamedDF).name == "D" (codeRegistryBuilder.getDataFetcher(objectType("parentType3"), field("fieldE")) as NamedDF).name == "E" - codeRegistryBuilder.getDataFetcher(objectType("parentType2"), field("A")) instanceof PropertyDataFetcher // a default one + codeRegistryBuilder.getDataFetcher(objectType("parentType2"), field("A")) instanceof SingletonPropertyDataFetcher // a default one when: def codeRegistry = codeRegistryBuilder.build() @@ -108,7 +109,7 @@ class GraphQLCodeRegistryTest extends Specification { (codeRegistry.getDataFetcher(objectType("parentType3"), field("fieldD")) as NamedDF).name == "D" (codeRegistry.getDataFetcher(objectType("parentType3"), field("fieldE")) as NamedDF).name == "E" - codeRegistry.getDataFetcher(objectType("parentType2"), field("A")) instanceof PropertyDataFetcher // a default one + codeRegistry.getDataFetcher(objectType("parentType2"), field("A")) instanceof SingletonPropertyDataFetcher // a default one } def "data fetchers can be retrieved using field coordinates"() { @@ -125,7 +126,7 @@ class GraphQLCodeRegistryTest extends Specification { (codeRegistryBuilder.getDataFetcher(FieldCoordinates.coordinates("parentType3", "fieldD"), field("fieldD")) as NamedDF).name == "D" (codeRegistryBuilder.getDataFetcher(FieldCoordinates.coordinates("parentType3", "fieldE"), field("fieldE")) as NamedDF).name == "E" - codeRegistryBuilder.getDataFetcher(FieldCoordinates.coordinates("parentType2", "A"), field("A")) instanceof PropertyDataFetcher // a default one + codeRegistryBuilder.getDataFetcher(FieldCoordinates.coordinates("parentType2", "A"), field("A")) instanceof SingletonPropertyDataFetcher // a default one when: def codeRegistry = codeRegistryBuilder.build() @@ -135,7 +136,7 @@ class GraphQLCodeRegistryTest extends Specification { (codeRegistry.getDataFetcher(FieldCoordinates.coordinates("parentType3", "fieldD"), field("fieldD")) as NamedDF).name == "D" (codeRegistry.getDataFetcher(FieldCoordinates.coordinates("parentType3", "fieldE"), field("fieldE")) as NamedDF).name == "E" - codeRegistry.getDataFetcher(FieldCoordinates.coordinates("parentType2", "A"), field("A")) instanceof PropertyDataFetcher // a default one + codeRegistry.getDataFetcher(FieldCoordinates.coordinates("parentType2", "A"), field("A")) instanceof SingletonPropertyDataFetcher // a default one } def "records type resolvers against unions and interfaces"() { @@ -179,13 +180,13 @@ class GraphQLCodeRegistryTest extends Specification { (schema.getCodeRegistry().getFieldVisibility() as NamedFieldVisibility).name == "B" } - def "PropertyDataFetcher is the default data fetcher used when no data fetcher is available"() { + def "SingletonPropertyDataFetcher is the default data fetcher used when no data fetcher is available"() { when: def codeRegistry = GraphQLCodeRegistry.newCodeRegistry().build() def dataFetcher = codeRegistry.getDataFetcher(StarWarsSchema.humanType, StarWarsSchema.humanType.getFieldDefinition("name")) then: - dataFetcher instanceof PropertyDataFetcher + dataFetcher instanceof SingletonPropertyDataFetcher } def "custom DF can be the default data fetcher used when no data fetcher is available"() { @@ -251,8 +252,8 @@ class GraphQLCodeRegistryTest extends Specification { er.errors.isEmpty() er.data == [codeRegistryField: "codeRegistryFieldValue", nonCodeRegistryField: "nonCodeRegistryFieldValue", neitherSpecified: "neitherSpecifiedValue"] - // when nothing is specified then its a plain old PropertyDataFetcher - schema.getCodeRegistry().getDataFetcher(queryType, queryType.getFieldDefinition("neitherSpecified")) instanceof PropertyDataFetcher + // when nothing is specified then its a plain old SingletonPropertyDataFetcher + schema.getCodeRegistry().getDataFetcher(queryType, queryType.getFieldDefinition("neitherSpecified")) instanceof SingletonPropertyDataFetcher } @@ -287,7 +288,7 @@ class GraphQLCodeRegistryTest extends Specification { // when nothing is specified then its a plain old PropertyDataFetcher def queryType = schema.getObjectType("Query") - schema.getCodeRegistry().getDataFetcher(queryType, queryType.getFieldDefinition("neitherSpecified")) instanceof PropertyDataFetcher + schema.getCodeRegistry().getDataFetcher(queryType, queryType.getFieldDefinition("neitherSpecified")) instanceof SingletonPropertyDataFetcher } def "will detect system versus user data fetchers"() { diff --git a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy index bf18e083c2..f3eabc1eeb 100644 --- a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy @@ -508,7 +508,7 @@ class GraphQLSchemaTest extends Specification { def newDF = newRegistry.getDataFetcher(dogType, dogType.getField("name")) newDF !== nameDF - newDF instanceof PropertyDataFetcher // defaulted in + newDF instanceof LightDataFetcher // defaulted in } def "can get by field co-ordinate"() { diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index b9fae5e3f0..7903f14229 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -1,6 +1,7 @@ package graphql.schema import graphql.ExecutionInput +import graphql.Scalars import graphql.TestUtil import graphql.schema.fetching.ConfusedPojo import graphql.schema.somepackage.ClassWithDFEMethods @@ -16,6 +17,14 @@ import java.util.function.Function import static graphql.schema.DataFetchingEnvironmentImpl.newDataFetchingEnvironment +/** + * Note : That `new PropertyDataFetcher("someProperty")` and `SingletonPropertyDataFetcher.singleton()` + * should really be the equivalent since they both go via `PropertyDataFetcherHelper.getPropertyValue` + * under the covers. + * + * But where we can we have tried to use `where` blocks to test both + * + */ @SuppressWarnings("GroovyUnusedDeclaration") class PropertyDataFetcherTest extends Specification { @@ -26,9 +35,11 @@ class PropertyDataFetcherTest extends Specification { PropertyDataFetcherHelper.setUseLambdaFactory(true) } - def env(obj) { + def env(String propertyName, Object obj) { + def fieldDefinition = GraphQLFieldDefinition.newFieldDefinition().name(propertyName).type(Scalars.GraphQLString).build() newDataFetchingEnvironment() .source(obj) + .fieldDefinition(fieldDefinition) .arguments([argument1: "value1", argument2: "value2"]) .build() } @@ -38,14 +49,20 @@ class PropertyDataFetcherTest extends Specification { } def "null source is always null"() { - def environment = env(null) - def fetcher = new PropertyDataFetcher("someProperty") + given: + def environment = env("someProperty", null) + expect: fetcher.get(environment) == null + + where: + fetcher | _ + new PropertyDataFetcher("someProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "function based fetcher works with non null source"() { - def environment = env(new SomeObject(value: "aValue")) + def environment = env("notused", new SomeObject(value: "aValue")) Function f = { obj -> obj['value'] } def fetcher = PropertyDataFetcher.fetching(f) expect: @@ -53,7 +70,7 @@ class PropertyDataFetcherTest extends Specification { } def "function based fetcher works with null source"() { - def environment = env(null) + def environment = env("notused", null) Function f = { obj -> obj['value'] } def fetcher = PropertyDataFetcher.fetching(f) expect: @@ -61,46 +78,87 @@ class PropertyDataFetcherTest extends Specification { } def "fetch via map lookup"() { - def environment = env(["mapProperty": "aValue"]) - def fetcher = PropertyDataFetcher.fetching("mapProperty") + given: + def environment = env("mapProperty", ["mapProperty": "aValue"]) + expect: fetcher.get(environment) == "aValue" + + where: + fetcher | _ + PropertyDataFetcher.fetching("mapProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via public getter with private subclass"() { - def environment = env(TestClass.createPackageProtectedImpl("aValue")) - def fetcher = new PropertyDataFetcher("packageProtectedProperty") + given: + def environment = env("packageProtectedProperty", TestClass.createPackageProtectedImpl("aValue")) + expect: fetcher.get(environment) == "aValue" + + where: + fetcher | _ + new PropertyDataFetcher("packageProtectedProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via method that isn't present"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("valueNotPresent") + given: + def environment = env("valueNotPresent", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == null + + where: + fetcher | _ + new PropertyDataFetcher("valueNotPresent") | _ + SingletonPropertyDataFetcher.singleton() | _ + } def "fetch via method that is private"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("privateProperty") + given: + def environment = env("privateProperty", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == "privateValue" + + where: + fetcher | _ + new PropertyDataFetcher("privateProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ + } def "fetch via method that is private with setAccessible OFF"() { + given: PropertyDataFetcher.setUseSetAccessible(false) - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("privateProperty") + def environment = env("privateProperty", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == null + + where: + fetcher | _ + new PropertyDataFetcher("privateProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ + } def "fetch via record method"() { - def environment = env(new RecordLikeClass()) + given: + def environment = env("recordProperty", new RecordLikeClass()) + when: def fetcher = new PropertyDataFetcher("recordProperty") def result = fetcher.get(environment) @@ -144,57 +202,95 @@ class PropertyDataFetcherTest extends Specification { result == "toString" } + def "fetch via record method with singleton fetcher"() { + given: + def environment = env("recordProperty", new RecordLikeClass()) + + when: + def fetcher = SingletonPropertyDataFetcher.singleton() + def result = fetcher.get(environment) + then: + result == "recordProperty" + } + def "can fetch record like methods that are public and on super classes"() { - def environment = env(new RecordLikeTwoClassesDown()) + given: + def environment = env("recordProperty", new RecordLikeTwoClassesDown()) + when: - def fetcher = new PropertyDataFetcher("recordProperty") def result = fetcher.get(environment) + then: result == "recordProperty" + + where: + fetcher | _ + new PropertyDataFetcher("recordProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via record method without lambda support"() { + given: PropertyDataFetcherHelper.setUseLambdaFactory(false) PropertyDataFetcherHelper.clearReflectionCache() when: - def environment = env(new RecordLikeClass()) + def environment = env("recordProperty", new RecordLikeClass()) def fetcher = new PropertyDataFetcher("recordProperty") def result = fetcher.get(environment) + then: result == "recordProperty" when: - environment = env(new RecordLikeTwoClassesDown()) + environment = env("recordProperty", new RecordLikeTwoClassesDown()) fetcher = new PropertyDataFetcher("recordProperty") result = fetcher.get(environment) + then: result == "recordProperty" } def "fetch via record method without lambda support in preference to getter methods"() { + given: PropertyDataFetcherHelper.setUseLambdaFactory(false) PropertyDataFetcherHelper.clearReflectionCache() when: - def environment = env(new ConfusedPojo()) - def fetcher = new PropertyDataFetcher("recordLike") + def environment = env("recordLike", new ConfusedPojo()) def result = fetcher.get(environment) + then: result == "recordLike" + + where: + fetcher | _ + new PropertyDataFetcher("recordLike") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via public method"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("publicProperty") + given: + def environment = env("publicProperty", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == "publicValue" + + where: + fetcher | _ + new PropertyDataFetcher("publicProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ + } def "fetch via public method declared two classes up"() { - def environment = env(new TwoClassesDown("aValue")) + given: + def environment = env("publicProperty", new TwoClassesDown("aValue")) def fetcher = new PropertyDataFetcher("publicProperty") + when: def result = fetcher.get(environment) then: @@ -208,42 +304,69 @@ class PropertyDataFetcherTest extends Specification { } def "fetch via property only defined on package protected impl"() { - def environment = env(TestClass.createPackageProtectedImpl("aValue")) - def fetcher = new PropertyDataFetcher("propertyOnlyDefinedOnPackageProtectedImpl") + given: + def environment = env("propertyOnlyDefinedOnPackageProtectedImpl", TestClass.createPackageProtectedImpl("aValue")) + + when: def result = fetcher.get(environment) - expect: + + then: result == "valueOnlyDefinedOnPackageProtectedIpl" + + where: + fetcher | _ + new PropertyDataFetcher("propertyOnlyDefinedOnPackageProtectedImpl") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via public field"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("publicField") + given: + + def environment = env("publicField", new TestClass()) def result = fetcher.get(environment) + expect: result == "publicFieldValue" + + where: + fetcher | _ + new PropertyDataFetcher("publicField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via private field"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("privateField") + given: + def environment = env("privateField", new TestClass()) def result = fetcher.get(environment) + expect: result == "privateFieldValue" + + where: + fetcher | _ + new PropertyDataFetcher("privateField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via private field when setAccessible OFF"() { + given: PropertyDataFetcher.setUseSetAccessible(false) - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("privateField") + def environment = env("privateField", new TestClass()) def result = fetcher.get(environment) + expect: result == null + + where: + fetcher | _ + new PropertyDataFetcher("privateField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch when caching is in place has no bad effects"() { - def environment = env(new TestClass()) + def environment = env("publicProperty", new TestClass()) def fetcher = new PropertyDataFetcher("publicProperty") when: def result = fetcher.get(environment) @@ -317,8 +440,10 @@ class PropertyDataFetcherTest extends Specification { } def "support for DFE on methods"() { - def environment = env(new ClassWithDFEMethods()) + given: + def environment = env("methodWithDFE", new ClassWithDFEMethods()) def fetcher = new PropertyDataFetcher("methodWithDFE") + when: def result = fetcher.get(environment) then: @@ -370,7 +495,7 @@ class PropertyDataFetcherTest extends Specification { def "finds interface methods"() { when: - def environment = env(new ClassWithInterfaces()) + def environment = env("methodYouMustImplement", new ClassWithInterfaces()) def fetcher = new PropertyDataFetcher("methodYouMustImplement") def result = fetcher.get(environment) then: @@ -397,7 +522,7 @@ class PropertyDataFetcherTest extends Specification { } def "finds interface methods with inheritance"() { - def environment = env(new ClassWithInteritanceAndInterfaces.StartingClass()) + def environment = env("methodYouMustImplement", new ClassWithInteritanceAndInterfaces.StartingClass()) when: def fetcher = new PropertyDataFetcher("methodYouMustImplement") @@ -411,7 +536,7 @@ class PropertyDataFetcherTest extends Specification { then: result == "methodThatIsADefault" - def environment2 = env(new ClassWithInteritanceAndInterfaces.InheritedClass()) + def environment2 = env("methodYouMustImplement", new ClassWithInteritanceAndInterfaces.InheritedClass()) when: fetcher = new PropertyDataFetcher("methodYouMustImplement") @@ -440,7 +565,7 @@ class PropertyDataFetcherTest extends Specification { def "ensure DFE is passed to method"() { - def environment = env(new ClassWithDFEMethods()) + def environment = env("methodUsesDataFetchingEnvironment", new ClassWithDFEMethods()) def fetcher = new PropertyDataFetcher("methodUsesDataFetchingEnvironment") when: def result = fetcher.get(environment) @@ -455,7 +580,7 @@ class PropertyDataFetcherTest extends Specification { } def "negative caching works as expected"() { - def environment = env(new ClassWithDFEMethods()) + def environment = env("doesNotExist", new ClassWithDFEMethods()) def fetcher = new PropertyDataFetcher("doesNotExist") when: def result = fetcher.get(environment) @@ -546,69 +671,81 @@ class PropertyDataFetcherTest extends Specification { def "search for private getter in class hierarchy"() { given: Bar bar = new Baz() - PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("something") - def dfe = Mock(DataFetchingEnvironment) - dfe.getSource() >> bar + def dfe = env("something", bar) + when: - def result = propertyDataFetcher.get(dfe) + def result = fetcher.get(dfe) then: result == "bar" // repeat - should be cached when: - result = propertyDataFetcher.get(dfe) + result = fetcher.get(dfe) then: result == "bar" + + where: + fetcher | _ + new PropertyDataFetcher("something") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "issue 3247 - record like statics should not be used"() { given: def payload = new UpdateOrganizerSubscriptionPayload(true, new OrganizerSubscriptionError()) - PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("success") - def dfe = Mock(DataFetchingEnvironment) - dfe.getSource() >> payload + def dfe = env("success", payload) + when: - def result = propertyDataFetcher.get(dfe) + def result = fetcher.get(dfe) then: result == true // repeat - should be cached when: - result = propertyDataFetcher.get(dfe) + result = fetcher.get(dfe) then: result == true + + where: + fetcher | _ + new PropertyDataFetcher("success") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "issue 3247 - record like statics should not be found"() { given: def errorShape = new OrganizerSubscriptionError() - PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("message") - def dfe = Mock(DataFetchingEnvironment) - dfe.getSource() >> errorShape + def dfe = env("message", errorShape) + when: - def result = propertyDataFetcher.get(dfe) + def result = fetcher.get(dfe) then: result == null // not found as its a static recordLike() method // repeat - should be cached when: - result = propertyDataFetcher.get(dfe) + result = fetcher.get(dfe) then: result == null + + where: + fetcher | _ + new PropertyDataFetcher("message") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "issue 3247 - getter statics should be found"() { given: def objectInQuestion = new BarClassWithStaticProperties() + def dfe = env("foo", objectInQuestion) PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("foo") - def dfe = Mock(DataFetchingEnvironment) - dfe.getSource() >> objectInQuestion + when: def result = propertyDataFetcher.get(dfe) @@ -657,15 +794,22 @@ class PropertyDataFetcherTest extends Specification { Locale oldLocale = Locale.getDefault() Locale.setDefault(new Locale("tr", "TR")) - def environment = env(new OtherObject(id: "aValue")) - def fetcher = PropertyDataFetcher.fetching("id") + def environment = env("id", new OtherObject(id: "aValue")) when: + def fetcher = PropertyDataFetcher.fetching("id") String propValue = fetcher.get(environment) then: propValue == 'aValue' + when: + fetcher = SingletonPropertyDataFetcher.singleton() + propValue = fetcher.get(environment) + + then: + propValue == 'aValue' + cleanup: Locale.setDefault(oldLocale) } diff --git a/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy index d8895370c6..20000c8bc8 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy @@ -2282,6 +2282,11 @@ class SchemaGeneratorTest extends Specification { DataFetcher get(DataFetcherFactoryEnvironment environment) { return df } + + @Override + DataFetcher get(GraphQLFieldDefinition fieldDefinition) { + return df + } } GraphQLCodeRegistry codeRegistry = newCodeRegistry() diff --git a/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy b/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy index 476c13ac13..fe12c3a1b1 100644 --- a/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy +++ b/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy @@ -12,6 +12,7 @@ import graphql.schema.GraphQLScalarType import graphql.schema.GraphQLTypeUtil import graphql.schema.GraphQLUnionType import graphql.schema.PropertyDataFetcher +import graphql.schema.SingletonPropertyDataFetcher import graphql.schema.TypeResolver class TestLiveMockedWiringFactory implements WiringFactory { @@ -74,7 +75,7 @@ class TestLiveMockedWiringFactory implements WiringFactory { @Override DataFetcher getDataFetcher(FieldWiringEnvironment environment) { - return new PropertyDataFetcher(environment.getFieldDefinition().getName()) + return SingletonPropertyDataFetcher.singleton() } @Override diff --git a/src/test/groovy/graphql/schema/idl/TestMockedWiringFactory.groovy b/src/test/groovy/graphql/schema/idl/TestMockedWiringFactory.groovy index 9082c7b79b..827fadafbb 100644 --- a/src/test/groovy/graphql/schema/idl/TestMockedWiringFactory.groovy +++ b/src/test/groovy/graphql/schema/idl/TestMockedWiringFactory.groovy @@ -49,12 +49,8 @@ class TestMockedWiringFactory implements WiringFactory { @Override boolean providesDataFetcher(FieldWiringEnvironment environment) { - return true - } - - @Override - DataFetcher getDataFetcher(FieldWiringEnvironment environment) { - return new PropertyDataFetcher(environment.getFieldDefinition().getName()) + // rely on defaulting in code registry + return false } @Override