Skip to content

Conversation

@VinceOPS
Copy link

@VinceOPS VinceOPS commented Jan 21, 2018

Hi,

Using nestjs recently, we've been through the frustration of duplicating a lot of validation decorators in our DTO's as we're adding more endpoints for our users, in order to do "partial updates" (and other operations requiring partial DTO's).

Explanations (using fake codes, for the sake of the example):

export class UserDto {
  @isNumber()
  readonly id: number;
  
  @isEmail()
  readonly email: string;

  @IsString()
  @MaxLength(30)
  readonly firstName: string;

  @IsString()
  @Length(8, 32)
  readonly password: string;
}

Let's assume this is our complete User DTO.
At some point, we'll propose our users to update, for instance, their password. We can either create a DTO with all these fields set as Optional, and copy/paste all other validation rules. Or we can create a new DTO with only the fields we expect to be used by the client app, but we still need to copy/paste all other validation rules. And for our administrators (using the backoffice), we'll have to create different DTO's for different fields, again.
By the way, we enjoy using specific classes as return-types for our Controllers, and it helps a lot using Nestjs's swagger module.

The issue concerns the duplication of the decorators and the maintainance of all the rules.

This PR intends to fix it using a new decorator used to copy the ValidationMetadata of a given property.

Following our previous example, we would simply do:

import {UserDto} from "./somewhere/user.dto.ts";

class UpdatePasswordUserDto {
  @InheritValidation(UserDto)
  readonly password: string;
}

But it could also be used to "share" validation metadatas between properties of the same class (let's say, "firstName" and "lastName").

I also shared my unit tests while I was at it. However, I'm using Jest (and ts-jest) in most of my projects, so this won't work with Mocha. If you like the idea and want to accept it, I'll rewrite my unit tests to work with Mocha.
(In the mean time, you can check the unit tets work (they do :-) ) by running jest against the file inherit-validation.spec;ts).

Thanks for all your work!

Regards

@VinceOPS
Copy link
Author

VinceOPS commented Jan 21, 2018

Regarding the Travis CI failure: gulp tests runs successfully once the Jest tests are removed.

@VinceOPS
Copy link
Author

@NoNameProvided Hi! Could you give me your opinion about this decorator/idea when you get a chance? Thanks! (I can remove the Jest Tests and rewrite them for Mocha, so the Travis CI will succeed, if you agree with the feature).

@NoNameProvided
Copy link
Member

The idea is great thanks for the contribution! 🎉

I have one question:

const toPropertyName: string = toProperty instanceof Symbol ?
      typeof toProperty :
      toProperty;

This way every Symbol instance will have the symbol name, we can use toProperty.toString() instead but it's even easier just to remove Symbol altogether. Is there any particular reason you added it here?

Otherwise, it looks good for me for the first glance, so it will be good to go as we can fix these:

  • I would like to avoid using lodash please try to remove it,
  • please follow the conventions in the repo, I don't like it either but for now please attach the decorator to the end of the decorators.ts file
  • please implement this decorator the same way as the other decorators are written
  • also as you mentioned, please rewrite the test to use mocha
  • please rename your DTOs into more neutral names, like User, Admin, etc. Not everyone else is a Java dev, and they won't know what DTO refers to.

@NoNameProvided NoNameProvided added the type: feature Issues related to new features. label Jan 24, 2018
@VinceOPS
Copy link
Author

Hi @NoNameProvided
Thanks for your feedback. Will work on this soon.
Regarding string | symbol, I wanted to add/handle symbol because this is how PropertyDecorator is defined. What do you think? If I'd like to keep it as a PropertyDecorator, what would be an acceptable solution for you?

@VinceOPS VinceOPS force-pushed the feature/inherit-validation-decorator branch from 9640a37 to 7d5d199 Compare May 20, 2018 10:36
@vlapo
Copy link
Contributor

vlapo commented Oct 15, 2019

@VinceOPS What is status of this PR? Are you still interested?

@kenberkeley
Copy link

This feature is awesome! Nice to have!

@kenberkeley
Copy link

Related: tannerntannern/ts-mixer#15

@kenberkeley
Copy link

Tips: @InheritValidation does not inherit @Type, have to add this manually...

@tmtron
Copy link
Contributor

tmtron commented Apr 14, 2020

could we make the decorator type-safe so that we don't accidentially have typos in the property name? e.g. maybe something like this:

function InheritValidation<T>(fromClass: ClassType<T>, fromProperty?: keyof T)

@kenberkeley
Copy link

This PR no longer works in v0.12+

@NoNameProvided NoNameProvided changed the base branch from master to develop August 2, 2020 11:49
@NoNameProvided
Copy link
Member

Hi @VinceOPS

Thanks for this contribution and sorry for the delayed response. Are still interested in this PR? If so can you rebase these changes to the current develop branch?

We have

  • moved to Jest so you need to adapt your tests (it's straightforward)
  • added Prettier you need to format your code with npm run prettier:fix
  • move the files around a bit (split decorators into their own file for example)

After rebase I will do a review in a timely manner.

@NoNameProvided NoNameProvided added the status: awaiting answer Awaiting answer from the author of issue or PR. label Aug 7, 2020
@NoNameProvided NoNameProvided added status: invalid / expired Issues with no action to take. and removed status: awaiting answer Awaiting answer from the author of issue or PR. labels Nov 20, 2022
@NoNameProvided NoNameProvided changed the title (feat): Decorator "@InheritValidation": copy validation metadatas feat: add @InheritValidation decorator to copy validation metadata Nov 20, 2022
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

status: invalid / expired Issues with no action to take. type: feature Issues related to new features.

Development

Successfully merging this pull request may close these issues.

5 participants