graphql / graphql-js

A reference implementation of GraphQL for JavaScript
http://graphql.org/graphql-js/
MIT License
20.06k stars 2.02k forks source link

Validation in Input types proposal #361

Open syrusakbary opened 8 years ago

syrusakbary commented 8 years ago

Validations in GraphQL-js

I would like to open a discussion around validation.

I think could be a great idea permit validation in input elements: GraphQLInputField and GraphQLArgument. This would not involve any spec change as it relies on the implementation (as the resolve method).

Right now is possible to achieve this by using a custom GraphQLScalar per validation. However, by doing this we have to create a new type just for validation and is not easy to compose multiple validation rules without creating a type for each composition.

Also, is possible to achieve it by add the validation logic in the resolver method, however things get trickier when we want to validate nested input data.

So, will be much simpler to have this in the implementation.

Handling errors

We could have two ways for handling validation errors:

const queryType = new GraphQLObjectType({
  name: 'Query',
  fields: () => ({
    getUser: {
      type: User,
      args: {
        email: {
          description: 'The email of the user',
          validators: [email_validator],
          type: String
        }
      },
      resolve: (root, { email }) => {
        // We know we received a proper input, so this simplifies the resolver logic
        return getUserByEmail(email)
      },
    },
  })
});

Implementation

The implementation of this should be really simple, as it could be achieved by creating a new validation rule.

helfer commented 8 years ago

I like the idea, definitely gives the code more structure.

As for what should happen when validation fails: I strongly believe that the whole query should be rejected, because otherwise the developer has to start reasoning about all possible cases where part of the arguments get accepted, but others not. It's just much easier to deal with clear success/failure and not having to worry about the whole gray area in between.

leebyron commented 8 years ago

This is a good idea.

Currently you can emulate it by doing something like:

const queryType = new GraphQLObjectType({
  name: 'Query',
  fields: () => ({
    getUser: {
      type: User,
      args: {
        email: {
          description: 'The email of the user',
          type: String
        }
      },
      resolve: (root, { email }) => {
        email_validator(email);
        // We know we received a proper input, so this simplifies the resolver logic
        return getUserByEmail(email)
      },
    },
  })
});

Assuming the email_validator is a function which works like an assertion: returning void and throwing an error if invalid.

However this waits until field execution time which doesn't match @helfer's suggestion of rejecting the entire query. I agree that bad input should be considered a "developer error" and therefore block the whole query.

Validation is perhaps the right place to put this (and variable value validation, right before execution) - however we can run into the case where client tools which perform validation may falsely mark a query as valid when the server would then reject it because only the server knows about these validation rules. Maybe this is just okay? I'm not sure.

syrusakbary commented 8 years ago

Thanks for the reply @leebyron :)

we can run into the case where client tools which perform validation may falsely mark a query as valid when the server would then reject it because only the server knows about these validation rules.

That's completely right, but happens the same with scalar types, in which both client and server have to know what this scalar type means and how to deal with it. I think is worth to think about validation in the validate context and not in the executor.

New GraphQL type proposal: GraphQLValidationType

Maybe adding a validation type to the schema would be a good approach. So both server and client have to know how to deal with that.

schema {
  query: Query
}

validation EmailValidator

type Query {
  name: String! # String[Required] ?
  email: String[EmailValidator]
}

Perhaps GraphQLNonNull is just another kind of validation on top of a type šŸ˜‰

Validation operation proposal

Another thought I had was adding a new operation type asides of query, mutation, subscription; validation. So actually clients could validate queries without actually executing them in server-side.

PS: I'm just throwing some thoughts about how we can approach this problem, with the willing of opening a discussion that could help GraphQL usage in more scenarios

syrusakbary commented 8 years ago

(maybe this is a conversation that could be opened in the spec repo instead of here)

OlegIlyenko commented 8 years ago

After looking at decorators proposal, I got a feeling that it may also this use-case as well: https://github.com/apollostack/graphql-decorators. We had a discussion about it here https://github.com/apollostack/graphql-decorators/issues/1 - with some modifications to proposal it would be possible to perform the validation before the actual execution and reject the whole query if it is invalid.

iamchenxin commented 8 years ago

