Missing coercion or validation for default values

[eapache]

Describe the bug

@benjie pointed out in graphql/graphql-spec#793 that the GraphQL Spec and JS reference implementations are both somewhat confusing/broken when dealing with mistyped default values. I haven't gone through and tried to reproduce all the various combinations of this issue, but I did demonstrate a simple case to show that graphql-ruby suffers from at least a similar class of bug.

Versions

graphql version: 1.11.4 w/ interpreter, but I'm pretty sure it applies to master too

GraphQL schema

  field :test, :integer, null: true do |field|
    field.argument(:arg, :integer, required: true, default_value: "string")
  end

  def test(arg:)
    arg
  end

GraphQL query

query {
  test
}

{
  "data": {
    "test": null
  }
}

Expected vs actual behaviour

There are two interesting points in this example:

• GraphiQL actually says the sample query is invalid; apparently the default_value is not making it through into the introspection output, so GraphiQL claims the argument needs a value. However, if you actually run it then it works (sorta).
• When you run it, the default_value doesn't make it through into the resolver method, which gets nil instead. This could really mess with resolver code which assumes a value (since the argument is required-with-a-default at the GraphQL layer).

---

@rmosolgo there is still some debate on what the final specified behaviour should be, but I wanted to put this on your radar. The fact that the default value is getting stripped out in places suggests to me that we are validating it already somewhere, we're just not handling validation failures aggressively enough?

cc @swalkinshaw.

[rmosolgo]

Thanks for the heads up on this! I bet the default value gets dropped in coerce_input, which returns nil for unmanageable values:

graphql-ruby/lib/graphql/types/int.rb

Line 13 in 74bbeea

return if !value.is_a?(Integer)

Like you said, I bet it's never validated.

Is there any reason not to apply validation during configuration (ie, during the argument(...) method evaluation)?

[eapache]

Two potential complications came up during the Working Group meeting:

• Default values are mostly specified as language-native types, not as GraphQL AST tokens or on-the-wire transport bytes, so what it means to validate them is confusing/different from the normally-specified flow. (Though this is maybe more of a spec issue than an implementation issue.)
• You can do some messy things with default types that themselves have default fields which can lead to potentially infinite recursion in the validator if we're not careful. Something like

class A < GraphApi::InputObject
  field :a, A, required: true, default_value: A.new
end

[jeffsirocki]

Hey @eapache and @rmosolgo,
I'm wondering if there should be an additional note in https://github.com/rmosolgo/graphql-ruby/blob/master/guides/queries/interpreter.md pointing out that when migrating to the new Interpreter some default_value configurations may need to be updated - maybe something like?

Some default_value configurations must be updated too. For example:

- argument :field, String, default_value: :id, required: false
+ argument :field, String, default_value: 'id', required: false

(If the argument has a default value, {defaultValue} must be compatible with {argumentType} as per the coercion rules for that type, and coercion of {defaultValue} must not cause an infinite loop.)

When opting-in to use the Interpreter (with graphql v1.11.6) I also came across this in the following line:
argument :sort_by_column, String, default_value: :id, required: false - in my opinion adding this info in the docs may be useful to others migrating.

[rmosolgo]

It sounds like a specific behavior is in draft in the spec: graphql/graphql-wg#645 (comment)

[benjie]

Thanks for the heads up on this! I bet the default value gets dropped in coerce_input, which returns nil for unmanageable values:

The GraphQL Spec states that this situation should throw, I think:

https://spec.graphql.org/draft/#sel-NANRHHCJFTDFBBCAACEW0yS

[eapache]

@rmosolgo I took a look into fixing this and ran into a bunch of interesting points:

• You'd think in most schemas we should be able to validate default values at schema definition time, but all the coercion methods that actually do this depend on having a context available. Late-bound types also make this impossible.
• Runtime argument objects do no validation at all and are not set up for error handling, because they assume the static query validation has taken care of it.
• Static query validation only has access to the arguments defined in the query, which precisely excludes the arguments whose default values we care about.

Options I've considered:

• Set up a dummy context and just do it at schema-build time (or whenever the late-bound argument type is resolved, if I can find the right hook). This is mostly the best, except that it will break for any weird custom scalars which have context-dependent coercion logic.
• Write a new static validation rule which looks at every argument-containing element in the query and fully recurses its arguments and validates the default one... gross.
• Plumb through error handling code deep into runtime argument objects just for this one case... gross.

Is there another option I'm missing? Do we care about context in custom scalar coercion (at a quick glance none of Shopify's scalars use it)?

