Default coercion and validation is... inconsistent

[eapache]

@rmosolgo another spin out of https://github.com/rmosolgo/graphql-ruby/pull/3448. Filing as an issue not a PR because this might have implications across many classes, and probably needs some discussion.

The built-in scalars are confusingly inconsistent in what they will coerce for output, and how they produce errors.

• The default Int scalar uses to_i when coercing outputs and then calls schema.type_error if the resulting int is out of bounds (GraphQL requires ints to be in the 32-bit signed range). In ruby "non_numeric_string".to_i produces 0, not an error.
• BigInt and Float behave similarly, though Float is not bounded (which I believe it should be?)
• Boolean is even more liberal, it just runs !! which as far as I know will convert any ruby object of any type whatsoever into a boolean.
• ID, similarly, just calls to_s which is a method that exists on every ruby object (even nil.to_s returns "").
• ISO8601Date uses Date.parse which I believe raises errors on invalid arguments?
• ISO8601DateTime uses Time.parse which raises on invalid arguments, but then rescues all exceptions and converts them into GraphQL::Errors.
• JSON is the best yet, it does a complete literal pass-through: https://github.com/rmosolgo/graphql-ruby/blob/5ab3e5e96a95a52d8c2d353346132fcac188fa3f/lib/graphql/types/json.rb#L20-L22
• String calls to_s but then validates the string is in UTF-8 (mutating the passed-in string if it isn't frozen!?) and reports encoding errors using the schema.type_error callback.

All of these are inconsistent with each other, but also with scalar coercion on input where AFAICT the best practice is for your coercion method to return nil on invalid input?

Questions

1. Should outputs be validated at all or are these output coercion methods just conveniences and we should trust the developer?
2. Should input (and maybe output) validation be tied up with coercion like this or should validation and coercion be separate methods?
3. What is the right way to signal an error in these cases? Raising? Calling schema.type_error? Returning nil?

I think whatever our answers to the above, some of the default scalars will need changing.

cc @benjie @swalkinshaw

[eapache]

Looks like I ran into part of this same issue way back in 2016 😂 😬

https://github.com/rmosolgo/graphql-ruby/issues/139

[rmosolgo]

Thanks for writing up this issue, it certainly reflects the one-patch-at-a-time approach to development 😆 But if it only comes up once every 5 years, that's not too bad!

I can share my own opinion on the questions above:

Should outputs be validated at all or are these output coercion methods just conveniences and we should trust the developer?

In my opinion, the best would be a choice. A similar question exists for non-null validations: should GraphQL-Ruby guarantee that the schema never violates the spec? I think that's a good default behavior. But at the same time, it'd be nice to offer the option to remove that validation for when devs want to reduce the overhead of execution.

Should input (and maybe output) validation be tied up with coercion like this or should validation and coercion be separate methods?

I definitely think it'd be worth revisiting this. Another problem is accepting input from query string parameters, where everything is a string. It'd be nice to have a way to support more forgiving coercion in that case, but there isn't one now.

Also worth noting is the difficulty between literal validators, which can be validated statically, and variable values, which take a different codepath. I suspect that some of the current complexity arose from that difficulty (although it could probably be done better!).

What is the right way to signal an error in these cases? Raising? Calling schema.type_error? Returning nil?

The most compelling reason for schema.type_error (or, at least, it's the reason that I added it, IIRC) is for compatibility when we added some of the proper validations. For example, GitHub's schema serves very big numbers as Ints -- we needed some way to maintain that behavior. I'm not sure that's the best approach, but I thought I'd share the background there.

[eapache]

Without digging too much into the implications my default preference would be:

• Keep validation and coercion together since in a lot of more complex cases coercing requires validating of some sort anyway.
• Make "raise an exception" the only way to raise an error during coercion (input or output); the gem rescues GraphQL::Error (?) during coercing calls and turns it into schema.type_error for you.
• Returning nil means it coerced to nil, nothing more.
• Calling schema.type_error yourself during coercion... technically still works but is kinda weird I guess? If you want to return multiple errors for a single scalar for some reason?
• Add as much missing output validation to the built-in scalars as makes sense on a first pass.

I still need to think about literals-vs-variables, typed-vs-strings, etc. That part is definitely messy.