[Rework] Input argument validation

[grcooper]

This PR #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

• Right now this makes a list of the required input args that are missing and sets them to be nil.
• This is a bit of a waste of space, perhaps it could be a list of keys rather than a hash of nil values?
• Is there a better way to handle required arguments rather than stating they are not nil? Should this be handled later down the stack?
• 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)

---

2. 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 Only check used input arguments visibility #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.

[Adeynack]

---

EDIT: The problem was on my side, but didn't find it in time to remove this message before it got answered. More here.

---

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

The error I get does not include any path, sadly. Is there some configuration I might be missing to include more detail in the errors? Or an extension?

It is in this current state very difficult to understand what was missing or wrong.

I can see here and in this current issue that the problem is supposed to be resolved, but I can't observe that on my own use.

Here's what I get:

 [
    {
      "message": "Variable $register of type RegisterForCreateInput! was provided invalid value",
      "locations": [
        {
          "line": 1,
          "column": 25
        }
      ],
      "extensions": {
        "value": null,
        "problems": [
          {
            "path": [

            ],
            "explanation": "Expected value to not be null"
          }
        ]
      }
    }
  ]

More information about the query and arguments:

    mutation CreateRegister($register: RegisterForCreateInput!) {
      createRegister(input: {register: $register}) {
        register {
          id
          createdAt
          name
          type
          bookId
          parentId
          startsAt
          expiresAt
          currencyIsoCode
          notes
          initialBalance
          active
          defaultCategoryId
          institutionName
          accountNumber
          iban
          annualInterestRate
          creditLimit
          cardNumber
        }
      }
    }

    {
      "name": "InCAD",
      "type": "Income",
      "bookId": "e5d3b0b6-908b-49a7-a771-62c40d5bad96",
      "initialBalance": 0,
      "currencyIsoCode": "CAD",
      "active": true
    }

[grcooper]

@Adeynack You are not hitting the same case in your example.

In the case that the PR I created originally solves is when you don't pass in a variable that should be there, we then insert nil automatically, which my PR that was merged solved. In that case, I added a path to the add_problem method:

https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/schema/input_object.rb#L180

Unfortunately for the case you are hitting:

https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/schema/input_object.rb#L183

This does not actually add a path to the validate_input method which means that it is not added to the error output.

What you are asking does seem to be a problem, but is probably worth a different ticket, as that seems to be the case in a couple of places where we are not passing along the path to validate_input which then calls add_problem without it.

[grcooper]

As an aside, I realize I never got back to this issue... I switched some teams and stopped working on this stuff. @rmosolgo looking at the code I think this is still kind of a problem, but it works for now? I can go back and do my due-diligence and fix this up though if we think it's worth fixing. It's been a couple of years since I've looked at the code so I'm unsure if things have changed to make this not relevant.

[Adeynack]

@grcooper : I found my mistake and it's a case of facepalm – and I am sorry I did not delete my message in time before you answered! – I am simply not providing the register root object at all in my variables. That is my problem. Not the library, in this case.

When I change my variables from

    {
      "name": "InCAD" // etc...
    }

to

    {
      "register": {
          "name": "InCAD" // etc...
      }
    }

of course then it works.

Thanks for your quick reply @grcooper !