class-validator icon indicating copy to clipboard operation
class-validator copied to clipboard

[Question] How to "remove" inherit decorator ?

Open miramo opened this issue 7 years ago • 13 comments

There is an elegant way to "remove" inherit decorator like @IsOptional in child class ?

I'm using class-validator with Nest to validate my input data with some DTO class.

I need to have a parent class with all fields optional and a child class with some mandatory fields.

So i have a CreateAccountDto wich inherit from AccountDto

The idea behind is to have different inputs DTO like CreateAccountDto and avoid repetition.

Actually i have :

export class AccountDto {
  @IsUUID('4')
  @IsOptional()
  @ApiModelProperty()
  readonly account_id: string;

  @IsString()
  @IsNotEmpty()
  @MaxLength(200)
  @IsEmail()
  @IsOptional()
  @ApiModelProperty()
  readonly email: string;

  @IsString()
  @IsNotEmpty()
  @MaxLength(100)
  @IsOptional()
  @ApiModelProperty()
  readonly password: string;
}

and

export class CreateAccountDto extends AccountDto {
  @Equals(undefined)
  @RemoveApiModelProperty()
  readonly account_id: string;

  @RemoveIsOptional()
  readonly email: string;

  @RemoveIsOptional()
  readonly password: string;
}
const removeValidationPropertyDecorator = (metakey: string): PropertyDecorator => {
  return (target: object, propertyKey: string | symbol) => {
    const validationMetadatasKey = 'validationMetadatas';
    const validationMetadatas: ValidationMetadata[] = getFromContainer(MetadataStorage)[validationMetadatasKey];

    _.remove(validationMetadatas, (validationMetadata: ValidationMetadata) => {
      return validationMetadata.propertyName === propertyKey && validationMetadata.type === metakey;
    });
  };
};
export const RemoveIsOptional = (): PropertyDecorator => {
  return removeValidationPropertyDecorator(ValidationTypes.CONDITIONAL_VALIDATION);
};

My actual solution is to have a custom @RemoveIsOptional decorator, but i'm not sure is the best way to accomplish my goal !?

miramo avatar Jan 12 '18 15:01 miramo

Hi @miramo!

Currently, there is no documented way to do this.

Btw why do you specify your properties again? You can just add the new properties to the extended class. Eg, why email is optional on your AccountDto payload?

NoNameProvided avatar Jan 13 '18 09:01 NoNameProvided

Hi,

Thanks for your answer. It's because I want to have a parent DTO, which is iso to my typeorm entity. And I want some child inherit from it and "select" (like a white list) the mandatory fields. Like that I don't have to rewrite all the validators for each child.

But maybe I'm going in the wrong direction.

miramo avatar Jan 15 '18 10:01 miramo

Have you tried groups? You can add groups like post, patch, put. There is one gotcha, if you add it to some decorators, then you need to decorate all the remaining otherwise they won't get called.

NoNameProvided avatar Mar 17 '18 20:03 NoNameProvided

Came here cause the groups atm don't feel nice to use (few pending PR would solve this) Anyhow this @miramo is a nice way to do it. Thanks for sharing the code

nolazybits avatar Feb 05 '20 02:02 nolazybits

@nolazybits what pending PRs would solve this issue?

vlapo avatar Mar 27 '20 20:03 vlapo

The PR for groups would be those ones https://github.com/typestack/class-validator/pull/241 https://github.com/typestack/class-validator/pull/153

Groups is a nice idea but the implementation makes it unusable

Thanks

nolazybits avatar Mar 29 '20 20:03 nolazybits

Hi! Are there any updates on this issue?

mopcweb avatar Nov 19 '20 13:11 mopcweb

@miramo Thanks for you solution !)

In my case it doesn't work until i changed this

const validationMetadatas: ValidationMetadata[] = getFromContainer(MetadataStorage)[validationMetadatasKey];

to this

const validationMetadatas: ValidationMetadata[] = getMetadataStorage()[validationMetadatasKey];

full snippet

import { ValidationTypes, getMetadataStorage } from 'class-validator';
import { ValidationMetadata } from 'class-validator/types/metadata/ValidationMetadata';
import _ from 'lodash';

function removeValidationPropertyDecorator(metakey: string): PropertyDecorator {
  return (_target: object, propertyKey: string | symbol): void => {
    const validationMetadatasKey = 'validationMetadatas';
    const validationMetadatas: ValidationMetadata[] = getMetadataStorage()[validationMetadatasKey];

    _.remove(validationMetadatas, (validationMetadata: ValidationMetadata) => (
      validationMetadata.propertyName === propertyKey && validationMetadata.type === metakey
    ));
  };
}

