Improve Scalar result coercion spec compliance #5306

swalkinshaw · 2025-03-29T18:57:56Z

The spec says that scalars should return query/field errors when input or result coercion values don't conform to the spec.

Int example: https://spec.graphql.org/draft/#sel-GAHXRFHCAACGS6rU

This updates the logic to return GraphQL::CoercionError (execution errors) instead of raised RuntimeErrors which would not result in a client error by default.

The logic and error messages are were taken from the graphql-js scalar implementation.
Note that this is likely a breaking change for schema developers. For clients, it's an improvement since they might get some sort of "internal server error" otherwise.

@rmosolgo I didn't update all the necessary specs yet because I figured this would need some further discussion. It's a fairly large behaviour change from the schema's perspective but ultimately we should try to support this to be spec compliant.

rmosolgo · 2025-03-31T10:21:35Z

I'm definitely interested in complying with the spec by default eventually, but I think that before changing the defaults, we should release a version with warnings that the default behavior is going to change with a link to the docs about how to either override the defaults (to retain current behavior without the warning) or opt into the future default behavior.

The other thing is to make sure these still show up in peoples' bug trackers. The intention behind raising errors was to make sure developers become aware that their data and schema don't match -- something needs to be fixed on the backend. What would have to happen to make sure these CoercionErrors show up to developers?

swalkinshaw · 2025-04-01T14:11:06Z

Yeah that's the hard part because the gem has no abstraction for error "reporting". I think the best we can do is a hybrid approach:

call the type_error hook with an instantiated exception
add an empty branch to the default type_error implementation for a no-op
return the execution error

Then at least this gives people a place to implement the behaviour they want but it's still opt-in and a "breaking change".

Unless I'm missing something, there's no way to both report an exception and return an execution error currently. This gem would have to check for various bug tracker integrations.

With the Railtie we could default to using Rails.error.report but that still assumes that the application is actually using that newer interface.

So it's a catch 22 where I don't think it's possible to get the ideal behaviour. We need to raise an exception... but also not raise one.

rmosolgo · 2025-04-01T22:08:40Z

What if we preserve the current default behavior, but also:

Add a new configuration, eg MySchema.raise_scalar_type_errors = nil
In the default type_error behavior:
- if that configuration wasn't set to true, warn that the default behavior is changing in a future version and that the new setting should be assigned to either true or else custom handling should be put in def self.type_error to preserve the current behavior in future versions
- if the configuration is set to true, do the new behavior (without any warning)

Then, in a future version, we change the default to return errors and deprecate the now-useless config. That would move us progressively toward spec compliance while warning people about what's going on. What do you think?

abstraction for error "reporting"

That's cool, I didn't know Rails had Rails.errors.report(err). I wonder if GraphQL-Ruby could hand errors off to that somehow 🤔

swalkinshaw · 2025-04-07T15:09:01Z

MySchema.raise_scalar_type_errors sounds good 👍 If you want to prioritize feel free. Otherwise I'll try over the next week or two

swalkinshaw · 2025-05-21T19:05:43Z

Okay made a lot of updates and I'll update the PR description after if this is looking mergeable.

Added the Schema.spec_compliant_scalar_coercion_errors option. I didn't love the raise_scalar_type_errors name since "raise" is confusing as exceptions can technically raised in both cases.
I've focused this on result coercion only. I think input coercion is mostly spec compliant though the error messages are a bit inconsistent now. That can be looked at after.
I replaced any existing scalar specific encoding exception classes with a single new LegacyScalarCoercionError to make the type_error implementation easier. If you don't like that, it should be possible to restore the old ones. Technically this is a tiny breaking change since it will change the errors reported in apps but this also provides a signal that it's "legacy".
I improved the error response of coerce_result which was previously missing path, locations, and data.

swalkinshaw · 2025-05-21T19:06:48Z

lib/graphql.rb

  autoload :DateEncodingError, "graphql/date_encoding_error"
  autoload :DurationEncodingError, "graphql/duration_encoding_error"


Side note, I believe both of these should be Decoding since they are only used in coerce_input methods. I can fix them separately.

swalkinshaw · 2025-05-21T19:07:25Z

lib/graphql/execution/interpreter/runtime.rb

+            rescue GraphQL::ExecutionError => ex_err
+              return continue_value(ex_err, field, is_non_null, ast_node, result_name, selection_result)


Any possible issues here? No tests break but this will change error messages, though it will improve and make them more spec compliant.

swalkinshaw · 2025-05-21T19:08:25Z

spec/graphql/types/string_spec.rb

+              "locations" => [{ "line" => 1, "column" => 3 }],
+              "path" => ["badString"],
+            },
+          ],
+          "data" => {
+            "badString" => nil,
+          }


This shows the improved error messages for result coersion errors.

swalkinshaw · 2025-05-21T20:55:06Z

In hindsight, it's better to keep the type specific encoding exceptions for now but have them inherit from a new common base so I've made this even less breaking.

Previously many result coercion failures for built-in scalar types raised Ruby exceptions instead of GraphQL execution errors. The GraphQL spec says that any time a scalar value cannot be coerced to a spec complaint result, an execution error should be raised. Example: https://spec.graphql.org/draft/#sel-GAHXRFHCAACGS6rU This introduces a backwards compatible way to opt into this new spec compliant behaviour. `Schema.spec_compliant_scalar_coercion_errors` controls this behaviour and setting it to `true` will return execution errors instead of raising Ruby exceptions. `Schema.spec_compliant_scalar_coercion_errors` defaults to being `nil` (disabled) and will produce a warning about the intention to change this default in a future version. To preserve this legacy (and spec non-compliant) behaviour, you can customize the error handling logic in `Schema.type_error`. This also improves scalar result coercion errors and makes them more spec compliant. Before: ```ruby { "errors" => [ { "message" => "Int cannot represent non 32-bit signed integer value: 2147483648", }, ], }, ``` ```ruby { "errors" => [ { "message" => "Int cannot represent non 32-bit signed integer value: 2147483648", "locations" => [{ "line" => 1, "column" => 3 }], "path" => ["someField"], }, ], "data" => { "someField" => nil, } }, ```

rmosolgo · 2025-05-22T16:23:38Z

spec/graphql/types/int_spec.rb

@@ -26,21 +26,77 @@
        assert_equal(-(2**31), GraphQL::Types::Int.coerce_result(-(2**31), context))
      end

-      it "replaces values, if configured to do so" do
-        assert_equal Dummy::Schema::MAGIC_INT_COERCE_VALUE, GraphQL::Types::Int.coerce_result(99**99, context)
-      end


Does this have to be removed? It seems like the override in the example schema could be retained, but a method call would have to be changed.... or is this behavior not possible anymore?

(Maybe the old override could be retained as-is if an alias accessor was added to the new IntegerEncodingError)

No it doesn't have to be I don't think. I'll check

rmosolgo · 2025-05-22T16:28:36Z

Thanks for all your work on this, I'm definitely looking forward to getting this stuff on track.

The last question I have is about developer experience. Currently, the default behavior is to return the error to the client but not notify the developer in any way that something has gone wrong. (Maybe some tooling is checking the "errors" key in the response?)

I wonder if a good enough approach would be to modify the generated default schema to use Rails.error.report, and include a nudge that direction in the changelog and/or warning. Does any other solution come to mind for you in that area?

swalkinshaw · 2025-05-22T17:27:28Z

I was considering adding a basic error reporter as part of this as well, or separately. The default would be a no-op (since there's also no logger built in) but then the Railtie would configure it to Rails.error.report which would cover most cases.

I'm running this branch against our test suite as well to see what breaks or not.

swalkinshaw force-pushed the spec-compliant-scalar-coercion branch 2 times, most recently from a8ee142 to c921f90 Compare May 21, 2025 19:01

swalkinshaw force-pushed the spec-compliant-scalar-coercion branch from c921f90 to 29c4cdb Compare May 21, 2025 19:05

swalkinshaw commented May 21, 2025

View reviewed changes

swalkinshaw changed the title ~~Update Int and Float coercion logic to be spec compliant~~ Improve Scalar coercion spec compliance May 21, 2025

swalkinshaw changed the title ~~Improve Scalar coercion spec compliance~~ Improve Scalar result coercion spec compliance May 21, 2025

swalkinshaw force-pushed the spec-compliant-scalar-coercion branch from 29c4cdb to 8000e19 Compare May 21, 2025 20:54

swalkinshaw force-pushed the spec-compliant-scalar-coercion branch from 8000e19 to 6735bba Compare May 21, 2025 20:58

rmosolgo reviewed May 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve Scalar result coercion spec compliance #5306

Improve Scalar result coercion spec compliance #5306

Uh oh!

swalkinshaw commented Mar 29, 2025

Uh oh!

rmosolgo commented Mar 31, 2025

Uh oh!

swalkinshaw commented Apr 1, 2025

Uh oh!

rmosolgo commented Apr 1, 2025

Uh oh!

swalkinshaw commented Apr 7, 2025

Uh oh!

swalkinshaw commented May 21, 2025 •

edited

Loading

Uh oh!

swalkinshaw May 21, 2025

Uh oh!

swalkinshaw May 21, 2025

Uh oh!

swalkinshaw May 21, 2025

Uh oh!

swalkinshaw commented May 21, 2025

Uh oh!

rmosolgo May 22, 2025

Uh oh!

swalkinshaw May 22, 2025

Uh oh!

rmosolgo commented May 22, 2025

Uh oh!

swalkinshaw commented May 22, 2025

Uh oh!

Uh oh!

		autoload :DateEncodingError, "graphql/date_encoding_error"
		autoload :DurationEncodingError, "graphql/duration_encoding_error"

		rescue GraphQL::ExecutionError => ex_err
		return continue_value(ex_err, field, is_non_null, ast_node, result_name, selection_result)

Improve Scalar result coercion spec compliance #5306

Are you sure you want to change the base?

Improve Scalar result coercion spec compliance #5306

Uh oh!

Conversation

swalkinshaw commented Mar 29, 2025

Uh oh!

rmosolgo commented Mar 31, 2025

Uh oh!

swalkinshaw commented Apr 1, 2025

Uh oh!

rmosolgo commented Apr 1, 2025

Uh oh!

swalkinshaw commented Apr 7, 2025

Uh oh!

swalkinshaw commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swalkinshaw May 21, 2025

Choose a reason for hiding this comment

Uh oh!

swalkinshaw May 21, 2025

Choose a reason for hiding this comment

Uh oh!

swalkinshaw May 21, 2025

Choose a reason for hiding this comment

Uh oh!

swalkinshaw commented May 21, 2025

Uh oh!

rmosolgo May 22, 2025

Choose a reason for hiding this comment

Uh oh!

swalkinshaw May 22, 2025

Choose a reason for hiding this comment

Uh oh!

rmosolgo commented May 22, 2025

Uh oh!

swalkinshaw commented May 22, 2025

Uh oh!

Uh oh!

swalkinshaw commented May 21, 2025 •

edited

Loading