@syrusakbary I made a solution (flow-dynamic ) for this too. dynamic check data,and also gives them a corresponding Flow Type. ex:

const resolver = (src:any) => {
  // .... doing something
  return src;
});

can be checked like this

const wrappedFn = check1(
(v) => ({
  mail:pro.isString.isEmail(v.mail) // check src.mail.
}), (v) => { // Now, the input data is validated and have a Flow type {mail:string}
  // .... doing something
});

The demonstration is here graphql-relay pr:89 (detail codes)

syrusakbary commented 8 years ago

@OlegIlyenko I think a better validation system is more related to the core than hacking around decorators. @iamchenxin doing the validation in the resolving phase will not stop execution, and for validation I think is somehow essential.

A recent comment in Stackoverflow about it: http://stackoverflow.com/questions/37665539/mixing-of-schema-level-and-app-level-errors-in-graphql

Extra thoughts

Adding a new optional validate token into the language. It could let us to do validation without any execution.

validate query MyQuery {
    # ...
}

or

validate mutation MyMutation {
    # ...
}

@leebyron ?

iamchenxin commented 8 years ago

@syrusakbary Error process for resolver is in execute.js#L603. Maybe it should give users more control for error behavior.(by token or by a graphql special exception name). .or maybe give them both? in addition to token, also can assign an exception name/class which Let us to have a chance to stop current execution(pass though that exception out of graphql). and maybe there should be some standard exception name in graphql which means STOP ,not only ignore.To stop a execution , exception is one of the standard code pattern. maybe cause i come from other language,that makes me more like using exception to express an exception.

OlegIlyenko commented 8 years ago

@syrusakbary I think it depends on what shape decorators will take. I agree that that it would be great to have validators as a core concept in the library, or at least very good integrated. On the other hand all implementations already have a query validation mechanism, which is also present in the reference implementation. I think it can be beneficial to use it, but in order to do so one need a bit more information/meta-information on a field and argument level.

It's similar concept to the one I described on the decorators discussion. One nice property of this approach is that validation mechanism (as implemented in reference implementation) is able to see the whole query and all of the arguments of a field. This makes it easier to implement validation rules that validate several interdependent arguments at once (like, for instance, password and passwordConfirmation).

jedwards1211 commented 8 years ago

@OlegIlyenko yes, being able to compare arguments during validation is very important -- but such a thing could also be achieved by simply having a validate function called with all the same arguments as the resolve function before the query is executed.

jedwards1211 commented 8 years ago

One of the cases where client/server differences could arise (concerning context-free syntax validation) would be if old clients are connecting to a server with an updated schema. So I think an app must be structured so that the server can send back errors that the client will handle just the same as if they had occured in client-side validation.

xpepermint commented 8 years ago

Ow yeah... built-in validations - GraphQL would become a perfect tool! We need this... :). I've been using custom scalars for months but I found it not appropriate. Also most of my projects were using forms (form validation) and I needed to send user-friendly validation errors to a user (to a form; a custom validation error code would be enough here).

I think that user-friendly validation errors are important. What I use now in my GraphQL projects is similar to Konstantin Tarkus's proposal - smart.

mutation {
  createUser() { // returns UserMutationType
    data: {name} // null if invalid
    errors: {message, code} // validation errors
  }
}

To make my resolvers small I wrote a helper package which handles validation and error handling in a way I need (e.g. MongoDB E11000 error can also be treated as validation error thus I can respond with already taken validation error message instead of the unreadable MongoError warning). Here is how my resolvers currently look like using Approved.js:

import {user} from '../approvals/users';

export async function createUser(root, args, ctx) {
  let data, errors = null;
  try {
    await user.validate(data, ctx);
    let res = await ctx.mongo.collection('users').insert(data);
    data = res.ops[0];
  } catch(err) {
    errors = await user.handle(err, ctx);
    if (!errors) throw err;
  }
  return {data, errors};
}

Having this kind of functionality as part of GraphQL would be just amazing!

jedwards1211 commented 8 years ago

@syrusakbary I agree with what you're saying here:

Right now is possible to achieve this by using a custom GraphQLScalar per validation. However, by doing this we have to create a new type just for validation and is not easy to compose multiple validation rules without creating a type for each composition.