export function RemoveIsOptional(): PropertyDecorator {
  return removeValidationPropertyDecorator(ValidationTypes.CONDITIONAL_VALIDATION);
}

mopcweb avatar Nov 19 '20 14:11 mopcweb

Unfortunately, RemoveIsOptional from example above doesn't work correctly. I've been using it for some time when i realized that it totally removes IsOptional event form parent class, so

class Parent {
  @IsOptional()
  @IsString()
  public property?: string
}

class Child {
  @RemoveIsOptional()
  public property?: string
}

validateSync(new Parent()); // Here will be error: `property` should be a string

in this example if run validation even on Parent class - there will be error, as property will be marked as required because of RemoveIsOptional

mopcweb avatar Nov 20 '20 23:11 mopcweb

Digging deeper i found next solution for my use case, maybe it still would be useful for others. Insipred by https://github.com/typestack/class-validator/issues/164#issuecomment-369874196

setupOptionalValidators.ts

import { getMetadataStorage } from 'class-validator';
import { ValidationMetadata } from 'class-validator/types/metadata/ValidationMetadata';

export function setupOptionalValidators(): void {
  const validationMetadatasKey = 'validationMetadatas';
  const validationMetadatas: ValidationMetadata[] = getMetadataStorage()[validationMetadatasKey];

  const forUpdate = validationMetadatas.filter((item) => !item.groups?.length);
  forUpdate.forEach((_item, i) => { forUpdate[i].always = true; });
}
// setupOptionalValidators just iterates over all ValidationMetadata(s) and addes `{ always: true }` for thos without `groups` option
// So basically it is not necessary to use it, you could add manually `{ always: true }` to all `class-validator` decorators without groups
// Because without `{ always: true }` those decorators will not work properly (

This one should be called after importing all DTOs. In my case i'm using TsED v5, so i call it if $afterRoutesInit.

Now i can write my models in convenient way: (@Property and @Required are TsED v5 decorators used here for swagger schema)

import { SomeEnum } from 'path/to/SomeEnum';
import { SomeEntity } from 'path/to/SomeEntity';

export class UpdateDto implements Partial<SomeEntity> {
  @Property()
  @IsOptional({ groups: ['update'] })
  @IsString()
  public propertyOne?: string;

  @Property()
  @IsOptional({ groups: ['update'] })
  @IsEnum(SomeEnum)
  public propertyTwo?: SomeEnum;

  @Property()
  @IsOptional({ groups: ['update'] })
  @IsString()
  public propertyThree?: string;

  @Property()
  @IsOptional()
  @IsString()
  public propertyFour?: string;

  @Property()
  @IsOptional()
  @IsBoolean()
  public propertyFive?: boolean;
}

export class CreateDto extends UpdateDto implements SomeEntity {
  @Required() public propertyOne: string;
  @Required() public propertyTwo: SomeEnum;
  @Required() public propertyThree: string;
}

End when i validating my requests - i just provide ['create'] or ['update'] groups for validate();

P.S. For validation inside TsED v5 i am overriding default ValidationPipe:

@OverrideProvider(ValidationPipe)
export class ClassValidationPipe extends ValidationPipe implements IPipe {
  private _validateOptions: ValidatorOptions = { whitelist: true, forbidNonWhitelisted: true };

  public async transform(value: any, metadata: ParamMetadata): Promise<any> {
    if (!this.shouldValidate(metadata)) return value;
    const options: ValidatorOptions = { ...this._validateOptions, groups: metadata.required ? ['create'] : ['update'] };

    const dataToValidate = plainToClass(metadata.type, value);
    const result = Array.isArray(dataToValidate)
      ? await this.validateList(dataToValidate, options)
      : await validate(dataToValidate, options);

    if (result.length > 0) throwHttpError.badRequest(this.removeRestrictedProperties(result));
    return value;
  }

  protected async validateList<T>(list: T[], options: ValidatorOptions = { }): Promise<ValidationError[]> {
    let result: ValidationError[] = [];
    const promises = list.map((item) => validate(item, options).then((errors) => { result = result.concat(errors); }));
    await Promise.all(promises);
    return result;
  }

  protected shouldValidate(metadata: ParamMetadata): boolean {
    const types: Function[] = [String, Boolean, Number, Array, Object];

    return !super.shouldValidate(metadata) || !types.includes(metadata.type);
  }

