Implement ability to skip over list items if their field resolution raises an error #6

amomchilov · 2023-05-31T21:11:38Z

What

A working proposal for implementing rmosolgo#4496

This PR adds new skip_nodes_on_raise and on_raise arguments to the field API which lets you declare that a List's items can be skipped on error:

graphql-ruby/spec/graphql/execution/interpreter/list_item_skipping_spec.rb

Lines 215 to 223 in d50e635

    
           field :array_of_nullable_skippable_items, 
        
                 [ItemType, null: true], 
        
                 method: :items_list, 
        
                 null: false, 
        
                 skip_nodes_on_raise: true, 
        
                 on_raise: ->(err, current_object, args, context, current_field, nearest_skippable_list_item) { 
        
                   puts(:array_of_required_skippable_items) 
        
                   context.skip_from_parent_list 
        
                 }

Other

Two of the test suites fail, not sure why, the failures don't seem related to my changes.

I made the target dev branch just as a placeholder for this PR.

juanxo · 2023-06-01T16:19:06Z

lib/graphql/execution/interpreter/runtime.rb

+                end
+
+                selection_result.graphql_skip_at(result_name)
+              else # Propograte up to find the first list this item can be skipped from.


Typo in Propograte

sean-dang · 2023-06-01T20:29:19Z

lib/graphql/execution/interpreter/runtime.rb

@@ -566,6 +609,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


Where is value set to GraphQL::Execution::SKIP_FROM_PARENT_LIST?

Oh, I see in your example the callback sets it. I was expecting the library to do this automatically since the user has already opted into this functionality. In any case it's not super clear the callback is supposed to do this.

Yeah you're right, perhaps it should be the default in the case of skip_nodes_on_raise: true, on_raise: nil?

Yeah, I think it should be the default. It might be good to make it the behavior even if on_raise is set. In the callback we can perform some logging and let the section be skipped by default. Only in rare cases would we override the skip behavior in the callback.

sean-dang · 2023-06-01T20:29:42Z

lib/graphql/execution/interpreter/runtime.rb

+                  if nearest_skippable_parent_result && (on_raise_callback = nearest_skippable_parent_result.field.on_raise_callback)
+                    begin
+                      nearest_skippable_list_item = nearest_skippable_parent_result.represented_value.object
+                      on_raise_callback.call(err, object, kwarg_arguments, context, nearest_skippable_parent_result.field, nearest_skippable_list_item)


Is the callback expected to return anything?

Yep! It has the same behaviour as rescue_from (which is what ends up being called by query.handle_or_reraise(err) a few lines below).

I list the possibilities in the "Extend the existing schema-level rescue_from callback" section of the parent issue.

In addition to those preexisting options, you now also have the choice to return context.skip_from_parent_list which will trigger the new skipping behaviour.

Ah, I should have read the "supplementary materials" more closely :)

sean-dang · 2023-06-01T20:30:42Z

lib/graphql/schema/field.rb

+      # @param fallback_value [Object] A fallback value if the method is not defined
+      # @param skip_nodes_on_raise [Boolean] true if this field's list items should be skipped, if resolving them led to an error being raised. Only applicable to List types.
+      # @param on_raise [Proc] A callback that's invoked when an error is raised while resolving this field's list items. Only applicable to List types.
+      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, on_raise: nil, &definition_block)


Nit: can we put each param on its own line for readability and cleaner diffs?

Not a nit, the git conflicts on this line are pretty painful 😅

cc @rmosolgo

amomchilov force-pushed the Alex/item_skipping/per-field-callback-API branch 3 times, most recently from 6015655 to 8b0a52d Compare May 31, 2023 22:39

amomchilov mentioned this pull request May 31, 2023

WIP: Skip over list items that raise errors amomchilov/graphql-ruby#1

Closed

amomchilov force-pushed the Alex/item_skipping/per-field-callback-API branch 4 times, most recently from 0abb3b0 to d50e635 Compare June 1, 2023 13:45

amomchilov marked this pull request as ready for review June 1, 2023 13:59

amomchilov self-assigned this Jun 1, 2023

amomchilov requested review from tgwizard, tgturner-shopify, tednology, sean-dang and swalkinshaw June 1, 2023 14:00

amomchilov mentioned this pull request Jun 1, 2023

Add a way to skip items of a list when resolving their fields raises an error. rmosolgo/graphql-ruby#4496

Closed

juanxo reviewed Jun 1, 2023

View reviewed changes

sean-dang reviewed Jun 1, 2023

View reviewed changes

amomchilov force-pushed the dev branch from f88ec1f to 71bc874 Compare June 15, 2023 15:25

amomchilov force-pushed the Alex/item_skipping/per-field-callback-API branch from d50e635 to 6bb41e8 Compare June 15, 2023 19:32

amomchilov added 4 commits June 16, 2023 11:43

Implement ability to skip list items on raise

9a21dc9

Store represented_value in the result objects

d147ee4

Store field in the result objects

77cf72f

Implement ability to skip list items on raise

f25db79

amomchilov force-pushed the Alex/item_skipping/per-field-callback-API branch from 6bb41e8 to f25db79 Compare June 16, 2023 15:44

tgwizard removed their request for review November 7, 2023 12:59

tgturner-shopify removed their request for review March 7, 2024 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ability to skip over list items if their field resolution raises an error #6

Implement ability to skip over list items if their field resolution raises an error #6

amomchilov commented May 31, 2023 •

edited

Loading

juanxo Jun 1, 2023

sean-dang Jun 1, 2023

sean-dang Jun 1, 2023

amomchilov Jun 1, 2023 •

edited

Loading

sean-dang Jun 1, 2023

sean-dang Jun 1, 2023

amomchilov Jun 1, 2023 •

edited

Loading

sean-dang Jun 1, 2023

sean-dang Jun 1, 2023

amomchilov Jun 1, 2023

	field :array_of_nullable_skippable_items,
	[ItemType, null: true],
	method: :items_list,
	null: false,
	skip_nodes_on_raise: true,
	on_raise: ->(err, current_object, args, context, current_field, nearest_skippable_list_item) {
	puts(:array_of_required_skippable_items)
	context.skip_from_parent_list
	}

Implement ability to skip over list items if their field resolution raises an error #6

Are you sure you want to change the base?

Implement ability to skip over list items if their field resolution raises an error #6

Conversation

amomchilov commented May 31, 2023 • edited Loading

What

Other

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov commented May 31, 2023 •

edited

Loading

amomchilov Jun 1, 2023 •

edited

Loading

amomchilov Jun 1, 2023 •

edited

Loading