-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validation in Input types proposal #361
Comments
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. |
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 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. |
Thanks for the reply @leebyron :)
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: GraphQLValidationTypeMaybe 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 Validation operation proposalAnother thought I had was adding a new operation type asides of 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 |
(maybe this is a conversation that could be opened in the spec repo instead of here) |
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. |
@syrusakbary I made a solution (flow-dynamic ) for this too. dynamic check data,and also gives them a corresponding Flow Type. ex:
can be checked like this
The demonstration is here graphql-relay pr:89 (detail codes) |
@OlegIlyenko I think a better validation system is more related to the core than hacking around decorators. A recent comment in Stackoverflow about it: http://stackoverflow.com/questions/37665539/mixing-of-schema-level-and-app-level-errors-in-graphql Extra thoughtsAdding a new optional validate query MyQuery {
# ...
} or validate mutation MyMutation {
# ...
} |
@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 |
@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, |
@OlegIlyenko yes, being able to compare arguments during validation is very important -- but such a thing could also be achieved by simply having a |
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. |
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 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! |
@syrusakbary I agree with what you're saying here:
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. |
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: |
@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 |
@jedwards1211 My comments were directed more towards the general conversation and OP's mention that
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. |
@IamCornholio cool, like what? |
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
as against my proposal which goes something like this:
|
Thanks for bringing up the discussion. Regex validation is only one use case of multiple kinds of validation.
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. |
@syrusakbary for compounded validation wouldn't it be simpler to just have a |
+1. Things get complicated really fast when you start considering this for output types.
+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. |
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
|
Some new about this? |
@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 #44 may also be relevant) |
@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. |
@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 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. |
@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. |
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 |
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. |
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:
I've also opened the following RFC, since having those extra fields that way seems kinda hacky: #1527 |
You can't solve all usecases with middleware, e.g. you can't validate directive args or input fields inside those args. Anyway, I think it's a pretty big change that should be planned for |
Inspired by 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. |
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? |
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 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. |
@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 Like for example, I'd like to use I tried mutating the 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 Do you |
@cerino-ligutom To trim a string we implemented a scalar type called 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 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. 😅 |
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
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.
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 inspirationInspired by a verification scheme in a PHP framework called laravel |
Adding a +1 to this. Does anyone know if anyone has made PR's and concrete steps to move this forward? |
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. Works well for our use case so far, though a native solution would probably be better.
Have you decided yet? 😁 |
@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. |
After 6 years, I'm still wishing for custom input processing on types. Why do output types get 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()}`}
},
}) |
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 . |
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
andGraphQLArgument
.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:
Example usage
Implementation
The implementation of this should be really simple, as it could be achieved by creating a new validation rule.
The text was updated successfully, but these errors were encountered: