rmosolgo / rmosolgo/graphql-ruby
Default coercion and validation is... inconsistent
Nobody has claimed this yet.
- Dominant language
- Ruby
- Stars
- 5.4k
- Forks
- 1.4k
- Avg merge
- 23h 19m
- Merged PRs (30d)
- 28
Description
@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
Intscalar usesto_iwhen coercing outputs and then callsschema.type_errorif the resulting int is out of bounds (GraphQL requires ints to be in the 32-bit signed range). In ruby"non_numeric_string".to_iproduces0, not an error. BigIntandFloatbehave similarly, thoughFloatis not bounded (which I believe it should be?)Booleanis 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_swhich is a method that exists on every ruby object (evennil.to_sreturns"").ISO8601DateusesDate.parsewhich I believe raises errors on invalid arguments?ISO8601DateTimeusesTime.parsewhich raises on invalid arguments, but then rescues all exceptions and converts them intoGraphQL::Errors.JSONis the best yet, it does a complete literal pass-through: https://github.com/rmosolgo/graphql-ruby/blob/5ab3e5e96a95a52d8c2d353346132fcac188fa3f/lib/graphql/types/json.rb#L20-L22Stringcallsto_sbut 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_errorcallback.
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
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the built-in scalar coercion behavior described in the issue and inspect lib/graphql/types/json.rb, then compare the output and input coercion paths for Int, BigInt, Float, Boolean, ID, ISO8601Date, ISO8601DateTime, JSON, and String. Done requires resolving the validation, coercion, and error-signaling questions and identifying which default scalars should change; the issue does not yet define a concrete implementation target.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- graphql, ruby
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100