Implement ability to skip list items on raise

amomchilov · amomchilov · commit 6315fcb45b87 · 2023-05-31T16:32:59.000-04:00
diff --git a/lib/graphql/execution.rb b/lib/graphql/execution.rb
@@ -14,5 +14,12 @@ class Skip < GraphQL::Error; end
     # Just a singleton for implementing {Query::Context#skip}
     # @api private
     SKIP = Skip.new
+
+    # @api private
+    class SkipFromParentList < GraphQL::Error; end
+
+    # Just a singleton for implementing {Query::Context#skip}
+    # @api private
+    SKIP_FROM_PARENT_LIST = SkipFromParentList.new
   end
 end
diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb
@@ -30,6 +30,18 @@ def initialize(result_name, parent_result)
           attr_accessor :graphql_non_null_field_names
           # @return [nil, true]
           attr_accessor :graphql_non_null_list_items
+          # True when this result is a List that was marked `skip_items_on_raise: true`.
+          # Descendants of this result will have `can_be_skipped?` be true.
+          # @return [nil, true]
+          attr_accessor :graphql_skip_list_items_that_raise
+
+          # A result can be skipped when it's an ancestor of an item in a List marked `skip_items_on_raise: true`.
+          # @return [Boolean]
+          def can_be_skipped?
+            return @can_be_skipped if defined?(@can_be_skipped)
+
+            @can_be_skipped = graphql_skip_list_items_that_raise || !!(graphql_parent && graphql_parent.can_be_skipped?)
+          end
 
           # @return [Hash] Plain-Ruby result data (`@graphql_metadata` contains Result wrapper objects)
           attr_accessor :graphql_result_data
