Validation of argument default values

[danielrearden]

Any default values provided when defining an argument are currently not validated against that argument's type. So a Float argument could be passed a string as a default value and this schema will be considered valid. Moreover, the default value will be passed as-is to the resolver instead of being coerced into an appropriate value.

As far as I can tell, the current behavior is inline with the spec. But it seems counterintuitive that doing something like this would be valid:

const assert = require('assert')
const { buildSchema, validateSchema } = require('graphql')

const schema = buildSchema(`
  type Query {
    foo(bar: Float = "ABC"): Float
  }
`)

const errors = validateSchema(schema)
assert(errors.length > 0)

We do validate default values on variables, so I think it's reasonable to expect at least consistency with that behavior. At the very least, I think validateSchema should be modified to validate the provided values against the argument type. I'm less certain about coercing the values, but off-hand it seems like desirable behavior as well.

[IvanGoncharov]

@danielrearden Thanks for extracting this issue 👍
Meta: I added PR: labels to generate changelog and to track PRs I think it's just a bug and if something could be breaking for user than you just setup milestone to the next major release.

It's a general issue with SDL validation and should be done in validateSDL(called by buildSchema) and not in validateSchema.
validateSchema happening too late in the chain and should be reserved only for stuff that can't be validated earlier. It's important because validateSDL can be used by LSP since it supports incomplete schema and is pretty close to AST, but validateSchema expect full schema (all extension are merged, query root type defined, etc.).

It's even more serious in a sense we don't validate argument of directives it's serious problem related to how validateSDL works (it doesn't have ability to work with built types only AST node for them) since it's hard to validate default value defined in the same SDL as type it should be validated against:

input SomeInput {
  field: SomeOtherInputObject = { enum: INVALID }
}

input SomeOtherInputObject {
  enum: SomeEnum
}

enum SomeEnum {
  TEST
}

It's planned for 16.0.0-alpha that we hopefully start somewhere around next month.

[danielrearden]

@IvanGoncharov Thanks for the context and for the clarification around labeling.

I used SDL in the above example, but the same issue applies to schemas built programmatically:

const assert = require('assert')
const {
  validateSchema, 
  GraphQLFloat,
  GraphQLObjectType,
  GraphQLSchema
} = require('graphql')

const schema = new GraphQLSchema({
  query: new GraphQLObjectType({
    name: 'Query',
    fields: {
      foo: {
        args: {
          bar: {
            type: GraphQLFloat,
            defaultValue: 'ABC',
          }
        },
        type: GraphQLFloat,
      }
    }
  })
})

const errors = validateSchema(schema)
assert(errors.length > 0)

[IvanGoncharov]

@danielrearden It's kind of hard to do validation of default values inside GraphQL*Types since they are in internal representation and scalars (custom and standard) doesn't have a way to check if internal representation is correct or not.
So it also a non-trivial issue that we need to try to solve in 16.0.0-alpha.

[danielrearden]

👍 Just wanted to clarify the scope of the issue.

[danielrearden]

Although... I do wonder if we couldn't just utilize coerceInputValue for this 🤔

[IvanGoncharov]

@danielrearden coerceInputValue transforms from variables into "internal" representation so we can't use it for validation. Technically we can use scalar.serialize(value) since "serialization should validate "internal" representation but it's an awkward method. This issue is not critical and almost certainly requires some API changes so it's better to leave it until "alpha".

[IvanGoncharov]

@danielrearden To be honest default values and custom scalars are two most problematic places at the moment and their combination is an absolute nightmare.
With so many issues including issues with GraphQL spec itself that it deserves its own dedicated refactoring.

[spawnia]

@IvanGoncharov Can scalar.parseLiteral(value) be used to validate correctness, or am I missing something here?

It appears that valueFromAST() is already used in that way:

graphql-js/src/utilities/valueFromAST.js

Line 137 in a75e95b

result = type.parseLiteral(valueNode, variables);

. To make it suitable for validation, we would have to let the errors bubble up so it is possible to differentiate between having no default value and a wrong default value.

[yaacovCR]

Closing this issue, tracked in #3814