Validation issues on version 2.0.0-beta.1 with @[email protected]

[YMSpektor]

Describe the Bug
I'm using type-graphql v2.0.0-beta.1 together with @[email protected]. (As mentioned here type-graphql v1.1.1 doesn't work fine with apollo-server v4) There are some issues related to validation:

1. I'm not able to call a mutation passing correct variables in the body, it leads to Argument Validation Error in the response. The issue seems to be related to "forbidUnknownValues" option in the class-validator library, at least setting it to false allows to avoid the error.

2. The"validationErrors" field is always missing in the ArgumentValidationError response (even if I set forbidUnknownValues to false, add some validator like MaxLength and pass incorrect value - "validationErrors" will be missing in the response JSON)

To Reproduce
Here is the code:

import "reflect-metadata";
import { ApolloServer } from "@apollo/server";
import express from "express";
import { expressMiddleware } from "@apollo/server/express4"
import cors from "cors";
import http from "http";
import { ApolloServerPluginDrainHttpServer } from "@apollo/server/plugin/drainHttpServer";
import bodyParser from "body-parser";
import { buildSchema } from "type-graphql";
import { Arg, Mutation, Query, Resolver } from "type-graphql";
import { ObjectType, Field, ID, InputType } from "type-graphql";

@ObjectType()
export class User {
    @Field(() => ID)
    id: string;

    @Field()
    email: string;

    @Field({nullable: true})
    firstName?: string;

    @Field({nullable: true})
    lastName?: string;
}

@InputType()
export class AddUserInput {
    @Field()
    email: string;

    @Field({nullable: true})
    firstName?: string;

    @Field({nullable: true})
    lastName?: string;
}

@Resolver(User)
export class UserResolver {
    users: User[] = [];

    @Query(() => [User], { name: "users" })
    getAll(): User[] {
        return this.users;
    }

    @Mutation(() => User, { name: "addUser" })
    add(@Arg("user") userInput: AddUserInput): User {
        const user = new User();
        Object.assign(user, userInput);
        user.id = Math.floor(Math.random() * Number.MAX_SAFE_INTEGER).toString();
        this.users.push(user);
        return user;
    }
}

const bootstrap = async () => {
    const app = express();
    const httpServer = http.createServer(app);

    const schema = await buildSchema({
        resolvers: [UserResolver],
        //validate: {
        //    forbidUnknownValues: false  !!! Only works with this setting
        //}
    });

    const apolloServer = new ApolloServer({
        schema,
        plugins: [ApolloServerPluginDrainHttpServer({ httpServer })]
    });

    await apolloServer.start();

    app.use(
        cors(),
        bodyParser.json(),
        expressMiddleware(apolloServer)
    );

    const port = 4000;
    httpServer.listen( {port }, () => {
        console.log(`Server started at http://localhost:${port}`);
    });
}

bootstrap();

Here is my package.json:

{
  "name": "graphql_api",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "dev": "ts-node-dev --respawn src/index.ts",
    "prebuild": "rimraf build",
    "build": "tsc && copyfiles package.json build",
    "start": "node index.js"
  },
  "author": "",
  "license": "ISC",
  "devDependencies": {
    "@types/cors": "^2.8.13",
    "@types/graphql": "^14.5.0",
    "@types/node": "^18.11.14",
    "copyfiles": "^2.4.1",
    "rimraf": "^3.0.2",
    "ts-node-dev": "^2.0.0",
    "typescript": "^4.9.4"
  },
  "dependencies": {
    "@apollo/server": "^4.3.0",
    "body-parser": "^1.20.1",
    "class-validator": "^0.14.0",
    "cors": "^2.8.5",
    "express": "^4.18.2",
    "graphql": "^16.6.0",
    "reflect-metadata": "^0.1.13",
    "type-graphql": "^2.0.0-beta.1"
  }
}

And this is the Mutation I'm trying to call:

mutation Mutation($user: AddUserInput!) {
  addUser(user: $user) {
    id
    email
    firstName
    lastName
  }
}

Variables:

{
  "user": {
    "email": "[email protected]",
    "firstName": "John",
    "lastName": "Smith"
  }
}

The error response I get:

