From f701485e4350aee8b03a3ece247bda576ece3d2b Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Mon, 2 Sep 2019 07:13:17 -0400 Subject: [PATCH 1/2] Add error handling for the interpreter --- guides/errors/error_handling.md | 66 ++++++++++++++ guides/errors/overview.md | 4 + lib/graphql/execution.rb | 1 + lib/graphql/execution/errors.rb | 57 ++++++++++++ lib/graphql/execution/interpreter/runtime.rb | 38 ++++---- spec/graphql/execution/errors_spec.rb | 93 ++++++++++++++++++++ 6 files changed, 242 insertions(+), 17 deletions(-) create mode 100644 guides/errors/error_handling.md create mode 100644 lib/graphql/execution/errors.rb create mode 100644 spec/graphql/execution/errors_spec.rb diff --git a/guides/errors/error_handling.md b/guides/errors/error_handling.md new file mode 100644 index 0000000000..d3912cdcb8 --- /dev/null +++ b/guides/errors/error_handling.md @@ -0,0 +1,66 @@ +--- +layout: guide +doc_stub: false +search: true +section: Errors +title: Error Handling +desc: Rescuing application errors from field resolvers +index: 3 +--- + +You can configure your schema to rescue application errors during field resolution. Errors during batch loading will also be rescued. + +__Note:__ This feature is for class-based schemas using the {% internal_link "interpreter runtime", "/queries/interpreter" %} only. For `.define`-based schemas, use [exAspArk/graphql-errors](https://github.com/exaspark/graphql-errors) instead. + +Thanks to [`@exAspArk`] for the [`graphql-errors`](https://github.com/exAspArk/graphql-errors) gem which inspired this behavior and [`@thiago-sydow`](https://github.com/thiago-sydow) who [suggested](https://github.com/rmosolgo/graphql-ruby/issues/2139#issuecomment-524913594) and implementation like this. + +## Setup + +Add error handling to your schema with `use GraphQL::Execution::Errors`. (This will be the default in a future graphql-ruby version.) + +```ruby +class MySchema < GraphQL::Schema + # Use the new runtime & analyzers: + use GraphQL::Execution::Interpreter + use GraphQL::Analysis::AST + # Also use the new error handling: + use GraphQL::Execution::Errors +end +``` + +## Add error handlers + +Handlers are added with `rescue_from` configurations in the schema: + +```ruby +class MySchema < GraphQL::Schema + # ... + + rescue_from(ActiveRecord::NotFound) do |err, obj, args, ctx, field| + # Raise a graphql-friendly error with a custom message + raise GraphQL::ExecutionError, "#{field.type.unwrap.graphql_name} not found" + end + + rescue_from(SearchIndex::UnavailableError) do |err, obj, args, ctx, field| + # Log the error + Bugsnag.notify(err) + # replace it with nil + nil + end +end +``` + +The handler is called with several arguments: + +- __`err`__ is the error that was raised during field execution, then rescued +- __`obj`__ is the object which was having a field resolved against it +- __`args`__ is the the Hash of arguments passed to the resolver +- __`ctx`__ is the query context +- __`field`__ is the {{ "GraphQL::Schema::Field" | api_doc }} instance for the field where the error was rescued + +Inside the handler, you can: + +- Raise a GraphQL-friendly {{ "GraphQL::ExecutionError" | api_doc }} to return to the user +- Re-raise the given `err` to crash the query and halt execution. (The error will propagate to your application, eg, the controller.) +- Report some metrics from the error, if applicable +- Return a new value to be used for the error case (if not raising another error) diff --git a/guides/errors/overview.md b/guides/errors/overview.md index 94cd33535f..67b58f8b25 100644 --- a/guides/errors/overview.md +++ b/guides/errors/overview.md @@ -46,6 +46,10 @@ The GraphQL specification provides for a top-level `"errors"` key which may incl In your own schema, you can add to the `"errors"` key by raising `GraphQL::ExecutionError` (or subclasses of it) in your code. Read more in the {% internal_link "Execution Errors guide", "/errors/execution_errors" %}. +## Handled Errors + +A schema can be configured to handle certain errors during field execution with handlers that you give it, using `rescue_from`. Read more in the {% internal_link "Error Handling guide", "/errors/error_handling" %}. + ## Unhandled Errors (Crashes) When a `raise`d error is not `rescue`d, the GraphQL query crashes entirely and the surrounding code (like a Rails controller) must handle the exception. diff --git a/lib/graphql/execution.rb b/lib/graphql/execution.rb index 711f18e8a6..b00befd841 100644 --- a/lib/graphql/execution.rb +++ b/lib/graphql/execution.rb @@ -8,3 +8,4 @@ require "graphql/execution/lookahead" require "graphql/execution/multiplex" require "graphql/execution/typecast" +require "graphql/execution/errors" diff --git a/lib/graphql/execution/errors.rb b/lib/graphql/execution/errors.rb new file mode 100644 index 0000000000..f2ae6203df --- /dev/null +++ b/lib/graphql/execution/errors.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module GraphQL + module Execution + # A tracer that wraps query execution with error handling. + # Supports class-based schemas and the new {Interpreter} runtime only. + # + # @example Handling ActiveRecord::NotFound + # + # class MySchema < GraphQL::Schema + # use GraphQL::Execution::Errors + # + # rescue_from(ActiveRecord::NotFound) do |err, obj, args, ctx, field| + # ErrorTracker.log("Not Found: #{err.message}") + # nil + # end + # end + # + class Errors + def self.use(schema) + schema_class = schema.is_a?(Class) ? schema : schema.target.class + schema.tracer(self.new(schema_class)) + end + + def initialize(schema) + @schema = schema + end + + def trace(event, data) + case event + when "execute_field", "execute_field_lazy" + with_error_handling(data) { yield } + else + yield + end + end + + private + + def with_error_handling(trace_data) + yield + rescue StandardError => err + rescues = @schema.rescues + _err_class, handler = rescues.find { |err_class, handler| err.is_a?(err_class) } + if handler + obj = trace_data[:object] + args = trace_data[:arguments] + ctx = trace_data[:query].context + field = trace_data[:field] + handler.call(err, obj, args, ctx, field) + else + raise err + end + end + end + end +end diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index 0bf3fbbf13..bbcee72417 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -205,13 +205,17 @@ def evaluate_selections(path, owner_object, owner_type, selections, root_operati field_result = resolve_with_directives(object, ast_node) do # Actually call the field resolver and capture the result - app_result = query.trace("execute_field", {owner: owner_type, field: field_defn, path: next_path, query: query}) do - field_defn.resolve(object, kwarg_arguments, context) + app_result = begin + query.trace("execute_field", {owner: owner_type, field: field_defn, path: next_path, query: query, object: object, arguments: kwarg_arguments}) do + field_defn.resolve(object, kwarg_arguments, context) + end + rescue GraphQL::ExecutionError => err + err end - after_lazy(app_result, owner: owner_type, field: field_defn, path: next_path) do |inner_result| + after_lazy(app_result, owner: owner_type, field: field_defn, path: next_path, owner_object: object, arguments: kwarg_arguments) do |inner_result| continue_value = continue_value(next_path, inner_result, field_defn, return_type.non_null?, ast_node) if HALT != continue_value - continue_field(next_path, continue_value, field_defn, return_type, ast_node, next_selections, false) + continue_field(next_path, continue_value, field_defn, return_type, ast_node, next_selections, false, object, kwarg_arguments) end end end @@ -274,7 +278,7 @@ def continue_value(path, value, field, is_non_null, ast_node) # Location information from `path` and `ast_node`. # # @return [Lazy, Array, Hash, Object] Lazy, Array, and Hash are all traversed to resolve lazy values later - def continue_field(path, value, field, type, ast_node, next_selections, is_non_null) + def continue_field(path, value, field, type, ast_node, next_selections, is_non_null, owner_object, arguments) # rubocop:disable Metrics/ParameterLists case type.kind.name when "SCALAR", "ENUM" r = type.coerce_result(value, context) @@ -282,7 +286,7 @@ def continue_field(path, value, field, type, ast_node, next_selections, is_non_n r when "UNION", "INTERFACE" resolved_type_or_lazy = query.resolve_type(type, value) - after_lazy(resolved_type_or_lazy, owner: type, path: path, field: field) do |resolved_type| + after_lazy(resolved_type_or_lazy, owner: type, path: path, field: field, owner_object: owner_object, arguments: arguments) do |resolved_type| possible_types = query.possible_types(type) if !possible_types.include?(resolved_type) @@ -293,7 +297,7 @@ def continue_field(path, value, field, type, ast_node, next_selections, is_non_n nil else resolved_type = resolved_type.metadata[:type_class] - continue_field(path, value, field, resolved_type, ast_node, next_selections, is_non_null) + continue_field(path, value, field, resolved_type, ast_node, next_selections, is_non_null, owner_object, arguments) end end when "OBJECT" @@ -302,7 +306,7 @@ def continue_field(path, value, field, type, ast_node, next_selections, is_non_n rescue GraphQL::ExecutionError => err err end - after_lazy(object_proxy, owner: type, path: path, field: field) do |inner_object| + after_lazy(object_proxy, owner: type, path: path, field: field, owner_object: owner_object, arguments: arguments) do |inner_object| continue_value = continue_value(path, inner_object, field, is_non_null, ast_node) if HALT != continue_value response_hash = {} @@ -323,11 +327,11 @@ def continue_field(path, value, field, type, ast_node, next_selections, is_non_n idx += 1 set_type_at_path(next_path, inner_type) # This will update `response_list` with the lazy - after_lazy(inner_value, owner: inner_type, path: next_path, field: field) do |inner_inner_value| + after_lazy(inner_value, owner: inner_type, path: next_path, field: field, owner_object: owner_object, arguments: arguments) do |inner_inner_value| # reset `is_non_null` here and below, because the inner type will have its own nullability constraint continue_value = continue_value(next_path, inner_inner_value, field, false, ast_node) if HALT != continue_value - continue_field(next_path, continue_value, field, inner_type, ast_node, next_selections, false) + continue_field(next_path, continue_value, field, inner_type, ast_node, next_selections, false, owner_object, arguments) end end end @@ -338,7 +342,7 @@ def continue_field(path, value, field, type, ast_node, next_selections, is_non_n inner_type = resolve_if_late_bound_type(inner_type) # Don't `set_type_at_path` because we want the static type, # we're going to use that to determine whether a `nil` should be propagated or not. - continue_field(path, value, field, inner_type, ast_node, next_selections, true) + continue_field(path, value, field, inner_type, ast_node, next_selections, true, owner_object, arguments) else raise "Invariant: Unhandled type kind #{type.kind} (#{type})" end @@ -389,23 +393,23 @@ def resolve_if_late_bound_type(type) # @param field [GraphQL::Schema::Field] # @param eager [Boolean] Set to `true` for mutation root fields only # @return [GraphQL::Execution::Lazy, Object] If loading `object` will be deferred, it's a wrapper over it. - def after_lazy(obj, owner:, field:, path:, eager: false) + def after_lazy(lazy_obj, owner:, field:, path:, owner_object:, arguments:, eager: false) @interpreter_context[:current_path] = path @interpreter_context[:current_field] = field - if schema.lazy?(obj) + if schema.lazy?(lazy_obj) lazy = GraphQL::Execution::Lazy.new(path: path, field: field) do @interpreter_context[:current_path] = path @interpreter_context[:current_field] = field # Wrap the execution of _this_ method with tracing, # but don't wrap the continuation below - inner_obj = query.trace("execute_field_lazy", {owner: owner, field: field, path: path, query: query}) do + inner_obj = query.trace("execute_field_lazy", {owner: owner, field: field, path: path, query: query, object: owner_object, arguments: arguments}) do begin - schema.sync_lazy(obj) + schema.sync_lazy(lazy_obj) rescue GraphQL::ExecutionError, GraphQL::UnauthorizedError => err yield(err) end end - after_lazy(inner_obj, owner: owner, field: field, path: path, eager: eager) do |really_inner_obj| + after_lazy(inner_obj, owner: owner, field: field, path: path, owner_object: owner_object, arguments: arguments, eager: eager) do |really_inner_obj| yield(really_inner_obj) end end @@ -417,7 +421,7 @@ def after_lazy(obj, owner:, field:, path:, eager: false) lazy end else - yield(obj) + yield(lazy_obj) end end diff --git a/spec/graphql/execution/errors_spec.rb b/spec/graphql/execution/errors_spec.rb new file mode 100644 index 0000000000..3661d2bd00 --- /dev/null +++ b/spec/graphql/execution/errors_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true +require "spec_helper" + +describe "GraphQL::Execution::Errors" do + class ErrorsTestSchema < GraphQL::Schema + class ErrorA < RuntimeError; end + class ErrorB < RuntimeError; end + class ErrorC < RuntimeError + attr_reader :value + def initialize(value:) + @value = value + super + end + end + + use GraphQL::Execution::Interpreter + use GraphQL::Analysis::AST + use GraphQL::Execution::Errors + + rescue_from(ErrorA) do |err, obj, args, ctx, field| + ctx[:errors] << "#{err.message} (#{obj.class.name}.#{field.graphql_name}, #{args.inspect})" + nil + end + + rescue_from(ErrorB) do |*| + raise GraphQL::ExecutionError, "boom!" + end + + rescue_from(ErrorC) do |err, *| + err.value + end + + class Query < GraphQL::Schema::Object + field :f1, Int, null: true do + argument :a1, Int, required: false + end + + def f1(a1: nil) + raise ErrorA, "f1 broke" + end + + field :f2, Int, null: true + def f2 + GraphQL::Execution::Lazy.new { raise ErrorA, "f2 broke" } + end + + field :f3, Int, null: true + + def f3 + raise ErrorB + end + + field :f4, Int, null: false + def f4 + raise ErrorC.new(value: 20) + end + + end + + query(Query) + end + + describe "rescue_from handling" do + it "can replace values with `nil`" do + ctx = { errors: [] } + res = ErrorsTestSchema.execute "{ f1(a1: 1) }", context: ctx, root_value: :abc + assert_equal({ "data" => { "f1" => nil } }, res) + assert_equal ["f1 broke (ErrorsTestSchema::Query.f1, {:a1=>1})"], ctx[:errors] + end + + it "rescues errors from lazy code" do + ctx = { errors: [] } + res = ErrorsTestSchema.execute("{ f2 }", context: ctx) + assert_equal({ "data" => { "f2" => nil } }, res) + assert_equal ["f2 broke (ErrorsTestSchema::Query.f2, {})"], ctx[:errors] + end + + it "can raise new errors" do + res = ErrorsTestSchema.execute("{ f3 }") + expected_error = { + "message"=>"boom!", + "locations"=>[{"line"=>1, "column"=>3}], + "path"=>["f3"] + } + assert_equal({ "data" => { "f3" => nil }, "errors" => [expected_error] }, res) + end + + it "can replace values with non-nil" do + res = ErrorsTestSchema.execute("{ f4 }") + assert_equal({ "data" => { "f4" => 20 } }, res) + end + end +end From 6ca370938c5f419716e4f71a376281382ae1dd0a Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Mon, 2 Sep 2019 07:36:01 -0400 Subject: [PATCH 2/2] Add a test for subclass; unwrap objects before passing to handler --- lib/graphql/execution/errors.rb | 3 +++ spec/graphql/execution/errors_spec.rb | 19 ++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/graphql/execution/errors.rb b/lib/graphql/execution/errors.rb index f2ae6203df..a9bae6f40b 100644 --- a/lib/graphql/execution/errors.rb +++ b/lib/graphql/execution/errors.rb @@ -47,6 +47,9 @@ def with_error_handling(trace_data) args = trace_data[:arguments] ctx = trace_data[:query].context field = trace_data[:field] + if obj.is_a?(GraphQL::Schema::Object) + obj = obj.object + end handler.call(err, obj, args, ctx, field) else raise err diff --git a/spec/graphql/execution/errors_spec.rb b/spec/graphql/execution/errors_spec.rb index 3661d2bd00..26aede04ae 100644 --- a/spec/graphql/execution/errors_spec.rb +++ b/spec/graphql/execution/errors_spec.rb @@ -13,12 +13,14 @@ def initialize(value:) end end + class ErrorASubclass < ErrorA; end + use GraphQL::Execution::Interpreter use GraphQL::Analysis::AST use GraphQL::Execution::Errors rescue_from(ErrorA) do |err, obj, args, ctx, field| - ctx[:errors] << "#{err.message} (#{obj.class.name}.#{field.graphql_name}, #{args.inspect})" + ctx[:errors] << "#{err.message} (#{field.owner.name}.#{field.graphql_name}, #{obj.inspect}, #{args.inspect})" nil end @@ -55,6 +57,10 @@ def f4 raise ErrorC.new(value: 20) end + field :f5, Int, null: true + def f5 + raise ErrorASubclass, "raised subclass" + end end query(Query) @@ -65,14 +71,14 @@ def f4 ctx = { errors: [] } res = ErrorsTestSchema.execute "{ f1(a1: 1) }", context: ctx, root_value: :abc assert_equal({ "data" => { "f1" => nil } }, res) - assert_equal ["f1 broke (ErrorsTestSchema::Query.f1, {:a1=>1})"], ctx[:errors] + assert_equal ["f1 broke (ErrorsTestSchema::Query.f1, :abc, {:a1=>1})"], ctx[:errors] end it "rescues errors from lazy code" do ctx = { errors: [] } res = ErrorsTestSchema.execute("{ f2 }", context: ctx) assert_equal({ "data" => { "f2" => nil } }, res) - assert_equal ["f2 broke (ErrorsTestSchema::Query.f2, {})"], ctx[:errors] + assert_equal ["f2 broke (ErrorsTestSchema::Query.f2, nil, {})"], ctx[:errors] end it "can raise new errors" do @@ -89,5 +95,12 @@ def f4 res = ErrorsTestSchema.execute("{ f4 }") assert_equal({ "data" => { "f4" => 20 } }, res) end + + it "rescues subclasses" do + context = { errors: [] } + res = ErrorsTestSchema.execute("{ f5 }", context: context) + assert_equal({ "data" => { "f5" => nil } }, res) + assert_equal ["raised subclass (ErrorsTestSchema::Query.f5, nil, {})"], context[:errors] + end end end