Only check used input arguments visibility #2985

grcooper · 2020-06-11T14:49:53Z

This line:

visible_arguments_map = warden.arguments(self).reduce({}) { |m, f| m[f.name] = f; m}

Was straightforward, but was also making visibility calls to arguments that we did not need to check.

I have sacrificed a small bit of readability here in order to reduce the number of visible? calls being made in warden.

There are three steps to this process to make sure it remained the same and didn't break tests:

Inject all required arguments as nil that were missing from input. This is to satisfy this test:

graphql-ruby/spec/graphql/query/executor_spec.rb

Line 293 in 39b78e5

it "returns a variable validation error" do
Check to see if the input/required args are visible on the object. Satisfies this test:

graphql-ruby/spec/graphql/query/executor_spec.rb

Line 313 in 39b78e5

it "returns a variable validation error" do
Confirm that the value being provided is valid. Satisfies this test: https://github.com/rmosolgo/graphql-ruby/pull/2985/files#diff-3f549290bd93783c3126a4eeded156ceR336

spec/graphql/query/executor_spec.rb

eapache · 2020-06-11T14:54:09Z

Inject all required arguments as nil that were missing from input.

Is this going to break .key? on the input for those arguments?

grcooper · 2020-06-11T14:58:26Z

@eapache Where are we using .key? ? merge only returns a merged hash, it doesn't mutated input so as long as I am not using it in that particular block then we are good?

lib/graphql/schema/input_object.rb

eapache · 2020-06-11T14:58:15Z

lib/graphql/schema/input_object.rb

-            argument_result = argument.type.validate_input(input[name], ctx)
-            if !argument_result.valid?
-              result.merge_result!(name, argument_result)
+          input.merge(required_inputs).each do |argument_name, value|


if we merged this the other way around we could build required_inputs much more simply?

I think the solution here using select looks something like:

required_inputs = self.arguments.select do |argument_name, argument| !input.key?(argument_name) && argument.type.non_null? && warden.get_argument(self, argument_name) end.map { |argument_name, argument| [argument_name, nil] }.to_h

I don't think this is too much better than the reduce, but can change if necessary.

eapache · 2020-06-11T15:01:44Z

Where are we using .key? ? merge only returns a merged hash, it doesn't mutated input so as long as I am not using it in that particular block then we are good?

KCCO I made some bad assumptions based on how you worded the PR description.

lib/graphql/schema/warden.rb

eapache · 2020-06-11T17:08:55Z

lib/graphql/schema/input_object.rb

-              result.add_problem("Field is not defined on #{self.graphql_name}", [name])
+          # Inject missing required arguments
+          required_inputs = self.arguments.reduce({}) do |m, (argument_name, argument)|
+            if !input.key?(argument_name) && argument.type.non_null? && warden.get_argument(self, argument_name)


Will get_argument still trigger our logging? 'cause we're still calling it here on keys that are explicitly not in the input.

Yes it will still trigger a call to visibility.

There isn't a great way around this with required arguments however, as we need to check if they are visible on the field in order to ensure that a value is being passed in for those arguments. If the argument is not visible then we don't care about it being required.

The only case we will trigger a visibility check and over-report our logging is when there is a required argument that is not supplied by the user and it is visible on the field.

If it is invisible we will still do the visibility check but it will not log.

The only case we will trigger a visibility check and over-report our logging is when there is a required argument that is not supplied by the user and it is visible on the field.

These requests are broken anyway, correct, meaning they'll fail with an error for a missing argument?

I think this is probably a solid upstream improvement and good short-term fix for our issue (since IIRC none of the args we care about short-term are required), but the corner case worries me a bit. Let's ship this, but continue to think about a long-term solution.

eapache · 2020-06-11T18:21:47Z

lib/graphql/schema/input_object.rb

-              result.add_problem("Field is not defined on #{self.graphql_name}", [name])
+          # Inject missing required arguments
+          required_inputs = self.arguments.reduce({}) do |m, (argument_name, argument)|
+            if !input.key?(argument_name) && argument.type.non_null? && warden.get_argument(self, argument_name)


The only case we will trigger a visibility check and over-report our logging is when there is a required argument that is not supplied by the user and it is visible on the field.

These requests are broken anyway, correct, meaning they'll fail with an error for a missing argument?

I think this is probably a solid upstream improvement and good short-term fix for our issue (since IIRC none of the args we care about short-term are required), but the corner case worries me a bit. Let's ship this, but continue to think about a long-term solution.