@@ -513,10 +525,14 @@ def evaluate_selection_with_args(arguments, field_defn, next_path, ast_node, fie
               rescue GraphQL::ExecutionError => err
                 err
               rescue StandardError => err
-                begin
-                  query.handle_or_reraise(err)
-                rescue GraphQL::ExecutionError => ex_err
-                  ex_err
+                if selection_result.can_be_skipped?
+                  context.skip_from_parent_list # Skip silently without informing the user's `rescue_from` block.
+                else
+                  begin
+                    query.handle_or_reraise(err)
+                  rescue GraphQL::ExecutionError => ex_err
+                    ex_err
+                  end
                 end
               end
               after_lazy(app_result, owner: owner_type, field: field_defn, path: next_path, ast_node: ast_node, owner_object: object, arguments: resolved_arguments, result_name: result_name, result: selection_result) do |inner_result|
@@ -566,6 +582,19 @@ def set_result(selection_result, result_name, value)
                 set_result(parent, name_in_parent, nil)
                 set_graphql_dead(selection_result)
               end
+            elsif value == GraphQL::Execution::SKIP_FROM_PARENT_LIST
+              if selection_result.graphql_skip_list_items_that_raise # This is the first list that this item can be skipped from.
+                unless selection_result.is_a?(GraphQLResultArray)
+                  raise "Invariant: unexpected result class #{selection_result.class} (#{selection_result.inspect})"
+                end
+
+                selection_result.graphql_skip_at(result_name)
+              else # Propograte up to find the first list this item can be skipped from.
+                parent = selection_result.graphql_parent
+                name_in_parent = selection_result.graphql_result_name
+                set_result(parent, name_in_parent, GraphQL::Execution::SKIP_FROM_PARENT_LIST)
+                set_graphql_dead(selection_result)
+              end
             else
               selection_result[result_name] = value
             end
@@ -647,6 +676,12 @@ def continue_value(path, value, parent_type, field, is_non_null, ast_node, resul
                 raise "Invariant: unexpected result class #{selection_result.class} (#{selection_result.inspect})"
               end
               HALT
+            elsif GraphQL::Execution::SKIP_FROM_PARENT_LIST == value
+              unless selection_result.can_be_skipped?
+                raise "Cannot skip list items from lists not marked `skip_items_on_raise: true`"
+              end
+
+              set_result(selection_result, result_name, GraphQL::Execution::SKIP_FROM_PARENT_LIST)
             else
               # What could this actually _be_? Anyhow,
               # preserve the default behavior of doing nothing with it.
@@ -780,6 +815,7 @@ def continue_field(path, value, owner_type, field, current_type, ast_node, next_
             use_dataloader_job = !inner_type.unwrap.kind.input?
             response_list = GraphQLResultArray.new(result_name, selection_result)
             response_list.graphql_non_null_list_items = inner_type.non_null?
+            response_list.graphql_skip_list_items_that_raise = current_type.skip_nodes_on_raise?
             set_result(selection_result, result_name, response_list)
             result_was_set = false
             idx = 0
diff --git a/lib/graphql/query/context.rb b/lib/graphql/query/context.rb
@@ -11,6 +11,15 @@ def skip
           GraphQL::Execution::SKIP
         end
 
+        # Return this value to tell the runtime
+        # to exclude this whole object from parent list.
+        #
+        # The runtime will find the find the nearest parent list marked `skip_items_on_raise: true`,
+        # and exclude the entire list item (including this object).
+        def skip_from_parent_list
+          GraphQL::Execution::SKIP_FROM_PARENT_LIST
+        end
+
         # Add error at query-level.
         # @param error [GraphQL::ExecutionError] an execution error
         # @return [void]
diff --git a/lib/graphql/schema/build_from_definition.rb b/lib/graphql/schema/build_from_definition.rb
@@ -466,7 +466,7 @@ def build_resolve_type(lookup_hash, directives, missing_type_handler)
             when GraphQL::Language::Nodes::NonNullType
               resolve_type_proc.call(ast_node.of_type).to_non_null_type
             when GraphQL::Language::Nodes::ListType
-              resolve_type_proc.call(ast_node.of_type).to_list_type
+              resolve_type_proc.call(ast_node.of_type).to_list_type(skip_nodes_on_raise: false)
             when String
               directives[ast_node]
             else
diff --git a/lib/graphql/schema/field.rb b/lib/graphql/schema/field.rb
@@ -219,7 +219,7 @@ def method_conflict_warning?
       # @param method_conflict_warning [Boolean] If false, skip the warning if this field's method conflicts with a built-in method
       # @param validates [Array<Hash>] Configurations for validating this field
       # @fallback_value [Object] A fallback value if the method is not defined
-      def initialize(type: nil, name: nil, owner: nil, null: nil, description: :not_given, deprecation_reason: nil, method: nil, hash_key: nil, dig: nil, resolver_method: nil, connection: nil, max_page_size: :not_given, default_page_size: :not_given, scope: nil, introspection: false, camelize: true, trace: nil, complexity: nil, ast_node: nil, extras: EMPTY_ARRAY, extensions: EMPTY_ARRAY, connection_extension: self.class.connection_extension, resolver_class: nil, subscription_scope: nil, relay_node_field: false, relay_nodes_field: false, method_conflict_warning: true, broadcastable: nil, arguments: EMPTY_HASH, directives: EMPTY_HASH, validates: EMPTY_ARRAY, fallback_value: :not_given, &definition_block)
+      def initialize(type: nil, name: nil, owner: nil, null: nil, description: :not_given, deprecation_reason: nil, method: nil, hash_key: nil, dig: nil, resolver_method: nil, connection: nil, max_page_size: :not_given, default_page_size: :not_given, scope: nil, introspection: false, camelize: true, trace: nil, complexity: nil, ast_node: nil, extras: EMPTY_ARRAY, extensions: EMPTY_ARRAY, connection_extension: self.class.connection_extension, resolver_class: nil, subscription_scope: nil, relay_node_field: false, relay_nodes_field: false, method_conflict_warning: true, broadcastable: nil, arguments: EMPTY_HASH, directives: EMPTY_HASH, validates: EMPTY_ARRAY, fallback_value: :not_given, skip_nodes_on_raise: false, &definition_block)
         if name.nil?
           raise ArgumentError, "missing first `name` argument or keyword `name:`"
         end
@@ -271,6 +271,7 @@ def initialize(type: nil, name: nil, owner: nil, null: nil, description: :not_gi
         else
           true
         end
+        @skip_nodes_on_raise = skip_nodes_on_raise
         @connection = connection
         @has_max_page_size = max_page_size != :not_given
         @max_page_size = max_page_size == :not_given ? nil : max_page_size
@@ -349,6 +350,11 @@ def initialize(type: nil, name: nil, owner: nil, null: nil, description: :not_gi
 
         self.extensions.each(&:after_define_apply)
         @call_after_define = true
+
+        # TODO: should this check be centralized in `GraphQL::Schema::Member::BuildType#parse_type` instead?
+        if skip_nodes_on_raise && !self.type.list?
+          raise ArgumentError, "The `skip_nodes_on_raise` option is only applicable to lists."
+        end
       end
 
       # If true, subscription updates with this field can be shared between viewers
@@ -572,9 +578,10 @@ def type
             raise MissingReturnTypeError, "Can't determine the return type for #{self.path} (it has `resolver: #{@resolver_class}`, perhaps that class is missing a `type ...` declaration, or perhaps its type causes a cyclical loading issue)"
           end
           nullable = @return_type_null.nil? ? @resolver_class.null : @return_type_null
-          Member::BuildType.parse_type(return_type, null: nullable)
+          skip_nodes_on_raise = @skip_nodes_on_raise.nil? ? @resolver_class.skip_nodes_on_raise : @skip_nodes_on_raise
+          Member::BuildType.parse_type(return_type, null: nullable, skip_nodes_on_raise: skip_nodes_on_raise)
         else
-          @type ||= Member::BuildType.parse_type(@return_type_expr, null: @return_type_null)
+          @type ||= Member::BuildType.parse_type(@return_type_expr, null: @return_type_null, skip_nodes_on_raise: @skip_nodes_on_raise)
         end
       rescue GraphQL::Schema::InvalidDocumentError, MissingReturnTypeError => err
         # Let this propagate up
diff --git a/lib/graphql/schema/late_bound_type.rb b/lib/graphql/schema/late_bound_type.rb
@@ -21,8 +21,13 @@ def to_non_null_type
         @to_non_null_type ||= GraphQL::Schema::NonNull.new(self)
       end
 
-      def to_list_type
-        @to_list_type ||= GraphQL::Schema::List.new(self)
+      # can we just inherit this from graphql/schema/member/type_system_helpers.rb?
+      def to_list_type(skip_nodes_on_raise: false)
+        if skip_nodes_on_raise
+          @to_skipping_list_type ||= GraphQL::Schema::List.new(self, skip_nodes_on_raise: true)
+        else
+          @to_list_type ||= GraphQL::Schema::List.new(self)
+        end
       end
 
       def inspect
diff --git a/lib/graphql/schema/list.rb b/lib/graphql/schema/list.rb
@@ -8,6 +8,11 @@ class Schema
     class List < GraphQL::Schema::Wrapper
       include Schema::Member::ValidatesInput
 
+      def initialize(of_type, skip_nodes_on_raise: false)
+        super(of_type)
+        @skip_nodes_on_raise = skip_nodes_on_raise
+      end
+
       # @return [GraphQL::TypeKinds::LIST]
       def kind
         GraphQL::TypeKinds::LIST
@@ -18,6 +23,10 @@ def list?
         true
       end
 
+      def skip_nodes_on_raise?
+        @skip_nodes_on_raise
+      end
+
       def to_type_signature
         "[#{@of_type.to_type_signature}]"
       end
diff --git a/lib/graphql/schema/member/build_type.rb b/lib/graphql/schema/member/build_type.rb
@@ -9,7 +9,7 @@ module BuildType
         module_function
         # @param type_expr [String, Class, GraphQL::BaseType]
         # @return [GraphQL::BaseType]
-        def parse_type(type_expr, null:)
+        def parse_type(type_expr, null:, skip_nodes_on_raise: false)
           list_type = false
 
           return_type = case type_expr
@@ -85,7 +85,7 @@ def parse_type(type_expr, null:)
           # Apply list_type first, that way the
           # .to_non_null_type applies to the list type, not the inner type
           if list_type
-            return_type = return_type.to_list_type
+            return_type = return_type.to_list_type(skip_nodes_on_raise: skip_nodes_on_raise)
           end
 
           if !null
diff --git a/lib/graphql/schema/member/type_system_helpers.rb b/lib/graphql/schema/member/type_system_helpers.rb
@@ -17,15 +17,24 @@ def to_non_null_type
         end
 
         # @return [Schema::List] Make a list-type representation of this type
-        def to_list_type
-          @to_list_type ||= GraphQL::Schema::List.new(self)
+        def to_list_type(skip_nodes_on_raise: false)
+          if skip_nodes_on_raise
+            @to_skipping_list_type ||= GraphQL::Schema::List.new(self, skip_nodes_on_raise: true)
+          else
+            @to_list_type ||= GraphQL::Schema::List.new(self)
+          end
         end
 
         # @return [Boolean] true if this is a non-nullable type. A nullable list of non-nullables is considered nullable.
         def non_null?
           false
         end
 
+        # @return [Boolean] true if this field's nodes should be skipped, if resolving them led to an error being raised.
+        def skip_nodes_on_raise?
+          false
+        end
+
         # @return [Boolean] true if this is a list type. A non-nullable list is considered a list.
         def list?
           false
diff --git a/lib/graphql/schema/non_null.rb b/lib/graphql/schema/non_null.rb
@@ -18,6 +18,11 @@ def non_null?
         true
       end
 
+      # @return [Boolean] true if this field's nodes should be skipped, if resolving them led to an error being raised.
+      def skip_nodes_on_raise?
+        @of_type.skip_nodes_on_raise?
+      end
+
       # @return [Boolean] True if this type wraps a list type
       def list?
         @of_type.list?
diff --git a/lib/graphql/schema/resolver.rb b/lib/graphql/schema/resolver.rb
@@ -252,6 +252,9 @@ def null(allow_null = nil)
           @null.nil? ? (superclass.respond_to?(:null) ? superclass.null : true) : @null
         end
 
+        # @return [Boolean]
+        attr_reader :skip_nodes_on_raise
+
         def resolver_method(new_method_name = nil)
           if new_method_name
             @resolver_method = new_method_name
@@ -266,17 +269,19 @@ def resolver_method(new_method_name = nil)
         # TODO unify with {#null}
         # @param new_type [Class, Array<Class>, nil] If a type definition class is provided, it will be used as the return type of the field
         # @param null [true, false] Whether or not the field may return `nil`
+        # @param null [true, false] Whether or not the field may skip list items that raise errors. Only applicable to lists.
         # @return [Class] The type which this field returns.
-        def type(new_type = nil, null: nil)
+        def type(new_type = nil, null: nil, skip_nodes_on_raise: false)
           if new_type
             if null.nil?
               raise ArgumentError, "required argument `null:` is missing"
             end
             @type_expr = new_type
             @null = null
+            @skip_nodes_on_raise = skip_nodes_on_raise
           else
             if type_expr
-              GraphQL::Schema::Member::BuildType.parse_type(type_expr, null: self.null)
+              GraphQL::Schema::Member::BuildType.parse_type(type_expr, null: self.null, skip_nodes_on_raise: self.skip_nodes_on_raise)
             elsif superclass.respond_to?(:type)
               superclass.type
             else
diff --git a/lib/graphql/types/relay/connection_behaviors.rb b/lib/graphql/types/relay/connection_behaviors.rb
@@ -13,6 +13,7 @@ def self.included(child_class)
           child_class.default_relay(true)
           child_class.has_nodes_field(true)
           child_class.node_nullable(true)
+          child_class.skip_nodes_on_raise(false)
           child_class.edges_nullable(true)
           child_class.edge_nullable(true)
           add_page_info_field(child_class)
@@ -36,7 +37,7 @@ module ClassMethods
           # class name to set defaults. You can call it again in the class definition
           # to override the default (or provide a value, if the default lookup failed).
           # @param field_options [Hash] Any extra keyword arguments to pass to the `field :edges, ...` and `field :nodes, ...` configurations
-          def edge_type(edge_type_class, edge_class: GraphQL::Pagination::Connection::Edge, node_type: edge_type_class.node_type, nodes_field: self.has_nodes_field, node_nullable: self.node_nullable, edges_nullable: self.edges_nullable, edge_nullable: self.edge_nullable, field_options: nil)
+          def edge_type(edge_type_class, edge_class: GraphQL::Pagination::Connection::Edge, node_type: edge_type_class.node_type, nodes_field: self.has_nodes_field, node_nullable: self.node_nullable, skip_nodes_on_raise: self.skip_nodes_on_raise, edges_nullable: self.edges_nullable, edge_nullable: self.edge_nullable, field_options: nil)
             # Set this connection's graphql name
             node_type_name = node_type.graphql_name
 
@@ -60,7 +61,7 @@ def edge_type(edge_type_class, edge_class: GraphQL::Pagination::Connection::Edge
 
             field(**base_field_options)
 
-            define_nodes_field(node_nullable, field_options: field_options) if nodes_field
+            define_nodes_field(node_nullable, skip_nodes_on_raise, field_options: field_options) if nodes_field
 
             description("The connection type for #{node_type_name}.")
           end
@@ -71,8 +72,8 @@ def scope_items(items, context)
           end
 
           # Add the shortcut `nodes` field to this connection and its subclasses
-          def nodes_field(node_nullable: self.node_nullable, field_options: nil)
-            define_nodes_field(node_nullable, field_options: field_options)
+          def nodes_field(node_nullable: self.node_nullable, skip_nodes_on_raise: self.skip_nodes_on_raise, field_options: nil)
+            define_nodes_field(node_nullable, skip_nodes_on_raise, field_options: field_options)
           end
 
           def authorized?(obj, ctx)
@@ -94,6 +95,14 @@ def node_nullable(new_value = nil)
             end
           end
 
+          def skip_nodes_on_raise(new_value = nil)
+            if new_value.nil?
+              defined?(@skip_nodes_on_raise) ? @skip_nodes_on_raise : superclass.skip_nodes_on_raise
+            else
+              @skip_nodes_on_raise = new_value
+            end
+          end
+
           # Set the default `edges_nullable` for this class and its child classes. (Defaults to `true`.)
           # Use `edges_nullable(false)` in your base class to make non-null `edges` fields.
           def edges_nullable(new_value = nil)
@@ -126,7 +135,7 @@ def has_nodes_field(new_value = nil)
 
           private
 
-          def define_nodes_field(nullable, field_options: nil)
+          def define_nodes_field(nullable, skip_nodes_on_raise, field_options: nil)
             base_field_options = {
               name: :nodes,
               type: [@node_type, null: nullable],
@@ -135,6 +144,7 @@ def define_nodes_field(nullable, field_options: nil)
               connection: false,
               # Assume that the connection was scoped before this step:
               scope: false,
+              skip_nodes_on_raise: skip_nodes_on_raise,
             }
             if field_options
               base_field_options.merge!(field_options)
diff --git a/spec/graphql/execution/interpreter/list_item_skipping_spec.rb b/spec/graphql/execution/interpreter/list_item_skipping_spec.rb
