Graphql-ruby: [Rework] Input argument validation

[grcooper]

This PR https://github.com/rmosolgo/graphql-ruby/pull/2985 changed how we do input validation.

The PR was pushed along fairly quickly and this issue is here to capture the questions that were asked but unanswered in favour of time to ship.

1. We need to handle required arguments better: https://github.com/rmosolgo/graphql-ruby/pull/2985/files#diff-2956f3a6180049cec79c9edd49fe26f3R185
2. Right now this makes a list of the required input args that are missing and sets them to be nil.
3. This is a bit of a waste of space, perhaps it could be a list of keys rather than a hash of nil values?
4. Is there a better way to handle required arguments rather than stating they are not nil? Should this be handled later down the stack?
5. We could generate the nil error more manually for required arguments rather than doing it automatically here: https://github.com/rmosolgo/graphql-ruby/pull/2985/files#diff-2956f3a6180049cec79c9edd49fe26f3R201

Recommendation for 1:
I believe that the best solution for 1 is to handle required arguments in their own section rather than doing it automatically with a nil check.

This would be something along the lines of:

for arguments in type do
  add_error if argument.required && input[argument].nil?
end

Do we want this error messaging to stay the same however, or do we want to change it to something more specific around required arguments?

Current error messages look like:
"Variable $input of type [DairyProductInput] was provided invalid value for 0.source (Expected value to not be null)"

This happens when source is a required input. (Taken from the test here: https://github.com/rmosolgo/graphql-ruby/pull/2985/files#diff-3f549290bd93783c3126a4eeded156ceR317)

---

1. Benchmark the new get_argument method and see if it needs further caching: https://github.com/rmosolgo/graphql-ruby/pull/2985/files#diff-edaedf574068dd9aee5988de3b6198fcR110

[rmosolgo]

The other thing on my mind is, the _reason_ for that previous PR (IIUC) is because Shopify expects .visible? to only be called for arguments which are present in the query. That expectation wasn't satisfied in 1.10.x, and still isn't, as in the case of required-but-absent arguments. We should either:

• Continue leaving that behavior _undefined_ in graphql-ruby, and document that there's no guarantee of when .visible? will be called;
• Or, add tests for the expectation which was restored (-ish) in #2985, so that it's not broken in the future.

Did I understand that issue previously? How do y'all want to go forward with it?

[eapache]

That's a great summary, thanks Robert. We're chatting internally but we're coming around to the idea that the logging we want should ultimately be decoupled from visible? - that's going to be a bigger project for us though.

Related to that: there's a weaker/inverse expectation we're using that visible? is called for at least all the schema elements which are present in the query. This seems more obviously true (I hope) and probably easier to enforce. It would be good to explicitly support (or drop) that expectation as well, separate from the one you mention.