eapache · 2020-06-11T18:22:45Z

lib/graphql/schema/input_object.rb

-            if visible_arguments_map[name].nil?
-              result.add_problem("Field is not defined on #{self.graphql_name}", [name])
+          # Inject missing required arguments
+          required_inputs = self.arguments.reduce({}) do |m, (argument_name, argument)|


I might call this missing_required_inputs

eapache · 2020-06-11T18:24:03Z

lib/graphql/schema/warden.rb

@@ -106,6 +106,12 @@ def get_field(parent_type, field_name)
        @visible_parent_fields[parent_type][field_name]
      end

+      # @return [GraphQL::Argument, nil] The argument named `argument_name` on `parent_type`, if it exists and is visible
+      def get_argument(parent_type, argument_name)


I believe the intent of this method was to be cached which it is not right now?

visible_argument?(argument) is cached through the visible? method on types. So I believe this is ok to leave non-cached?

I can make it more similar to the above methods though and cache it further.

I'll defer to @rmosolgo's judgement here. I did note that we are now calling get_argument twice on the same argument in some scenarios.

Yeah, the call to the application's implementation would be cached, I don't know whether using read_through { ... } here is worth it or not, no preference without actually seeing a benchmark.

eapache · 2020-06-11T18:25:12Z

lib/graphql/schema/input_object.rb

            end
+            # Items in the input that are expected, but have invalid values
+            argument_result = argument.type.validate_input(value, ctx)


@rmosolgo it's weird to me that we handle missing required inputs by adding them as nil, and then letting the validation fail that nil isn't an accepted value? Could we not just add_problem directly when we notice they're missing?

Yah I was also not sure about this behaviour, I would assume this should actually be handled by a different part of code.

We can add a specific problem if we notice they are missing rather than doing what I am currently doing.

adding them as nil

Do you mean, it worked this way in the old code? AFAIK the old code checked each argument and made sure that the provided value was valid, without special nil handling. I see that in the proposed change here, it has to handle nil differently.

As I understand it the old code was kind of implicit - it checks every visible argument, and if the argument was required but had no value provided, then argument.type.validate_input would produce an error.

Now we're being a bit more explicit about generating a list of missing_required_inputs. Rather than merging those explicit nils into the input just so we can call validate_input on nil to generate the error... I think we could just generate the error instead of building missing_required_inputs at all?

generate the error instead of building missing_required_inputs