{
    "errors": [
        {
            "message": "Argument Validation Error",
            "locations": [
                {
                    "line": 1,
                    "column": 43
                }
            ],
            "path": [
                "addUser"
            ],
            "extensions": {
                "code": "INTERNAL_SERVER_ERROR",
                "stacktrace": [
                    "Error: Argument Validation Error",
                    "    at validateArg (D:\\Temp\\TypeGraphQL_Issue\\node_modules\\type-graphql\\dist\\resolvers\\validate-arg.js:55:15)",
                    "    at processTicksAndRejections (internal/process/task_queues.js:93:5)",
                    "    at async Promise.all (index 0)"
                ]
            }
        }
    ],
    "data": null
}

Environment (please complete the following information):

• OS: Windows 10
• Node v14.15.3
• Package version 2.0.0-beta.1
• TypeScript version 4.9.4
• Apollo server version: 4.3.0

[carlocorradini]

First problem

From class-validator v0.14.0 CHANGELOG:

forbidUnknownValues option is enabled by default
From this release the forbidUnknownValues is enabled by default. This is the desired behavior for majority of use-cases, but this change may break validation for some. The two scenarios that results in failed validation:

1. When attempting to validate a class instance without metadata for it
2. When using group validation and the specified validation group results in zero validation applied

The old behavior can be restored via specifying forbidUnknownValues: false option when calling the validate functions.

What we are interested is the first point: When attempting to validate a class instance without metadata for it
If you have a class that does not have even a single metadata (decorator) you encounter the error above. As far as I can see in your example none of the classes have a class-validator decorator; implying that you will hit the check and an error is thrown.
Note that this check is automatically done by GraphQL engine.
Therefore, add the following option duting schema build:

const schema = await buildSchema({
  // ...
  validate: { forbidUnknownValues: false } // <--
});

This will solve the issue.

Second problem

Error handling has changed in both GraphQL > 16 and Apollo Server. For more information see Apollo Server v4 errors.
When you create an Apollo Server instance edit the formatError option as follows:

const apolloServer = new ApolloServer<MyContext>({
  schema,
  formatError: myFormatError // <--
});

The formatError hook receives two arguments: the first is the error formatted as a JSON object (to be spent with the response), and the second is the original error.
Create a custom formatError (e.g. myFormatError) that looks something like this:

import type { GraphQLFormattedError } from 'graphql';
import { ArgumentValidationError } from 'type-graphql';
import { unwrapResolverError } from '@apollo/server/errors';
import { ValidationError } from './ValidationError ';

function myFormatError(
  formattedError: GraphQLFormattedError,
  error: unknown
): GraphQLFormattedError {
  const originalError = unwrapResolverError(error);

  // Validation
  if (originalError instanceof ArgumentValidationError) {
    return new ValidationError(originalError.validationErrors);
  }

  // Generic
  return formattedError;
}

ValidationError is a custom error class that extends GraphQLError:

import { GraphQLError } from 'graphql';
import type { ValidationError as ClassValidatorValidationError } from 'class-validator';

type IValidationError = Pick<
  ClassValidatorValidationError,
  'property' | 'value' | 'constraints' | 'children'
>;

function formatValidationErrors(
  validationError: IValidationError
): IValidationError {
  return {
    property: validationError.property,
    ...(validationError.value && { value: validationError.value }),
    ...(validationError.constraints && {
      constraints: validationError.constraints
    }),
    ...(validationError.children &&
      validationError.children.length !== 0 && {
        children: validationError.children.map((child) =>
          formatValidationErrors(child)
        )
      })
  };
}

export class ValidationError extends GraphQLError {
  public constructor(validationErrors: ClassValidatorValidationError[]) {
    super('Validation Error', {
      extensions: {
        code: 'BAD_USER_INPUT',
        validationErrors: validationErrors.map((validationError) =>
          formatValidationErrors(validationError)
        )
      }
    });

    Object.setPrototypeOf(this, ValidationError.prototype);
  }
}

PS: For an example project see reCluster server:

• Apollo Server instance
• Custom formatError function
• Validation error

[YMSpektor]

@carlocorradini Thanks a lot for such detailed answer! I've already found these workarounds in your repository and it seems to work great

For the first problem as I understand there is nothing can be done in the type-graphql package, but I would like to have this documented, it's not obvious and will make someone to spend quite a lot of time to figure out what's wrong and how to make it work. Especially together with the second problem

For the second problem, I'm just wondering, is it possible to have such behaviour in the ArgumentValidationError class itself as it's part of type-graphql?

[carlocorradini]

1. Agree, this must be documented. Do you want to create a PR? 🥳
2. ArgumentValidationError must extends GraphQLError and not Error. All errors must extends GraphQLError

[carlocorradini]

@YMSpektor Can we close this?

[YMSpektor]

