[GraphQL::Schema::Visibility] Change in Static Validation of Variable Types

[cocoahero]

When using the GraphQL::Schema::Visibility plugin, the behavior of how static validation of variables changes. Consider the following example schema:

class MyInputType < GraphQL::Schema::InputObject
  argument :some_field, String, required: true
end

class MyInternalMutation < GraphQL::Schema::Mutation
  argument :input, MyInputType, required: true

  field :success, Boolean, null: false

  def self.visible?(ctx)
    ctx[:internal_client] == true
  end

  def resolve(input:)
    { success: true }
  end
end

class MutationRoot < GraphQL::Schema::Object
  field :my_mutation, mutation: MyInternalMutation
end

class MySchema < GraphQL::Schema
  use(GraphQL::Schema::Visibility)

  mutation(MutationRoot)
end

query = <<~GRAPHQL
  mutation MyMutation($input: MyInput!) {
    myMutation(input: $input) {
      success
    }
  }
GRAPHQL

result = MySchema.execute(
  query,
  context: { internal_client: false },
  variables: { "input" => { "someField" => "someValue" } },
)

When operating in GraphQL::Schema::Warden mode, the following expected error is returned:

{"errors":[{"message":"Field 'myMutation' doesn't exist on type 'MutationRoot'","locations":[{"line":2,"column":3}],"path":["mutation MyMutation","myMutation"],"extensions":{"code":"undefinedField","typeName":"MutationRoot","fieldName":"myMutation"}},{"message":"Variable $input is declared by MyMutation but not used","locations":[{"line":1,"column":1}],"path":["mutation MyMutation"],"extensions":{"code":"variableNotUsed","variableName":"input"}}]}

However, with visibility, we get a different error stating that the otherwise public input type does not exist:

{"errors":[{"message":"MyInput isn't a defined input type (on $input)","locations":[{"line":1,"column":21}],"path":["mutation MyMutation"],"extensions":{"code":"variableRequiresValidType","typeName":"MyInput","variableName":"input"}},{"message":"Field 'myMutation' doesn't exist on type 'MutationRoot'","locations":[{"line":2,"column":3}],"path":["mutation MyMutation","myMutation"],"extensions":{"code":"undefinedField","typeName":"MutationRoot","fieldName":"myMutation"}},{"message":"Variable $input is declared by MyMutation but not used","locations":[{"line":1,"column":1}],"path":["mutation MyMutation"],"extensions":{"code":"variableNotUsed","variableName":"input"}}]}

It is unclear whether this is a regression or just a change in behavior. I believe it is due to the fact that MyInput is unreachable by any public path, but this was not something enforced by warden.

[rmosolgo]

It seems like they're both valid (er, they're both reasons the query is invalid...) I wonder if the spec prescribes an order to validations.

[cocoahero]

It seems like they're both valid (er, they're both reasons the query is invalid...)

Yeah, thus the dilemma.

I only mention it because we have tests against the old behavior, designed to check visibility based access controls. It’s easy enough to change the string matched, but it feels incorrect to base an expectation on an indirect symptom. If the input type were to ever be used somewhere else publicly, the expectation would have to be changed again.