Ah, yeah, fine with me. (And I guess it must have worked this way in the past, if it wasn't triggering .visible? 🤔 )

generate the error instead of building missing_required_inputs

In that case, I'm a bit unsure of how this should interact/overlap with the existing static validation of input object attributes.

A casual reading of that file initially suggested to me that this check would be performed earlier, removing the need to check for required + visible + not provided input arguments via GraphQL::Schema::InputObject.validate_non_null_input.

On a related note: the following diff produces errors only in tests related to variable coercion, which makes me think that the passing tests for required input object arguments are hitting only the static validation checks.

diff

diff --git a/lib/graphql/schema/input_object.rb b/lib/graphql/schema/input_object.rb index c4fb241fd..0a3a4c52d 100644 --- a/lib/graphql/schema/input_object.rb +++ b/lib/graphql/schema/input_object.rb @@ -181,16 +181,13 @@ module GraphQL end end - # Inject missing required arguments - missing_required_inputs = self.arguments.reduce({}) do |m, (argument_name, argument)| + self.arguments.each do |argument_name, argument| if !input.key?(argument_name) && argument.type.non_null? && warden.get_argument(self, argument_name) - m[argument_name] = nil + result.add_problem("TODO on #{self.graphql_name}", [argument_name]) end - - m end - input.merge(missing_required_inputs).each do |argument_name, value| + input.each do |argument_name, value| argument = warden.get_argument(self, argument_name) # Items in the input that are unexpected unless argument

test output

Failure: GraphQL::Query::Executor::variable coercion::for input objects with unknown keys in value#test_0001_returns a variable validation error [/Users/chris/src/github.com/grcooper/graphql-ruby/spec/graphql/query/executor_spec.rb:329] Minitest::Assertion: --- expected +++ actual @@ -1,11 +1,10 @@ {"errors"=> [{"message"=> - "Variable $input of type [DairyProductInput] was provided invalid value for 0.foo (Field is not defined on DairyProductInput), 0.source (Expected value to not be null)", + "Variable $input of type [DairyProductInput] was provided invalid value for 0.source (TODO on DairyProductInput), 0.foo (Field is not defined on DairyProductInput)", "locations"=>[{"line"=>1, "column"=>10}], "extensions"=> {"value"=>[{"foo"=>"bar"}], "problems"=> - [{"path"=>[0, "foo"], - "explanation"=>"Field is not defined on DairyProductInput"}, - {"path"=>[0, "source"], - "explanation"=>"Expected value to not be null"}]}}]} + [{"path"=>[0, "source"], "explanation"=>"TODO on DairyProductInput"}, + {"path"=>[0, "foo"], + "explanation"=>"Field is not defined on DairyProductInput"}]}}]} Failure: GraphQL::Query::Executor::variable coercion::for required input object fields#test_0001_returns a variable validation error [/Users/chris/src/github.com/grcooper/graphql-ruby/spec/graphql/query/executor_spec.rb:306] Minitest::Assertion: --- expected +++ actual @@ -1,9 +1 @@ -{"errors"=> - [{"message"=> - "Variable $input of type ReplaceValuesInput! was provided invalid value for values (Expected value to not be null)", - "locations"=>[{"line"=>1, "column"=>13}], - "extensions"=> - {"value"=>{}, - "problems"=> - [{"path"=>["values"], - "explanation"=>"Expected value to not be null"}]}}]} +#<GraphQL::Query::Result @query=... @to_h={"errors"=>[{"message"=>"Variable $input of type ReplaceValuesInput! was provided invalid value for values (TODO on ReplaceValuesInput)", "locations"=>[{"line"=>1, "column"=>13}], "extensions"=>{"value"=>{}, "problems"=>[{"path"=>["values"], "explanation"=>"TODO on ReplaceValuesInput"}]}}]}> 1602 tests, 7612 assertions, 2 failures, 0 errors, 0 skips

My original comment here was an architecture/style question, not a behavioural one. It sounds like there's more we need to dig into here, but if we're confident that the current code works correctly could we get that out and then follow up on the other points?

chrisbutcher · 2020-06-11T20:37:25Z

👋 I'm taking over the PR as of today while Graham is 🌴

chrisbutcher · 2020-06-12T14:36:50Z

lib/graphql/schema/input_object.rb

-            argument_result = argument.type.validate_input(input[name], ctx)
-            if !argument_result.valid?
-              result.merge_result!(name, argument_result)
+          input.to_h.merge(required_inputs).each do |argument_name, value|


Trying this out in Shopify, input is sometimes a ActionController::Parameters instance, e.g. <ActionController::Parameters {"barcode"=>"53941047", "id"=>"gid://shopify/ProductVariant/30322695"} permitted: false>.

This causes input.to_h to raise ActionController::UnfilteredParameters: unable to convert unpermitted parameters to hash.

I notice that above this line we have this block:

begin input.to_h rescue begin # Handle ActionController::Parameters: input.to_unsafe_h rescue # We're not sure it'll act like a hash, so reject it: result.add_problem(INVALID_OBJECT_MESSAGE % { object: JSON.generate(input, quirks_mode: true) }) return result end end

Which doesn't actually convert input if it's a ActionController::Parameters. Should it do so?

Side question: Should we be permitting, to_h'ing input earlier?

Elsewhere in this gem, I see handling for this same scenario, checking if value.respond_to?(:to_unsafe_h).

Previously, it required that .each yielded key-value pairs. I think making a hash out of it is fine, if you have to, too 🤷

rmosolgo · 2020-06-12T17:04:54Z

lib/graphql/schema/warden.rb

@@ -106,6 +106,12 @@ def get_field(parent_type, field_name)
        @visible_parent_fields[parent_type][field_name]
      end

+      # @return [GraphQL::Argument, nil] The argument named `argument_name` on `parent_type`, if it exists and is visible
+      def get_argument(parent_type, argument_name)
+        argument = parent_type.arguments[argument_name]


#arguments climbs the inheritance chain and merges argument definitions into a hash. When you only need one object, better to use .get_argument which searches the chain until it finds a hit, and doesn't merge any hashes:

Suggested change

argument = parent_type.arguments[argument_name]

argument = parent_type.get_argument(argument_name)

👀 I may be missing something, but I don't see get_argument defined on GraphQL::Schema::InputObject

https://graphql-ruby.org/api-doc/1.10.11/GraphQL/Schema/InputObject.html

Ah, sorry, I was thinking it would be around here, but I was mistaken:

graphql-ruby/lib/graphql/schema/member/has_arguments.rb

Lines 50 to 59 in 12947f4

# @return [Hash<String => GraphQL::Schema::Argument] Arguments defined on this thing, keyed by name. Includes inherited definitions

def arguments

inherited_arguments = ((self.is_a?(Class) && superclass.respond_to?(:arguments)) ? superclass.arguments : nil)

# Local definitions override inherited ones

if inherited_arguments

inherited_arguments.merge(own_arguments)

else

own_arguments

end

end

It could be added like get_field:

graphql-ruby/lib/graphql/schema/member/has_fields.rb

Lines 29 to 40 in 12947f4

def get_field(field_name)

if (f = own_fields[field_name])

f

else

for ancestor in ancestors

if ancestor.respond_to?(:own_fields) && f = ancestor.own_fields[field_name]

return f

end

end

nil

end

end

Good idea! Added :)

