Fix CoercionError handling with null

[rmosolgo]

Fixes #4740

[swalkinshaw]

I'm curious about this change because CoercionError went from inheriting GraphQL::Error to ExecutionError which has different behaviour. GraphQL::Error results in an "unhandled" standard error which in our case is returned as a generic "Internal server error". This change means that all coerce_result methods returning a CoercionError now return a execution error. Was this intentional or just a side effect or re-using some other error handling in validate_input?

[rmosolgo]

CoercionError was supposed to be into a client-facing error all along, see for example:

graphql-ruby/spec/graphql/schema/scalar_spec.rb

Lines 116 to 145 in c2e666d

	describe "raising CoercionError" do
	class CoercionErrorSchema < GraphQL::Schema
	class CustomScalar < GraphQL::Schema::Scalar
	def self.coerce_input(val, ctx)
	raise GraphQL::CoercionError, "#{val.inspect} can't be Custom value"
	end
	end
	class Query < GraphQL::Schema::Object
	field :f1, String do
	argument :arg, CustomScalar
	end
	end
	query(Query)
	end
	it "makes a nice validation error" do
	result = CoercionErrorSchema.execute("{ f1(arg: \"a\") }")
	expected_error = {
	"message" => "\"a\" can't be Custom value",
	"locations" => [{"line"=>1, "column"=>3}],
	"path" => ["query", "f1", "arg"],
	"extensions" => {
	"code"=>"argumentLiteralsIncompatible",
	"typeName"=>"CoercionError"
	}
	}
	assert_equal [expected_error], result["errors"]
	end

graphql-ruby/spec/graphql/static_validation/rules/argument_literals_are_compatible_spec.rb

Lines 336 to 349 in c2e666d

	describe "with a shallow coercion" do
	it "sets error message from a CoercionError if raised" do
	assert_equal 1, errors.length
	assert_includes errors, {
	"message"=> "cannot coerce to Float",
	"locations"=>[{"line"=>3, "column"=>9}],
	"path"=>["query", "time", "value"],
	"extensions"=>{"code"=>"argumentLiteralsIncompatible", "typeName"=>"CoercionError"}
	}
	end
	end
	describe "with a deep coercion" do

graphql-ruby/spec/graphql/static_validation/rules/variable_default_values_are_correctly_typed_spec.rb

Lines 183 to 192 in c2e666d

	it "sets error message from a CoercionError if raised" do
	assert_equal 1, errors.length
	assert_includes errors, {
	"message"=> "cannot coerce to Float",
	"locations"=>[{"line"=>3, "column"=>9}],
	"path"=>["query"],
	"extensions"=>{"code"=>"defaultValueInvalidType", "variableName"=>"value", "typeName"=>"Time"}
	}
	end

graphql-ruby/spec/graphql/schema/dynamic_members_spec.rb

Lines 138 to 158 in c2e666d

	class LegacyScream < BaseScalar
	graphql_name "Scream"
	description "An all-uppercase saying"
	def self.coerce_input(value, ctx)
	if value.upcase != value
	raise GraphQL::CoercionError, "scream must be SCREAMING"
	end
	value
	end
	self.future_schema = false
	end
	class Scream < BaseScalar
	description "A saying ending with at least four exclamation points"
	def self.coerce_input(value, ctx)
	if !value.end_with?("!!!!")
	raise GraphQL::CoercionError, "scream must be screaming!!!!"
	end
	value
	end

So I thought that by inheriting from ExecutionError, I could get some of that rescue .... behavior for free.

If there were cases where CoercionError propagated all the way up the stack (and apparently there were...), they were due to an oversight on my part. Do you have backtraces for them? I'd love to figure out what changed here and, if necessary, we could come up with a plan for compatibility.

[swalkinshaw]

I think this is partly due to a single CoercionError being used for both coerce_input and coerce_result but they can be semantically different. For input it should mostly be a client error because they supplied the value. But in our case for coerce_result the server is responsible for coercion and returning that result so it's not a client error (most of the time) and we, as the developers, treat these as exceptions which should be fixed.

Here's a backtrace of a CoercionError that was raised in a scalar's coerce_result:

["/some/path/id_type.rb:21:in `coerce_result'",
 "gems/graphql-ruby-284542f2c94f/lib/graphql/execution/interpreter/runtime.rb:563:in `continue_field'",
 "gems/graphql-ruby-284542f2c94f/lib/graphql/execution/interpreter/runtime.rb:374:in `block (2 levels) in evaluate_selection_with_resolved_keyword_args'",

It was being handled here:

graphql-ruby/lib/graphql/execution/interpreter/runtime.rb

Lines 572 to 574 in 04104f1

	current_type.coerce_result(value, context)
	rescue StandardError => err
	schema.handle_or_reraise(context, err)

Now that it's an ExecutionError, it won't go through that code path.

[rmosolgo]

Oh... CoercionError on outgoing values -- yes, agreed that it's a bug from using the same error in both cases.

That's in a custom scalar, right? I'm trying to see if CoercionError is used that way in GraphQL-Ruby, or documented for that use, and I can't find one in the source.

The mention in the docs only addresses incoming values:

graphql-ruby/guides/type_definitions/scalars.md

Line 101 in 04104f1

When incoming data is incorrect, the method may raise {{ "GraphQL::CoercionError" | api_doc }}, which will be returned to the client in the `"errors"` key.

Maybe the best thing here is to replace that usage of CoercionError with something plain-Ruby, like an ArgumentError (or something custom to that scalar?) which would be handled by rescue_from. Would that approach work for you?

If it does, I'll improve the docs to suggest that approach for validating outgoing values.

[swalkinshaw]

That's in a custom scalar, right?

Yep custom. We can definitely just raise another error that doesn't inherit from ExecutionError so we'll do that regardless. Mainly just wanted to see if this original change was intentional or not. Thanks for talking through it.

[swalkinshaw]

One other wrinkle to this... a lot of our scalars implement logic like this:

class SomeScalar
  def coerce_input(value, context)
    return if value.nil?

    validate_value(value, context)
    value
  end

  def coerce_result(value, context)
    validate_value(value, context)
    value
  end

  private

  def validate_value(value, context)
    unless value.something
      raise GraphModel::CoercionError, "invalid value #{value}"
    end
  end
end

This worked great as is but now we'd have to specify which error gets raised in each case. Again, not a huge deal but just wanted to point out our usage.