Should I use GraphQL for fine-grained validation? about graphql-js HOT 31 CLOSED

Example: Input validation and user-friendly error messages in GraphQL mutations on Medium.com

from graphql-js.

Thanks for the note, I'll add a comment in code to explain what's going on here.

+value calls ToNumber internally.

num === num is a cheap way to identify if num is the value NaN. NaN is not equal to anything, including itself. Everything else will be a number.

from graphql-js.

FYI, if you follow this pattern, be sure to do validation for value coercion in addition to literal coercion.

I think this is an area we will need to cover in deeper documentation. As long as the validation has semantic value, I think this pattern is a good one. For example, at Facebook we have a type called "URL" which explains very clearly what kind of value will come from such a field and allows us to do validation if it's provided as input.

IMHO, doing this for "Email" makes a lot of sense, and I imagine you would want to use this not just to describe semantic input but also output

from graphql-js.

From slack talk:

currently we have “coerceLiteral” for this type of thing. However this is quite underspecified (especially in terms of errors) and we should firm this up before we finalize the spec

from graphql-js.

Ok, thank you.
So it could be useful to expose Kind from lib/language...

And also in the comment, it is only specified coerce and not coerceLiteral but if you don't provide both you get an error.

from graphql-js.

Nice catch. I'm leaving this open as a reminder to improve comments and docs

from graphql-js.

Could you give a quick explanations about the difference between both coerce and coerceLitteral?
The one that validate from the AST make sense but the other one, I am not sure when it is used...

from graphql-js.

There are two scenarios:

1. when used as the type of a field of an object, the value returned by the resolve function will be passed through coerce first. This ensures, for example, that Boolean is always Boolean, Int is always an integer, and String is always a string.

2. when input is provided as a query variable, the value of that query variable passed at runtime is first passed through coerce to ensure its of the correct type.

from graphql-js.

Allright, clear as crystal.

from graphql-js.

Not really related, but since I am digging into scalars....
This line for GraphQLFloat seems wired

// [...]
coerce(value) {
    var num = +value;
    return num === num ? num : null; // <= 
  },

from graphql-js.

Sorry, if it sounds like a stupid question but the schema is supposed to be server-side only, or it can be embedded in client?