rmosolgo · 2020-06-12T17:05:33Z

lib/graphql/schema/warden.rb

@@ -106,6 +106,12 @@ def get_field(parent_type, field_name)
        @visible_parent_fields[parent_type][field_name]
      end

+      # @return [GraphQL::Argument, nil] The argument named `argument_name` on `parent_type`, if it exists and is visible
+      def get_argument(parent_type, argument_name)


Yeah, the call to the application's implementation would be cached, I don't know whether using read_through { ... } here is worth it or not, no preference without actually seeing a benchmark.

rmosolgo · 2020-06-12T17:31:29Z

lib/graphql/schema/input_object.rb

            end
+            # Items in the input that are expected, but have invalid values
+            argument_result = argument.type.validate_input(value, ctx)


adding them as nil

Do you mean, it worked this way in the old code? AFAIK the old code checked each argument and made sure that the provided value was valid, without special nil handling. I see that in the proposed change here, it has to handle nil differently.

rmosolgo · 2020-06-12T17:45:47Z

lib/graphql/schema/input_object.rb

-            if visible_arguments_map[name].nil?
-              result.add_problem("Field is not defined on #{self.graphql_name}", [name])
+          # Inject missing required arguments
+          required_inputs = self.arguments.reduce({}) do |m, (argument_name, argument)|


self.arguments can be expensive, for input objects that inherit arguments from their parent classes, because it merges hashes. In practice, it might not happen much, and the most likely case (no inherited arguments) is optimized to return own_arguments. But, the nice part about warden.arguments(self) here was that it was cached in any case. Not a show-stopper but thought I'd mention it in case it comes up later.

chrisbutcher · 2020-06-12T18:02:48Z

lib/graphql/schema/input_object.rb

@@ -171,7 +171,7 @@ def validate_non_null_input(input, ctx)
          # We're not actually _using_ the coerced result, we're just
          # using these methods to make sure that the object will
          # behave like a hash below, when we call `each` on it.
-          begin
+          input = begin


This resolves #2985 (comment)

The comment on this block is now wrong

Good catch! 🔥

eapache · 2020-06-12T20:00:16Z

lib/graphql/schema/input_object.rb

-            if visible_arguments_map[name].nil?
-              result.add_problem("Field is not defined on #{self.graphql_name}", [name])
+          # Inject missing required arguments
+          missing_required_inputs = self.arguments.reduce({}) do |m, (argument_name, argument)|


this hash has entirely nil values... I don't know if there's actually a nicer way to build it, or if we could get away with just an array of keys instead or something? but don't let it hold this up in the short term

eapache

There's some follow-up work to do but this solves our immediate problem so let's stop the bleeding.

rmosolgo · 2020-06-16T17:32:56Z

Just shipped in 1.10.13!

Thanks @grcooper for opening the followup issue. I'll add some thoughts there

…arguments-visibility Only check used input arguments visibility

Replaces more usages of `warden.arguments` during static validation (since they trigger `visible?` calls for all arguments on a type regardless if they are specified or not) with more selective `warden.get_argument`. rmosolgo#2985 which fixed some of these occurrences, but not all of them. This also required additional backwards compatible work to port `get_argument` to the legacy types.

…arguments-visibility Only check used input arguments visibility