What kinds of validation do you imagine composing on an email address - for instance making sure it's not likely a botnet or throwaway email address? I definitely agree that these kinds of checks don't belong in custom scalar methods. But I can't imagine any drawbacks to using a custom scalar Email type that merely checks whether a syntactically valid email is provided, and apart from that use a system like you're proposing for contextual validation, like the subset of syntactically valid emails that the server will accept.

I think usernames or passwords would be a better example than Email; I don't think it would be as wise to use a custom scalar for usernames or passwords, since one might decide to change the restrictions down the road.

JeSuisNikhil commented 8 years ago

I don't see what's wrong with adding it to the spec itself. Validation that can be defined per input field at field definition instead of custom scalars could be very useful. I've asked for it here: https://github.com/facebook/graphql/issues/184

jedwards1211 commented 8 years ago

@IamCornholio I'm not saying I'm opposed to this proposal here, it's just that this is related to a debate about whether it's appropriate to use custom scalars for validation, and I think it's not necessarily ideal to keep email fields as type String and do all of the validation in a per field validator, as in the OP's example. Plus it seems like the OP is opposed to doing any kind of validation with custom scalars, and I'm debating that.

JeSuisNikhil commented 8 years ago

@jedwards1211 My comments were directed more towards the general conversation and OP's mention that

This would not involve any spec change as it relies on the implementation (as the resolve method).

Apologies for not setting the context appropriately.

I agree though that email could be implemented as a custom scalar rather having to add a validator to each resolve function or add a validation per field. My per field validation recommendation is assuming that the validation requirement is very specific to the field in context (such as a new password mutation with the argument restriction/validation of needing Uppercase, lowercase, numbers, symbols etc)

Also just noticed that are quite a few competing (and perhaps more interesting) proposals out there for this.

jedwards1211 commented 8 years ago

@IamCornholio cool, like what?

JeSuisNikhil commented 8 years ago

Also just noticed that are quite a few competing (and perhaps more interesting) proposals out there for this.

Again, referring to the per field validation proposal. Just the stuff already mentioned on this thread..

https://github.com/apollostack/graphql-decorators see validateRange and the long discussion here https://github.com/apollostack/graphql-decorators/issues/1

OP's proposal applied to passwords instead of email perhaps

schema {
  query: Query
}

validation EmailValidator

type Query {
  name: String! # String[Required] ?
  email: String[EmailValidator]
}

as against my proposal which goes something like this:

type Human {
  id: String![0-9a-z]{16} // 16 digit alphanumeric
  name: String![a-zA-Z ]{1,30} // 01-30 characters
  email: String+@+ // must have @
  age: int{1,} // min value 1, max val undefined
  money: float{2} // precision 2 decimal places
}
syrusakbary commented 8 years ago

Thanks for bringing up the discussion.

Regex validation is only one use case of multiple kinds of validation. There are other validation cases like:

As I read in other issues about validation in this repo, it might be interesting to have this types of validation only for Input types/fields.

The reasoning behind creating or not Validation types in GraphQL is being able to reference them between client and server side and being able to reuse validation rules across different types/fields.

jedwards1211 commented 8 years ago

@syrusakbary for compounded validation wouldn't it be simpler to just have a validate function called before any resolve with the same args, instead of having validators in the fields themselves?

JeSuisNikhil commented 8 years ago

As I read in other issues about validation in this repo, it might be interesting to have this types of validation only for Input types/fields.

+1. Things get complicated really fast when you start considering this for output types.

The reasoning behind creating or not Validation types in GraphQL is being able to reference them between client and server side and being able to reuse validation rules across different types/fields.

+1. IMO it'd be nice to have a declared validation type over needing to call a helper/validator function everywhere (which I assume would be available as an option anyway for really complex validations that are not easy to declare). With a Validation type the validation becomes part of the language spec instead of being implementation specific. At some point I hope there are tools that will let you define a GraphQL API in the GraphQL language and then generate basic model/stubs in Java/JS et al.

pflannery commented 7 years ago

Would stackable decorators work?


# contrived
input SomeInput {

  @Length(min: 6, max: 14, error: "Invalid length etc....")
  name: String

  @OneOf(items: ["red", "green", "blue"], error: "Invalid colour can only be one of ${items}......")
  favouriteColour: String

}