@carlocorradini I think about creating a PR with the following changes:

1. validate-arg.ts:

export async function validateArg<T extends object>(
  argValue: T | undefined,
  argType: TypeValue,
  globalValidate: ValidateSettings,
  argValidate: ValidateSettings | undefined,
): Promise<T | undefined> {
  const validate = argValidate !== undefined ? argValidate : globalValidate;
  if (validate === false || !shouldArgBeValidated(argValue)) {
    return argValue;
  }

  if (typeof validate === "function") {
    await validate(argValue, argType);
    return argValue;
  }

  const validatorOptions: ValidatorOptions = Object.assign(
    {},
    typeof globalValidate === "object" ? globalValidate : {},
    typeof argValidate === "object" ? argValidate : {},
  );
  if (validatorOptions.skipMissingProperties !== false) {
    validatorOptions.skipMissingProperties = true;
  }

  // --------------New added code--------------------
  if (validatorOptions.forbidUnknownValues !== true) {
    validatorOptions.forbidUnknownValues = false;
  }
  //--------------------------------------------------

  const { validateOrReject } = await import("class-validator");
  try {
    if (Array.isArray(argValue)) {
      await Promise.all(
        argValue
          .filter(shouldArgBeValidated)
          .map(argItem => validateOrReject(argItem, validatorOptions)),
      );
    } else {
      await validateOrReject(argValue as T, validatorOptions);
    }
    return argValue;
  } catch (err) {
    throw new ArgumentValidationError(err);
  }
}

I think this small change allows to avoid this wierd issue with the class-validator v14, which is the current version of this package. But I haven't tested this change yet.

2. As you propose, extend GraphQLError for ArgumentValidationError (and possibly other error types), but need test this to ensure it doesn't break the current functionality.

In my opinion these two issues are bugs, because they don't work as described in the documentation with the current versons of class-validator and apollo server and can be fixed in the source code (Apollo server v3 is now deprecated). I don't agree with tagging this issue as a question. Unfortunatelly I have no much time to work on such PR for this moment, so if possible I would like to have this issue open to be able to work on PR when I have more time (if you agree with my opinion and with the proposed code changes)

What do you think, @carlocorradini @MichalLytek ?

[carlocorradini]

1. Why not? I agree with you. As written before, this check is done automatically by GraphQL. @MichalLytek What do you think?
2. GraphQLError already extends Error. Therefore, I think that extending all error(s) to GraphQLError is way better. @MichalLytek What do you think?
3. They are not bugs. class-validator and graphql libraries have been updated, and these new changes will be available and reflected in TypeGraphQL V2 (have you seen it?) :) 

@MichalLytekDo you want me to take care of updating all errors?  

[YMSpektor]

3. They are not bugs. class-validator and graphql libraries have been updated, and these new changes will be available and reflected in TypeGraphQL V2 (have you seen it?) :)

I got these issues using type-graphql 2.0.0-beta.1

[carlocorradini]

It's still a beta :)
There is a lot of work to do and all can help :)
If @MichalLytek agree, I'll update all errors

[YMSpektor]

@carlocorradini thanks again!

I understand that it's a beta, and I don't have any complaints :) Just wanted to said that it seems not to be reflected in V2 for this moment and I wanted to draw your attention to it. TypeGraphQL is my absolute favorite GraphQL library, I used it in my recent projects (and plan to use in future) and it worked great!

[carlocorradini]

@YMSpektor Awesome 🥳

[carlocorradini]

Hey @YMSpektor
See PR #1400 -> Trying to increase the maintainability of the project.
Add your thoughts if you have any ideas or something similar! 

[georgeselkhoury]

This happened to me. I had to add a validation on the input to get around it.

[carlocorradini]

This happened to me. I had to add a validation on the input to get around it.

Yep, it's in the changelog:
When attempting to validate a class instance without metadata for it

[MichalLytek]

As #1331 is merged, the forbidUnknownValues is now set by default. I will try to release beta.2 shortly.

For using with other older version, we have a workaround for providing that option manually via validate.

So nothing more to add here, closing this issue 🔒

[hakobpogh]

Just a note: I had the same issue, and forbidUnknownValues=false fixed it for me.

[itpropro]

As #1331 is merged, the forbidUnknownValues is now set by default. I will try to release beta.2 shortly.

For using with other older version, we have a workaround for providing that option manually via validate.

So nothing more to add here, closing this issue 🔒

Any update on beta-2 @MichalLytek ? Even if it has only this fix in it, it would be worth it :)