fix: overwrite validation decorators from parent class when defined in child

[NickKelly1]

Description

Fixes a bug that caused decorated properties which are also decorated in inherited classes, to skip their inherited validation.

Checklist

• the pull request title describes what this PR does (not a vague title like Update index.md)
• the pull request targets the default branch of the repository (develop)
• the code follows the established code style of the repository
  • npm run prettier:check passes
  • npm run lint:check passes
• tests are added for the changes I made (if any source code was modified)
• documentation added or updated
• I have run the project locally and verified that there are no errors

Fixes

fixes #633 , fixes #622

[NickKelly1]

Please don't merge this yet, I've added some additional tests and found some issues. I Will comment again once resolved.

[NickKelly1]

This PR is now ready for merge

Please note:

Currently in v0.12.2, validator-decorated properties of subclasses remove validators registered against those properties from parent classes (see 633). It appears this has been a problem for a long time. Validator equality is done by comparing a validators type, which in v0.12.2 is "customValidation" for almost every single validator.

The bug became apparent after v0.11.1 (11a7b8b#diff-6a5d045641599b923bd58ec30032f09dL1) wherein most validators had a unique ValidationType allowing subclassing to override them (note that custom validators still had type "customValidation", and thus would all override each-other by subclassing).

This PR changes equality to be based on name, rather than type, and falling back to type if name is not given. This makes subclassing to work closer to as described in the documentation. If name is now used to uniquely identify a validator, that implies it should be a required property. To not break backwards compatibility, it's left optional here with TODO's to add it where necessary if it becomes required later.

[cduff]

Any update on this? I got hit by it again today. It's quite confusing at the moment as some decorators are inherited (e.g. @IsOptional) and others aren't.

[rafaelbattesti]

Hey folks, any expectations on merging this change? It seems to me the semantic commit + PR is blocking it. Thanks!

[FacundoVazquez]

Please merge this fix, more than one is forced to use version 0.11.1. Thanks!

@NoNameProvided

[NoNameProvided]

Hi! I pinned the tab, I will take a look next week.

[ghost]

Any update on the merge?

[Clashsoft]

Looking forward to this to be merged as well. In the meantime, I worked around the problem using ValidationTypes.isValid = () => true;, but it broke validation (everything is valid now). I'd like to use the type for a project that seems to depend on this PR completely.

[slavco86]

@NickKelly1
Any chance you can have a look at the conflicts, please?
Dying to get this merged and released. Please.

[pahuta]

@NoNameProvided , @NickKelly1 , did you decide to do not merge it?

[kaseyreed]

👋🏻 Hi there, just wondering if this fix will land or if y'all have decided not to support this?

[slavco86]

This should really be merged... this is obviously a gap/bug, which anyone requiring this functionality would expect to be working? I mean this is a recursiveness problem, which should not be present in a production grade package.
Quite surprised it's still not merged/fixed 😢

[AdirTrichter]

Any updates on this PR? Would it merge soon?

[AdirTrichter]

Does anyone know a workaround that can be done so it would inherit the validations from the parent till this PR merges?
*other than rewriting the validations from the inherited class ofc 🙏

[adi518]

I worked around the buggy decorator inheritance by fixing the InheritValidation decorator from @VinceOPS PR. This works with 0.14.1. However, you must patch-package class-validator to add the missing name property to the IsOptional decorator.

You can then do:

export class SuperUserDto extends UserDto {
  @InheritValidation(UserDto, "name", { omitOptional: true })
  @IsNotEmpty()
  override name!: NonNullable<UserDto["name"]>;
}

import { IS_OPTIONAL } from "class-validator";
import { cloneDeep, defaults } from "lodash";

import type { ValidationMetadata } from "class-validator/types/metadata/ValidationMetadata";

// https://github.com/typestack/class-validator/blob/develop/src/decorator/common/IsOptional.ts
// https://github.com/typestack/class-validator/issues/1288
// https://github.com/typestack/class-validator/pull/748
// https://github.com/typestack/class-validator/pull/161
// https://github.com/VinceOPS/class-validator/blob/7d5d199ccba7408893687dacb0d92169e2acfaae/src/decorator/inherit-validation/inherit-validation.ts

/**
 * Allow copying validation metadatas set by `class-validator` from
 * a given Class property to another. Copied `ValidationMetadata`s
 * will have their `target` and `propertyName` changed according to
 * the decorated class and property.
 *
 * @param fromClass    Class to inherit validation metadata from.
 * @param fromProperty Name of the target property (default to decorated property).
 *
 * @return {PropertyDecorator} Responsible for copying and registering `ValidationMetadata`s.
 *
 * @example
 * class SubDto {
 *   @InheritValidation(Dto)
 *   readonly id: number;
 *
 *   @InheritValidation(Dto, 'name')
 *   readonly firstName: string;
 *
 *   @InheritValidation(Dto, 'name')
 *   readonly lastName: string;
 * }
 */
export function InheritValidation<T>(
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  fromClass: new (...args: any[]) => T,
  fromProperty?: keyof T,
  options?: { omitOptional?: boolean }
): PropertyDecorator {
  const opts = defaults(options, { omitOptional: false });

  const validationMetadatas: ValidationMetadata[] =
    globalThis.classValidatorMetadataStorage.validationMetadatas.get(fromClass);

  /**
   * Change the `target` and `propertyName` of each `ValidationMetadata`
   * and add it to `MetadataStorage`. Thus, `class-validator` uses it
   * during validation.
   *
   * @param toClass    Class owning the decorated property.
   * @param toProperty Name of the decorated property.
   */
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  return (toClass: object, toProperty: any) => {
    const toPropertyName = toProperty as string;

    const sourceProperty = fromProperty || toProperty;

    const metadatasCopy = cloneDeep(
      validationMetadatas.filter((vm) => vm.target === fromClass && vm.propertyName === sourceProperty)
    );

    const targetClass = toClass.constructor;
    const targetValidationMetadatas = globalThis.classValidatorMetadataStorage.validationMetadatas.get(targetClass);

    metadatasCopy.forEach((vm) => {
      if (opts.omitOptional && vm.name === IS_OPTIONAL) {
        return;
      }

      vm.target = targetClass;
      vm.propertyName = toPropertyName;

      if (targetValidationMetadatas) {
        targetValidationMetadatas.push(vm);
      } else {
        globalThis.classValidatorMetadataStorage.validationMetadatas.set(targetClass, [vm]);
      }
    });
  };
}

Package patch:
This was done with pnpm, but even then, create your own patch and match the diff.

diff --git a/cjs/decorator/common/IsOptional.js b/cjs/decorator/common/IsOptional.js
index bb95c0cc51de1e9df550b002d798f462f823494c..e8086acc5b6a106d4b99d91015153d3480ae96aa 100644
--- a/cjs/decorator/common/IsOptional.js
+++ b/cjs/decorator/common/IsOptional.js
@@ -4,6 +4,8 @@ exports.IsOptional = void 0;
 const ValidationTypes_1 = require("../../validation/ValidationTypes");
 const ValidationMetadata_1 = require("../../metadata/ValidationMetadata");
 const MetadataStorage_1 = require("../../metadata/MetadataStorage");
+const IS_OPTIONAL = 'isOptional';
+exports.IS_OPTIONAL = IS_OPTIONAL;
 /**
  * Checks if value is missing and if so, ignores all validators.
  */
@@ -11,6 +13,7 @@ function IsOptional(validationOptions) {
     return function (object, propertyName) {
         const args = {
             type: ValidationTypes_1.ValidationTypes.CONDITIONAL_VALIDATION,
+            name: IS_OPTIONAL,
             target: object.constructor,
             propertyName: propertyName,
             constraints: [
diff --git a/esm2015/decorator/common/IsOptional.js b/esm2015/decorator/common/IsOptional.js
index 4de1e9d259bc5159bc0e25806561784e9cc1b410..1cbb10e3901915fcbb0db4cb69ef9c38047cf2af 100644
--- a/esm2015/decorator/common/IsOptional.js
+++ b/esm2015/decorator/common/IsOptional.js
@@ -1,6 +1,7 @@
 import { ValidationTypes } from '../../validation/ValidationTypes';
 import { ValidationMetadata } from '../../metadata/ValidationMetadata';
 import { getMetadataStorage } from '../../metadata/MetadataStorage';
+export const IS_OPTIONAL = 'isOptional';
 /**
  * Checks if value is missing and if so, ignores all validators.
  */
@@ -8,6 +9,7 @@ export function IsOptional(validationOptions) {
     return function (object, propertyName) {
         const args = {
             type: ValidationTypes.CONDITIONAL_VALIDATION,
+            name: IS_OPTIONAL,
             target: object.constructor,
             propertyName: propertyName,
             constraints: [
diff --git a/esm5/decorator/common/IsOptional.js b/esm5/decorator/common/IsOptional.js
index c10abe4f7d04d10304cccaedeace30b75ab2bdde..2003c84dce720d977013d3f42a6c678a4094f96f 100644
--- a/esm5/decorator/common/IsOptional.js
+++ b/esm5/decorator/common/IsOptional.js
@@ -1,6 +1,7 @@
 import { ValidationTypes } from '../../validation/ValidationTypes';
 import { ValidationMetadata } from '../../metadata/ValidationMetadata';
 import { getMetadataStorage } from '../../metadata/MetadataStorage';
+export const IS_OPTIONAL = 'isOptional';
 /**
  * Checks if value is missing and if so, ignores all validators.
  */
@@ -8,6 +9,7 @@ export function IsOptional(validationOptions) {
     return function (object, propertyName) {
         var args = {
             type: ValidationTypes.CONDITIONAL_VALIDATION,
+            name: IS_OPTIONAL,
             target: object.constructor,
             propertyName: propertyName,
             constraints: [
diff --git a/types/decorator/common/IsOptional.d.ts b/types/decorator/common/IsOptional.d.ts
index 47d58aef82d25ad2180ce36b66bb0a0280bcd3cb..c1975b473baec74c56d91e6e64efd98bf695a188 100644
--- a/types/decorator/common/IsOptional.d.ts
+++ b/types/decorator/common/IsOptional.d.ts
@@ -1,4 +1,5 @@
 import { ValidationOptions } from '../ValidationOptions';
+export declare const IS_OPTIONAL = "isOptional";
 /**
  * Checks if value is missing and if so, ignores all validators.
  */

[AlekseyMelikov]

Are there any plans to merge this PR? The problem needs to be solved