From 689e40171b88e123adc413797d5d64bfc6c5ef0c Mon Sep 17 00:00:00 2001 From: bbaker Date: Mon, 25 Nov 2024 15:44:30 +1100 Subject: [PATCH 01/11] Use a singleton PropertyFetcher by default --- .../graphql/schema/GraphQLCodeRegistry.java | 2 +- .../graphql/schema/PropertyDataFetcher.java | 53 +++- .../groovy/graphql/DataFetcherTest.groovy | 42 ++- .../graphql/LargeSchemaDataFetcherTest.groovy | 43 +++ .../InstrumentationTest.groovy | 2 +- .../schema/PropertyDataFetcherTest.groovy | 272 +++++++++++++----- 6 files changed, 333 insertions(+), 81 deletions(-) create mode 100644 src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy diff --git a/src/main/java/graphql/schema/GraphQLCodeRegistry.java b/src/main/java/graphql/schema/GraphQLCodeRegistry.java index 768d13897e..98877bae36 100644 --- a/src/main/java/graphql/schema/GraphQLCodeRegistry.java +++ b/src/main/java/graphql/schema/GraphQLCodeRegistry.java @@ -189,7 +189,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 = PropertyDataFetcher.singletonFactory(); private boolean changed = false; private Builder() { diff --git a/src/main/java/graphql/schema/PropertyDataFetcher.java b/src/main/java/graphql/schema/PropertyDataFetcher.java index 77cfdcf062..48aca93176 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcher.java +++ b/src/main/java/graphql/schema/PropertyDataFetcher.java @@ -33,6 +33,35 @@ @PublicApi public class PropertyDataFetcher implements LightDataFetcher { + private static final PropertyDataFetcher SINGLETON_FETCHER = new PropertyDataFetcher<>() { + @Override + Object fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fieldDefinition, Supplier environmentSupplier) { + return super.fetchImpl(fieldDefinition.getName(), source, fieldDefinition, environmentSupplier); + } + }; + + private static final DataFetcherFactory SINGLETON_FETCHER_FACTORY = environment -> SINGLETON_FETCHER; + + /** + * This returns the same singleton {@link PropertyDataFetcher} that fetches property values + * based on the name of the field that iis passed into it. + * + * @return a singleton property data fetcher + */ + public static PropertyDataFetcher 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 final String propertyName; private final Function function; @@ -53,6 +82,11 @@ private PropertyDataFetcher(Function function) { this.propertyName = null; } + private PropertyDataFetcher() { + this.function = null; + this.propertyName = null; + } + /** * Returns a data fetcher that will use the property name to examine the {@link DataFetchingEnvironment#getSource()} object * for a getter method or field with that name, or if it's a map, it will look up a value using @@ -109,17 +143,26 @@ public String getPropertyName() { @Override public T get(GraphQLFieldDefinition fieldDefinition, Object source, Supplier environmentSupplier) throws Exception { - return getImpl(source, fieldDefinition.getType(), environmentSupplier); + return fetchImpl(propertyName, source, fieldDefinition, environmentSupplier); } @Override public T get(DataFetchingEnvironment environment) { - Object source = environment.getSource(); - return getImpl(source, environment.getFieldType(), () -> environment); + return fetchImpl(propertyName, environment.getSource(), environment.getFieldDefinition(), () -> environment); } + /** + * This is our implementation of property fetching + * + * @param propertyName the name of the property to fetch in the source object + * @param source the source object to fetch from + * @param fieldDefinition the field definition of the field being fetched for + * @param environmentSupplier a supplied of thee {@link DataFetchingEnvironment} + * + * @return a value of type T + */ @SuppressWarnings("unchecked") - private T getImpl(Object source, GraphQLOutputType fieldDefinition, Supplier environmentSupplier) { + T fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fieldDefinition, Supplier environmentSupplier) { if (source == null) { return null; } @@ -128,7 +171,7 @@ private T getImpl(Object source, GraphQLOutputType fieldDefinition, Supplier 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 = mkDFE("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 = mkDFE("mapProperty", ["mapProperty": "aValue"]) + expect: fetcher.get(environment) == "aValue" + + where: + fetcher | _ + PropertyDataFetcher.fetching("mapProperty") | _ + PropertyDataFetcher.singleton() | _ } def "fetch via public getter with private subclass"() { - def environment = env(TestClass.createPackageProtectedImpl("aValue")) - def fetcher = new PropertyDataFetcher("packageProtectedProperty") + given: + def environment = mkDFE("packageProtectedProperty", TestClass.createPackageProtectedImpl("aValue")) + expect: fetcher.get(environment) == "aValue" + + where: + fetcher | _ + new PropertyDataFetcher("packageProtectedProperty") | _ + PropertyDataFetcher.singleton() | _ } def "fetch via method that isn't present"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("valueNotPresent") + given: + def environment = mkDFE("valueNotPresent", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == null + + where: + fetcher | _ + new PropertyDataFetcher("valueNotPresent") | _ + PropertyDataFetcher.singleton() | _ + } def "fetch via method that is private"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("privateProperty") + given: + def environment = mkDFE("privateProperty", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == "privateValue" + + where: + fetcher | _ + new PropertyDataFetcher("privateProperty") | _ + PropertyDataFetcher.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 = mkDFE("privateProperty", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == null + + where: + fetcher | _ + new PropertyDataFetcher("privateProperty") | _ + PropertyDataFetcher.singleton() | _ + } def "fetch via record method"() { - def environment = env(new RecordLikeClass()) + given: + def environment = mkDFE("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 = mkDFE("recordProperty", new RecordLikeClass()) + + when: + def fetcher = PropertyDataFetcher.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 = mkDFE("recordProperty", new RecordLikeTwoClassesDown()) + when: - def fetcher = new PropertyDataFetcher("recordProperty") def result = fetcher.get(environment) + then: result == "recordProperty" + + where: + fetcher | _ + new PropertyDataFetcher("recordProperty") | _ + PropertyDataFetcher.singleton() | _ } def "fetch via record method without lambda support"() { + given: PropertyDataFetcherHelper.setUseLambdaFactory(false) PropertyDataFetcherHelper.clearReflectionCache() when: - def environment = env(new RecordLikeClass()) + def environment = mkDFE("recordProperty", new RecordLikeClass()) def fetcher = new PropertyDataFetcher("recordProperty") def result = fetcher.get(environment) + then: result == "recordProperty" when: - environment = env(new RecordLikeTwoClassesDown()) + environment = mkDFE("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 = mkDFE("recordLike", new ConfusedPojo()) def result = fetcher.get(environment) + then: result == "recordLike" + + where: + fetcher | _ + new PropertyDataFetcher("recordLike") | _ + PropertyDataFetcher.singleton() | _ } def "fetch via public method"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("publicProperty") + given: + def environment = mkDFE("publicProperty", new TestClass()) + + when: def result = fetcher.get(environment) - expect: + + then: result == "publicValue" + + where: + fetcher | _ + new PropertyDataFetcher("publicProperty") | _ + PropertyDataFetcher.singleton() | _ + } def "fetch via public method declared two classes up"() { - def environment = env(new TwoClassesDown("aValue")) + given: + def environment = mkDFE("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 = mkDFE("propertyOnlyDefinedOnPackageProtectedImpl", TestClass.createPackageProtectedImpl("aValue")) + + when: def result = fetcher.get(environment) - expect: + + then: result == "valueOnlyDefinedOnPackageProtectedIpl" + + where: + fetcher | _ + new PropertyDataFetcher("propertyOnlyDefinedOnPackageProtectedImpl") | _ + PropertyDataFetcher.singleton() | _ } def "fetch via public field"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("publicField") + given: + + def environment = mkDFE("publicField", new TestClass()) def result = fetcher.get(environment) + expect: result == "publicFieldValue" + + where: + fetcher | _ + new PropertyDataFetcher("publicField") | _ + PropertyDataFetcher.singleton() | _ } def "fetch via private field"() { - def environment = env(new TestClass()) - def fetcher = new PropertyDataFetcher("privateField") + given: + def environment = mkDFE("privateField", new TestClass()) def result = fetcher.get(environment) + expect: result == "privateFieldValue" + + where: + fetcher | _ + new PropertyDataFetcher("privateField") | _ + PropertyDataFetcher.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 = mkDFE("privateField", new TestClass()) def result = fetcher.get(environment) + expect: result == null + + where: + fetcher | _ + new PropertyDataFetcher("privateField") | _ + PropertyDataFetcher.singleton() | _ } def "fetch when caching is in place has no bad effects"() { - def environment = env(new TestClass()) + def environment = mkDFE("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 = mkDFE("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 = mkDFE("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 = mkDFE("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 = mkDFE("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 = mkDFE("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 = mkDFE("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 = mkDFE("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") | _ + PropertyDataFetcher.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 = mkDFE("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") | _ + PropertyDataFetcher.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 = mkDFE("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") | _ + PropertyDataFetcher.singleton() | _ } def "issue 3247 - getter statics should be found"() { given: def objectInQuestion = new BarClassWithStaticProperties() + def dfe = mkDFE("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 = mkDFE("id", new OtherObject(id: "aValue")) when: + def fetcher = PropertyDataFetcher.fetching("id") String propValue = fetcher.get(environment) then: propValue == 'aValue' + when: + fetcher = PropertyDataFetcher.singleton() + propValue = fetcher.get(environment) + + then: + propValue == 'aValue' + cleanup: Locale.setDefault(oldLocale) } From a5a22c09921d84503fd0f3d4cddedcff7497ea82 Mon Sep 17 00:00:00 2001 From: bbaker Date: Mon, 25 Nov 2024 15:56:03 +1100 Subject: [PATCH 02/11] Use a singleton PropertyFetcher by default - schema generator will use it --- .../java/graphql/schema/idl/SchemaGeneratorHelper.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java index 0b4e74c45b..68bfdd5683 100644 --- a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java +++ b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java @@ -849,7 +849,7 @@ private DataFetcherFactory buildDataFetcherFactory(BuildContext buildCtx, if (codeRegistryDFF != null) { return codeRegistryDFF; } - dataFetcher = dataFetcherOfLastResort(wiringEnvironment); + dataFetcher = dataFetcherOfLastResort(); } } } @@ -1087,9 +1087,8 @@ private Optional getOperationNamed(String name, Map dataFetcherOfLastResort(FieldWiringEnvironment environment) { - String fieldName = environment.getFieldDefinition().getName(); - return new PropertyDataFetcher(fieldName); + private DataFetcher dataFetcherOfLastResort() { + return PropertyDataFetcher.singleton(); } private List directivesOf(List> typeDefinitions) { From 0d80213f72936ba9a4d6448dc721a70b12c499f5 Mon Sep 17 00:00:00 2001 From: bbaker Date: Mon, 25 Nov 2024 16:11:33 +1100 Subject: [PATCH 03/11] Use a singleton PropertyFetcher by default - renamed back to env for diff reasons --- .../groovy/graphql/DataFetcherTest.groovy | 10 +-- .../schema/PropertyDataFetcherTest.groovy | 66 +++++++++---------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/test/groovy/graphql/DataFetcherTest.groovy b/src/test/groovy/graphql/DataFetcherTest.groovy index 1c3efec655..e06b134489 100644 --- a/src/test/groovy/graphql/DataFetcherTest.groovy +++ b/src/test/groovy/graphql/DataFetcherTest.groovy @@ -56,14 +56,14 @@ class DataFetcherTest extends Specification { } - def mkDFE(String propertyName, GraphQLOutputType type) { + def env(String propertyName, GraphQLOutputType type) { def fieldDefinition = GraphQLFieldDefinition.newFieldDefinition().name(propertyName).type(type).build() newDataFetchingEnvironment().source(dataHolder).fieldType(type).fieldDefinition(fieldDefinition).build() } def "get property value"() { given: - def environment = mkDFE("property", GraphQLString) + def environment = env("property", GraphQLString) when: def result = fetcher.get(environment) then: @@ -77,7 +77,7 @@ class DataFetcherTest extends Specification { def "get Boolean property value"() { given: - def environment = mkDFE("booleanField", GraphQLBoolean) + def environment = env("booleanField", GraphQLBoolean) when: def result = fetcher.get(environment) then: @@ -91,7 +91,7 @@ class DataFetcherTest extends Specification { def "get Boolean property value with get"() { given: - def environment = mkDFE("booleanFieldWithGet", GraphQLBoolean) + def environment = env("booleanFieldWithGet", GraphQLBoolean) when: def result = fetcher.get(environment) then: @@ -105,7 +105,7 @@ class DataFetcherTest extends Specification { def "get public field value as property"() { given: - def environment = mkDFE("publicField", GraphQLString) + def environment = env("publicField", GraphQLString) when: def result = fetcher.get(environment) then: diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index bfea90afa7..ceadabe88a 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -35,7 +35,7 @@ class PropertyDataFetcherTest extends Specification { PropertyDataFetcherHelper.setUseLambdaFactory(true) } - def mkDFE(String propertyName, Object obj) { + def env(String propertyName, Object obj) { def fieldDefinition = GraphQLFieldDefinition.newFieldDefinition().name(propertyName).type(Scalars.GraphQLString).build() newDataFetchingEnvironment() .source(obj) @@ -50,7 +50,7 @@ class PropertyDataFetcherTest extends Specification { def "null source is always null"() { given: - def environment = mkDFE("someProperty", null) + def environment = env("someProperty", null) expect: fetcher.get(environment) == null @@ -62,7 +62,7 @@ class PropertyDataFetcherTest extends Specification { } def "function based fetcher works with non null source"() { - def environment = mkDFE("notused", new SomeObject(value: "aValue")) + def environment = env("notused", new SomeObject(value: "aValue")) Function f = { obj -> obj['value'] } def fetcher = PropertyDataFetcher.fetching(f) expect: @@ -70,7 +70,7 @@ class PropertyDataFetcherTest extends Specification { } def "function based fetcher works with null source"() { - def environment = mkDFE("notused", null) + def environment = env("notused", null) Function f = { obj -> obj['value'] } def fetcher = PropertyDataFetcher.fetching(f) expect: @@ -79,7 +79,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via map lookup"() { given: - def environment = mkDFE("mapProperty", ["mapProperty": "aValue"]) + def environment = env("mapProperty", ["mapProperty": "aValue"]) expect: fetcher.get(environment) == "aValue" @@ -92,7 +92,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via public getter with private subclass"() { given: - def environment = mkDFE("packageProtectedProperty", TestClass.createPackageProtectedImpl("aValue")) + def environment = env("packageProtectedProperty", TestClass.createPackageProtectedImpl("aValue")) expect: fetcher.get(environment) == "aValue" @@ -105,7 +105,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via method that isn't present"() { given: - def environment = mkDFE("valueNotPresent", new TestClass()) + def environment = env("valueNotPresent", new TestClass()) when: def result = fetcher.get(environment) @@ -122,7 +122,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via method that is private"() { given: - def environment = mkDFE("privateProperty", new TestClass()) + def environment = env("privateProperty", new TestClass()) when: def result = fetcher.get(environment) @@ -140,7 +140,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via method that is private with setAccessible OFF"() { given: PropertyDataFetcher.setUseSetAccessible(false) - def environment = mkDFE("privateProperty", new TestClass()) + def environment = env("privateProperty", new TestClass()) when: def result = fetcher.get(environment) @@ -157,7 +157,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via record method"() { given: - def environment = mkDFE("recordProperty", new RecordLikeClass()) + def environment = env("recordProperty", new RecordLikeClass()) when: def fetcher = new PropertyDataFetcher("recordProperty") @@ -204,7 +204,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via record method with singleton fetcher"() { given: - def environment = mkDFE("recordProperty", new RecordLikeClass()) + def environment = env("recordProperty", new RecordLikeClass()) when: def fetcher = PropertyDataFetcher.singleton() @@ -215,7 +215,7 @@ class PropertyDataFetcherTest extends Specification { def "can fetch record like methods that are public and on super classes"() { given: - def environment = mkDFE("recordProperty", new RecordLikeTwoClassesDown()) + def environment = env("recordProperty", new RecordLikeTwoClassesDown()) when: def result = fetcher.get(environment) @@ -235,7 +235,7 @@ class PropertyDataFetcherTest extends Specification { PropertyDataFetcherHelper.clearReflectionCache() when: - def environment = mkDFE("recordProperty", new RecordLikeClass()) + def environment = env("recordProperty", new RecordLikeClass()) def fetcher = new PropertyDataFetcher("recordProperty") def result = fetcher.get(environment) @@ -243,7 +243,7 @@ class PropertyDataFetcherTest extends Specification { result == "recordProperty" when: - environment = mkDFE("recordProperty", new RecordLikeTwoClassesDown()) + environment = env("recordProperty", new RecordLikeTwoClassesDown()) fetcher = new PropertyDataFetcher("recordProperty") result = fetcher.get(environment) @@ -257,7 +257,7 @@ class PropertyDataFetcherTest extends Specification { PropertyDataFetcherHelper.clearReflectionCache() when: - def environment = mkDFE("recordLike", new ConfusedPojo()) + def environment = env("recordLike", new ConfusedPojo()) def result = fetcher.get(environment) then: @@ -271,7 +271,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via public method"() { given: - def environment = mkDFE("publicProperty", new TestClass()) + def environment = env("publicProperty", new TestClass()) when: def result = fetcher.get(environment) @@ -288,7 +288,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via public method declared two classes up"() { given: - def environment = mkDFE("publicProperty", new TwoClassesDown("aValue")) + def environment = env("publicProperty", new TwoClassesDown("aValue")) def fetcher = new PropertyDataFetcher("publicProperty") when: @@ -305,7 +305,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via property only defined on package protected impl"() { given: - def environment = mkDFE("propertyOnlyDefinedOnPackageProtectedImpl", TestClass.createPackageProtectedImpl("aValue")) + def environment = env("propertyOnlyDefinedOnPackageProtectedImpl", TestClass.createPackageProtectedImpl("aValue")) when: def result = fetcher.get(environment) @@ -323,7 +323,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via public field"() { given: - def environment = mkDFE("publicField", new TestClass()) + def environment = env("publicField", new TestClass()) def result = fetcher.get(environment) expect: @@ -337,7 +337,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via private field"() { given: - def environment = mkDFE("privateField", new TestClass()) + def environment = env("privateField", new TestClass()) def result = fetcher.get(environment) expect: @@ -352,7 +352,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch via private field when setAccessible OFF"() { given: PropertyDataFetcher.setUseSetAccessible(false) - def environment = mkDFE("privateField", new TestClass()) + def environment = env("privateField", new TestClass()) def result = fetcher.get(environment) expect: @@ -366,7 +366,7 @@ class PropertyDataFetcherTest extends Specification { def "fetch when caching is in place has no bad effects"() { - def environment = mkDFE("publicProperty", new TestClass()) + def environment = env("publicProperty", new TestClass()) def fetcher = new PropertyDataFetcher("publicProperty") when: def result = fetcher.get(environment) @@ -441,7 +441,7 @@ class PropertyDataFetcherTest extends Specification { def "support for DFE on methods"() { given: - def environment = mkDFE("methodWithDFE", new ClassWithDFEMethods()) + def environment = env("methodWithDFE", new ClassWithDFEMethods()) def fetcher = new PropertyDataFetcher("methodWithDFE") when: @@ -495,7 +495,7 @@ class PropertyDataFetcherTest extends Specification { def "finds interface methods"() { when: - def environment = mkDFE("methodYouMustImplement", new ClassWithInterfaces()) + def environment = env("methodYouMustImplement", new ClassWithInterfaces()) def fetcher = new PropertyDataFetcher("methodYouMustImplement") def result = fetcher.get(environment) then: @@ -522,7 +522,7 @@ class PropertyDataFetcherTest extends Specification { } def "finds interface methods with inheritance"() { - def environment = mkDFE("methodYouMustImplement", new ClassWithInteritanceAndInterfaces.StartingClass()) + def environment = env("methodYouMustImplement", new ClassWithInteritanceAndInterfaces.StartingClass()) when: def fetcher = new PropertyDataFetcher("methodYouMustImplement") @@ -536,7 +536,7 @@ class PropertyDataFetcherTest extends Specification { then: result == "methodThatIsADefault" - def environment2 = mkDFE("methodYouMustImplement", new ClassWithInteritanceAndInterfaces.InheritedClass()) + def environment2 = env("methodYouMustImplement", new ClassWithInteritanceAndInterfaces.InheritedClass()) when: fetcher = new PropertyDataFetcher("methodYouMustImplement") @@ -565,7 +565,7 @@ class PropertyDataFetcherTest extends Specification { def "ensure DFE is passed to method"() { - def environment = mkDFE("methodUsesDataFetchingEnvironment", new ClassWithDFEMethods()) + def environment = env("methodUsesDataFetchingEnvironment", new ClassWithDFEMethods()) def fetcher = new PropertyDataFetcher("methodUsesDataFetchingEnvironment") when: def result = fetcher.get(environment) @@ -580,7 +580,7 @@ class PropertyDataFetcherTest extends Specification { } def "negative caching works as expected"() { - def environment = mkDFE("doesNotExist", new ClassWithDFEMethods()) + def environment = env("doesNotExist", new ClassWithDFEMethods()) def fetcher = new PropertyDataFetcher("doesNotExist") when: def result = fetcher.get(environment) @@ -671,7 +671,7 @@ class PropertyDataFetcherTest extends Specification { def "search for private getter in class hierarchy"() { given: Bar bar = new Baz() - def dfe = mkDFE("something", bar) + def dfe = env("something", bar) when: def result = fetcher.get(dfe) @@ -695,7 +695,7 @@ class PropertyDataFetcherTest extends Specification { def "issue 3247 - record like statics should not be used"() { given: def payload = new UpdateOrganizerSubscriptionPayload(true, new OrganizerSubscriptionError()) - def dfe = mkDFE("success", payload) + def dfe = env("success", payload) when: def result = fetcher.get(dfe) @@ -719,7 +719,7 @@ class PropertyDataFetcherTest extends Specification { def "issue 3247 - record like statics should not be found"() { given: def errorShape = new OrganizerSubscriptionError() - def dfe = mkDFE("message", errorShape) + def dfe = env("message", errorShape) when: def result = fetcher.get(dfe) @@ -743,7 +743,7 @@ class PropertyDataFetcherTest extends Specification { def "issue 3247 - getter statics should be found"() { given: def objectInQuestion = new BarClassWithStaticProperties() - def dfe = mkDFE("foo", objectInQuestion) + def dfe = env("foo", objectInQuestion) PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("foo") when: @@ -794,7 +794,7 @@ class PropertyDataFetcherTest extends Specification { Locale oldLocale = Locale.getDefault() Locale.setDefault(new Locale("tr", "TR")) - def environment = mkDFE("id", new OtherObject(id: "aValue")) + def environment = env("id", new OtherObject(id: "aValue")) when: def fetcher = PropertyDataFetcher.fetching("id") From 612084e025c4ca4e5ed04b989a58eba2b4ff7fbd Mon Sep 17 00:00:00 2001 From: bbaker Date: Mon, 25 Nov 2024 16:17:12 +1100 Subject: [PATCH 04/11] Use a singleton PropertyFetcher by default - tweaked test wiring --- src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy | 6 +++--- .../graphql/schema/idl/TestMockedWiringFactory.groovy | 8 ++------ 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy b/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy index 6c68eb72de..b445c09ff3 100644 --- a/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy +++ b/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy @@ -21,15 +21,15 @@ class LargeSchemaDataFetcherTest extends Specification { def schema = TestUtil.schema(sdl) def codeRegistry = schema.getCodeRegistry() + def singletonDF = PropertyDataFetcher.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) - - // in the future we hope to make this the same DF instance - assert df instanceof PropertyDataFetcher + assert df == singletonDF } } 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 From b8bfd4bb03b5d3fffb0c9d9d1b24d45ecd40ba6f Mon Sep 17 00:00:00 2001 From: bbaker Date: Mon, 25 Nov 2024 16:21:08 +1100 Subject: [PATCH 05/11] Use a singleton PropertyFetcher by default - tweaked test wiring and mocks --- src/main/java/graphql/schema/idl/MockedWiringFactory.java | 2 +- .../graphql/schema/idl/TestLiveMockedWiringFactory.groovy | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/schema/idl/MockedWiringFactory.java b/src/main/java/graphql/schema/idl/MockedWiringFactory.java index 3dc4b1ca2b..1f6c698e09 100644 --- a/src/main/java/graphql/schema/idl/MockedWiringFactory.java +++ b/src/main/java/graphql/schema/idl/MockedWiringFactory.java @@ -44,7 +44,7 @@ public boolean providesDataFetcher(FieldWiringEnvironment environment) { @Override public DataFetcher getDataFetcher(FieldWiringEnvironment environment) { - return new PropertyDataFetcher(environment.getFieldDefinition().getName()); + return PropertyDataFetcher.singleton(); } @Override diff --git a/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy b/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy index 476c13ac13..d72a87417d 100644 --- a/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy +++ b/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy @@ -74,7 +74,7 @@ class TestLiveMockedWiringFactory implements WiringFactory { @Override DataFetcher getDataFetcher(FieldWiringEnvironment environment) { - return new PropertyDataFetcher(environment.getFieldDefinition().getName()) + return PropertyDataFetcher.singleton() } @Override From 70bac09b89a6970137b6ea4e3480f53ae9a17ffe Mon Sep 17 00:00:00 2001 From: bbaker Date: Mon, 25 Nov 2024 17:38:50 +1100 Subject: [PATCH 06/11] Use a singleton PropertyFetcher by default - now with SchemaGeneratorHelper that does NOT put in an entry for defaulted values --- .../graphql/schema/DataFetcherFactory.java | 13 ++++++++++ .../graphql/schema/GraphQLCodeRegistry.java | 12 ++++++--- .../graphql/schema/PropertyDataFetcher.java | 12 ++++++++- .../schema/idl/SchemaGeneratorHelper.java | 26 ++++++++++++------- 4 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/main/java/graphql/schema/DataFetcherFactory.java b/src/main/java/graphql/schema/DataFetcherFactory.java index ece0dcb6ea..4a3263b38a 100644 --- a/src/main/java/graphql/schema/DataFetcherFactory.java +++ b/src/main/java/graphql/schema/DataFetcherFactory.java @@ -22,4 +22,17 @@ public interface DataFetcherFactory { */ 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 getViaField(GraphQLFieldDefinition fieldDefinition) { + return null; + } + } diff --git a/src/main/java/graphql/schema/GraphQLCodeRegistry.java b/src/main/java/graphql/schema/GraphQLCodeRegistry.java index 98877bae36..a59c8a25ee 100644 --- a/src/main/java/graphql/schema/GraphQLCodeRegistry.java +++ b/src/main/java/graphql/schema/GraphQLCodeRegistry.java @@ -95,9 +95,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.getViaField(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) { diff --git a/src/main/java/graphql/schema/PropertyDataFetcher.java b/src/main/java/graphql/schema/PropertyDataFetcher.java index 48aca93176..962244dedf 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcher.java +++ b/src/main/java/graphql/schema/PropertyDataFetcher.java @@ -40,7 +40,17 @@ Object fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fiel } }; - private static final DataFetcherFactory SINGLETON_FETCHER_FACTORY = environment -> SINGLETON_FETCHER; + private static final DataFetcherFactory SINGLETON_FETCHER_FACTORY = new DataFetcherFactory() { + @Override + public DataFetcher get(DataFetcherFactoryEnvironment environment) { + return SINGLETON_FETCHER; + } + + @Override + public DataFetcher getViaField(GraphQLFieldDefinition fieldDefinition) { + return SINGLETON_FETCHER; + } + }; /** * This returns the same singleton {@link PropertyDataFetcher} that fetches property values diff --git a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java index 68bfdd5683..efcb15d0ef 100644 --- a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java +++ b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java @@ -801,23 +801,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,7 +851,9 @@ 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(); } @@ -856,7 +862,7 @@ private DataFetcherFactory buildDataFetcherFactory(BuildContext buildCtx, } dataFetcherFactory = DataFetcherFactories.useDataFetcher(dataFetcher); } - return dataFetcherFactory; + return Optional.of(dataFetcherFactory); } GraphQLArgument buildArgument(BuildContext buildCtx, InputValueDefinition valueDefinition) { From b8b2e2614df37dc88063a51c1c91a2e58eb058bd Mon Sep 17 00:00:00 2001 From: bbaker Date: Tue, 26 Nov 2024 07:29:08 +1100 Subject: [PATCH 07/11] Use a singleton PropertyFetcher by default - now with SchemaGeneratorHelper that does NOT put in an entry for defaulted values- DFF tweaked --- .../java/graphql/schema/DataFetcherFactories.java | 12 +++++++++++- .../graphql/schema/DataFetcherFactoriesTest.groovy | 10 ++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/main/java/graphql/schema/DataFetcherFactories.java b/src/main/java/graphql/schema/DataFetcherFactories.java index e9f6a88bb5..28460a7be7 100644 --- a/src/main/java/graphql/schema/DataFetcherFactories.java +++ b/src/main/java/graphql/schema/DataFetcherFactories.java @@ -20,7 +20,17 @@ public class DataFetcherFactories { * @return a data fetcher factory that always returns the provided data fetcher */ public static DataFetcherFactory useDataFetcher(DataFetcher dataFetcher) { - return fieldDefinition -> dataFetcher; + return new DataFetcherFactory() { + @Override + public DataFetcher get(DataFetcherFactoryEnvironment environment) { + return dataFetcher; + } + + @Override + public DataFetcher getViaField(GraphQLFieldDefinition fieldDefinition) { + return dataFetcher; + } + }; } /** diff --git a/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy b/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy index 5050ba0ef1..369e1123ba 100644 --- a/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy +++ b/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy @@ -37,4 +37,14 @@ class DataFetcherFactoriesTest extends Specification { then: value == "goodbye" } + + def "will use given df via field"() { + def fetcherFactory = DataFetcherFactories.useDataFetcher(pojoDF) + + when: + def value = fetcherFactory.getViaField(null).get(null) + + then: + value == "goodbye" + } } From 424cac7dad93c6009c7d460c3adacf0434dfecde Mon Sep 17 00:00:00 2001 From: bbaker Date: Tue, 26 Nov 2024 15:01:02 +1100 Subject: [PATCH 08/11] Use a singleton PropertyFetcher by default - broke out the code into a SingletonPropertyDataFetcher class --- .../graphql/schema/GraphQLCodeRegistry.java | 4 +- .../graphql/schema/PropertyDataFetcher.java | 53 ++------------ .../schema/SingletonPropertyDataFetcher.java | 60 ++++++++++++++++ .../schema/idl/MockedWiringFactory.java | 3 +- .../schema/idl/SchemaGeneratorHelper.java | 3 +- .../groovy/graphql/DataFetcherTest.groovy | 54 +++++++++++--- .../graphql/LargeSchemaDataFetcherTest.groovy | 6 +- .../InstrumentationTest.groovy | 3 +- .../schema/GraphQLCodeRegistryTest.groovy | 19 ++--- .../graphql/schema/GraphQLSchemaTest.groovy | 2 +- .../schema/PropertyDataFetcherTest.groovy | 70 +++++++++---------- .../idl/TestLiveMockedWiringFactory.groovy | 3 +- 12 files changed, 167 insertions(+), 113 deletions(-) create mode 100644 src/main/java/graphql/schema/SingletonPropertyDataFetcher.java diff --git a/src/main/java/graphql/schema/GraphQLCodeRegistry.java b/src/main/java/graphql/schema/GraphQLCodeRegistry.java index 98877bae36..240dd4a4eb 100644 --- a/src/main/java/graphql/schema/GraphQLCodeRegistry.java +++ b/src/main/java/graphql/schema/GraphQLCodeRegistry.java @@ -149,7 +149,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 +189,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 = PropertyDataFetcher.singletonFactory(); + private DataFetcherFactory defaultDataFetcherFactory = SingletonPropertyDataFetcher.singletonFactory(); private boolean changed = false; private Builder() { diff --git a/src/main/java/graphql/schema/PropertyDataFetcher.java b/src/main/java/graphql/schema/PropertyDataFetcher.java index 48aca93176..77cfdcf062 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcher.java +++ b/src/main/java/graphql/schema/PropertyDataFetcher.java @@ -33,35 +33,6 @@ @PublicApi public class PropertyDataFetcher implements LightDataFetcher { - private static final PropertyDataFetcher SINGLETON_FETCHER = new PropertyDataFetcher<>() { - @Override - Object fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fieldDefinition, Supplier environmentSupplier) { - return super.fetchImpl(fieldDefinition.getName(), source, fieldDefinition, environmentSupplier); - } - }; - - private static final DataFetcherFactory SINGLETON_FETCHER_FACTORY = environment -> SINGLETON_FETCHER; - - /** - * This returns the same singleton {@link PropertyDataFetcher} that fetches property values - * based on the name of the field that iis passed into it. - * - * @return a singleton property data fetcher - */ - public static PropertyDataFetcher 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 final String propertyName; private final Function function; @@ -82,11 +53,6 @@ private PropertyDataFetcher(Function function) { this.propertyName = null; } - private PropertyDataFetcher() { - this.function = null; - this.propertyName = null; - } - /** * Returns a data fetcher that will use the property name to examine the {@link DataFetchingEnvironment#getSource()} object * for a getter method or field with that name, or if it's a map, it will look up a value using @@ -143,26 +109,17 @@ public String getPropertyName() { @Override public T get(GraphQLFieldDefinition fieldDefinition, Object source, Supplier environmentSupplier) throws Exception { - return fetchImpl(propertyName, source, fieldDefinition, environmentSupplier); + return getImpl(source, fieldDefinition.getType(), environmentSupplier); } @Override public T get(DataFetchingEnvironment environment) { - return fetchImpl(propertyName, environment.getSource(), environment.getFieldDefinition(), () -> environment); + Object source = environment.getSource(); + return getImpl(source, environment.getFieldType(), () -> environment); } - /** - * This is our implementation of property fetching - * - * @param propertyName the name of the property to fetch in the source object - * @param source the source object to fetch from - * @param fieldDefinition the field definition of the field being fetched for - * @param environmentSupplier a supplied of thee {@link DataFetchingEnvironment} - * - * @return a value of type T - */ @SuppressWarnings("unchecked") - T fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fieldDefinition, Supplier environmentSupplier) { + private T getImpl(Object source, GraphQLOutputType fieldDefinition, Supplier environmentSupplier) { if (source == null) { return null; } @@ -171,7 +128,7 @@ T fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fieldDefi return (T) function.apply(source); } - return (T) PropertyDataFetcherHelper.getPropertyValue(propertyName, source, fieldDefinition.getType(), environmentSupplier); + return (T) PropertyDataFetcherHelper.getPropertyValue(propertyName, source, fieldDefinition, environmentSupplier); } /** 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 1f6c698e09..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 PropertyDataFetcher.singleton(); + 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 68bfdd5683..241416ff35 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; @@ -1088,7 +1089,7 @@ private Optional getOperationNamed(String name, Map dataFetcherOfLastResort() { - return PropertyDataFetcher.singleton(); + 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 e06b134489..2f57c41efd 100644 --- a/src/test/groovy/graphql/DataFetcherTest.groovy +++ b/src/test/groovy/graphql/DataFetcherTest.groovy @@ -4,6 +4,7 @@ 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 @@ -57,63 +58,94 @@ class DataFetcherTest extends Specification { } def env(String propertyName, GraphQLOutputType type) { - def fieldDefinition = GraphQLFieldDefinition.newFieldDefinition().name(propertyName).type(type).build() + 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("property", GraphQLString) + def field = mkField("property", GraphQLString) when: def result = fetcher.get(environment) then: result == "propertyValue" + when: + result = fetcher.get(field, dataHolder, { environment }) + then: + result == "propertyValue" + where: - fetcher | _ - new PropertyDataFetcher("property") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("property") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "get Boolean property value"() { given: def environment = env("booleanField", GraphQLBoolean) + def field = mkField("booleanField", GraphQLBoolean) + when: def result = fetcher.get(environment) then: result == true + when: + result = fetcher.get(field, dataHolder, { environment }) + then: + result == true + where: - fetcher | _ - new PropertyDataFetcher("booleanField") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("booleanField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "get Boolean property value with get"() { given: def environment = env("booleanFieldWithGet", GraphQLBoolean) + def field = mkField("booleanFieldWithGet", GraphQLBoolean) + when: def result = fetcher.get(environment) then: result == false + when: + result = fetcher.get(field, dataHolder, { environment }) + then: + result == false + where: fetcher | _ new PropertyDataFetcher("booleanFieldWithGet") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } def "get public field value as property"() { given: def environment = env("publicField", GraphQLString) + def field = mkField("publicField", GraphQLString) + when: def result = fetcher.get(environment) then: result == "publicValue" + when: + result = fetcher.get(field, dataHolder, { environment }) + then: + result == "publicValue" + where: - fetcher | _ - new PropertyDataFetcher("publicField") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("publicField") | _ + SingletonPropertyDataFetcher.singleton() | _ } } diff --git a/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy b/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy index b445c09ff3..1aa971c6e7 100644 --- a/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy +++ b/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy @@ -4,13 +4,14 @@ 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 a property data fetcher by default"() { + def "large schema with lots of fields has the same property data fetcher by default"() { def sdl = """ type Query { ${mkFields()} @@ -20,8 +21,7 @@ class LargeSchemaDataFetcherTest extends Specification { when: def schema = TestUtil.schema(sdl) def codeRegistry = schema.getCodeRegistry() - - def singletonDF = PropertyDataFetcher.singleton() + def singletonDF = SingletonPropertyDataFetcher.singleton() then: diff --git a/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy b/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy index a1a85b617e..bd5237700c 100644 --- a/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy @@ -13,6 +13,7 @@ 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.StaticDataFetcher import org.awaitility.Awaitility @@ -99,7 +100,7 @@ class InstrumentationTest extends Specification { instrumentation.dfClasses.size() == 2 instrumentation.dfClasses[0] == StaticDataFetcher.class - PropertyDataFetcher.isAssignableFrom(instrumentation.dfClasses[1]) + LightDataFetcher.class.isAssignableFrom(instrumentation.dfClasses[1]) instrumentation.dfInvocations.size() == 2 diff --git a/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy b/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy index eef53c9794..d3266bf336 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 LightDataFetcher } 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 ceadabe88a..7903f14229 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -18,7 +18,7 @@ import java.util.function.Function import static graphql.schema.DataFetchingEnvironmentImpl.newDataFetchingEnvironment /** - * Note : That `new PropertyDataFetcher("someProperty")` and `PropertyDataFetcher.singleton()` + * Note : That `new PropertyDataFetcher("someProperty")` and `SingletonPropertyDataFetcher.singleton()` * should really be the equivalent since they both go via `PropertyDataFetcherHelper.getPropertyValue` * under the covers. * @@ -56,9 +56,9 @@ class PropertyDataFetcherTest extends Specification { fetcher.get(environment) == null where: - fetcher | _ - new PropertyDataFetcher("someProperty") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("someProperty") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "function based fetcher works with non null source"() { @@ -87,7 +87,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ PropertyDataFetcher.fetching("mapProperty") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via public getter with private subclass"() { @@ -100,7 +100,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ new PropertyDataFetcher("packageProtectedProperty") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via method that isn't present"() { @@ -116,7 +116,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ new PropertyDataFetcher("valueNotPresent") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } @@ -133,7 +133,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ new PropertyDataFetcher("privateProperty") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } @@ -151,7 +151,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ new PropertyDataFetcher("privateProperty") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } @@ -207,7 +207,7 @@ class PropertyDataFetcherTest extends Specification { def environment = env("recordProperty", new RecordLikeClass()) when: - def fetcher = PropertyDataFetcher.singleton() + def fetcher = SingletonPropertyDataFetcher.singleton() def result = fetcher.get(environment) then: result == "recordProperty" @@ -226,7 +226,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ new PropertyDataFetcher("recordProperty") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via record method without lambda support"() { @@ -264,9 +264,9 @@ class PropertyDataFetcherTest extends Specification { result == "recordLike" where: - fetcher | _ - new PropertyDataFetcher("recordLike") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("recordLike") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via public method"() { @@ -282,7 +282,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ new PropertyDataFetcher("publicProperty") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } @@ -316,7 +316,7 @@ class PropertyDataFetcherTest extends Specification { where: fetcher | _ new PropertyDataFetcher("propertyOnlyDefinedOnPackageProtectedImpl") | _ - PropertyDataFetcher.singleton() | _ + SingletonPropertyDataFetcher.singleton() | _ } @@ -330,9 +330,9 @@ class PropertyDataFetcherTest extends Specification { result == "publicFieldValue" where: - fetcher | _ - new PropertyDataFetcher("publicField") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("publicField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via private field"() { @@ -344,9 +344,9 @@ class PropertyDataFetcherTest extends Specification { result == "privateFieldValue" where: - fetcher | _ - new PropertyDataFetcher("privateField") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("privateField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch via private field when setAccessible OFF"() { @@ -359,9 +359,9 @@ class PropertyDataFetcherTest extends Specification { result == null where: - fetcher | _ - new PropertyDataFetcher("privateField") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("privateField") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "fetch when caching is in place has no bad effects"() { @@ -687,9 +687,9 @@ class PropertyDataFetcherTest extends Specification { result == "bar" where: - fetcher | _ - new PropertyDataFetcher("something") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("something") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "issue 3247 - record like statics should not be used"() { @@ -711,9 +711,9 @@ class PropertyDataFetcherTest extends Specification { result == true where: - fetcher | _ - new PropertyDataFetcher("success") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("success") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "issue 3247 - record like statics should not be found"() { @@ -735,9 +735,9 @@ class PropertyDataFetcherTest extends Specification { result == null where: - fetcher | _ - new PropertyDataFetcher("message") | _ - PropertyDataFetcher.singleton() | _ + fetcher | _ + new PropertyDataFetcher("message") | _ + SingletonPropertyDataFetcher.singleton() | _ } def "issue 3247 - getter statics should be found"() { @@ -804,7 +804,7 @@ class PropertyDataFetcherTest extends Specification { propValue == 'aValue' when: - fetcher = PropertyDataFetcher.singleton() + fetcher = SingletonPropertyDataFetcher.singleton() propValue = fetcher.get(environment) then: diff --git a/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy b/src/test/groovy/graphql/schema/idl/TestLiveMockedWiringFactory.groovy index d72a87417d..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 PropertyDataFetcher.singleton() + return SingletonPropertyDataFetcher.singleton() } @Override From 886d745e762a6c43a524bd97dc75494c301f0856 Mon Sep 17 00:00:00 2001 From: bbaker Date: Tue, 26 Nov 2024 15:09:04 +1100 Subject: [PATCH 09/11] Use a singleton PropertyFetcher by default - tweaked tests --- .../execution/instrumentation/InstrumentationTest.groovy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy b/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy index bd5237700c..a98cf833e2 100644 --- a/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy +++ b/src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy @@ -15,6 +15,7 @@ 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 @@ -100,7 +101,7 @@ class InstrumentationTest extends Specification { instrumentation.dfClasses.size() == 2 instrumentation.dfClasses[0] == StaticDataFetcher.class - LightDataFetcher.class.isAssignableFrom(instrumentation.dfClasses[1]) + instrumentation.dfClasses[1] == SingletonPropertyDataFetcher.class instrumentation.dfInvocations.size() == 2 From 9ca4d3069856007fde9c81a8290e8b017b66a99b Mon Sep 17 00:00:00 2001 From: bbaker Date: Tue, 26 Nov 2024 15:16:55 +1100 Subject: [PATCH 10/11] Use a singleton PropertyFetcher by default - tweaked tests moar --- src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy b/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy index d3266bf336..e528f5619f 100644 --- a/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy @@ -288,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 LightDataFetcher + schema.getCodeRegistry().getDataFetcher(queryType, queryType.getFieldDefinition("neitherSpecified")) instanceof SingletonPropertyDataFetcher } def "will detect system versus user data fetchers"() { From b13b7c8309bd428358bcfc8ddb2e3edd49689659 Mon Sep 17 00:00:00 2001 From: bbaker Date: Tue, 26 Nov 2024 15:30:26 +1100 Subject: [PATCH 11/11] Use a singleton PropertyFetcher by default - deprecated DFF method --- src/main/java/graphql/schema/DataFetcherFactories.java | 7 ++++--- src/main/java/graphql/schema/DataFetcherFactory.java | 5 ++++- .../java/graphql/schema/DataFetcherFactoryEnvironment.java | 4 ++++ src/main/java/graphql/schema/GraphQLCodeRegistry.java | 3 ++- .../groovy/graphql/schema/DataFetcherFactoriesTest.groovy | 4 ++-- .../groovy/graphql/schema/idl/SchemaGeneratorTest.groovy | 5 +++++ 6 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/main/java/graphql/schema/DataFetcherFactories.java b/src/main/java/graphql/schema/DataFetcherFactories.java index 28460a7be7..2345535d56 100644 --- a/src/main/java/graphql/schema/DataFetcherFactories.java +++ b/src/main/java/graphql/schema/DataFetcherFactories.java @@ -20,14 +20,15 @@ public class DataFetcherFactories { * @return a data fetcher factory that always returns the provided data fetcher */ public static DataFetcherFactory useDataFetcher(DataFetcher dataFetcher) { - return new DataFetcherFactory() { + //noinspection deprecation + return new DataFetcherFactory<>() { @Override public DataFetcher get(DataFetcherFactoryEnvironment environment) { return dataFetcher; } @Override - public DataFetcher getViaField(GraphQLFieldDefinition fieldDefinition) { + public DataFetcher get(GraphQLFieldDefinition fieldDefinition) { return dataFetcher; } }; @@ -42,7 +43,7 @@ public DataFetcher getViaField(GraphQLFieldDefinition fieldDefinition) { * * @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 4a3263b38a..9e7eafa872 100644 --- a/src/main/java/graphql/schema/DataFetcherFactory.java +++ b/src/main/java/graphql/schema/DataFetcherFactory.java @@ -19,7 +19,10 @@ 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); /** @@ -31,7 +34,7 @@ public interface DataFetcherFactory { * @return a data fetcher */ - default DataFetcher getViaField(GraphQLFieldDefinition fieldDefinition) { + 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 4d11ebecee..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); @@ -96,7 +97,7 @@ private static DataFetcher getDataFetcherImpl(FieldCoordinates coordinates, G } } // call direct from the field - cheaper to not make a new environment object - DataFetcher dataFetcher = dataFetcherFactory.getViaField(fieldDefinition); + DataFetcher dataFetcher = dataFetcherFactory.get(fieldDefinition); if (dataFetcher == null) { DataFetcherFactoryEnvironment factoryEnvironment = newDataFetchingFactoryEnvironment() .fieldDefinition(fieldDefinition) diff --git a/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy b/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy index 369e1123ba..3fb58ed1e7 100644 --- a/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy +++ b/src/test/groovy/graphql/schema/DataFetcherFactoriesTest.groovy @@ -32,7 +32,7 @@ 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" @@ -42,7 +42,7 @@ class DataFetcherFactoriesTest extends Specification { def fetcherFactory = DataFetcherFactories.useDataFetcher(pojoDF) when: - def value = fetcherFactory.getViaField(null).get(null) + def value = fetcherFactory.get((GraphQLFieldDefinition) null).get(null) then: value == "goodbye" 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()