Merge pull request #3069 from rmosolgo/better-method-warnings

Improve method conflict warnings

Author: Robert Mosolgo

lib/graphql/schema/member/has_fields.rb

@@ -58,9 +58,11 @@ def get_field(field_name)
         # Register this field with the class, overriding a previous one if needed.
         # @param field_defn [GraphQL::Schema::Field]
         # @return [void]
-        def add_field(field_defn)
-          if CONFLICT_FIELD_NAMES.include?(field_defn.resolver_method) && field_defn.original_name == field_defn.resolver_method && field_defn.method_conflict_warning?
-            warn "#{self.graphql_name}'s `field :#{field_defn.name}` conflicts with a built-in method, use `resolver_method:` to pick a different resolver method for this field (for example, `resolver_method: :resolve_#{field_defn.resolver_method}` and `def resolve_#{field_defn.resolver_method}`). Or use `method_conflict_warning: false` to suppress this warning."
+        def add_field(field_defn, method_conflict_warning: field_defn.method_conflict_warning?)
+          # Check that `field_defn.original_name` equals `resolver_method` and `method_sym` --
+          # that shows that no override value was given manually.
+          if method_conflict_warning && CONFLICT_FIELD_NAMES.include?(field_defn.resolver_method) && field_defn.original_name == field_defn.resolver_method && field_defn.original_name == field_defn.method_sym
+            warn(conflict_field_name_warning(field_defn))
           end
           own_fields[field_defn.name] = field_defn
           nil
@@ -92,6 +94,14 @@ def global_id_field(field_name)
         def own_fields
           @own_fields ||= {}
         end
+
+        private
+
+        # @param [GraphQL::Schema::Field]
+        # @return [String] A warning to give when this field definition might conflict with a built-in method
+        def conflict_field_name_warning(field_defn)
+          "#{self.graphql_name}'s `field :#{field_defn.original_name}` conflicts with a built-in method, use `resolver_method:` to pick a different resolver method for this field (for example, `resolver_method: :resolve_#{field_defn.resolver_method}` and `def resolve_#{field_defn.resolver_method}`). Or use `method_conflict_warning: false` to suppress this warning."
+        end
       end
     end
   end

lib/graphql/schema/mutation.rb

@@ -78,6 +78,10 @@ def visible?(context)
 
         private
 
+        def conflict_field_name_warning(field_defn)
+          "#{self.graphql_name}'s `field :#{field_defn.name}` conflicts with a built-in method, use `hash_key:` or `method:` to pick a different resolve behavior for this field (for example, `hash_key: :#{field_defn.resolver_method}_value`, and modify the return hash). Or use `method_conflict_warning: false` to suppress this warning."
+        end
+
         # Override this to attach self as `mutation`
         def generate_payload_type
           payload_class = super

lib/graphql/schema/resolver/has_payload_type.rb

@@ -58,7 +58,8 @@ def generate_payload_type
             resolver_fields.each do |name, f|
               # Reattach the already-defined field here
               # (The field's `.owner` will still point to the mutation, not the object type, I think)
-              add_field(f)
+              # Don't re-warn about a method conflict. Since this type is generated, it should be fixed in the resolver instead.
+              add_field(f, method_conflict_warning: false)
             end
           end
         end

spec/graphql/schema/mutation_spec.rb

@@ -165,4 +165,25 @@
       assert override_mutation.field_options[:null]
     end
   end
+
+  it "warns once for possible conflict methods" do
+    expected_warning = "X's `field :module` conflicts with a built-in method, use `hash_key:` or `method:` to pick a different resolve behavior for this field (for example, `hash_key: :module_value`, and modify the return hash). Or use `method_conflict_warning: false` to suppress this warning.\n"
+    assert_output "", expected_warning do
+      # This should warn:
+      mutation = Class.new(GraphQL::Schema::Mutation) do
+        graphql_name "X"
+        field :module, String, null: true
+      end
+      # This should not warn again, when generating the payload type with the same fields:
+      mutation.payload_type
+    end
+
+    assert_output "", "" do
+      mutation = Class.new(GraphQL::Schema::Mutation) do
+        graphql_name "X"
+        field :module, String, null: true, hash_key: :module_value
+      end
+      mutation.payload_type
+    end
+  end
 end

spec/graphql/schema/object_spec.rb

@@ -357,7 +357,7 @@ def call(o, a, c)
       end
     end
 
-    it "doesn't warn with an override" do
+    it "doesn't warn with a resolver_method: override" do
       assert_output "", "" do
         Class.new(GraphQL::Schema::Object) do
           graphql_name "X"
@@ -366,6 +366,15 @@ def call(o, a, c)
       end
     end
 
+    it "doesn't warn with a method: override" do
+      assert_output "", "" do
+        Class.new(GraphQL::Schema::Object) do
+          graphql_name "X"
+          field :module, String, null: true, method: :mod
+        end
+      end
+    end
+
     it "doesn't warn with a suppression" do
       assert_output "", "" do
         Class.new(GraphQL::Schema::Object) do