Replaces more usages of `warden.arguments` during static validation (since they trigger `visible?` calls for all arguments on a type regardless if they are specified or not) with more selective `warden.get_argument`. rmosolgo#2985 which fixed some of these occurrences, but not all of them. This also required additional backwards compatible work to port `get_argument` to the legacy types.

swalkinshaw reviewed Jun 11, 2020

View reviewed changes

spec/graphql/query/executor_spec.rb Outdated Show resolved Hide resolved

eapache reviewed Jun 11, 2020

View reviewed changes

chrisbutcher reviewed Jun 11, 2020

View reviewed changes

lib/graphql/schema/warden.rb Outdated Show resolved Hide resolved

grcooper force-pushed the only-check-used-input-arguments-visibility branch from 1bae945 to b14b8c3 Compare June 11, 2020 15:56

eapache reviewed Jun 11, 2020

View reviewed changes

chrisbutcher reviewed Jun 12, 2020

View reviewed changes

rmosolgo reviewed Jun 12, 2020

View reviewed changes

chrisbutcher reviewed Jun 12, 2020

View reviewed changes

eapache reviewed Jun 12, 2020

View reviewed changes

rmosolgo changed the base branch from master to 1.10.x June 13, 2020 12:21

eapache approved these changes Jun 15, 2020

View reviewed changes

chrisbutcher force-pushed the only-check-used-input-arguments-visibility branch 2 times, most recently from 61b2604 to f2dcfa6 Compare June 16, 2020 13:55

grcooper and others added 4 commits June 16, 2020 09:58

Only check included and required inputs for input object validation

a7ae596

keep .to_h'd input, rename var to missing_required_inputs

342d5a3

Add HasArguments#get_argument

6a2add0

Remove outdated comment

7408d45

chrisbutcher force-pushed the only-check-used-input-arguments-visibility branch from f2dcfa6 to 7408d45 Compare June 16, 2020 13:58

grcooper mentioned this pull request Jun 16, 2020

[Rework] Input argument validation #2994

Open

rmosolgo merged commit 324aa6d into rmosolgo:1.10.x Jun 16, 2020

swalkinshaw pushed a commit to swalkinshaw/graphql-ruby that referenced this pull request Jun 17, 2020

Merge pull request rmosolgo#2985 from grcooper/only-check-used-input-…

fc97455

…arguments-visibility Only check used input arguments visibility

swalkinshaw mentioned this pull request Jun 17, 2020

Remove more visible? calls to unused arguments #3000

Merged

swalkinshaw pushed a commit to swalkinshaw/graphql-ruby that referenced this pull request Jun 17, 2020

Merge pull request rmosolgo#2985 from grcooper/only-check-used-input-…

0684cc3

…arguments-visibility Only check used input arguments visibility

rmosolgo added this to the 1.10.13 milestone Jun 23, 2020

swalkinshaw mentioned this pull request Jul 14, 2020

Only check used input arguments visibility [for 1.11.x] #3030

Merged

	argument = parent_type.arguments[argument_name]
	argument = parent_type.get_argument(argument_name)

	# @return [Hash<String => GraphQL::Schema::Argument] Arguments defined on this thing, keyed by name. Includes inherited definitions
	def arguments
	inherited_arguments = ((self.is_a?(Class) && superclass.respond_to?(:arguments)) ? superclass.arguments : nil)
	# Local definitions override inherited ones
	if inherited_arguments
	inherited_arguments.merge(own_arguments)
	else
	own_arguments
	end
	end

	def get_field(field_name)
	if (f = own_fields[field_name])
	f
	else
	for ancestor in ancestors
	if ancestor.respond_to?(:own_fields) && f = ancestor.own_fields[field_name]
	return f
	end
	end
	nil
	end
	end

Only check used input arguments visibility #2985

Only check used input arguments visibility #2985

Conversation

grcooper commented Jun 11, 2020 • edited Loading

eapache commented Jun 11, 2020

grcooper commented Jun 11, 2020

Choose a reason for hiding this comment

grcooper Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

eapache commented Jun 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbutcher commented Jun 11, 2020

chrisbutcher Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapache left a comment

Choose a reason for hiding this comment

rmosolgo commented Jun 16, 2020

grcooper commented Jun 11, 2020 •

edited

Loading

grcooper Jun 11, 2020 •

edited

Loading

chrisbutcher Jun 12, 2020 •

edited

Loading