[FEAT] Improve error message of isEnum validator #830

Bendakh · 2023-12-07T16:43:17Z

Description

This PR aims to improve the error message of the IsEnum validator. The new error will display the invalid value of the property that was not validated.

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

amarlankri · 2023-12-08T09:06:22Z

packages/custom-decorators/test/is-enum-decorator.test.ts

+function validateValues(
+  object: { testProperty: any },
+  values: any[],
+  validatorOptions?: ValidatorOptions,
+): Promise<any> {


Avoid to use any. If there is no way to type the function properly (using an interface or generics), it is better to use unknown. Here, actually, it is not big deal because it is a test implementation, but it is better for readability and test maintenance.

I see. I used the any because I got an error with Object.entries saying that it can't accept arguments of type unkown

amarlankri · 2023-12-08T09:13:41Z

packages/custom-decorators/test/is-enum-decorator.test.ts

+function validateValues(
+  object: { testProperty: any },
+  values: any[],
+  validatorOptions?: ValidatorOptions,
+): Promise<any> {
+  const validator = new Validator();
+  const promises = values.map((value) => {
+    object.testProperty = value;
+    return validator.validate(object, validatorOptions).then((errors) => {
+      expect(errors.length).toEqual(0);
+      if (errors.length > 0) {
+        console.log(`Unexpected errors: ${JSON.stringify(errors)}`);
+        throw new Error('Unexpected validation errors');
+      }
+    });
+  });
+
+  return Promise.all(promises);
+}
+
+function checkReturnedError(
+  object: { testProperty: any },
+  values: any[],
+  validationType: string,
+  messages: string[],
+  validatorOptions?: ValidatorOptions,
+): Promise<any> {
+  let messagesIncrementor: number = 0;
+  const validator = new Validator();
+  const promises = values.map((value) => {
+    object.testProperty = value;
+    return validator.validate(object, validatorOptions).then((errors) => {
+      expect(errors.length).toEqual(1);
+      expect(errors[0].target).toEqual(object);
+      expect(errors[0].property).toEqual('testProperty');
+      expect(errors[0].constraints).toEqual({ [validationType]: messages[messagesIncrementor] });
+      expect(errors[0].value).toEqual(value);
+      messagesIncrementor++;
+    });
+  });
+
+  return Promise.all(promises);
+}


In my opinion, those utility function adds more complexity than they solve. And both are used once, so they didn't reduce duplication.

Do you think they are useful? 🤔

I though that maybe we can use them for later and keep the tests simple with the minimum number of code lines

amarlankri · 2023-12-08T09:28:15Z

packages/custom-decorators/src/is-enum.decorator.ts

Actually, I noticed that it is possible to use the decorator as a normal function (without annotation @). As a result, there is no need to duplicate the implementation of the original decorator. Instead, you can write:

import { IsEnum as OriginalIsEnum, ValidationOptions } from 'class-validator'; export const IsEnum = (entity: object, validationOptions?: ValidationOptions): PropertyDecorator => OriginalIsEnum(entity, { message: '$property has the value $value but must be one of the following values: $constraint2', ...validationOptions });

amarlankri · 2023-12-08T09:28:48Z

tsconfig.json

@@ -25,7 +25,8 @@

    /* Experimental Options */                
    "experimentalDecorators": true,        /* Enables experimental support for ES7 decorators. */
-    "emitDecoratorMetadata": true          /* Enables experimental support for emitting type metadata for decorators. */
+    "emitDecoratorMetadata": true,         /* Enables experimental support for emitting type metadata for decorators. */
+    "strictPropertyInitialization": false,      /* Disables mandatory property initialization */


Why did you change this config?

It's for the testing classes. The initialization of the properties is mandatory so with this config we can avoid the error that we are getting if we don't initialize them

Not sure it is a good idea to "relax" a TypeScript option just for a test implementation. You cannot initialise them instead?

I'll do that instead then

g-ongenae · 2023-12-08T11:17:44Z

packages/custom-decorators/package.json

@@ -36,7 +36,8 @@
    "url": "https://github.com/algoan/nestjs-components/issues"
  },
  "dependencies": {
-    "jwt-decode": "^4.0.0"
+    "jwt-decode": "^4.0.0",
+    "class-validator": "^0.14.0"


Maybe this should be a peer dependency instead?

The next task is to re export all the validators from class-validator so that we import validators through a single package (i.e. @algoan/custom-decorators). With this coming feature, I think it makes sense to have it as direct dependency.

Ah! I didn't know that. Then, I think it would be better to create a new package @algoan/nestjs-class-validator instead of updating @algoan/nestjs-custom-decorators.

The current decorators in this package are dedicated to parameters instead of validation and are few in numbers. Adding the whole class-validator will create some confusion, I fear. Hence, a new package.

g-ongenae · 2023-12-08T15:20:22Z

packages/custom-decorators/package.json

This file should not be changed by this PR anymore.

g-ongenae · 2023-12-08T15:20:55Z

tsconfig.json

packages/nestjs-class-validators/src/index.ts

g-ongenae · 2023-12-08T15:23:54Z

packages/nestjs-class-validators/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "nestjs-class-validators",


Suggested change

"name": "nestjs-class-validators",

"name": "@algoan/nestjs-class-validators",

It also should be added it here:

nestjs-components/package.json

Lines 91 to 92 in 5e32721

"dependencies": {

"@algoan/nestjs-custom-decorators": "file:packages/custom-decorators",

g-ongenae · 2023-12-08T15:27:13Z

packages/nestjs-class-validators/package.json

The folder can be named class-validators directly for coherency with the other packages. (I.e. nestjs in the package name, not in the folder name.)

Reading your comment, I realised that the validators are not really linked to nestjs 😅 So I don't know if it is okay?

This should be put in our mono repo instead 🤪

qhdinh · 2023-12-08T15:31:57Z

packages/custom-decorators/test/is-enum-decorator.test.ts

+    return validator.validate(object, validatorOptions).then((errors) => {
+      expect(errors.length).toEqual(0);
+      if (errors.length > 0) {
+        console.log(`Unexpected errors: ${JSON.stringify(errors)}`);
+        throw new Error('Unexpected validation errors');
+      }
+    });


why expecting errors.length to be 0 then if errors.length > 0?

We can remove the expect in this case

But in a test there must be expecting something. In your test, you just call validateValues().
I think it'd be cleaner validateValues() just return a list of validation errors. And in the test you expect the list after the function call.

I see your point. I didn't change much of the implementation for the tests to as I said in another comment avoid breaking anything.

qhdinh · 2023-12-08T15:35:21Z

packages/custom-decorators/test/is-enum-decorator.test.ts

+): Promise<any> {
+  const validator = new Validator();
+  const promises = values.map((value) => {
+    object.testProperty = value;


there is just one instance of object but you set and test many async validation parallelly on the same property, I'm not sure it works correctly as expected

It's the test code used in the class-validator repository. I didn't want to change it so that I don't break anything

qhdinh · 2023-12-08T15:39:57Z

packages/custom-decorators/src/is-enum.decorator.ts

+
+export const IsEnum = (entity: object, validationOptions?: ValidationOptions): PropertyDecorator =>
+  OriginalIsEnum(entity, {
+    message: '$property has the value $value but must be one of the following values: $constraint2',


$constraint1 refers to entity, so $constraint2 refers to what in this case?

To the validEnumEntities, see code.

Okay, thanks. I don't see they indicate in the offical doc page

amarlankri · 2023-12-11T10:44:21Z

packages/class-validators/src/is-enum.decorator.ts

@@ -0,0 +1,7 @@
+import { IsEnum as OriginalIsEnum, ValidationOptions } from 'class-validator';
+
+export const IsEnum = (entity: object, validationOptions?: ValidationOptions): PropertyDecorator =>


Since this function is part of the public API, you should probably add JSDoc.

amarlankri

Since this package is public, we need to be stricter on the documentation.

amarlankri · 2023-12-11T13:32:01Z

packages/class-validators/README.md

@@ -0,0 +1,21 @@
+# Nestjs-class-validators


Suggested change

# Nestjs-class-validators

# Nestjs class validators

amarlankri · 2023-12-11T13:32:19Z

packages/class-validators/README.md

+
+## IsEnum
+
+A class validators that validates the enum type.


Suggested change

A class validators that validates the enum type.

A class validator that validates the enum type.

amarlankri · 2023-12-11T13:32:28Z

packages/class-validators/README.md

+
+A class validators that validates the enum type.
+
+### Usage:


Suggested change

### Usage:

### Usage

amarlankri · 2023-12-11T13:35:06Z

packages/class-validators/README.md

@@ -0,0 +1,21 @@
+# Nestjs-class-validators
+
+A set of class validators.


In the description, I think it is important to say that it is based on the existing class-validators (with a link) since most of the validators are just re-exported.

amarlankri

Thanks for the adjustments!

Bendakh requested review from g-ongenae, qhdinh, meriamBenSassi and amarlankri December 7, 2023 16:45

Bendakh force-pushed the feature/OverrideIsEnum branch from 01bf2db to d0d221e Compare December 7, 2023 16:54

g-ongenae added the enhancement New feature or request label Dec 8, 2023

g-ongenae changed the title ~~[ALG-1758] Improve error message of isEnum validator~~ [FEAT] Improve error message of isEnum validator Dec 8, 2023

amarlankri reviewed Dec 8, 2023

View reviewed changes

Bendakh requested a review from amarlankri December 8, 2023 10:45

g-ongenae reviewed Dec 8, 2023

View reviewed changes

Bendakh requested a review from g-ongenae December 8, 2023 15:17

g-ongenae reviewed Dec 8, 2023

View reviewed changes

qhdinh reviewed Dec 8, 2023

View reviewed changes

Bendakh added 7 commits December 11, 2023 11:18

refactor: improve error message of isenum validator

941b4da

refactor: refactor the new is enum function

ec2142b

refactor: create a new package for the class validators

eec2950

feat: re export the nestjs class validators

82b4c06

chore: change new folder name

59b4482

chore: config cleanup

722ad71

chore: update readme

5909a1d

Bendakh force-pushed the feature/OverrideIsEnum branch from 5d041a9 to 5909a1d Compare December 11, 2023 10:22

Bendakh requested review from qhdinh and g-ongenae December 11, 2023 10:22

amarlankri approved these changes Dec 11, 2023

View reviewed changes

qhdinh approved these changes Dec 11, 2023

View reviewed changes

chore: add readme file to the class validator package

f961358

amarlankri reviewed Dec 11, 2023

View reviewed changes

Bendakh added 2 commits December 11, 2023 15:04

chore: update the class validators readme

ea8e090

chore: add jsdoc to the isenum validator

d084e4b

amarlankri approved these changes Dec 11, 2023

View reviewed changes

g-ongenae approved these changes Dec 11, 2023

View reviewed changes

g-ongenae merged commit 787895d into algoan:master Dec 11, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Improve error message of isEnum validator #830

[FEAT] Improve error message of isEnum validator #830

Bendakh commented Dec 7, 2023 •

edited by g-ongenae

Loading

amarlankri Dec 8, 2023

Bendakh Dec 8, 2023

amarlankri Dec 8, 2023

Bendakh Dec 8, 2023

amarlankri Dec 8, 2023

amarlankri Dec 8, 2023

Bendakh Dec 8, 2023

amarlankri Dec 8, 2023

Bendakh Dec 8, 2023

g-ongenae Dec 8, 2023

amarlankri Dec 8, 2023

g-ongenae Dec 8, 2023

g-ongenae Dec 8, 2023

g-ongenae Dec 8, 2023

g-ongenae Dec 8, 2023

g-ongenae Dec 8, 2023

amarlankri Dec 8, 2023

qhdinh Dec 8, 2023

qhdinh Dec 8, 2023

Bendakh Dec 8, 2023

qhdinh Dec 8, 2023

Bendakh Dec 11, 2023 •

edited

Loading

qhdinh Dec 8, 2023

Bendakh Dec 8, 2023

qhdinh Dec 8, 2023

g-ongenae Dec 8, 2023

qhdinh Dec 8, 2023

amarlankri Dec 11, 2023

amarlankri left a comment

amarlankri Dec 11, 2023

amarlankri Dec 11, 2023

amarlankri Dec 11, 2023

amarlankri Dec 11, 2023

amarlankri left a comment

	"name": "nestjs-class-validators",
	"name": "@algoan/nestjs-class-validators",

	"dependencies": {
	"@algoan/nestjs-custom-decorators": "file:packages/custom-decorators",

		@@ -0,0 +1,7 @@
		import { IsEnum as OriginalIsEnum, ValidationOptions } from 'class-validator';

		export const IsEnum = (entity: object, validationOptions?: ValidationOptions): PropertyDecorator =>

	A class validators that validates the enum type.
	A class validator that validates the enum type.

		@@ -0,0 +1,21 @@
		# Nestjs-class-validators

		A set of class validators.

[FEAT] Improve error message of isEnum validator #830

[FEAT] Improve error message of isEnum validator #830

Conversation

Bendakh commented Dec 7, 2023 • edited by g-ongenae Loading

Description

Motivation and Context

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bendakh Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarlankri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarlankri left a comment

Choose a reason for hiding this comment

Bendakh commented Dec 7, 2023 •

edited by g-ongenae

Loading

Bendakh Dec 11, 2023 •

edited

Loading