parameterized query variable values aren't passed to parseLiteral of GraphQLScalarTypes

[jedwards1211]

When I call my login method like this, it calls my Password type's parseLiteral method which throws an error as expected:

query {
  login(username:"andy", password:"aaaaaaaaa") {
    authToken
  }
}

Result:

{ errors: 
   [ [Error: Password did not met strength requirements:
     The password must be at least 10 characters long.
     The password may not contain sequences of three or more repeated characters.] ] }

But if I change that to a parameterized query with the same values in the variables, it never calls my Password type's parseLiteral, and it succeeds. Am I misunderstanding something, or is this a bug? I couldn't find anything in the documentation about different validation behavior for parameterized queries.

query ($username: Username, $email: Email, $password: Password!) {
  login(username: $username, email: $email, password: $password) {
    authToken
  }
}

Variables:

{
  "username": "andy",
  "password": "aaaaaaaaa"
}

Result:

{
  "data": {
    "login": {
      "authToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6IjFlMzM3MzliLTlhYjktNDczZi04NTMxLThmYjhkOWFjMjYyZCIsImlhdCI6MTQ2ODQ2NTc2MywiZXhwIjoxNDY5MDcwNTYzfQ.pcedaOASV8U2-fBHnGxQlHs26SyhuTzLZC1T3EVgndI"
    }
  }
}

Here is my Password type:

export const GraphQLPasswordType = new GraphQLScalarType({
  name: 'Password',
  serialize: value => String(value),
  parseValue: value => String(value),
  parseLiteral: ast => {
    if (ast.kind !== Kind.STRING) {
      throw new GraphQLError(`Password is not a string, it is a: ${ast.kind}`, [ast])
    }
    const result = testPassword(ast.value)
    if (result.requiredTestErrors && result.requiredTestErrors.length) {
      throw new GraphQLError(`Password did not met strength requirements:
${result.requiredTestErrors.join('\n')}`, [ast])
    }
    return String(ast.value)
  }
})

And the login query schema:

  login: {
    type: UserWithAuthToken,
    args: {
      username: {type: GraphQLUsernameType},
      email: {type: GraphQLEmailType},
      password: {type: new GraphQLNonNull(GraphQLPasswordType)}
    },
    async resolve(source, args) {
      ...

[leebyron]

This is correct behavior - I'm curious to know what code or docs you read that lead you to this? I'd like to improve them to avoid this confusion in the future.

parseLiteral is responsible for taking a literal (in the form of an AST) and parsing it into an internally useful value.

parseValue is responsible for taking a runtime-provided value (in the form of a JS value) and coercing it into an internally useful value.

Since parseValue doesn't have the same checking as parseLiteral, that is why you're not seeing the same errors for runtime provided variable values as you do for in-query written literals.

[leebyron]

However - unrelated to your issue but just a point of critique based on best practices:

Custom scalars are really intended to be used when the underlying programming environment contains a type of scalar which is semantically meaningfully different which you want to expose via your API (e.g. Floats vs Ints). Emails, Passwords, Usernames, etc are typically all easily represented internally as String, and so most GraphQL APIs also represent them as String.

[jedwards1211]

Okay, that makes sense, the docs don't go into much detail about parseLiteral and parseValue so I wasn't sure what happens in this case.

I got this idea from Meatier, which defines custom scalars for emails, passwords, URLs, etc. I'll let Matt Krick, the maintainer, know...

[jedwards1211]

@leebyron I think this kind of confusion could persist without careful wording in the documentation. For instance, who would consider a query with email: ":)" valid?
Maybe the docs need to explain the difference between syntactic and semantic validity?

[jedwards1211]

@leebyron plus I can understand the temptation to use GraphQL for all validation, for the sake of not having to write a second layer of validation.

[jedwards1211]

mattkrick/meatier#13

Now that I'm using graphql on the server, I use custom scalars to do
validation there, eliminating the need for JOI on the server.
Once I get a client cache built, my intent is to use those same custom
scalars on the client. It'll take a little tweaking to eliminate the
dependencies that the custom scalars need, but the end result will be using
graphql as a query language and validator universally. Stay tuned

If it's easy enough to use GraphQL for all validation, it might become common...

[leebyron]

I suppose so - in my opinion it's a mental burden cost on someone using your API in exchange for reduced work on behalf of the server, usually a bad trade. In my experience I've found validation being documented and implemented relative to field arguments to be easier to follow.

I think this probably is a good argument in favor of some of the ideas discussed in #361 around providing better input validation functionality without requiring the creation of new scalar types.

Honestly though it's a fairly minor point, I don't think you're likely to end up with any serious issues because of custom scalars vs any other form of input validation. There are definitely pros and cons to all approaches here and I should be careful to speak too much about best practices while GraphQL is still a young technology :)

[jedwards1211]

I don't completely understand your first paragraph there, if GraphQLScalars validate fields isn't that literally validation "implemented relative to field arguments"? And I don't understand how it would affect the documentation. For instance explaining what emails and passwords are valid is independent of how the validation is implemented. Or are you saying that the structure of error messages would change and that would be harder to document?

[leebyron]

Sorry for the confusion, let me restate.

Validation relative to field arguments would be defining the validation rules next to the field argument itself, this is what was proposed in #361.

field: {
  type: WhateverType,
  args: {
    someArg: { type: String }
  }
  resolve(obj, { someArg }) {
    if (!myValidationFn(someArg)) {
      throw new Error('Informative message');
    }
    // ...
  }
}

This in contrast to implementing validation relative to types which is what defining types for each kind of input is doing.

As per documentation, it's mostly a question about knowing what is or isn't allowed for a given type. E.g. if I had a custom scalar called Date, is it safe to pass a unix timestamp numerically? Or is it safe to pass a string-formatted date? If a type is String or Int instead, you know exactly what you can and can't send. Not to say that it's a clear win on either side of this - obviously having custom types also implies something about what input is allowed and that's a good thing, it's more something to think about and consider tradeoffs.

[jedwards1211]

Ah gotcha. Yeah that makes sense. Here's why I'm leaning toward validation relative to types though: validation relative to the field makes a semantic distinction between the type and the valid forms of that type for that field. But all types have a specific set of valid forms to begin with, so is it a useful distinction? In my mind, it's simpler to just represent emails as a separate type. Besides, it's an easier way to ensure that my queries never get run with invalid emails (if I use validation relative to fields, I have to refer to my email validator in every email field in any query).

Now if it comes down to checking that a field makes sense in the environment, e.g. checking if an email is in the system, I definitely wouldn't want to check that in a custom scalar. But any restrictions that don't depend on context seem inherent to the type to me.

[jedwards1211]

@leebyron since exploring this more I've realized that it's not very easy with custom scalars to generate errors that indicate which field was invalid. I guess it might be possible for GraphQL to return the field name in the errors someday, but for the time being I've just gone back to validating arguments in resolve.

[yaacovCR]

Closing this as I believe the original question was answered.