rmosolgo / rmosolgo/graphql-ruby

Default coercion and validation is... inconsistent

Open
#3,459 3 comments 1 reaction 0 assignees View on GitHub

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 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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.