Ideally we're be able to

raderio commented 7 years ago

Some new about this?

rsp commented 7 years ago

@syrusakbary This issue has been open for 1.5 year now. Any updates on that?

The only way to achieve that right now seems to be:

Or did I miss something?

(issue https://github.com/graphql/graphql-js/issues/44 may also be relevant)

stephenhandley commented 7 years ago

Happy to adapt graphql-validated-types as needed if helpful, currently its a fluent api but would be easy to accept validation args via constructor. I meant to more properly document the base class api for adding validators and the validation process.. that's here https://github.com/stephenhandley/graphql-validated-types/blob/master/src/GraphQLValidatedScalar.js#L47-L60

jedwards1211 commented 6 years ago

I just realized that this isn't much of a problem for me anymore because I'm now using Sequelize and doing most of my field validations in Sequelize itself.

Which makes me think:

My earlier thinking about this was skewed because I was using Rethink and didn't have any ORM or handwritten model abstraction enforcing a schema. At the time I thought GraphQL was a fine place to enforce the schema, but I realize now, it was probably not the best place.

wmertens commented 6 years ago

Extra verification is not really a problem is it?

Also, in my experience, having the GrapQL schema be auto-generated from the DB paints you in a corner for later DB changes.

The schema your client app uses is very similar to the schema the server app uses, but not the same.

hisapy commented 6 years ago

@jedwards1211 I was thinking the same but there might be a problem. I think that tools like GraphiQL rely on the GraphQL types to validate input, including arguments for mutations. That said, I'm trying to find a way/function that I can provide an object representing an input object for a mutation to validate the input before submitting it to the server.

jedwards1211 commented 6 years ago

@hisapy even if GraphQL types don't throw any validation errors, your query/mutation resolvers can check whatever they want and if they throw any error it will show up in GraphiQL and any other consumers of your API. I like to validate objects in my resolvers with flow-runtime, check it out.

hisapy commented 6 years ago

I mean that those tools, validate input (not data) as you type, because they know the schema so they don't have to send a request to the server render schema GraphQL errors locally.

I'm using Relay and I'd like to re-use the GraphQL types to validate the input object for a mutation. Currently mutation only shows a warning but submits anyway. I would like to avoid commit if the args don't pass the GraphQL validation.

jedwards1211 commented 6 years ago

@hisapy oh, that's an interesting point, I can definitely see the value in better validation as you type! Though even if the schema supports custom validation functions for input types I don't think it would work, because there's not really a good way for GraphQL to serialize those functions into the schema that GraphiQL requests. As far as Relay, that's not something I can comment on, I've only used Apollo so far.

hisapy commented 6 years ago

For example, GraphiQL gets to know the schema via an introspection query. The relay-compiler compiles the queries and fragments in the components based on a schema file provided to the compiler. I don't know Apollo but it would be great if we could validate an object based on schema rules in Relay, given the .graphql.js files the compiler generates. Of course this is not something for this thread specifically

negezor commented 6 years ago

And how about adding the ability to install optional resolvers in the input? Because often you have to conduct more checks for the same actions. And the mutation handler is no longer so compact. https://github.com/apollographql/graphql-tools/issues/652

JCMais commented 5 years ago

I've been using graphql middlewares to do that, I've previously written about it here: https://itnext.io/graphql-mutation-arguments-validation-with-yup-using-graphql-middleware-645822fb748

And a lib related to it can be found here: https://github.com/JCMais/graphql-yup-middleware

The idea is that you add a yup schema when you define your types, something like:

const resolvers = {
  // ...
  Mutation: {
    AddUser: {
      validationSchema: yupSchemaHere,
      resolve: async (root, args, context, info) => {
        // ...
      },
    },
  },
};

I've also opened the following RFC, since having those extra fields that way seems kinda hacky: https://github.com/graphql/graphql-js/issues/1527

IvanGoncharov commented 5 years ago

You can't solve all usecases with middleware, e.g. you can't validate directive args or input fields inside those args. So I think we need to add validate callback to args and input fields. If you decide you want to implement validation based on SDL directives you would use schema transformation to assign validate to appropriate args and fields. Not sure about single function vs array through. Also maybe it worth to add validate into GraphQLScalarType since ATM validation logic is always duplicated in parseValue and parseLiteral.

Anyway, I think it's a pretty big change that should be planned for 15.0.0.

havinhthai commented 5 years ago

Inspired by express-validator, I just wrote graphql-validation to validate both args and input types. It's just GraphQL middleware that wraps validator.js validator functions so very lightweight and easy to use.

Example:

const { validator, validate } = require('graphql-validation'); // Import module

const resolver = {
  Mutation: {
    createPost: validator([ // <-- Validate here
      validate('title').not().isEmpty({ msg: 'Title is required' }),
      validate('content').isLength({ min: 10, max: 20 }),
    ], (parent, args, context, info) => {
      if (context.validationErrors) {
        // Validate failed
        console.log(context.validationErrors); // Do anything with this errors

        return;
      }

      // Validate successfully, time to create new post
    }),
  },
};
Input: { title: '', content: 'Hi!' }

// console.log(context.validateErrors);
Output: [
  { param: 'title', msg: 'Title is required' },
  { param: 'content', msg: 'Invalid value' },
]

Hope useful.

sapkra commented 5 years ago

We are using graphql-shield to add permission checks and validation to our graphql server.

You can use the yup library for the validation rules which can also be shared with the frontend while using a form library like Formik.

cerinoligutom commented 5 years ago

We are using graphql-shield to add permission checks and validation to our graphql server.

You can use the yup library for the validation rules which can also be shared with the frontend while using a form library like Formik.

Hi, I'm using graphql-shield strictly for permissions so far but have given it some thought as a middleware for validation. Been looking around who else tried this and only found your comment.

How did it work for you so far? I'd imagine all mutations would have their own unique "rule", is that your case currently? And did you happen to have some grey areas?

sapkra commented 5 years ago

It's working pretty good. You're right, we had to create a rule for each mutation but we are sharing the rules for the fields between them.

For example:

export const yupEmail = yup.string().email();
export const yupRequiredEmail = yupEmail.required();
export const yupRequiredString = yup.string().required();
export const yupUserPassword = yup.string().min(3).required();

export const signup = yup.object({
  email: yupRequiredEmail,
  firstName: yupRequiredString,
  lastName: yupRequiredString,
  password: yupUserPassword,
});

export const signin = yup.object({
  email: yupRequiredEmail,
  password: yupUserPassword,
});

For our backend we created the following rule helper but maybe it's also possible to use the new internal yup feature of graphql-shield.

const yupRule = (schema) => rule()(
  async (parent, args, ctx: Context) => {
    return (await schema.isValid(args)) || new Error("Invalid form");
  },
);

export const validation = shield({
  Mutation: {
    signup: yupRule(signup),
    signin: yupRule(signin),
  },
};

I simplified it a bit so that it is more understandable. Of course we split everything into multiple files and created a package which only includes all the validation rules. To manage all the packages in our monorepo we are using lerna.

And no, I don't think we have some grey areas right now. I hope it will not change in the future because hopefully someone will notice it in the code review before it's going to production.

cerinoligutom commented 5 years ago

@sapkra Thanks for the input, using it now as well and I like the separation of concerns. Though bumped into a somewhat different issue. How do you go on handling the supposedly validated args on the resolver?

Like for example, I'd like to use .trim() of yup and want the trimmed values in args to be received by the resolver after validating.

I tried mutating the args from a custom yupRule like you did above, no luck.

rule()(async (parent, args) => {
  // args properties at this point has some trailing whitespace
  const result = await schema.validate(args);
  // whitespaces should be removed at this point as per schema 

   // Tried mutating args in the hopes of getting the trimmed properties on my resolver but no luck
  args = { ...result }; 

  return true;
})

Tried also graphql shield's inputRule, no luck either. I'm still getting the raw input.

Do you yourYupSchema.cast args again on the resolver or somewhere? How are you handling such cases?

sapkra commented 5 years ago

@cerino-ligutom To trim a string we implemented a scalar type called TrimmedString using graphql-input-string.

import GraphQLInputString from "graphql-input-string";

export const TrimmedString = GraphQLInputString({
    name: "TrimmedString",
    trim: true,
    empty: true,
});

Now you can use the TrimmedString in you schema.

scalar TrimmedString

type Mutation {
    signin(email: TrimmedString!, password: String!): User!
    signup(email: TrimmedString!, firstName: TrimmedString!, lastName: TrimmedString!, password: String!): User!
}

Here you can find more information: https://github.com/joonhocho/graphql-input-string He also developed a very useful scalar type for numbers: https://github.com/joonhocho/graphql-input-number

I think I should prepare a talk for all this stuff and should apply for a conference... and yeah we did a lot of more things to protect our API. šŸ˜…

lijie1129 commented 5 years ago

Right now is possible to achieve this by using a custom GraphQLScalar per validation. However, by doing this we have to create a new type just for validation and is not easy to compose multiple validation rules without creating a type for each composition.

@syrusakbary

Here is the verification method I expected:

type Cat {
  name: String! (max:20|min:10)
}

type Query {
  cats (limit:Int! (max:50|min:5) ): [Cat]
}

Detailed description

name: String! (max:20|min:10)

This means that the name value of the string type has a maximum length of no more than 20 and a minimum of 10 or less, otherwise an exception will be thrown during the Validation Phase.

cats (limit:Int! (max:50|min:5) ): [Cat]

This means that the limit value of the integer type is no more than 50 and the minimum is not less than 5, otherwise an exception will be thrown during the Validation Phase.

Source of inspiration

Inspired by a verification scheme in a PHP framework called laravel Reference: https://laravel.com/docs/5.8/validation

bag-man commented 4 years ago

Adding a +1 to this. Does anyone know if anyone has made PR's and concrete steps to move this forward?

cerinoligutom commented 4 years ago

@cerino-ligutom To trim a string we implemented a scalar type called TrimmedString using graphql-input-string.

import GraphQLInputString from "graphql-input-string";

export const TrimmedString = GraphQLInputString({
    name: "TrimmedString",
    trim: true,
    empty: true,
});

Now you can use the TrimmedString in you schema.

scalar TrimmedString

type Mutation {
    signin(email: TrimmedString!, password: String!): User!
    signup(email: TrimmedString!, firstName: TrimmedString!, lastName: TrimmedString!, password: String!): User!
}

Here you can find more information: https://github.com/joonhocho/graphql-input-string He also developed a very useful scalar type for numbers: https://github.com/joonhocho/graphql-input-number

It's late but I thought I'd update here how we're tackling this. We try to minimize the use of scalars and we don't like to pollute our SDL either with schema directives.

Since graphql-shield's InputRule currently does not return the validated (processed) input, trimmed strings in this case, we created our own graphql-shield rule that merges the validated input to the incoming input.

image

Works well for our use case so far, though a native solution would probably be better.

I think I should prepare a talk for all this stuff and should apply for a conference... and yeah we did a lot of more things to protect our API. šŸ˜…

Have you decided yet? šŸ˜

sapkra commented 4 years ago

@cerino-ligutom I just had time to talk about this stuff at a local meetup but didn't find any time to prepare a talk. It's still on my list but it doesn't have any priority yet.

Your solution looks also really nice. I'll give it a try - thanks for sharing it.

wmertens commented 2 years ago

After 6 years, I'm still wishing for custom input processing on types.

Why do output types get resolve functions, but input types do not?

I'd love to be able to do this:

const Person = new GraphQLInputObject({
  name: 'Person'
  fields: {
    id: {type: GraphQLInt},
    name: {type: new GraphQLNonNull(GraphQLString), resolve: (name) => trim(name)}
  },
  resolve: (person, _, ctx) => {
    if (!person.name) throw new Error('name required')
    if (id < 1000 && !ctx.isAdmin) throw new Error('referring to this id is not allowed')
    return {...person, stuff: `${person.id}-${person.name.toUpperCase()}`}
  },
})
AhmedAyachi commented 11 months ago

Hello, I've worked with GraphQL lately and i encountered such scenarios where such feature is required to keep clean organized code. As I've used graphql v15.8.0 and such feature is not implemented i created it myself, check repo . If such features are now available in higher versions, please use em instead. Hope it helps somebody.