WIP: Skip over list items that raise errors #1
Conversation
amomchilov
force-pushed
the
Alex/skip-list-items-wip
branch
2 times, most recently
from
2e3636e to
3ff5afa
Compare
| # @return [nil, true] | ||
| attr_accessor :graphql_skip_list_items_that_raise | ||
|
|
||
| def has_graphql_graph_parent_that_skips_list_items_that_raise |
There was a problem hiding this comment.
TODO: comically long name lol
| # @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) |
There was a problem hiding this comment.
I used a default value here, because there's a fair number of callsites the way Addition#update_type_owner calls this reflectively can't pass an extra parameter (without changing it a fair bit)
graphql-ruby/lib/graphql/schema/addition.rb
Line 135 in f88ec1f
| 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 |
| rescue GraphQL::ExecutionError => ex_err | ||
| ex_err | ||
| if selection_result.has_graphql_graph_parent_that_skips_list_items_that_raise | ||
| nil |
There was a problem hiding this comment.
The existing nil-propagation mechanism works to ... propagate nil. I'm piggy-backing off it here. Should I propagate the raised error instead of nil?
If so, how do I identify that, down the line?
| query.handle_or_reraise(err) | ||
| rescue GraphQL::ExecutionError => ex_err | ||
| ex_err | ||
| if selection_result.has_graphql_graph_parent_that_skips_list_items_that_raise |
There was a problem hiding this comment.
API design question: before skipping an item, should we called the users rescue_from callback? (or perhaps make a new callback specific to item skipping?)
This if statement skips it for now, and silently skips the item. This isn't ideal, because the service owner wouldn't know items are being skipped. (e.g. if rescue_from reports to a bug tracker, it won't be called).
| def set_result(selection_result, result_name, value) | ||
| if !dead_result?(selection_result) | ||
| if value.nil? && | ||
| if value == GraphQL::Execution::SKIP |
There was a problem hiding this comment.
TODO: this is kinda bodged in. Rework.
| if selection_result.has_graphql_graph_parent_that_skips_list_items_that_raise | ||
| # This writes the `nil` in, we need to patch it to skip instead. | ||
| print("skip!") | ||
| set_result(selection_result, result_name, GraphQL::Execution::SKIP) |
There was a problem hiding this comment.
Fixme: this is abusing GraphQL::Execution::SKIP as a flag parameter/sentinel
| 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) |
There was a problem hiding this comment.
FIXME: how should this work?
| else | ||
| true | ||
| end | ||
| @skip_nodes_on_raise = skip_nodes_on_raise |
There was a problem hiding this comment.
This is just a hack to make this value avaiable in #type.
Should this be encoded in the @return_type_expr somehow?
lib/graphql/schema/non_null.rb
Outdated
| def to_s = inspect | ||
|
|
There was a problem hiding this comment.
For the debugger only; remove
amomchilov
force-pushed
the
Alex/skip-list-items-wip
branch
4 times, most recently
from
e904ef5 to
b2d9e9d
Compare
amomchilov
force-pushed
the
Alex/skip-list-items-wip
branch
from
b2d9e9d to
6315fcb
Compare
|
Opened a PR to our own fork of the gem: Shopify#6 |
No description provided.