Merge pull request #3211 from graphql-java/deprecate_fields_container_in_code_registry

bbakerman · web-flow · commit 053b824e7bed · 2023-05-19T03:18:04.000Z
Deprecate FieldsContainer on code registry
diff --git a/src/main/java/graphql/schema/GraphQLCodeRegistry.java b/src/main/java/graphql/schema/GraphQLCodeRegistry.java
@@ -1,6 +1,7 @@
 package graphql.schema;
 
 import graphql.Assert;
+import graphql.DeprecatedAt;
 import graphql.Internal;
 import graphql.PublicApi;
 import graphql.schema.visibility.GraphqlFieldVisibility;
@@ -56,11 +57,29 @@ public GraphqlFieldVisibility getFieldVisibility() {
      * @param fieldDefinition the field definition
      *
      * @return the DataFetcher associated with this field.  All fields have data fetchers
+     *
+     * @see #getDataFetcher(GraphQLObjectType, GraphQLFieldDefinition)
+     * @deprecated This is confusing because {@link GraphQLInterfaceType}s cant have data fetchers.  At runtime only a {@link GraphQLObjectType}
+     * can be used to fetch a field.  This method allows the mapping to be made, but it is never useful if an interface is passed in.
      */
+    @Deprecated
+    @DeprecatedAt("2023-05-13")
     public DataFetcher<?> getDataFetcher(GraphQLFieldsContainer parentType, GraphQLFieldDefinition fieldDefinition) {
         return getDataFetcherImpl(FieldCoordinates.coordinates(parentType, fieldDefinition), fieldDefinition, dataFetcherMap, systemDataFetcherMap, defaultDataFetcherFactory);
     }
 
+    /**
+     * Returns a data fetcher associated with a field within an object type
+     *
+     * @param parentType      the container type
+     * @param fieldDefinition the field definition
+     *
+     * @return the DataFetcher associated with this field.  All fields have data fetchers
+     */
+    public DataFetcher<?> getDataFetcher(GraphQLObjectType parentType, GraphQLFieldDefinition fieldDefinition) {
+        return getDataFetcherImpl(FieldCoordinates.coordinates(parentType, fieldDefinition), fieldDefinition, dataFetcherMap, systemDataFetcherMap, defaultDataFetcherFactory);
+    }
+
     /**
      * Returns a data fetcher associated with a field located at specified coordinates.
      *
@@ -242,11 +261,29 @@ private Builder markChanged(boolean condition) {
          * @param fieldDefinition the field definition
          *
          * @return the DataFetcher associated with this field.  All fields have data fetchers
+         *
+         * @see #getDataFetcher(GraphQLObjectType, GraphQLFieldDefinition)
+         * @deprecated This is confusing because {@link GraphQLInterfaceType}s cant have data fetchers.  At runtime only a {@link GraphQLObjectType}
+         * can be used to fetch a field.  This method allows the mapping to be made, but it is never useful if an interface is passed in.
          */
+        @Deprecated
+        @DeprecatedAt("2023-05-13")
         public DataFetcher<?> getDataFetcher(GraphQLFieldsContainer parentType, GraphQLFieldDefinition fieldDefinition) {
             return getDataFetcherImpl(FieldCoordinates.coordinates(parentType, fieldDefinition), fieldDefinition, dataFetcherMap, systemDataFetcherMap, defaultDataFetcherFactory);
         }
 
+        /**
+         * Returns a data fetcher associated with a field within an object type
+         *
+         * @param parentType      the container type
+         * @param fieldDefinition the field definition
+         *
+         * @return the DataFetcher associated with this field.  All fields have data fetchers
+         */
+        public DataFetcher<?> getDataFetcher(GraphQLObjectType parentType, GraphQLFieldDefinition fieldDefinition) {
+            return getDataFetcherImpl(FieldCoordinates.coordinates(parentType, fieldDefinition), fieldDefinition, dataFetcherMap, systemDataFetcherMap, defaultDataFetcherFactory);
+        }
+
         /**
          * Returns a data fetcher associated with a field located at specified coordinates.
          *
@@ -331,11 +368,31 @@ public Builder dataFetcher(FieldCoordinates coordinates, DataFetcher<?> dataFetc
          * @param dataFetcher     the data fetcher code for that field
          *
          * @return this builder
+         *
+         * @see #dataFetcher(GraphQLObjectType, GraphQLFieldDefinition, DataFetcher)
+         * @deprecated This is confusing because {@link GraphQLInterfaceType}s cant have data fetchers.  At runtime only a {@link GraphQLObjectType}
+         * can be used to fetch a field.  This method allows the mapping to be made, but it is never useful if an interface is passed in.
          */
+        @Deprecated
+        @DeprecatedAt("2023-05-13")
         public Builder dataFetcher(GraphQLFieldsContainer parentType, GraphQLFieldDefinition fieldDefinition, DataFetcher<?> dataFetcher) {
             return dataFetcher(FieldCoordinates.coordinates(parentType.getName(), fieldDefinition.getName()), dataFetcher);
         }
 
+
+        /**
+         * Sets the data fetcher for a specific field inside an object type
+         *
+         * @param parentType      the object type
+         * @param fieldDefinition the field definition
+         * @param dataFetcher     the data fetcher code for that field
+         *
+         * @return this builder
+         */
+        public Builder dataFetcher(GraphQLObjectType parentType, GraphQLFieldDefinition fieldDefinition, DataFetcher<?> dataFetcher) {
+            return dataFetcher(FieldCoordinates.coordinates(parentType.getName(), fieldDefinition.getName()), dataFetcher);
+        }
+
         /**
          * Called to place system data fetchers (eg Introspection fields) into the mix
          *
diff --git a/src/main/java/graphql/schema/idl/SchemaDirectiveWiringEnvironmentImpl.java b/src/main/java/graphql/schema/idl/SchemaDirectiveWiringEnvironmentImpl.java
@@ -130,7 +130,7 @@ public GraphQLFieldDefinition getFieldDefinition() {
     public DataFetcher<?> getFieldDataFetcher() {
         assertNotNull(fieldDefinition, () -> "An output field must be in context to call this method");
         assertNotNull(fieldsContainer, () -> "An output field container must be in context to call this method");
-        return codeRegistry.getDataFetcher(fieldsContainer, fieldDefinition);
+        return codeRegistry.getDataFetcher(FieldCoordinates.coordinates(fieldsContainer, fieldDefinition), fieldDefinition);
     }
 
     @Override
diff --git a/src/test/groovy/graphql/analysis/values/ValueTraverserTest.groovy b/src/test/groovy/graphql/analysis/values/ValueTraverserTest.groovy
@@ -17,6 +17,7 @@ import graphql.schema.GraphQLInputObjectType
 import graphql.schema.GraphQLInputSchemaElement
 import graphql.schema.GraphQLList
 import graphql.schema.GraphQLNamedSchemaElement
+import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLScalarType
 import graphql.schema.idl.SchemaDirectiveWiring
 import graphql.schema.idl.SchemaDirectiveWiringEnvironment
@@ -861,8 +862,12 @@ type Profile {
             GraphQLFieldDefinition onField(SchemaDirectiveWiringEnvironment<GraphQLFieldDefinition> env) {
                 GraphQLFieldsContainer fieldsContainer = env.getFieldsContainer()
                 GraphQLFieldDefinition fieldDefinition = env.getFieldDefinition()
+                if (! fieldsContainer instanceof GraphQLObjectType) {
+                    return fieldDefinition
+                }
+                GraphQLObjectType containingObjectType = env.getFieldsContainer() as GraphQLObjectType
 
-                final DataFetcher<?> originalDF = env.getCodeRegistry().getDataFetcher(fieldsContainer, fieldDefinition)
+                final DataFetcher<?> originalDF = env.getCodeRegistry().getDataFetcher(containingObjectType, fieldDefinition)
                 final DataFetcher<?> newDF = { DataFetchingEnvironment originalEnv ->
                     ValueVisitor visitor = new ValueVisitor() {
                         @Override
@@ -884,7 +889,7 @@ type Profile {
                     return originalDF.get(newEnv);
                 }
 
-                env.getCodeRegistry().dataFetcher(fieldsContainer, fieldDefinition, newDF)
+                env.getCodeRegistry().dataFetcher(containingObjectType, fieldDefinition, newDF)
 
                 return fieldDefinition
             }
diff --git a/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy
@@ -325,9 +325,14 @@ class SchemaGeneratorDirectiveHelperTest extends Specification {
             @Override
             GraphQLFieldDefinition onField(SchemaDirectiveWiringEnvironment<GraphQLFieldDefinition> directiveEnv) {
                 GraphQLFieldDefinition field = directiveEnv.getElement()
+                def container = directiveEnv.fieldsContainer
+                if (!container instanceof GraphQLObjectType) {
+                    return field
+                }
                 //
                 // we use the non shortcut path to the data fetcher here so prove it still works
-                def fetcher = directiveEnv.getCodeRegistry().getDataFetcher(directiveEnv.fieldsContainer, field)
+
+                def fetcher = directiveEnv.getCodeRegistry().getDataFetcher(container as GraphQLObjectType, field)
                 def newFetcher = wrapDataFetcher(fetcher, { dfEnv, value ->
                     def directiveName = directiveEnv.appliedDirective.name
                     if (directiveName == "uppercase") {
@@ -484,10 +489,15 @@ class SchemaGeneratorDirectiveHelperTest extends Specification {
             @Override
             GraphQLFieldDefinition onField(SchemaDirectiveWiringEnvironment<GraphQLFieldDefinition> environment) {
                 GraphQLFieldDefinition element = environment.getElement()
-                return wrapField(environment.fieldsContainer, element, environment.getBuildContext(), environment.getCodeRegistry())
+
+                def container = environment.fieldsContainer
+                if (! container instanceof GraphQLObjectType) {
+                    return element
+                }
+                return wrapField(container as GraphQLObjectType, element, environment.getBuildContext(), environment.getCodeRegistry())
             }
 
-            private GraphQLFieldDefinition wrapField(GraphQLFieldsContainer parentType, GraphQLFieldDefinition field, Map<String, Object> contextMap, GraphQLCodeRegistry.Builder codeRegistry) {
+            private GraphQLFieldDefinition wrapField(GraphQLObjectType parentType, GraphQLFieldDefinition field, Map<String, Object> contextMap, GraphQLCodeRegistry.Builder codeRegistry) {
                 def originalFetcher = codeRegistry.getDataFetcher(parentType, field)
 
                 String key = mkFieldKey(parentType.getName(), field.getName())

Original file line number	Diff line number	Diff line change
`@@ -130,7 +130,7 @@ public GraphQLFieldDefinition getFieldDefinition() {`
`130`	`130`	`public DataFetcher<?> getFieldDataFetcher() {`
`131`	`131`	`assertNotNull(fieldDefinition, () -> "An output field must be in context to call this method");`
`132`	`132`	`assertNotNull(fieldsContainer, () -> "An output field container must be in context to call this method");`
`133`		`- return codeRegistry.getDataFetcher(fieldsContainer, fieldDefinition);`
	`133`	`+ return codeRegistry.getDataFetcher(FieldCoordinates.coordinates(fieldsContainer, fieldDefinition), fieldDefinition);`
`134`	`134`	`}`
`135`	`135`
`136`	`136`	`@Override`