Merge branch 'master' into fewer-dead-result-checks

Author: rmosolgo

lib/graphql/execution/interpreter/runtime.rb

@@ -448,9 +448,14 @@ def evaluate_selection(result_name, field_ast_nodes_or_ast_node, owner_object, o
           total_args_count = field_defn.arguments(context).size
           if total_args_count == 0
             resolved_arguments = GraphQL::Execution::Interpreter::Arguments::EMPTY
-            evaluate_selection_with_args(resolved_arguments, field_defn, ast_node, field_ast_nodes, owner_type, object, is_eager_field, result_name, selections_result, parent_object, return_type, return_type_non_null)
+            if field_defn.extras.size == 0
+              evaluate_selection_with_resolved_keyword_args(
+                NO_ARGS, resolved_arguments, field_defn, ast_node, field_ast_nodes, owner_type, object, is_eager_field, result_name, selections_result, parent_object, return_type, return_type_non_null
+              )
+            else
+              evaluate_selection_with_args(resolved_arguments, field_defn, ast_node, field_ast_nodes, owner_type, object, is_eager_field, result_name, selections_result, parent_object, return_type, return_type_non_null)
+            end
           else
-            # TODO remove all arguments(...) usages?
             @query.arguments_cache.dataload_for(ast_node, field_defn, object) do |resolved_arguments|
               evaluate_selection_with_args(resolved_arguments, field_defn, ast_node, field_ast_nodes, owner_type, object, is_eager_field, result_name, selections_result, parent_object, return_type, return_type_non_null)
             end
@@ -464,9 +469,13 @@ def evaluate_selection_with_args(arguments, field_defn, ast_node, field_ast_node
               next
             end
 
-            kwarg_arguments = if resolved_arguments.empty? && field_defn.extras.empty?
-              # We can avoid allocating the `{ Symbol => Object }` hash in this case
-              NO_ARGS
+            kwarg_arguments = if field_defn.extras.empty?
+              if resolved_arguments.empty?
+                # We can avoid allocating the `{ Symbol => Object }` hash in this case
+                NO_ARGS
+              else
+                resolved_arguments.keyword_arguments
+              end
             else
               # Bundle up the extras, then make a new arguments instance
               # that includes the extras, too.
@@ -505,60 +514,65 @@ def evaluate_selection_with_args(arguments, field_defn, ast_node, field_ast_node
               resolved_arguments.keyword_arguments
             end
 
-            st = get_current_runtime_state
-            st.current_field = field_defn
-            st.current_object = object
-            st.current_arguments = resolved_arguments
-            st.current_result_name = result_name
-            st.current_result = selection_result
-            # Optimize for the case that field is selected only once
-            if field_ast_nodes.nil? || field_ast_nodes.size == 1
-              next_selections = ast_node.selections
-              directives = ast_node.directives
-            else
-              next_selections = []
-              directives = []
-              field_ast_nodes.each { |f|
-                next_selections.concat(f.selections)
-                directives.concat(f.directives)
-              }
-            end
-
-            field_result = call_method_on_directives(:resolve, object, directives) do
-              # Actually call the field resolver and capture the result
-              app_result = begin
-                query.current_trace.execute_field(field: field_defn, ast_node: ast_node, query: query, object: object, arguments: kwarg_arguments) do
-                  field_defn.resolve(object, kwarg_arguments, context)
-                end
-              rescue GraphQL::ExecutionError => err
-                err
-              rescue StandardError => err
-                begin
-                  query.handle_or_reraise(err)
-                rescue GraphQL::ExecutionError => ex_err
-                  ex_err
-                end
+            evaluate_selection_with_resolved_keyword_args(kwarg_arguments, resolved_arguments, field_defn, ast_node, field_ast_nodes, owner_type, object, is_eager_field, result_name, selection_result, parent_object, return_type, return_type_non_null)
+          end
+        end
+
+        def evaluate_selection_with_resolved_keyword_args(kwarg_arguments, resolved_arguments, field_defn, ast_node, field_ast_nodes, owner_type, object, is_eager_field, result_name, selection_result, parent_object, return_type, return_type_non_null)  # rubocop:disable Metrics/ParameterLists
+          st = get_current_runtime_state
+          st.current_field = field_defn
+          st.current_object = object
+          st.current_arguments = resolved_arguments
+          st.current_result_name = result_name
+          st.current_result = selection_result
+          # Optimize for the case that field is selected only once
+          if field_ast_nodes.nil? || field_ast_nodes.size == 1
+            next_selections = ast_node.selections
+            directives = ast_node.directives
+          else
+            next_selections = []
+            directives = []
+            field_ast_nodes.each { |f|
+              next_selections.concat(f.selections)
+              directives.concat(f.directives)
+            }
+          end
+
+          field_result = call_method_on_directives(:resolve, object, directives) do
+            # Actually call the field resolver and capture the result
+            app_result = begin
+              query.current_trace.execute_field(field: field_defn, ast_node: ast_node, query: query, object: object, arguments: kwarg_arguments) do
+                field_defn.resolve(object, kwarg_arguments, context)
               end
-              after_lazy(app_result, owner: owner_type, field: field_defn, ast_node: ast_node, owner_object: object, arguments: resolved_arguments, result_name: result_name, result: selection_result) do |inner_result|
-                continue_value = continue_value(inner_result, owner_type, field_defn, return_type.non_null?, ast_node, result_name, selection_result)
-                if HALT != continue_value
-                  continue_field(continue_value, owner_type, field_defn, return_type, ast_node, next_selections, false, object, resolved_arguments, result_name, selection_result)
-                end
+            rescue GraphQL::ExecutionError => err
+              err
+            rescue StandardError => err
+              begin
+                query.handle_or_reraise(err)
+              rescue GraphQL::ExecutionError => ex_err
+                ex_err
               end
             end
-
-            # If this field is a root mutation field, immediately resolve
-            # all of its child fields before moving on to the next root mutation field.
-            # (Subselections of this mutation will still be resolved level-by-level.)
-            if is_eager_field
-              Interpreter::Resolve.resolve_all([field_result], @dataloader)
-            else
-              # Return this from `after_lazy` because it might be another lazy that needs to be resolved
-              field_result
+            after_lazy(app_result, owner: owner_type, field: field_defn, ast_node: ast_node, owner_object: object, arguments: resolved_arguments, result_name: result_name, result: selection_result) do |inner_result|
+              continue_value = continue_value(inner_result, owner_type, field_defn, return_type_non_null, ast_node, result_name, selection_result)
+              if HALT != continue_value
+                continue_field(continue_value, owner_type, field_defn, return_type, ast_node, next_selections, false, object, resolved_arguments, result_name, selection_result)
+              end
             end
           end
+
+          # If this field is a root mutation field, immediately resolve
+          # all of its child fields before moving on to the next root mutation field.
+          # (Subselections of this mutation will still be resolved level-by-level.)
+          if is_eager_field
+            Interpreter::Resolve.resolve_all([field_result], @dataloader)
+          else
+            # Return this from `after_lazy` because it might be another lazy that needs to be resolved
+            field_result
+          end
         end
 
+
         def dead_result?(selection_result)
           selection_result.graphql_dead  # || ((parent = selection_result.graphql_parent) && parent.graphql_dead)
         end

lib/graphql/filter.rb

@@ -1,8 +1,13 @@
 # frozen_string_literal: true
+require "graphql/deprecation"
+
 module GraphQL
   # @api private
   class Filter
-    def initialize(only: nil, except: nil)
+    def initialize(only: nil, except: nil, silence_deprecation_warning: false)
+      if !silence_deprecation_warning
+        GraphQL::Deprecation.warn("GraphQL::Filter is deprecated and will be removed in v2.1.0. Implement `visible?` on your schema members instead (https://graphql-ruby.org/authorization/visibility.html).")
+      end
       @only = only
       @except = except
     end
@@ -17,7 +22,7 @@ def merge(only: nil, except: nil)
       onlies = [self].concat(Array(only))
       merged_only = MergedOnly.build(onlies)
       merged_except = MergedExcept.build(Array(except))
-      self.class.new(only: merged_only, except: merged_except)
+      self.class.new(only: merged_only, except: merged_except, silence_deprecation_warning: true)
     end
 
     private

lib/graphql/language/document_from_schema_definition.rb

@@ -24,7 +24,7 @@ def initialize(
         @include_built_in_directives = include_built_in_directives
         @include_one_of = false
 
-        filter = GraphQL::Filter.new(only: only, except: except)
+        filter = GraphQL::Filter.new(only: only, except: except, silence_deprecation_warning: true)
         if @schema.respond_to?(:visible?)
           filter = filter.merge(only: @schema.method(:visible?))
         end

lib/graphql/query.rb

@@ -105,7 +105,7 @@ def initialize(schema, query_string = nil, query: nil, document: nil, context: n
       end
 
       if context_tracers.any? && !(schema.trace_class <= GraphQL::Tracing::LegacyTrace)
-        raise ArgumentError, "context[:tracers] and context[:backtrace] are not supported without `tracer_class(GraphQL::Tracing::LegacyTrace)` in the schema configuration, please add it."
+        raise ArgumentError, "context[:tracers] and context[:backtrace] are not supported without `trace_class(GraphQL::Tracing::LegacyTrace)` in the schema configuration, please add it."
       end
 
 

lib/graphql/query/null_context.rb

@@ -24,7 +24,7 @@ def initialize
         @dataloader = GraphQL::Dataloader::NullDataloader.new
         @schema = NullSchema
         @warden = NullWarden.new(
-          GraphQL::Filter.new,
+          GraphQL::Filter.new(silence_deprecation_warning: true),
           context: self,
           schema: @schema,
         )

lib/graphql/schema.rb

@@ -221,7 +221,7 @@ def find(path)
       end
 
       def default_filter
-        GraphQL::Filter.new(except: default_mask)
+        GraphQL::Filter.new(except: default_mask, silence_deprecation_warning: true)
       end
 
       def default_mask(new_mask = nil)

lib/graphql/schema/member/has_fields.rb

@@ -14,42 +14,6 @@ def field(*args, **kwargs, &block)
           field_defn
         end
 
-        # @return [Hash<String => GraphQL::Schema::Field>] Fields on this object, keyed by name, including inherited fields
-        def fields(context = GraphQL::Query::NullContext)
-          warden = Warden.from_context(context)
-          is_object = self.respond_to?(:kind) && self.kind.object?
-          # Local overrides take precedence over inherited fields
-          visible_fields = {}
-          for ancestor in ancestors
-            if ancestor.respond_to?(:own_fields) &&
-                (is_object ? visible_interface_implementation?(ancestor, context, warden) : true)
-
-              ancestor.own_fields.each do |field_name, fields_entry|
-                # Choose the most local definition that passes `.visible?` --
-                # stop checking for fields by name once one has been found.
-                if !visible_fields.key?(field_name) && (f = Warden.visible_entry?(:visible_field?, fields_entry, context, warden))
-                  visible_fields[field_name] = f
-                end
-              end
-            end
-          end
-          visible_fields
-        end
-
-        def get_field(field_name, context = GraphQL::Query::NullContext)
-          warden = Warden.from_context(context)
-          is_object = self.respond_to?(:kind) && self.kind.object?
-          for ancestor in ancestors
-            if ancestor.respond_to?(:own_fields) &&
-                (is_object ? visible_interface_implementation?(ancestor, context, warden) : true) &&
-                (f_entry = ancestor.own_fields[field_name]) &&
-                (f = Warden.visible_entry?(:visible_field?, f_entry, context, warden))
-              return f
-            end
-          end
-          nil
-        end
-
         # A list of Ruby keywords.
         #
         # @api private
@@ -132,6 +96,86 @@ def all_field_definitions
           all_fields
         end
 
+        module InterfaceMethods
+          def get_field(field_name, context = GraphQL::Query::NullContext)
+            warden = Warden.from_context(context)
+            for ancestor in ancestors
+              if ancestor.respond_to?(:own_fields) &&
+                  (f_entry = ancestor.own_fields[field_name]) &&
+                  (f = Warden.visible_entry?(:visible_field?, f_entry, context, warden))
+                return f
+              end
+            end
+            nil
+          end
+
+          # @return [Hash<String => GraphQL::Schema::Field>] Fields on this object, keyed by name, including inherited fields
+          def fields(context = GraphQL::Query::NullContext)
+            warden = Warden.from_context(context)
+            # Local overrides take precedence over inherited fields
+            visible_fields = {}
+            for ancestor in ancestors
+              if ancestor.respond_to?(:own_fields)
+                ancestor.own_fields.each do |field_name, fields_entry|
+                  # Choose the most local definition that passes `.visible?` --
+                  # stop checking for fields by name once one has been found.
+                  if !visible_fields.key?(field_name) && (f = Warden.visible_entry?(:visible_field?, fields_entry, context, warden))
+                    visible_fields[field_name] = f
+                  end
+                end
+              end
+            end
+            visible_fields
+          end
+        end
+
+        module ObjectMethods
+          def get_field(field_name, context = GraphQL::Query::NullContext)
+            # Objects need to check that the interface implementation is visible, too
+            warden = Warden.from_context(context)
+            for ancestor in ancestors
+              if ancestor.respond_to?(:own_fields) &&
+                  visible_interface_implementation?(ancestor, context, warden) &&
+                  (f_entry = ancestor.own_fields[field_name]) &&
+                  (f = Warden.visible_entry?(:visible_field?, f_entry, context, warden))
+                return f
+              end
+            end
+            nil
+          end
+
+          # @return [Hash<String => GraphQL::Schema::Field>] Fields on this object, keyed by name, including inherited fields
+          def fields(context = GraphQL::Query::NullContext)
+            # Objects need to check that the interface implementation is visible, too
+            warden = Warden.from_context(context)
+            # Local overrides take precedence over inherited fields
+            visible_fields = {}
+            for ancestor in ancestors
+              if ancestor.respond_to?(:own_fields) && visible_interface_implementation?(ancestor, context, warden)
+                ancestor.own_fields.each do |field_name, fields_entry|
+                  # Choose the most local definition that passes `.visible?` --
+                  # stop checking for fields by name once one has been found.
+                  if !visible_fields.key?(field_name) && (f = Warden.visible_entry?(:visible_field?, fields_entry, context, warden))
+                    visible_fields[field_name] = f
+                  end
+                end
+              end
+            end
+            visible_fields
+          end
+        end
+
+        def self.included(child_class)
+          # Included in an interface definition methods module
+          child_class.include(InterfaceMethods)
+          super
+        end
+
+        def self.extended(child_class)
+          child_class.extend(ObjectMethods)
+          super
+        end
+
         private
 
         def inherited(subclass)

lib/graphql/static_validation/rules/fields_have_appropriate_selections.rb

@@ -26,11 +26,19 @@ def validate_field_selections(ast_node, resolved_type)
         msg = if resolved_type.nil?
           nil
         elsif resolved_type.kind.scalar? && ast_node.selections.any?
-          if ast_node.selections.first.is_a?(GraphQL::Language::Nodes::InlineFragment)
-            "Selections can't be made on scalars (%{node_name} returns #{resolved_type.graphql_name} but has inline fragments [#{ast_node.selections.map(&:type).map(&:name).join(", ")}])"
-          else
-            "Selections can't be made on scalars (%{node_name} returns #{resolved_type.graphql_name} but has selections [#{ast_node.selections.map(&:name).join(", ")}])"
+          selection_strs = ast_node.selections.map do |n|
+            case n
+            when GraphQL::Language::Nodes::InlineFragment
+              "\"... on #{n.type.name} { ... }\""
+            when GraphQL::Language::Nodes::Field
+              "\"#{n.name}\""
+            when GraphQL::Language::Nodes::FragmentSpread
+              "\"#{n.name}\""
+            else
+              raise "Invariant: unexpected selection node: #{n}"
+            end
           end
+          "Selections can't be made on scalars (%{node_name} returns #{resolved_type.graphql_name} but has selections [#{selection_strs.join(", ")}])"
         elsif resolved_type.kind.fields? && ast_node.selections.empty?
           "Field must have selections (%{node_name} returns #{resolved_type.graphql_name} but has no selections. Did you mean '#{ast_node.name} { ... }'?)"
         else