Graphql-ruby: [1.8] Defaulting of Field#null and Argument#required

[jturkel]

The new class based API in 1.8 requires the null option to be specified for fields and the required option to be specified for arguments:

module Types
  class User < ::GraphQL::Schema::Object
    field :id, String, null: false
    # raises ArgumentError (missing keyword argument null:)
    field :name, String
  end
end

module Types
  class UserInput < ::GraphQL::Schema::InputObject
    argument :id, String, required: true
    # raises ArgumentError (missing keyword argument required:)
    argument :name, String
  end
end

In 1.7 and the GraphQL spec fields default to nullable and arguments default to optional. The community seems to favor optional fields for easy schema evolution and better partial failure handling. Should the new 1.8 class based API also default to making fields nullable and arguments optional? I'd be happy to make the change if you think it's a good idea.

[rmosolgo]

I'm pretty torn. On the one hand, making them _required_ was borrowed from Shopify's GraphQL system, and I appreciate making people consider it.

However, I also agree that null: true and required: false are sane defaults; they're future-friendly, as you mentioned.

Let's leave this open for a bit and see if anyone else has an opinion!

[jturkel]

@rmosolgo - Looks like no one else has weighed in for or against this issue. How do you feel about moving forward with it? I'd be happy to put up a PR.

[eapache]

Just wanted to weigh in since I was one of the people who originally pushed for the current 1.8 approach; we've gone back and forth internally and settled on it being more useful to force people to answer the question. With a default people just leave everything nullable even when it's not, and that leads to worse APIs. I realize that's probably a contentious point though. It's also worth noting that we don't tend to use nullability for partial failure handling, so the majority of our fields are not nullable - also a contentious decision, but one that's worked well for us so far.

[KevinColemanInc]

@rmosolgo just to quip in, it would be cool if this followed rail's decision to make the default null: false for relational values. For my use case, most fields will require a value.

I'd rather have a default rather not no default, no matter what it is.

[jturkel]

I guess the beauty of the new class based API is that people can easily change the default behavior to suit their tests (i.e. default null to false, default null to true or make it a required configuration setting) with something like this:

module Types
  class BaseObject < GraphQL::Schema::Object
    class FieldWithDefaultOptionalArguments < GraphQL::Schema::Field
      def argument(*args, **options, &block)
        super(*args, **options.reverse_merge(required: false), &block)
      end
    end

    field_class FieldWithDefaultOptionalArguments

    def field(*args, **options, &block)
      super(*args, **options.reverse_merge(null: true), &block)
    end
  end
end

module Types
  class BaseInputObject < GraphQL::Schema::InputObject
    def self.argument(*args, **options, &block)
      super(*args, **options.reverse_merge(required: false), &block)
    end
  end
end

[rmosolgo]

Personally, I agree with @eapache, I'm interested in seeing how it goes making people consider the nullability of fields. If you're interested in adding defaults, @jturkel is spot-on about how it can be added to your system. (Another way is to extend Schema::Argument / Schema::Field and override initialize, then add those with argument_class(...) and field_class(...).

So, if you're interested, please experiment with it that way and let's revisit this issue after 1.8.0 is released. If folks have found that a built-in default would be beneficial, we could change it then.

[bbugh]

FYI the pass-through code can be written more simply by letting ruby implicitly handle the arguments:

def self.field(*, **options)
  options[:null] = true if options[:null].nil?
  super
end

PS: Not that it matters much unless you're optimizing for scale, but direct index access setting with []= is twice as fast as merge and reverse_merge.

[qrush]

I'm starting to upgrade our app @chatterbugapp to the class based API and this was one of the first roadblocks I came across. The error is extremely cryptic, even for a very small class:

class Types::StudyCard < GraphQL::Schema::Object
  field :id, ID
end

$ rails c
Loading development environment (Rails 5.2.2.1)
irb(main):001:0> Types::StudyCard
Traceback (most recent call last):
        3: from (irb):1
        2: from app/graphql/types/study_card.rb:1:in `<top (required)>'
        1: from app/graphql/types/study_card.rb:2:in `<class:StudyCard>'
ArgumentError (missing keyword argument null:)

Doing a quick grep, there's over 250 fields we'd have to add code to that wasn't required before.

$ ag field app/graphql/{types,mutations} | wc -l
     258

I feel this is a step backwards in making sure people get their DSLs upgraded to the class-based APIs and most likely causes confusion as new users start playing with GraphQL.

I feel this gem should provide a smart default, in one direction or the other, and perhaps make that a gem wide or schema wide configuration if you'd like all fields to be nullable or not by default. @rmosolgo has there been any movement on this since last April?

[dukejones]

Convention over configuration, sensible defaults. That's what has made the Ruby/Rails community so fast for prototyping and generally defaulting to the correct behavior without having to study everything first.

[rbUUbr]

any updates for it?