class-validator icon indicating copy to clipboard operation
class-validator copied to clipboard

feat: validateOrReject should throw instance of Error

Open bogdan opened this issue 5 years ago • 14 comments

validateOrReject throws an array of errors that does not have a stack. That is bad in case such error is not being caught and results in 500 Internal Server Error.

Is it possible to add something like validateOrError that would throw an instance like this instead:

class ObjectInvalid extends Error {
  errors: ValidationError[]
}

bogdan avatar Dec 20 '19 13:12 bogdan

I do not understand why you need another method. In case you use validateOrReject you know promise is rejected and you have to handle it. Otherwise you will use validateSync

vlapo avatar Mar 24 '20 21:03 vlapo

Not all of the rejects are being caught and processed. It means that if the error is displayed in console or sent to exception notification software, you will have no error stack available.

Example jest error when validateOrReject throws:

      161 |     });
      162 |
    > 163 |     it('fails', async () => {
          |     ^
      164 |       await validateOrReject(new Reseller())
      165 |     });
      166 |   });

You may see that jest can only tell the test where the error occurred not the line number and backtrace.

When you see something like this in console:

Failed: Array [
      ValidationError {
       ...
      },
    ]

You have no idea where to fix it.

Any linter will tell you that throwing something that doesn't inherit Error class is a bad idea:

https://palantir.github.io/tslint/rules/no-string-throw/ https://eslint.org/docs/rules/no-throw-literal

BTW, this is first time I see someone throws an Array. IMO it is even more terrible idea than throwing a string because when you see it in console you don't even realize what had happened.

bogdan avatar Mar 25 '20 11:03 bogdan

Sure I understand now. But it is breaking change. We have to bundle this to next major release.

vlapo avatar Mar 27 '20 21:03 vlapo

You can avoid breaking change by intoroducing a different method like validateOrThrow and maybe deprecating the old one as I suggested initially.

bogdan avatar Mar 28 '20 15:03 bogdan

Sure why not. Let's introduce new validateOrThrow method and mark validateOrReject as deprecated.

vlapo avatar Mar 28 '20 20:03 vlapo

@NoNameProvided @vlapo Any release date on this?

expelliamus avatar Aug 06 '20 13:08 expelliamus

I don't understand, the stack trace will be useless to you because it will originate inside class-validator and not your code.

You can already check if your received error is an instance of the ValidationError and process it in your error handler in a single place.

NoNameProvided avatar Aug 08 '20 22:08 NoNameProvided

@NoNameProvided the problem is that we don't get an instance of ValidationError but an array which contains an object of ValidationError instance

expelliamus avatar Aug 09 '20 09:08 expelliamus

Well then you can check via

Array.isArray(error) && error.every(e => e instanceof ValidationError)

right?

NoNameProvided avatar Aug 09 '20 09:08 NoNameProvided

Nevertheless, as mentioned in other places, the error format will be reworked, but I don't want to start ot monkey-patch it before that.

NoNameProvided avatar Aug 09 '20 10:08 NoNameProvided

@NoNameProvided I guess I can do so, and I definitely will, but what I think is something like this:

  1. Executing validation
  2. An error is raised
  3. I get the error from promise rejection which contains a single error per time (no array)

This could be really useful for an API consumer.

Also, it would be:

Array.isArray(err) && err.every(e => e instanceof ValidationError);

expelliamus avatar Aug 09 '20 17:08 expelliamus

I don't understand, the stack trace will be useless to you because it will originate inside class-validator and not your code.

Having adequate stacktraces is the core feature of any programming language. JS is the only place I saw where it is sometimes broken. Most stacktraces include both a library and application code because they are stack traces. This is only a problem of promises that lose a part of stacktrace under special conditions because of async call, but it should not be the case if you do it right.

The reason you need stacktrace is when you use the library to validate data that is not a user input. Like you are reading data from other API or there is just some tables in your DB where data need to follow certain convention and you communicate to your team what kind of format is required.

On the high level, you need to distinguish the user input error and the application internal error that it doesn't follow its own data conventions. You need stacktraces to locate where second type of error occur and how to fix it.

Typical example: a fake data for your tests that is not a user input, but need to be validated like a user input before running the actual test.

I still think that the most smooth way of developing a different error format is a different method like validateOrThrow that will allow to change the format without introducing an instant breaking change.

Nevertheless, as mentioned in other places, the error format will be reworked, but I don't want to start ot monkey-patch it before that.

Can you please give a link to the source discussion on that?

bogdan avatar Aug 10 '20 12:08 bogdan

I agree on this since some packages do check errors with error instanceof Error and if not, they just wrap it up like new Error(error). Which invokes ValidationError.prototype.toString.

In my case, apollo-server does. When I trying to access error in formatError, the error.originalError property is an direct instance of Error so unable to access ValidationError properties.

Seemspyo avatar Mar 17 '21 08:03 Seemspyo

Any updates on this?

clauRamirez avatar May 12 '22 14:05 clauRamirez