Whitelisting properties

[aljazerzen]

As discussed here I only solution to unwanted additional properties is to add a decorator to all wanted properties. I know this can become messy, but for now, it is better than nothing. If in the future Typescript will indeed set all properties to undefined, we can upgrade this without breaking any code written using @Allowed decorator.

Because this is a validator-class @Allowed is not stripping additional properties, but throwing a validation error. Should we add an option to strip instead?

I have tried to use formatting style used in this repo, I hope I did a good job :)

[aljazerzen]

@pleerock Have you reviewed this? Any thoughts? I would like use this feature in my project as soon as possible.

Thanks

[NoNameProvided]

Throwing error is not user-friendly, we should strip instead.

[pleerock]

@19majkel94 @NoNameProvided can you guys please review this one and let me know what do you think about this change?

[MichalLytek]

Looks ok! However I don't find it useful, validation is kinda like interface based, so we want the properties to be valid. Additional properties should be stipped off by class-transformer, not flagged as validation error. @Allowed decorator introduce more decorator noise, together with @IsDefined and @IsOptional, which looks bad for me.

[NoNameProvided]

Additional properties should be stipped off by class-transformer, not flagged as validation error.

Yes as I wrote I think the same, can the OP modify the PR?

ping @aljazerzen

[aljazerzen]

@19majkel94 So are you saying we should move implement this in class-transformer or just to change it so it stripes off additional properties?

[ramlez]

Guys, can you merge this PR? I totally agree that this job should be done by class-transformer but I've reviewed issues about that in the class-transformer project and it seems to be not possible to do. So in that situation, this PR makes sense for me.

BTW. Without stripping off additional properties this whole library is useless on production... I don't want to rewrite input manually everytime when it comes to my app just to make sure that it doesn't contain any unwanted additional fields.

[aljazerzen]

Sure, will implement stripping.

[ramlez]

I'd be happy if someone could merge this PR as it is. If you take look at Joi liblary it alos throws exceptions when object contains unallowed properties.

Due to fact that stipping properties it not so trivial it could be implemented in future.
@pleerock could you take look at this PR? And merge it or let me know that it's not possible.

[aljazerzen]

I have implemented stripping of the additional properties and added flag for throwing an error. (sorry it took me this long).

[aljazerzen]

@pleerock
But now i got an idea: we do not need the @Allow decorator, we could just check if a property has any of the existing decorators (for example @Min). If not, strip it. Simple. No additional decorators you have to append to every property. This is the intuitive behavior: if you don't define a property or if it does not have any decorator the validated object would not have it.

The only concern: this PR currently won't break any existing projects that except if they depend on non-defined properties (which they should't). But if I implement this idea it will. That's why I am asking you to confirm we want this behavior.

PS: why do I even need this stripping? I am putting the whole object I get from request into MongoDB via Mongoose. If someone for example passes a property isAdmin: true at register this would be saved into the db and user would be admin. That's why I have to validate objects first and then pick properties with lodash.pick(). This is not ideal because I have to pass array of properties to pick and it just feels like a hack.

[aljazerzen]

Refactored @Allowed validation to whitelisting:

• more obvious name
• it must be enabled in validationOptions which means that
• no existing projects will be broken
• another flag for throwing error instead of stripping

@pleerock this is my final version - can you merge it and get it on npm?

[aljazerzen]

@NoNameProvided can you merge this PR?

[NoNameProvided]

Hey @aljazerzen!

This PR looks good to me, but I would like to wait for a review from @pleerock on this one. (I cannot publish to npm anyway, so we need @pleerock to publish it.)

[pterblgh]

Hi @pleerock!

Do you have an estimation for merging this PR and publish a new release to NPM with this new feature?

Also thanks, for the amazing work @aljazerzen I was looking for such feature and I'm really glad you already implemented it. 👍

[NoNameProvided]

Hi @pterblgh!

I am actively going through issues and prs for class-validators, when I have a full picture of the current state, open features and future vision I will start merging these prs and implement the fitting features for the library.

You can expect this PR being merged/reworked in the next 1-2 week.

[NoNameProvided]

@aljazerzen thanks for your work! 🎉 🎉 🎉

[aljazerzen]

@NoNameProvided Hey! I am glad to see my code being approved!

Today I have updated to version 0.8.0 in my project and discovered a mistake: just some wrong types (false instead of boolean). When you have 2 minutes of time, please fix this on my behalf as I did in this commit.

Should I rather create a new pull request? (it is my first time contributing so I am not quite sure)

Thanks!

[NoNameProvided]

Thanks, fixed and released as 0.8.1!

[aljazerzen]

🥇

[fwoelffel]

Correct me if I'm wrong, but as I understand it, the validate function doesn't only perform validation, it also mutates the validated object?
Anyway, thanks for this :)

[aljazerzen]

Yes it does, but only if you pass it { validate: true }. If you do not like this behavior, you can add { forbidNonWhitelisted: true } and it won't mutate the validated object but throw errors.

[fwoelffel]

Fair enough 🙂

I still think that, with this (great) feature, the ValidationExecutor.execute function shouldn't mutate the input object in any case. It would be better to output a validated object and keep the input one untouched. However, this idea looks like it would bring a lot of breaking changes...

[NoNameProvided]

It would be better to output a validated object and keep the input one untouched.

I share your view, however creating a deep copy is expensive. This would need some benchmarking before implementation.

The stripping of properties is turned off by default, so we mutate the object only if you opt in for it.

[github-actions]

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.