Scalar ParseValue not validating variable inputs

[ghost]

I have a scalar type which I use for validating inputs.
I have a mutation which is used to create an object with an ID. The length of the ID should be minimum 3 characters. So if I have a mutation like this,

mutation M {
     CreateObject(id: "a")
}

this fails as the input a is less that 3 characters. But If I use a mutation with variables like this.

Mutation M($input: String|){
   CreateObject(id: $input)
}
with variables
{
   "input": "a"
}

It passes. As per graphql-js the variables are sent to the parseValue function to get validated and any error with throw an error. This doesn't seem to be happening.
Here is the example code

package main

import (
    "errors"
    "fmt"

    "github.com/graphql-go/graphql"
    "github.com/graphql-go/graphql/language/ast"
    "github.com/graphql-go/graphql/language/kinds"
)

func validate(value string) error {
    if len(value) < 3 {
        return errors.New("The minimum length required is 3")
    }
    return nil
}

func main() {
    ID := graphql.NewScalar(graphql.ScalarConfig{
        Name: "ID",
        Serialize: func(value interface{}) interface{} {
            println("Serialize")
            return value
        },
        ParseValue: func(value interface{}) interface{} {
            println("parsing Value")
            var err error
            switch value.(type) {
            case string:
                err = validate(value.(string))
            default:
                err = errors.New("Must be of type string")
            }
            if err != nil {
                panic(err) // TODO: This panic kills the server
            }
            return value
        },
        ParseLiteral: func(valueAst ast.Value) interface{} {
            println("parsing literal")
            if valueAst.GetKind() == kinds.StringValue {
                err := validate(valueAst.GetValue().(string))
                if err != nil {
                    panic(err)
                }
                return valueAst
            } else {
                panic("Must be of type string")
            }
        },
    })

    ObjectType := graphql.NewObject(graphql.ObjectConfig{
        Name:        "User",
        Description: "A typical user",
        Fields: graphql.FieldConfigMap{
            "id": &graphql.FieldConfig{
                Type: ID,
            },
        },
    })

    Schema, err := graphql.NewSchema(graphql.SchemaConfig{
        Query: graphql.NewObject(graphql.ObjectConfig{
            Name: "Query",
            Fields: graphql.FieldConfigMap{
                "object": &graphql.FieldConfig{
                    Type: ObjectType,
                    Resolve: func(p graphql.GQLFRParams) interface{} {
                        return map[string]interface{}{
                            "id": "test",
                        }
                    },
                },
            },
        }),
        Mutation: graphql.NewObject(graphql.ObjectConfig{
            Name: "Mutation",
            Fields: graphql.FieldConfigMap{
                "ObjectCreate": &graphql.FieldConfig{
                    Type: ObjectType,
                    Args: graphql.FieldConfigArgument{
                        "id": &graphql.ArgumentConfig{
                            Type: ID,
                        },
                    },
                    Resolve: func(p graphql.GQLFRParams) interface{} {
                        return map[string]interface{}{
                            "id": "test",
                        }
                    },
                },
            },
        }),
    })
    if err != nil {
        panic(err)
    }

    // Returns the right error
    params := graphql.Params{
        Schema: Schema,
        RequestString: `
      mutation M {
        ObjectCreate(id: "t") {
          id
        }
      }
    `,
        // VariableValues: variables,
    }
    result := graphql.Graphql(params)
    // fmt.Printf("Result: %#v\n", result)
    if result.HasErrors() {
        if result.Errors[0].Error() != "The minimum length required is 3" {
            panic("Result Not Equal")
        }
    }

    // Does not validate input
    params2 := graphql.Params{
        Schema: Schema,
        RequestString: `
      mutation M($input: String!) {
        ObjectCreate(id: $input) {
          id
        }
      }
    `,
        VariableValues: map[string]interface{}{
            "input": "t",
        },
    }
    result = graphql.Graphql(params2)
    fmt.Printf("Result: %#v\n", result)
    if result.HasErrors() {
        panic(result.Errors[0])
    }
}

[sogko]

Hi @pyros2097

I just got a chance to look at the code you posted and ran it through both graphql-go and graphql-js to verify the output, and you are right, at first glance, the behaviour is suspect.

Hang on tight cos this issue has multiple layers to it:

1.validator not implemented yet in graphql-go

Validation of arg inputs (for e.g. is the input matches the expected input type, etc) are supposed to be done through the validator before it gets passed to the executor.

But currently, validator has not been implemented yet.

2. panic() is not expected to be used in definition of scalars at the moment.

I see that currently you try to throw a panic() if the scalar value is unexpected.

If you study how built-in scalars are defined (both in graphql-go and graphql-js, you would notice that if a scalar value/literal can't be parsed/serialized, it will return nil.

Currently, if you throw a panic in ParseValue/ParseLiteral/Serialize (unlike Resolve in ObjectConfig), it won't be caught by graphql-go.

Side note: according to GraphQL spec, regarding Scalars:

• "The only requirement is that the server must yield values which adhere to the expected Scalar type"
• "raise a field error" if it can't parse the given value.

3. Query string in params2 does not get validated because it did not specify the correct type

The current query in params2 of your example:

      mutation M($input: String!) {
        ObjectCreate(id: $input) {
          id
        }
      }

But ObjectCreate field expected a nullable value of type ID, as specified here:

...
Mutation: graphql.NewObject(graphql.ObjectConfig{
            Name: "Mutation",
            Fields: graphql.FieldConfigMap{
                "ObjectCreate": &graphql.FieldConfig{
                    Args: graphql.FieldConfigArgument{
                        "id": &graphql.ArgumentConfig{
                            Type: ID, // <-- nullable value of type `ID`
                        },
                    },
                },
            },
        }),

Try to update it to:

      mutation M($input: ID) { // <--- Update it to nullable `ID`, as specified in `ObjectCreate` field
        ObjectCreate(id: $input) {
          id
        }
      }

After updating the query, now your ParseValue function will be executed and should throw fits all over ;)

The reason why your original didn't throw error when you specified the wrong type for the input argument is because validator has not been implemented. (see point 1).

If validator was doing its job, it should have an return error similar to this:

Variable "$input" of type "String!" used in position expecting type "ID".

---

So to address your original question, the reasons why your custom scalar does not seem to validate input are because:

• the query that used variables specified the wrong input type.
• and validator is not there to do its job to let you know in the first place.

---

This issue highlights at least two things that are incomplete in graphql-go implementation:

• validator, to validate queries
• how to handle errors in user-defined functions (e.g ParseValue, ParseLiteral, Serialize, Resolve etc)
  • Related issues How do you throw errors? #28 and API Improvement. #44
  • At least Resolve currently allows user to panic() and it would be recovered by executor.
  • Others are left uncaught I believe.
  • One suggestion in How do you throw errors? #28 is to discourage panic, be go-idiomatic and let user return error.

I'll create issues to highlight these flaws in implementation.

Cheers!

[ghost]

Hi @sogko,
Your'e exactly right. It wasn't validating my type because my query was wrong. Anyway it seems if we pass the input directly like in the first query it validates it correctly and the panic in the ParseLiteral is triggered and returned as a graphql error. Thanks for the reply. I'll be closing this issue.