[rmosolgo]

I agree that your first suggestion is the best. A dummy context will likely be good enough, and if those intense cases come up, we can investigate one of those more intense approaches.

Here's a dummy context used elsewhere for purposes like this:

graphql-ruby/lib/graphql/query/null_context.rb

Line 5 in bace1e4

class NullContext

[eapache]

👍 can you give me a hint where I should hook to handle late-bound types?

[rmosolgo]

☠️ Here there be dragons 🐉

When root types are added to the schema, any indirect type references (LateBoundType, String) are accumulated in an array:

graphql-ruby/lib/graphql/schema.rb

Lines 1785 to 1787 in bace1e4

	late_types = []
	new_types = Array(t)
	new_types.each { |t| add_type(t, owner: nil, late_types: late_types, path: [t.graphql_name]) }

After the normal type references are added, the indirect references are visited:

graphql-ruby/lib/graphql/schema.rb

Line 1789 in bace1e4

while (late_type_vals = late_types.shift)

The indirect references are dereferenced in their own ways:

graphql-ruby/lib/graphql/schema.rb

Lines 1791 to 1792 in bace1e4

	if lt.is_a?(String)
	type = Member::BuildType.constantize(lt)

graphql-ruby/lib/graphql/schema.rb

Lines 1797 to 1798 in bace1e4

	elsif lt.is_a?(LateBoundType)
	if (type = get_type(lt.graphql_name))

Then passed to update_type_owner, where the type which referenced the late-bound type is modified in-place, replacing the indirect reference with a direct one (eg, reassigning some accessor to contain a Class instead of a LateBoundType:

graphql-ruby/lib/graphql/schema.rb

Lines 1821 to 1874 in bace1e4

	def update_type_owner(owner, type)
	case owner
	when Class
	if owner.kind.union?
	# It's a union with possible_types
	# Replace the item by class name
	owner.assign_type_membership_object_type(type)
	own_possible_types[owner.graphql_name] = owner.possible_types
	elsif type.kind.interface? && owner.kind.object?
	new_interfaces = []
	owner.interfaces.each do |int_t|
	if int_t.is_a?(String) && int_t == type.graphql_name
	new_interfaces << type
	elsif int_t.is_a?(LateBoundType) && int_t.graphql_name == type.graphql_name
	new_interfaces << type
	else
	# Don't re-add proper interface definitions,
	# they were probably already added, maybe with options.
	end
	end
	owner.implements(*new_interfaces)
	new_interfaces.each do |int|
	pt = own_possible_types[int.graphql_name] ||= []
	if !pt.include?(owner)
	pt << owner
	end
	end
	end
	when nil
	# It's a root type
	own_types[type.graphql_name] = type
	when GraphQL::Schema::Field, GraphQL::Schema::Argument
	orig_type = owner.type
	# Apply list/non-null wrapper as needed
	if orig_type.respond_to?(:of_type)
	transforms = []
	while (orig_type.respond_to?(:of_type))
	if orig_type.kind.non_null?
	transforms << :to_non_null_type
	elsif orig_type.kind.list?
	transforms << :to_list_type
	else
	raise "Invariant: :of_type isn't non-null or list"
	end
	orig_type = orig_type.of_type
	end
	transforms.reverse_each { |t| type = type.public_send(t) }
	end
	owner.type = type
	else
	raise "Unexpected update: #{owner.inspect} #{type.inspect}"
	end
	end

My first guess would be to hack it inside update_owner_type (which also handles fields and arguments), since that's where these indirect type references eventually end up. It seems like you might extend some of the existing code there to update (or just check?) default_value when the argument's type was an indirect reference. But uh, I wouldn't be too surprised if I was wrong about that 😆

[eapache]

Possible a simple question, possibly a complicated one: should default values be prepared? (On a quick read of the code it looks like they are, though that seems slightly counter-intuitive to me given they're specified in ruby-object-land and not graphql-syntax-land).

[eapache]

@rmosolgo I spoke with @leebyron and a few other folks working on implementations of this at the Working Group meeting yesterday. graphql-js and most others have moved to a two-pass schema building system (resolve all the types, then do a complete second pass for validations and such). This makes a lot of sense and would provide us with a cleaner solution, but is a much larger refactor than I can contribute right now.

It sounds like you're OK with #3476 or perhaps a variation like 30722fb - I'm happy to polish up and add tests for whatever set of trade-offs you'd prefer to accept. Just let me know what direction you want to go.

[rmosolgo]

Fixed by #3496