fix inherited property validator overrides

NickKelly1 · NickKelly1 · commit 166f5306ba30 · 2020-09-08T11:23:18.000+10:00
diff --git a/README.md b/README.md
@@ -726,6 +726,7 @@ Lets create another custom validation decorator called `IsUserAlreadyExist`:
    export function IsUserAlreadyExist(validationOptions?: ValidationOptions) {
      return function (object: Object, propertyName: string) {
        registerDecorator({
+         name: 'IsUserAlreadyExist',
          target: object.constructor,
          propertyName: propertyName,
          options: validationOptions,
diff --git a/sample/sample6-custom-decorator/IsLongerThan.ts b/sample/sample6-custom-decorator/IsLongerThan.ts
@@ -7,6 +7,7 @@ import { ValidationArguments } from '../../src/validation/ValidationArguments';
 export function IsLongerThan(property: string, validationOptions?: ValidationOptions) {
   return function (object: Object, propertyName: string) {
     registerDecorator({
+      name: 'IsLongerThan',
       target: object.constructor,
       propertyName: propertyName,
       options: validationOptions,
diff --git a/src/decorator/common/Allow.ts b/src/decorator/common/Allow.ts
@@ -10,6 +10,7 @@ import { getMetadataStorage } from '../../metadata/MetadataStorage';
 export function Allow(validationOptions?: ValidationOptions): PropertyDecorator {
   return function (object: object, propertyName: string): void {
     const args: ValidationMetadataArgs = {
+      name: 'Allow',
       type: ValidationTypes.WHITELIST,
       target: object.constructor,
       propertyName: propertyName,
diff --git a/src/decorator/common/IsOptional.ts b/src/decorator/common/IsOptional.ts
@@ -10,6 +10,7 @@ import { getMetadataStorage } from '../../metadata/MetadataStorage';
 export function IsOptional(validationOptions?: ValidationOptions): PropertyDecorator {
   return function (object: object, propertyName: string): void {
     const args: ValidationMetadataArgs = {
+      name: 'IsOptional',
       type: ValidationTypes.CONDITIONAL_VALIDATION,
       target: object.constructor,
       propertyName: propertyName,
diff --git a/src/decorator/common/Validate.ts b/src/decorator/common/Validate.ts
@@ -26,6 +26,9 @@ export function ValidatorConstraint(options?: { name?: string; async?: boolean }
 /**
  * Performs validation based on the given custom validation class.
  * Validation class must be decorated with ValidatorConstraint decorator.
+ *
+ * TODO: allow passing in a `name` so the validator instance created can be uniquely identified
+ * until then, this validator will be overwritten by properties decorated with `Validate` on subclasses
  */
 export function Validate(constraintClass: Function, validationOptions?: ValidationOptions): PropertyDecorator;
 export function Validate(
@@ -40,6 +43,7 @@ export function Validate(
 ): PropertyDecorator {
   return function (object: object, propertyName: string): void {
     const args: ValidationMetadataArgs = {
+      name: 'Validate',
       type: ValidationTypes.CUSTOM_VALIDATION,
       target: object.constructor,
       propertyName: propertyName,
diff --git a/src/decorator/common/ValidateIf.ts b/src/decorator/common/ValidateIf.ts
@@ -6,13 +6,17 @@ import { getMetadataStorage } from '../../metadata/MetadataStorage';
 
 /**
  * Ignores the other validators on a property when the provided condition function returns false.
+ *
+ * TODO: allow passing in a `name` so the validator instance created can be uniquely identified
+ * until then, this validator will be overwritten by properties decorated with `Validate` on subclasses
  */
 export function ValidateIf(
   condition: (object: any, value: any) => boolean,
   validationOptions?: ValidationOptions
 ): PropertyDecorator {
   return function (object: object, propertyName: string): void {
     const args: ValidationMetadataArgs = {
+      name: 'ValidateIf',
       type: ValidationTypes.CONDITIONAL_VALIDATION,
       target: object.constructor,
       propertyName: propertyName,
diff --git a/src/decorator/common/ValidateNested.ts b/src/decorator/common/ValidateNested.ts
@@ -14,6 +14,7 @@ export function ValidateNested(validationOptions?: ValidationOptions): PropertyD
 
   return function (object: object, propertyName: string): void {
     const args: ValidationMetadataArgs = {
+      name: 'ValidateNested',
       type: ValidationTypes.NESTED_VALIDATION,
       target: object.constructor,
       propertyName: propertyName,
diff --git a/src/decorator/common/ValidatePromise.ts b/src/decorator/common/ValidatePromise.ts
@@ -10,6 +10,7 @@ import { getMetadataStorage } from '../../metadata/MetadataStorage';
 export function ValidatePromise(validationOptions?: ValidationOptions): PropertyDecorator {
   return function (object: object, propertyName: string): void {
     const args: ValidationMetadataArgs = {
+      name: 'ValidatePromise',
       type: ValidationTypes.PROMISE_VALIDATION,
       target: object.constructor,
       propertyName: propertyName,
diff --git a/src/metadata/MetadataStorage.ts b/src/metadata/MetadataStorage.ts
@@ -119,11 +119,25 @@ export class MetadataStorage {
     // filter out duplicate metadatas, prefer original metadatas instead of inherited metadatas
     const uniqueInheritedMetadatas = inheritedMetadatas.filter(inheritedMetadata => {
       return !originalMetadatas.find(originalMetadata => {
-        // expect validators to be duplicate if they point to the same validator function
-        return (
-          originalMetadata.propertyName === inheritedMetadata.propertyName &&
-          originalMetadata.constraintCls === inheritedMetadata.constraintCls
-        );
+        // We have no clean way to determine if 2 validators are the same, and thus can't easily determine
+        // which validators have been overwritten by a subclass
+        //  - Can't use `validatorCls` object/function: it's recreated on a per-usage basis so two decorators will give different instances
+        //  - Can't use `ValidationTypes`: this was useable until 11a7b8bb59c83d55bc723ebb236fdca912f49d88,
+        //    after which 90% of ValidationTypes were removed in favour of type "customValidation". Note that
+        //    some validators, including any custom validators, still had type "customValidation" before this, and therefore
+        //    did not work with inherited validation
+        //  - `name`: can be used to uniquely identify a validator, but is optional to not break backwards compatability
+        //    in a future release, it should be made required
+        const isSameProperty = originalMetadata.propertyName === inheritedMetadata.propertyName;
+        const isSameValidator =
+          originalMetadata.name && inheritedMetadata.name
+            ? // TODO: when names becomes required, ONLY compare by name
+              originalMetadata.name === inheritedMetadata.name
+            : // 95% of decorators are of type "customValidation", despite being different decorators
+              // therefore this equality comparison introduces lots of false positives
+              originalMetadata.type === inheritedMetadata.type;
+
+        return isSameProperty && isSameValidator;
       });
     });
 
diff --git a/src/metadata/ValidationMetadata.ts b/src/metadata/ValidationMetadata.ts
@@ -9,6 +9,11 @@ export class ValidationMetadata {
   // Properties
   // -------------------------------------------------------------------------
 
+  /**
+   * Validation name. Used to uniquely identify this validator.
+   */
+  name?: string;
+
   /**
    * Validation type.
    */
@@ -69,6 +74,7 @@ export class ValidationMetadata {
   // -------------------------------------------------------------------------
 
   constructor(args: ValidationMetadataArgs) {
+    this.name = args.name;
     this.type = args.type;
     this.target = args.target;
     this.propertyName = args.propertyName;
diff --git a/src/metadata/ValidationMetadataArgs.ts b/src/metadata/ValidationMetadataArgs.ts
@@ -4,6 +4,11 @@ import { ValidationOptions } from '../decorator/ValidationOptions';
  * Constructor arguments for ValidationMetadata class.
  */
 export interface ValidationMetadataArgs {
+  /**
+   * Validation name. Used to uniquely identify this validator.
+   */
+  name?: string;
+
   /**
    * Validation type.
    */
diff --git a/src/register-decorator.ts b/src/register-decorator.ts
@@ -75,6 +75,7 @@ export function registerDecorator(options: ValidationDecoratorOptions): void {
   }
 
   const validationMetadataArgs: ValidationMetadataArgs = {
+    name: options.name,
     type: options.name && ValidationTypes.isValid(options.name) ? options.name : ValidationTypes.CUSTOM_VALIDATION,
     target: options.target,
     propertyName: options.propertyName,
diff --git a/src/validation-schema/ValidationSchema.ts b/src/validation-schema/ValidationSchema.ts
@@ -16,6 +16,11 @@ export interface ValidationSchema {
      * Name of the object's property to be validated which holds an array of validation constraints.
      */
     [propertyName: string]: {
+      /**
+       * Validation name. Used to uniquely identify this validator.
+       */
+      name?: string;
+
       /**
        * Validation type. Should be one of the ValidationTypes value.
        */
diff --git a/src/validation-schema/ValidationSchemaToMetadataTransformer.ts b/src/validation-schema/ValidationSchemaToMetadataTransformer.ts
@@ -18,6 +18,7 @@ export class ValidationSchemaToMetadataTransformer {
           each: validation.each,
         };
         const args: ValidationMetadataArgs = {
+          name: validation.name,
           type: validation.type,
           target: schema.name,
           propertyName: property,
diff --git a/test/functional/custom-decorators.spec.ts b/test/functional/custom-decorators.spec.ts
@@ -11,11 +11,11 @@ describe('decorator with inline validation', () => {
   function IsLongerThan(property: string, validationOptions?: ValidationOptions) {
     return function (object: object, propertyName: string): void {
       registerDecorator({
+        name: 'isLongerThan',
         target: object.constructor,
         propertyName: propertyName,
         options: validationOptions,
         constraints: [property],
-        name: 'isLongerThan',
         validator: {
           validate(value: any, args: ValidationArguments): Promise<boolean> | boolean {
             const [relatedPropertyName] = args.constraints;
@@ -109,11 +109,11 @@ describe('decorator with default message', () => {
   function IsLonger(property: string, validationOptions?: ValidationOptions) {
     return function (object: object, propertyName: string): void {
       registerDecorator({
+        name: 'isLonger',
         target: object.constructor,
         propertyName: propertyName,
         options: validationOptions,
         constraints: [property],
-        name: 'isLonger',
         validator: {
           validate(value: any, args: ValidationArguments): boolean {
             const [relatedPropertyName] = args.constraints;
@@ -183,6 +183,7 @@ describe('decorator with separate validation constraint class', () => {
   function IsShorterThan(property: string, validationOptions?: ValidationOptions) {
     return function (object: object, propertyName: string): void {
       registerDecorator({
+        name: 'IsShorterThan',
         target: object.constructor,
         propertyName: propertyName,
         options: validationOptions,
diff --git a/test/functional/inherited-validation.spec.ts b/test/functional/inherited-validation.spec.ts
@@ -1,4 +1,4 @@
-import { Contains, MinLength } from '../../src/decorator/decorators';
+import { Contains, MinLength, Equals, Min } from '../../src/decorator/decorators';
 import { Validator } from '../../src/validation/Validator';
 
 const validator = new Validator();
@@ -61,4 +61,104 @@ describe('inherited validation', () => {
       expect(errors[0].value).toEqual('helo');
     });
   });
+
+  it('should override inherited validators in sub classes', () => {
+    expect.assertions(9);
+
+    class MyClass {
+      @Min(30)
+      age: number;
+
+      @Equals('validator')
+      first_name: string;
+
+      @Equals('class')
+      last_name: string;
+    }
+
+    class MySubClass extends MyClass {
+      @Min(40)
+      age: number;
+
+      @Equals('class')
+      first_name: string;
+
+      @Equals('validator')
+      last_name: string;
+    }
+
+    const model = new MySubClass();
+    model.age = 20; // fail validation (using sub classes constraint)
+    model.first_name = 'class'; // pass validation (overriding fail from parent)
+    model.last_name = 'class'; // fail validation (overriding pass from parent)
+
+    return validator.validate(model).then(errors => {
+      expect(errors.length).toEqual(2);
+      expect(errors[0].target).toEqual(model);
+      expect(errors[0].property).toEqual('age');
+      expect(errors[0].constraints).toEqual({
+        min: 'age must not be less than 40',
+      });
+      expect(errors[0].value).toEqual(20);
+
+      expect(errors[1].target).toEqual(model);
+      expect(errors[1].property).toEqual('last_name');
+      expect(errors[1].constraints).toEqual({
+        equals: 'last_name must be equal to validator',
+      });
+      expect(errors[1].value).toEqual('class');
+    });
+  });
+
+  it('should not override different validators of inherited properties in the parent class', () => {
+    expect.assertions(4);
+
+    class MyClass {
+      @Contains('parent-class')
+      title: string;
+    }
+
+    class MySubClass extends MyClass {
+      @Equals('sub-class')
+      title: string;
+    }
+
+    const model = new MySubClass();
+    model.title = 'sub-class';
+
+    return validator.validate(model).then(errors => {
+      expect(errors.length).toEqual(1);
+      expect(errors[0].target).toEqual(model);
+      expect(errors[0].property).toEqual('title');
+      expect(errors[0].constraints).toEqual({
+        contains: 'title must contain a parent-class string',
+      });
+    });
+  });
+
+  it('should not override different validators of inherited properties in the sub class', () => {
+    expect.assertions(4);
+
+    class MyClass {
+      @Contains('parent-class')
+      title: string;
+    }
+
+    class MySubClass extends MyClass {
+      @Equals('sub-class')
+      title: string;
+    }
+
+    const model = new MySubClass();
+    model.title = 'parent-class';
+
+    return validator.validate(model).then(errors => {
+      expect(errors.length).toEqual(1);
+      expect(errors[0].target).toEqual(model);
+      expect(errors[0].property).toEqual('title');
+      expect(errors[0].constraints).toEqual({
+        equals: 'title must be equal to sub-class',
+      });
+    });
+  });
 });
diff --git a/test/functional/sync-validation.spec.ts b/test/functional/sync-validation.spec.ts
@@ -18,12 +18,12 @@ describe('sync validation should ignore async validation constraints', () => {
   function IsLonger(property: string, validationOptions?: ValidationOptions) {
     return function (object: object, propertyName: string): void {
       registerDecorator({
+        name: 'isLonger',
         target: object.constructor,
         propertyName: propertyName,
         options: validationOptions,
         constraints: [property],
         async: true,
-        name: 'isLonger',
         validator: {
           validate(value: any, args: ValidationArguments): Promise<boolean> {
             return Promise.resolve(false);
diff --git a/test/functional/validation-functions-and-decorators.spec.ts b/test/functional/validation-functions-and-decorators.spec.ts
@@ -3656,13 +3656,13 @@ describe('Length', () => {
   });
 
   it('should return error object with proper data', () => {
-    const validationType = 'length';
+    const validationType = 'isLength';
     const message = 'someProperty must be longer than or equal to ' + constraint1 + ' characters';
     checkReturnedError(new MyClass(), ['', 'a'], validationType, message);
   });
 
   it('should return error object with proper data', () => {
-    const validationType = 'length';
+    const validationType = 'isLength';
     const message = 'someProperty must be shorter than or equal to ' + constraint2 + ' characters';
     checkReturnedError(new MyClass(), ['aaaa', 'azzazza'], validationType, message);
   });
diff --git a/test/functional/validation-options.spec.ts b/test/functional/validation-options.spec.ts
@@ -1079,11 +1079,11 @@ describe('context', () => {
     function IsLongerThan(property: string, validationOptions?: ValidationOptions) {
       return function (object: object, propertyName: string): void {
         registerDecorator({
+          name: 'isLongerThan',
           target: object.constructor,
           propertyName: propertyName,
           options: validationOptions,
           constraints: [property],
-          name: 'isLongerThan',
           validator: {
             validate(value: any, args: ValidationArguments): boolean {
               const [relatedPropertyName] = args.constraints;
