graphql-ruby
graphql-ruby copied to clipboard
Default coercion and validation is... inconsistent
@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 usesto_i
when coercing outputs and then callsschema.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
produces0
, not an error. -
BigInt
andFloat
behave similarly, thoughFloat
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 callsto_s
which is a method that exists on every ruby object (evennil.to_s
returns""
). -
ISO8601Date
usesDate.parse
which I believe raises errors on invalid arguments? -
ISO8601DateTime
usesTime.parse
which raises on invalid arguments, but then rescues all exceptions and converts them intoGraphQL::Error
s. -
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
callsto_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 theschema.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
- Should outputs be validated at all or are these output coercion methods just conveniences and we should trust the developer?
- Should input (and maybe output) validation be tied up with coercion like this or should validation and coercion be separate methods?
- 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
Looks like I ran into part of this same issue way back in 2016 😂 😬
https://github.com/rmosolgo/graphql-ruby/issues/139
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 Int
s -- 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.
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 intoschema.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.