Input validation

[stefanvanherwijnen]

Hi,

Currently the input of mutations is only validated by type. I.e. if I use setCustomerForOrder it will report if any of the values is not defined or not a String, but providing an empty string for any of the fields is accepted. Also, for the emailAddress field, a non-email string is also accepted.

I did a quick search and it seems that class-validator is used with TypeORM for input validations: https://github.com/typeorm/typeorm/blob/master/docs/validation.md

The resulting database error when inserting non-valid data could be wrapped in a ValidationError result such that the frontend also knows what went wrong. I also noticed that the customer service does a manual check for existing email addresses. Isn't it easier to insert the new customer and check for a unique conflict error by adding the @Unique() decorator?

[michaelbromley]

I agree that validation would be useful beyond the basic type checking of GraphQL. I'd like to explore the options available for this. The class-validator lib looks like a good bet. Might also be useful to make the validations customizable to some degree, so that validations can be specified/modified by the user. Maybe the non-decorator method would allow us to abstract this into a friendly extensible API.

also noticed that the customer service does a manual check for existing email addresses.

See this comment for an explanation of why we don't add a unique constraint on that.

[stefanvanherwijnen]

With soft-deletes it indeed makes sense to avoid the Unique constraint.

I think the validation should be split between the database level and the business-logic level. I.e. an email field in the database is required to be a valid email address (name@domain.ext), but on the business-logic level you might only want users with a '.com' address to be able to order. .com only addresses is not a data validity concern so it should not be handled at the database level. However, there should be some protection at the database level to avoid inserting non-valid data (e.g. user@@domain.com is not valid).

A default validator for validation at the database level which can be extended with business logic would indeed be the ideal solution. It looks like NestJS normally uses the ValidationPipe, but perhaps a custom Pipe can be implemented.

[stefanvanherwijnen]

I think ajv might be the better choice for flexibility and JSON schema validation seems to be a well supported standard. This would completely decouple it from the database and would require a schema for every entity, but the pro is that the user can edit the schema's easily, e.g. by importing a custom schema in the Vendure configuration as is done for custom payment methods etc.

import { CustomCustomerSchema } from './schemas'
export const config: VendureConfig = {
  ...
  JsonSchemas: {
    // entity: schema
    customer: CustomCustomerSchema  // Defaults to CustomerSchema in Vendure core
  }
}

Something like that.

[michaelbromley]

Thanks for the suggestion of ajv. I'll look into that. Another thought I just had and will note here for later:

An alternative could be to rely on custom GraphQL scalars to implement validation, which would allow you to use pre-built scalars such as https://www.graphql-scalars.dev/docs/introduction or create your own implementing business-specific requirements. This would have the advantage of requiring no new infrastructure to implement. I think you may already be able to do it by overriding a given GraphQL field in a plugin.

[stefanvanherwijnen]

I created a proof of concept with ajv (with JSON Type Definition instead of JSON Schema): https://github.com/stefanvanherwijnen/vendure/commit/6358fb62b2a4372137a925324462b2cd1e1a7b70

I used UsePipes() but it seems to be called twice on every request, once with the request body and once with the request context. It should only be called for the request body, but I can´t figure out why it is called twice.

Instead of UsePipes(), I think GraphQL Scalars could also be used. ajv is only used for validation purposes and can be customized any way you want (e.g. validate a single value instead of an object). I think in theory it should be possible to create a Scalar for every single different field and attach an unique validation schema to it, although for a single value it's probably easier to attach only a single validation function in parseValue(). The main challenge is making everything work out-of-the-box, but still provide the ability to customize any validators.

Just my two cents.

(https://brunoscheufler.com/blog/2021-01-17-centralized-validation-with-graphql-scalar-types) (https://stackoverflow.com/questions/66140889/nestjs-graphql-ensure-scalar-type)

[daniel-lxs]

Hello @michaelbromley. Is this going to be implemented in the next release, have you considered using something like https://typegraphql.com/, which is the current nestjs aproach for input validation using class-validator?

[michaelbromley]

Hi @driccio98, apologies for the slow reply. This is not currently planned for the next release. I've got it in the "planned" backlog but I have other higher-priority issues to handle first.

There is a PR where someone did a POC to implement code-first schema definitions like in TypeGraphQL, but it was not conclusive so far. It's an area that warrants more research for a future version I think.