Improve Scalar result coercion spec compliance - #5306
Conversation
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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:
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. |
Sorry, something went wrong.
|
What if we preserve the current default behavior, but also:
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?
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 🤔 |
Sorry, something went wrong.
|
MySchema.raise_scalar_type_errors sounds good 👍 If you want to prioritize feel free. Otherwise I'll try over the next week or two |
Sorry, something went wrong.
|
Okay made a lot of updates and I'll update the PR description after if this is looking mergeable.
|
Sorry, something went wrong.
| autoload :DateEncodingError, "graphql/date_encoding_error" | ||
| autoload :DurationEncodingError, "graphql/duration_encoding_error" |
There was a problem hiding this comment.
Side note, I believe both of these should be Decoding since they are only used in coerce_input methods. I can fix them separately.
Sorry, something went wrong.
| rescue GraphQL::ExecutionError => ex_err | ||
| return continue_value(ex_err, field, is_non_null, ast_node, result_name, selection_result) |
There was a problem hiding this comment.
Any possible issues here? No tests break but this will change error messages, though it will improve and make them more spec compliant.
Sorry, something went wrong.
| "locations" => [{ "line" => 1, "column" => 3 }], | ||
| "path" => ["badString"], | ||
| }, | ||
| ], | ||
| "data" => { | ||
| "badString" => nil, | ||
| } |
There was a problem hiding this comment.
This shows the improved error messages for result coersion errors.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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, } }, ```
|
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? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
…ards compat, add more context to warning and test the warning
|
I put that test back and added #integer_value to make it pass. This change looks good to me -- let me know how things look on your end and I'll merge it when we're all clear |
Sorry, something went wrong.
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.