  // This one is just for convenient errors in my architechure
  protected removeRestrictedProperties(errors: ValidationError[]): GenericObject[] {
    if (!errors || !errors.length) return;
    const result = errors.map((item) => ({
      property: item.property,
      value: item.value,
      errors: item.constraints,
    }));
    return result;
  }
}

So now when calling @BodyParams() inside Controller ValidationPipe will add ['update'] group, while calling @Required() @BodyParams() or @BodyParams({ required: true }) will add ['create'] group.

mopcweb avatar Nov 21 '20 17:11 mopcweb

For NestJS users, have you seen this: https://trilon.io/blog/introducing-mapped-types-for-nestjs ?

I don't know if this can satisfy all your needs, but this doesn't exist when I created this issue 🤷 And I think that would have helped us a lot.

In the meantime we have implemented/used another solution on our side ;

import { getFromContainer, MetadataStorage } from 'class-validator';
import cloneDeep from 'lodash/cloneDeep';

/**
 * Allow copying validation metadatas set by `class-validator` from
 * a given Class property to an other. Copied `ValidationMetadata`s
 * will have their `target` and `propertyName` changed according to
 * the decorated class and property.
 *
 * @param fromClass    Class to inherit validation metadatas from.
 * @param fromProperty Name of the target property (default to decorated property).
 *
 * @return {PropertyDecorator} Responsible for copying and registering `ValidationMetada`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>(fromClass: new (...args: any[]) => T, fromProperty?: keyof T): PropertyDecorator {
  const metadataStorage = getFromContainer(MetadataStorage);
  const validationMetadatas = metadataStorage.getTargetValidationMetadatas(fromClass, typeof 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.
   */
  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));

    metadatasCopy.forEach(vm => {
      vm.target = toClass.constructor;
      vm.propertyName = toPropertyName;
      metadataStorage.addValidationMetadata(vm);
    });
  };
}

Related PR : https://github.com/typestack/class-validator/pull/161

miramo avatar Dec 02 '20 15:12 miramo

Conditional validation sounds like a good idea, but I feel the problem here isn't quite solved yet. Class-validator inherits annotations from parents, i.e. from MetadataStorage.js:

        const filteredForInheritedMetadatasSearch = [];
        for (const [key, value] of this.validationMetadatas.entries()) {
            if (targetConstructor.prototype instanceof key) {
                filteredForInheritedMetadatasSearch.push(...value);
            }
        }

This is a great idea, but some annotations (most notably IsOptional) break here.

Assume this parent:

class Parent {
  @IsNumber()
  id: number;
  @IsOptional()
  @IsNumber()
  someOptional?: number;
}

I can create a child and add annotations, but cannot remove or override them!

class Child extends Parent {
  @IsPositive()
  // I can add annotations, thus e.g. making validations tighter. In this case I'm validating `id` to be >0
  id: number;
  @IsPositive()
  // OH NO: I wanted `someOptional` to be mandatory here, but there is no annotation to add that would counteract @IsOptional() from the Parent
  someOptional: number;
}

Looking at https://github.com/typestack/class-validator/issues/820 I found a recommendation by @vlapo to reverse the inheritance. This works there, but I believe this does not work here. Let's put Child as the parent

// No longer extend Parent
class Child {
  @IsPositive()
  // Working great
  id: number;
  @IsPositive()
  // Working great, this is now mandatory
  someOptional: number;
}

Which leaves the following problem:

class Parent extends Child {
  @IsNumber()
  // OH NO: @IsPositive() was inherited
  id: number;
  @IsOptional()
  @IsNumber()
  someOptional?: number;
}

Looking at the Liskov substitution principle I would argue that making field validations stronger in the child should be supported.

florian-besser avatar Dec 28 '23 09:12 florian-besser

I really don't understand why it is not a base feature, to update the validation via extension.

class Child {
  @IsPositive()
  @IsOptional()
  someOptional?: number | string;
}

class Parent extends Child {
  @RemoveIsOptional()
  @RemoveIsPositive()
  
  //or
  @RemoveAll()
  // then re validate whatever validation needed
  @IsNegative()
  @IsNumber()
  someOptional: number;
}

I know requesting this is maybe not going to result in anything. But it would be nice if we can do custom decorator that have this kind of function (not the one explained above by @mopcweb). Not sure if this feature is something big or against rules of class-validator for nestJS, but I really think this is very good since we can implement inheritance in dto class

martinharyanto avatar Jun 05 '24 07:06 martinharyanto