Use a singleton PropertyFetcher by default

bbakerman · Nov 26, 2024 · Nov 25, 2024 · Nov 25, 2024 · Nov 25, 2024 · Nov 25, 2024
commit 689e40171b88e123adc413797d5d64bfc6c5ef0c
diff --git 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<String, DataFetcherFactory<?>> systemDataFetcherMap = new LinkedHashMap<>();
         private final Map<String, TypeResolver> 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
@@ -33,6 +33,35 @@
 @PublicApi
 public class PropertyDataFetcher<T> implements LightDataFetcher<T> {
 
+    private static final PropertyDataFetcher<Object> SINGLETON_FETCHER = new PropertyDataFetcher<>() {
+        @Override
+        Object fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fieldDefinition, Supplier<DataFetchingEnvironment> 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<Object, Object> function;
 
@@ -53,6 +82,11 @@ private <O> PropertyDataFetcher(Function<O, T> 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<DataFetchingEnvironment> 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<DataFetchingEnvironment> environmentSupplier) {
+    T fetchImpl(String propertyName, Object source, GraphQLFieldDefinition fieldDefinition, Supplier<DataFetchingEnvironment> environmentSupplier) {
         if (source == null) {
             return null;
         }
@@ -128,7 +171,7 @@ private T getImpl(Object source, GraphQLOutputType fieldDefinition, Supplier<Dat
             return (T) function.apply(source);
         }
 
-        return (T) PropertyDataFetcherHelper.getPropertyValue(propertyName, source, fieldDefinition, environmentSupplier);
+        return (T) PropertyDataFetcherHelper.getPropertyValue(propertyName, source, fieldDefinition.getType(), environmentSupplier);
     }
 
     /**

diff --git a/src/test/groovy/graphql/DataFetcherTest.groovy b/src/test/groovy/graphql/DataFetcherTest.groovy
@@ -1,6 +1,7 @@
 package graphql
 
 
+import graphql.schema.GraphQLFieldDefinition
 import graphql.schema.GraphQLOutputType
 import graphql.schema.PropertyDataFetcher
 import spock.lang.Specification
@@ -55,43 +56,64 @@ class DataFetcherTest extends Specification {
 
     }
 
-    def env(GraphQLOutputType type) {
-        newDataFetchingEnvironment().source(dataHolder).fieldType(type).build()
+    def mkDFE(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 = env(GraphQLString)
+        def environment = mkDFE("property", GraphQLString)
         when:
-        def result = new PropertyDataFetcher("property").get(environment)
+        def result = fetcher.get(environment)
         then:
         result == "propertyValue"
+
+        where:
+        fetcher                             | _
+        new PropertyDataFetcher("property") | _
+        PropertyDataFetcher.singleton()     | _
     }
 
     def "get Boolean property value"() {
         given:
-        def environment = env(GraphQLBoolean)
+        def environment = mkDFE("booleanField", GraphQLBoolean)
         when:
-        def result = new PropertyDataFetcher("booleanField").get(environment)
+        def result = fetcher.get(environment)
         then:
         result == true
+
+        where:
+        fetcher                                 | _
+        new PropertyDataFetcher("booleanField") | _
+        PropertyDataFetcher.singleton()         | _
     }
 
     def "get Boolean property value with get"() {
         given:
-        def environment = env(GraphQLBoolean)
+        def environment = mkDFE("booleanFieldWithGet", GraphQLBoolean)
         when:
-        def result = new PropertyDataFetcher("booleanFieldWithGet").get(environment)
+        def result = fetcher.get(environment)
         then:
         result == false
+
+        where:
+        fetcher                                        | _
+        new PropertyDataFetcher("booleanFieldWithGet") | _
+        PropertyDataFetcher.singleton()                | _
     }
 
     def "get public field value as property"() {
         given:
-        def environment = env(GraphQLString)
+        def environment = mkDFE("publicField", GraphQLString)
         when:
-        def result = new PropertyDataFetcher("publicField").get(environment)
+        def result = fetcher.get(environment)
         then:
         result == "publicValue"
+
+        where:
+        fetcher                                | _
+        new PropertyDataFetcher("publicField") | _
+        PropertyDataFetcher.singleton()        | _
     }
 }
diff --git a/src/test/groovy/graphql/LargeSchemaDataFetcherTest.groovy 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 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 sdl = """
+            type Query {
+                ${mkFields()}
+            }
+        """
+
+        when:
+        def schema = TestUtil.schema(sdl)
+        def codeRegistry = schema.getCodeRegistry()
+
+        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
+        }
+    }
+
+    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
@@ -99,7 +99,7 @@ class InstrumentationTest extends Specification {
 
         instrumentation.dfClasses.size() == 2
         instrumentation.dfClasses[0] == StaticDataFetcher.class
-        instrumentation.dfClasses[1] == PropertyDataFetcher.class
+        PropertyDataFetcher.isAssignableFrom(instrumentation.dfClasses[1])
 
         instrumentation.dfInvocations.size() == 2