typestack / class-validator

Decorator-based property validation for classes.
MIT License
10.85k stars 785 forks source link

Inheriting Validation decorators #633

Open ghost opened 4 years ago

ghost commented 4 years ago

Description

From the docs:

If a property is redefined in the descendant class decorators will be applied on it both from that and the base class.

I'm not sure it works as it should.

Reproduction

import { MinLength, IsUppercase } from "class-validator";
import { validate } from "class-validator";

class BaseClass {
    @MinLength(10)
    someProperty: string;
}

class ChildClass extends BaseClass {
    @IsUppercase()
    someProperty: string;
}

const someObj = new ChildClass();
someObj.someProperty = 'HELLO';

validate(someObj).then(result => {
    console.log(result);
});

// the output is empty array []

From what I'm reading in docs, I thought @MinLength(10) is used alongside @IsUppercase(), but I don't see it. Moreover, I'm not sure what empty array means.

Environment

cduff commented 4 years ago

I'm seeing the same. It does not appear to behave as documented.

@shymanel The empty array output from your example indicates no validation errors.

ghost commented 4 years ago

@cduff yep, thanks, I figured it out. I though there should be an error, since HELLO is not 10 characters long

cduff commented 4 years ago

FYI, since commenting on this issue earlier I've been using class-validator more and have come to realise that not having it inherit decorators is useful sometimes.

For example, I can have code like:

class User {
  @Matches(hashPattern)
  passwordHash?: string;
}

class RegistrationRequest extends User {
  @Length(minLength, maxLength)
  password?: string;

  @IsEmpty()
  passwordHash?: string;
}

For a User a valid passwordHash is required. But for a RegistrationRequest a valid password should be supplied instead, from which the server will generate a valid passwordHash. This wouldn't work if the Matches decorator was inherited to the RegistrationRequest.passwordHash property. To achieve the same would require some verbose groups usage I imagine, noting that in reality the User has many other properties not shown above.

So now I'm wondering if the decorators are not inheriting by design?

ragrub commented 4 years ago

I encounter the same issue described by @shymanel. At least with version 0.11.1 it used to work as mentioned in the documentation, that decorators were inherited from base class.

https://github.com/typestack/class-validator#inheriting-validation-decorators "user.password = 'too short'; // password wil be validated not only against IsString, but against MinLength as well"

I would like to update to version 0.12.2, but this seems to be broken now and affects a lot of unit tests in my project.

NickKelly1 commented 3 years ago

There is now a PR for this

cduff commented 3 years ago

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.

Arcdie commented 2 years ago

While we are waiting for merge, you can make your own decorator and call it in child class.

import {getMetadataStorage} from 'class-validator';

function inheritParentDecorators() {
  return (
    target: any,
    propertyKey: string,
  ) => {
    const storage = getMetadataStorage();
    const parent = Object.getPrototypeOf(target.constructor);

    if (!parent) {
      return;
    }

    const targetMetadatas = storage.getTargetValidationMetadatas(
      parent,
      parent.name,
      false,
      false,
    );

    targetMetadatas
      .filter(e => e.propertyName === propertyKey)
      .forEach(e => {
        registerDecorator({
          name: e.type,
          target: target.constructor,
          propertyName: e.propertyName,
          // preserving custom error messages (thanks [slavco86](https://github.com/slavco86))
          options: {...e.validationTypeOptions, message: e.message},
          constraints: e.constraints,
          validator: e.constraintCls,
        });
      });
  };
}

class BaseClass {
    @MinLength(10)
    someProperty: string;
}

class ChildClass extends BaseClass {
    @inheritParentDecorators()
    @IsUppercase()
    someProperty: string;
}
slavco86 commented 2 years ago

Please, please, please - someone pick this up urgently. This is a clear regression from V11 and we are on V13 already without this being fixed. 🙏

arturohernandez10 commented 1 year ago

Just FYI.

It only makes sense to inherit if the decorators don't conflict with each other.

huybuidac commented 1 year ago

PR

hey bro, how did you fix this :(

josephdpurcell commented 5 months ago

The documentation says inheritance should work: https://github.com/typestack/class-validator/issues/633. But, it doesn't for me, I get:

Parameters violate constraints: policy must be one of the following values: [object Object]

I tried the solution from https://github.com/typestack/class-validator/issues/633#issuecomment-1110790596 and got the same error.

This is from @IsIn decorator on the parent class, but same error happens with other validators. I haven't dug in to find out why.

Within NestJS I've found a solution/hack that does the inheritance:

import { OmitType } from "@nestjs/swagger";

class ChildClass extends OmitType(BaseClass, []) {
    @IsUppercase()
    someProperty: string;
}

For now I can use the type helpers (pick/omit) from @nestjs/swagger but it would be great to see a better way to do this, even if its a custom decorator like @inheritParentDecorators.

Edit: clarified that I've read the docs.