I ask because if I want to share some enhanced types (ex: GraphQLExtrasString({min:2, max:10}), it is important to know if the build size matters.

UPDATE: I am not just worried about the the build size, but also about features like Buffer which is only available on the server.

from graphql-js.

It's expected that schema is represented client-side, usually in the form of code generation. You could imagine generating an NSObject, Java Object, or Flow type for every type represented in a GraphQL schema so clients can benefit from strong typed data.

Of course, if your server presents custom scalars, this can be a challenge for these tools. You could imagine defining a DateTime scalar in GraphQL and wanting to ensure that they become JS Date and Java DateTime, etc. That's custom logic in those tools.

from graphql-js.

So to be clear, this is wrong?

/**
 * Import dependencies
 */
import SomeLibWorkingOnlyServerSide from 'some-lib';
import {GraphQLScalarType} from 'graphql';
import {Kind} from 'graphql/lib/language';
import {GraphQLError} from 'graphql/lib/error';


/**
 * Export enhanced String scalar type
 */
export default class String {
  constructor(options = {}) {

    return new GraphQLScalarType({
      name: 'String',

      coerce(originalValue) {
        const {error, value} = SomeLibWorkingOnlyServerSide(originalValue, options);

        if (error) {
          throw new GraphQLError(error.message);
        }

        return value;
      },

      coerceLiteral(node) {
        if (node.kind !== Kind.STRING) {
          return null;
        }

        const {error, value} = SomeLibWorkingOnlyServerSide(node.value, options);

        if (error) {
          throw new GraphQLError(error.message, [node]);
        }

        return value;
      }
    });
  }
}

from graphql-js.

Well, at a high level no, but this implementation in particular has a lot of issues:

To be clear: there's nothing wrong with defining custom scalars, that's exactly why the GraphQLScalarType class constructor is exposed.

With this particular code there are a couple problems:

First, just from a JavaScript point of view, a class constructor should not return an object of another type (in fact, I'm surprised Babel isn't choking on this).

You should instead do:

export var CustomString = new GraphQLScalarType({

Next, you can't name your custom scalar String since the GraphQL spec defines the behavior of the String type which this doesn't adhere to, and this will result in multiple definitions of String which will soon be a validation error (when the related issue is completed).

Here would be code that is perfectly fine:

/**
 * Import dependencies
 */
import SomeLibWorkingOnlyServerSide from 'some-lib';
import {GraphQLScalarType} from 'graphql';
import {Kind} from 'graphql/lib/language';
import {GraphQLError} from 'graphql/lib/error';


/**
 * Export enhanced String scalar type
 */
export default function CustomString(typeName, options = {}) {
    return new GraphQLScalarType({
      name: typeName,

      coerce(originalValue) {
        const {error, value} = SomeLibWorkingOnlyServerSide(originalValue, options);

        if (error) {
          throw new GraphQLError(error.message);
        }

        return value;
      },

      coerceLiteral(node) {
        if (node.kind !== Kind.STRING) {
          return null;
        }

        const {error, value} = SomeLibWorkingOnlyServerSide(value, options);

        if (error) {
          throw new GraphQLError(error.message, [node]);
        }

        return value;
      }
    });
}

from graphql-js.

a class constructor should not return an object of another type

Indeed.

So, do you think it would be useful if I create a package like graphql-extras with some enhanced scalar types following this approach?

At first, my plan is to depends on Joi. And later make it more deps-free if people are enthusiastic.

Note: Joi doesn't work on Browser.

from graphql-js.

And thank you for your time/answers. I really appreciate it.

from graphql-js.

This seems fine - there's nothing wrong with custom scalars, the caveat is that client-side tools may not know about them and without building the rules for custom scalars into those tools, won't be as intelligent and helpful as possible.

from graphql-js.

Re: graphql-extras, I would suggest a more descriptive name. Maybe graphql-custom-strings or something like that.

from graphql-js.

graphql-validator-types or graphql-validator-scalar maybe....

from graphql-js.

Second thought, having custom in the name could be more explicit. Easier to see that they are not built-in types. So I think it will be graphql-custom-scalars.

Last question, about naming convention, some type ends with Type (ex: GraphQLScalarType), some without (ex: GraphQLString). What is the rule?

from graphql-js.

I think there is a bug with the parser, a scalar created within a function is not recognised as a scalar.

const emailType = createScalar('Email') // return new GraphQLScalarType(...)

[...] // in Query
args: {
  email: { 
    name: 'email', 
    type: new GraphQLNonNull(emailType)
  }
},

I get this error
Argument email expected type Email! but got: "[email protected]".
And the coerce/coerceLiteral of my custom scalar are not called.

Same when I define the type on a field

const userType = new GraphQLObjectType({
  name: 'User',
  fields: () => ({
   email: {
      type: emailType
    }
  })
});

I get this error
Field "email" of type Email must have a sub selection.

However it works if I use the same custom scalar directly (not created and returned by a function).

from graphql-js.

It's hard to see what your bug is without seeing how createScalar is implemented. It's almost certainly an issue within there.

Can you post the full code you're using to reproduce this issue on jsbin or requirebin?

from graphql-js.

Yes sure, I do it now.

from graphql-js.

While writing a clean example to reproduce the bug, it seems to works.
Must be something wrong in my implementation indeed, sorry about that.

from graphql-js.

If you figure out what went wrong, let me know. I'd like to make the error messages as descriptive as possible.

from graphql-js.

I have two questions related to custom scalar types:

1. In the coerce method we don't have access to the AST, should one still use a GraphQLError or should a simple Error be used? It seems like the response's errors[].locations is set to [undefined] in both cases, is this expected? http://facebook.github.io/graphql/#sec-Errors indicates that no locations should be set.
2. Like @SimonDegraeve points out Kind from lib/language is not exported (or is it perhaps available some other way?). Does that mean that one should use strings such as "StringValue" when checking the AST in the coerceLiteral method of a custom GraphQL type?

from graphql-js.

In the coerce method we don't have access to the AST, should one still use a GraphQLError or should a simple Error be used?

Right now we expect invalid coercions to result in returning null, but throwing is reasonable. Would you mind opening a specific issue for this concern?

Like @SimonDegraeve points out Kind from lib/language is not exported (or is it perhaps available some other way?). Does that mean that one should use strings such as "StringValue" when checking the AST in the coerceLiteral method of a custom GraphQL type?

This has since changed. It's not exported from the main module, but it is exported from the sub-module:

import { Kind } from 'graphql/language';

However checking the string value is also acceptable if you prefer that

from graphql-js.

@leebyron thank you for the update about the submodule.

from graphql-js.

If anyone wants to know more on validation and custom scalars go here https://github.com/mugli/learning-graphql to custom types

from graphql-js.

@pyros2097 A big thank you for the link. I managed to create a package here https://github.com/xpepermint/graphql-type-factory.

from graphql-js.

Thanks @ lot guys !

from graphql-js.