From acb050b3f3d0d52e7f728e184fe082568dd84c8f Mon Sep 17 00:00:00 2001 From: NickKelly1 Date: Tue, 8 Sep 2020 08:18:46 +1000 Subject: [PATCH 1/5] fix: decorating inherited property #633 --- src/metadata/MetadataStorage.ts | 3 ++- test/functional/inherited-validation.spec.ts | 27 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/metadata/MetadataStorage.ts b/src/metadata/MetadataStorage.ts index 286346ac90..aea8319eaa 100644 --- a/src/metadata/MetadataStorage.ts +++ b/src/metadata/MetadataStorage.ts @@ -138,9 +138,10 @@ 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.type === inheritedMetadata.type + originalMetadata.constraintCls === inheritedMetadata.constraintCls ); }); }); diff --git a/test/functional/inherited-validation.spec.ts b/test/functional/inherited-validation.spec.ts index 04254a9d3b..f2d4697027 100644 --- a/test/functional/inherited-validation.spec.ts +++ b/test/functional/inherited-validation.spec.ts @@ -34,4 +34,31 @@ describe('inherited validation', () => { expect(errors[1].value).toEqual('helo world'); }); }); + + it('should use validators from parent and child classes', () => { + expect.assertions(5); + + class MyClass { + @Contains('hello') + title: string; + } + + class MySubClass extends MyClass { + @MinLength(5) + title: string; + } + + const model = new MySubClass(); + model.title = 'helo'; + 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({ + minLength: 'title must be longer than or equal to 5 characters', + contains: 'title must contain a hello string' , + }); + expect(errors[0].value).toEqual('helo'); + }); + }); }); From 0bd38540b5633d238904bb082cc95eabb63a4285 Mon Sep 17 00:00:00 2001 From: NickKelly1 Date: Tue, 8 Sep 2020 08:23:10 +1000 Subject: [PATCH 2/5] run prettier --- test/functional/inherited-validation.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/inherited-validation.spec.ts b/test/functional/inherited-validation.spec.ts index f2d4697027..b54c82807a 100644 --- a/test/functional/inherited-validation.spec.ts +++ b/test/functional/inherited-validation.spec.ts @@ -56,7 +56,7 @@ describe('inherited validation', () => { expect(errors[0].property).toEqual('title'); expect(errors[0].constraints).toEqual({ minLength: 'title must be longer than or equal to 5 characters', - contains: 'title must contain a hello string' , + contains: 'title must contain a hello string', }); expect(errors[0].value).toEqual('helo'); }); From 06539664390c468266d73cee12f80b3c46621981 Mon Sep 17 00:00:00 2001 From: NickKelly1 Date: Tue, 8 Sep 2020 11:23:18 +1000 Subject: [PATCH 3/5] fix inherited property validator overrides --- README.md | 1 + .../sample6-custom-decorator/IsLongerThan.ts | 1 + src/decorator/common/Allow.ts | 1 + src/decorator/common/IsOptional.ts | 1 + src/decorator/common/Validate.ts | 4 + src/decorator/common/ValidateIf.ts | 4 + src/decorator/common/ValidateNested.ts | 1 + src/decorator/common/ValidatePromise.ts | 1 + src/metadata/MetadataStorage.ts | 24 ++++- src/metadata/ValidationMetadata.ts | 6 ++ src/metadata/ValidationMetadataArgs.ts | 5 + src/register-decorator.ts | 1 + src/validation-schema/ValidationSchema.ts | 5 + .../ValidationSchemaToMetadataTransformer.ts | 1 + test/functional/custom-decorators.spec.ts | 5 +- test/functional/inherited-validation.spec.ts | 102 +++++++++++++++++- test/functional/sync-validation.spec.ts | 2 +- test/functional/validation-options.spec.ts | 2 +- 18 files changed, 157 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 458a5e2de4..955e9dfe58 100644 --- a/README.md +++ b/README.md @@ -733,6 +733,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 index 21be1f3417..2ebe1d1d25 100644 --- 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 index 943722ec8c..11dc350ce6 100644 --- 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 index 25ef08915c..87e7488c3f 100644 --- 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 index 59c80cdd08..0fa8533c6d 100644 --- 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 index 14f1deeb77..929c7651bc 100644 --- a/src/decorator/common/ValidateIf.ts +++ b/src/decorator/common/ValidateIf.ts @@ -6,6 +6,9 @@ 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, @@ -13,6 +16,7 @@ export function ValidateIf( ): 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 index da56eaefa4..adccd8c451 100644 --- 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 index bd90519e86..2a2a5a04cd 100644 --- 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 index aea8319eaa..8f3518de2d 100644 --- a/src/metadata/MetadataStorage.ts +++ b/src/metadata/MetadataStorage.ts @@ -138,11 +138,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 index c1b1acce82..3abd309ac5 100644 --- 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. */ @@ -74,6 +79,7 @@ export class ValidationMetadata { // ------------------------------------------------------------------------- constructor(args: ValidationMetadataArgs) { + this.name = args.name; this.type = args.type; this.name = args.name; this.target = args.target; diff --git a/src/metadata/ValidationMetadataArgs.ts b/src/metadata/ValidationMetadataArgs.ts index ff28b3e0af..c71330b345 100644 --- 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 index ffe4bf86a5..6a9d46621f 100644 --- 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, name: options.name, target: options.target, diff --git a/src/validation-schema/ValidationSchema.ts b/src/validation-schema/ValidationSchema.ts index f76fec6807..89969aa053 100644 --- 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 index 7739b6da92..9e3820789c 100644 --- 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, name: validation.name, target: schema.name, diff --git a/test/functional/custom-decorators.spec.ts b/test/functional/custom-decorators.spec.ts index 39204addb8..d3397ee0a1 100644 --- 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 { 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 index b54c82807a..9696982a70 100644 --- 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 index 39ae097587..3299783aa5 100644 --- 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 { return Promise.resolve(false); diff --git a/test/functional/validation-options.spec.ts b/test/functional/validation-options.spec.ts index 99db80aa76..f4c1e38e97 100644 --- a/test/functional/validation-options.spec.ts +++ b/test/functional/validation-options.spec.ts @@ -1090,11 +1090,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; From 4056f3e45cffa496aba585f67af1cf9527fdd897 Mon Sep 17 00:00:00 2001 From: NickKelly1 Date: Tue, 8 Sep 2020 11:41:53 +1000 Subject: [PATCH 4/5] update readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 955e9dfe58..203ac9f10c 100644 --- a/README.md +++ b/README.md @@ -371,7 +371,7 @@ export class Post { ## Inheriting Validation decorators -When you define a subclass which extends from another one, the subclass will automatically inherit the parent's decorators. If a property is redefined in the descendant, class decorators will be applied on it from both its own class and the base class. +When you define a subclass which extends from another one, the subclass will automatically inherit the parent's decorators. If a property is redefined in the descendant, class decorators will be applied on it from both its own class and the base class. If a property-decorator pair is defined in both the sub-class and parent-class, the decorator from the sub-class will be used instead of the parent-class. ```typescript import { validate } from 'class-validator'; From 704d277e85c602bf38cb3f16cf904410572c57dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20Ol=C3=A1h?= Date: Sat, 3 Dec 2022 14:53:27 +0000 Subject: [PATCH 5/5] fix: remove duplicated property declarations --- src/metadata/ValidationMetadata.ts | 6 ------ src/metadata/ValidationMetadataArgs.ts | 5 ----- src/register-decorator.ts | 1 - src/validation-schema/ValidationSchema.ts | 5 ----- .../ValidationSchemaToMetadataTransformer.ts | 1 - 5 files changed, 18 deletions(-) diff --git a/src/metadata/ValidationMetadata.ts b/src/metadata/ValidationMetadata.ts index 3abd309ac5..c31b64c14f 100644 --- a/src/metadata/ValidationMetadata.ts +++ b/src/metadata/ValidationMetadata.ts @@ -19,11 +19,6 @@ export class ValidationMetadata { */ type: string; - /** - * Validator name. - */ - name?: string; - /** * Target class to which this validation is applied. */ @@ -81,7 +76,6 @@ export class ValidationMetadata { constructor(args: ValidationMetadataArgs) { this.name = args.name; this.type = args.type; - this.name = args.name; this.target = args.target; this.propertyName = args.propertyName; this.constraints = args?.constraints; diff --git a/src/metadata/ValidationMetadataArgs.ts b/src/metadata/ValidationMetadataArgs.ts index c71330b345..85af3107ac 100644 --- a/src/metadata/ValidationMetadataArgs.ts +++ b/src/metadata/ValidationMetadataArgs.ts @@ -14,11 +14,6 @@ export interface ValidationMetadataArgs { */ type: string; - /** - * Validator name. - */ - name?: string; - /** * Object that is used to be validated. */ diff --git a/src/register-decorator.ts b/src/register-decorator.ts index 6a9d46621f..1b5de8d326 100644 --- a/src/register-decorator.ts +++ b/src/register-decorator.ts @@ -77,7 +77,6 @@ export function registerDecorator(options: ValidationDecoratorOptions): void { const validationMetadataArgs: ValidationMetadataArgs = { name: options.name, type: options.name && ValidationTypes.isValid(options.name) ? options.name : ValidationTypes.CUSTOM_VALIDATION, - name: options.name, target: options.target, propertyName: options.propertyName, validationOptions: options.options, diff --git a/src/validation-schema/ValidationSchema.ts b/src/validation-schema/ValidationSchema.ts index 89969aa053..1bf74b7bc7 100644 --- a/src/validation-schema/ValidationSchema.ts +++ b/src/validation-schema/ValidationSchema.ts @@ -26,11 +26,6 @@ export interface ValidationSchema { */ type: string; - /** - * Validator name. - */ - name?: string; - /** * Constraints set by validation type. */ diff --git a/src/validation-schema/ValidationSchemaToMetadataTransformer.ts b/src/validation-schema/ValidationSchemaToMetadataTransformer.ts index 9e3820789c..3e42867355 100644 --- a/src/validation-schema/ValidationSchemaToMetadataTransformer.ts +++ b/src/validation-schema/ValidationSchemaToMetadataTransformer.ts @@ -20,7 +20,6 @@ export class ValidationSchemaToMetadataTransformer { const args: ValidationMetadataArgs = { name: validation.name, type: validation.type, - name: validation.name, target: schema.name, propertyName: property, constraints: validation.constraints,