Ruby 3: Don't pass empty hash as kwargs

[l15n]

Problem description

When dynamically generating a schema via GraphQL::Schema.define while running Ruby 3, we encountered an error like thiis

12:16:29 GraphQL::Schema::InvalidTypeError: Query is invalid: field "api_user_coupon" arguments must be a hash of {String => GraphQL::Argument}, not a NilClass (nil)
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/traversal.rb:223:in `validate_type'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/traversal.rb:142:in `visit'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/traversal.rb:128:in `block in visit'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/traversal.rb:128:in `each'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/traversal.rb:128:in `visit'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/traversal.rb:47:in `initialize'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema.rb:1828:in `new'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema.rb:1828:in `rebuild_artifacts'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema.rb:402:in `types'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/validation.rb:157:in `block in module:Rules'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/validation.rb:20:in `block (2 levels) in validate'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/validation.rb:19:in `each'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/validation.rb:19:in `block in validate'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/validation.rb:17:in `each'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema/validation.rb:17:in `validate'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/schema.rb:363:in `deprecated_define'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/define/instance_definable.rb:155:in `deprecated_define'
12:16:29 /mnt/vol/tmp/bundler/pull-request/ruby/3.0.0/gems/graphql-1.12.12/lib/graphql/define/instance_definable.rb:19:in `define'

Cause

This issue apparently occurs when dynamically defining a field without any arguments in Ruby 3 (this worked without issue for Ruby 2.7 and earlier).

In this case, an empty hash is being passed to the method_missing of DefinedObjectProxy when attempting to define the arguments on a GraphQL::Field as being an empty hash {}.:

graphql-ruby/lib/graphql/define/defined_object_proxy.rb

Lines 36 to 46 in bace1e4

	# Lookup a function from the dictionary and call it if it's found.
	def method_missing(name, *args, &block)
	definition = @dictionary[name]
	if definition
	definition.call(@target, *args, &block)
	else
	msg = "#{@target.class.name} can't define '#{name}'"
	raise NoDefinitionError, msg, caller
	end
	end
	ruby2_keywords :method_missing

This then calls

graphql-ruby/lib/graphql/define/instance_definable.rb

Lines 223 to 238 in bace1e4

	class AssignAttribute
	extend GraphQL::Ruby2Keywords
	def initialize(attr_name)
	@attr_assign_method = :"#{attr_name}="
	end
	# Even though we're just using the first value here,
	# We have to add a splat here to use `ruby2_keywords`,
	# so that it will accept a `[{}]` input from the caller.
	def call(defn, *value)
	defn.public_send(@attr_assign_method, value.first)
	end
	ruby2_keywords :call
	end
	end

which then just calls the accessor for arguments on GraphQL::Field. Note, also, the comment on AssignAttribute#call

        # Even though we're just using the first value here,
        # We have to add a splat here to use `ruby2_keywords`,
        # so that it will accept a `[{}]` input from the caller.

This is a clue to the solution, to be described below.

Explanation

In Ruby 2.7 (using ruby2_keywords), this is not a problem because the empty hash is retained as just an empty hash when the splatted hash is further passed in a method as *args. Here's a minimal Ruby example:

# Ruby 2.7
✦ ❯ irb
irb(main):001:1* class A
irb(main):002:2*   def m(*args)
irb(main):003:2*     p args
irb(main):004:1*   end
irb(main):005:1*   ruby2_keywords :m
irb(main):006:0> end
=> nil
irb(main):008:0> A.new.m(**{})
[{}]
=> [{}]

In Ruby 3, the behaviour of splatting a hash argument into keyword arguments changed. In this particular case, after splatting with ** the hash disappears and then interpreted as just [] when passed through *args. Here's a minimal example in Ruby:

# Ruby 3
❯ irb
irb(main):001:1* class A
irb(main):002:2*   def m(*args)
irb(main):003:2*     p args
irb(main):004:1*   end
irb(main):005:1*   ruby2_keywords :m
irb(main):006:0> end
=> nil
irb(main):007:0> A.new.m(**{})
[]

This empty array is used in AssignAttribute#call

graphql-ruby/lib/graphql/define/instance_definable.rb

Line 234 in bace1e4

defn.public_send(@attr_assign_method, value.first)

Which is why arguments is assigned as nil rather than {}, despite being called as arguments = {} at the very top.

Solution

The approach I'm suggesting in this PR is to avoid splatting an emptyHash into keyword arguments. This will be compatible with all versions of Ruby.

[rmosolgo]

Wow, thanks for the detailed writeup and the fix. Normally, I'd want to merge tests for this behavior to make sure it isn't broken in the future. But .define is hardly maintained anymore, so I doubt it will be any trouble.

Speaking of, may I ask, is there any particular reason your application still uses .define, instead of the class-based API?

[l15n]

Normally, I'd want to merge tests for this behavior to make sure it isn't broken in the future. But .define is hardly maintained anymore, so I doubt it will be any trouble.

That's understandable. I wasn't really sure where to begin with setting up a test case for this scenario, since it's dependent on the version of Ruby, and is fairly deep in the weeds with regards to what would need to be tested.

Speaking of, may I ask, is there any particular reason your application still uses .define, instead of the class-based API?

We're definitely an edge case here: our application is actually not a GraphQL API, but is a RESTful hypermedia API which allows a limited degree of querying. We're using graphql-ruby to generate a machine-readable schema (actually GraphQL schema) for documentation etc.

[rmosolgo]

Interesting! Basically, using the GraphQL SDL to document another API, nice. I can recommend moving to Class.new { ... } since .define will be removed in graphql-ruby 2.0. GraphQL-Ruby uses Class.new extensively under the hood, particularly for defining schemas from SDL text and from JSON.

[l15n]

Indeed. Thanks for the heads up on the migration path to graphql-ruby 2.0 😄