Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: class transformer should strip additional properties #200

Closed
adnan-kamili opened this issue Jul 7, 2017 · 37 comments
Closed

feature: class transformer should strip additional properties #200

adnan-kamili opened this issue Jul 7, 2017 · 37 comments
Labels
status: invalid Issues with no action to take. type: feature Issues related to new features.

Comments

@adnan-kamili
Copy link

adnan-kamili commented Jul 7, 2017

As per the doc, a new instance of class is generated by the class-transformer, then why it contains additional properties. For the following post data:

{
    "email": "adnan11@gmail.com",
    "name": "test name",
    "password": "password4",
    "company": "My Company"
}

And following ViewModel:

export class UserViewModel {

    @MaxLength(256)
    name: string;

    @IsEmail()
    email: string;

    @MinLength(6)
    @MaxLength(72)  // bcrypt input limit
    password: string;

    roles: Array<string>
}

The class instance generated for

    @HttpCode(201)
    @Post()
    create( @Body() user: UserViewModel) {
        console.log(user);
        return { message: "created"} ;
    }

is

UserViewModel {
  email: 'adnan11@gmail.com',
  name: 'test name',
  password: 'password4',
  company: 'My Company' }

Where the expected object was:

UserViewModel {
  email: 'adnan11@gmail.com',
  name: 'test name',
  password: 'password4',
  roles: null }
@NoNameProvided
Copy link
Member

NoNameProvided commented Jul 7, 2017

Only known properties are checked, others will be left untouched. If you want to exclude one specifically you can use the @Exclude() decorator.

@adnan-kamili
Copy link
Author

My concern is how is it an instance of UserViewModel if it doesn't fulfill the contract. User can add in fields like createtAt, updatedAt etc.in the post body which can cause undesired effects.

What we expect is a class instance not what the user provided json parsed object, to ensure safe data is saved into the database. Adding @exclude() decorator for properties which I don't expect is not intuitive, and unnecessarily would require adding all the properties to the view model which I don't want from users/attackers.

@NoNameProvided
Copy link
Member

NoNameProvided commented Jul 9, 2017

What we expect is a class instance

It is a class instance, all your functions specified in your class will be there.

to ensure safe data is saved into the database

Hmm, so adding @Exclude() won't work when saving the object into the database. It's used to strip properties when serializing them before sending down to client. (via classToPlain function)

Adding @Exclude() decorator for properties which I don't expect is not intuitive

That is both true and false, while it's never bad to explicitly says which properties are forbidden, it is a manual work to add them. This manual work can be easily lowered by extending your class likeexport class UpdateUserPayload extends ForbiddenFields. However again, this wont save you from saving the wrong data in the database.

The solution would be to add a @StictEquals or similar class decorator to allow only the selected fields and throw otherwise, and a @Equals to simply strip down the unknow properties.

@pleerock what do you think?

@adnan-kamili
Copy link
Author

All validation libraries like Joi, ajv, validate.js support stripping of properties which are not part of the schema, so this should be supported globally as a flag for class-transformer instead of adding one more decorator to each action. Adding too many decorators in itself also has an overhead for each action when number of actions is very high.

@pleerock
Copy link
Contributor

pleerock commented Sep 5, 2017

I didn't quite understood why you are talking about exclude here?

What I understood from this message:

UserViewModel {
  email: 'adnan11@gmail.com',
  name: 'test name',
  password: 'password4',
  company: 'My Company' }
Where the expected object was:

UserViewModel {
  email: 'adnan11@gmail.com',
  name: 'test name',
  password: 'password4',
  roles: null }

Is that he expects roles to be null, but class-transformer makes it undefined. Do you I get it right? If yes, then its expected behaviour and null and undefined are different things and since you don't send in body roles at all you should not have it in transformed object. Or did I understand it wrongly?

@NoNameProvided
Copy link
Member

Is that he expects roles to be null, but class-transformer makes it undefined. Do you I get it right?

No, the raised issue is about not removing the company property. It's doesn't exists on the model yet it exits on the transformed object. This is problematic in places like Mongo, when you will end up storing data you didn't want to.

@pleerock
Copy link
Contributor

pleerock commented Sep 5, 2017

Thats why people must use typeorm to store documents where you mark each column you want to store a with a column decorator 😃

Okay but seriously its not possible to implement this feature.

In TypeScript its NOT POSSIBLE TO GET KNOWN WHAT PROPERTIES ARE DEFINED IN THE CLASS.
It means we simply don't know if your model has or not "company" property.

@pleerock pleerock added the status: invalid Issues with no action to take. label Sep 5, 2017
@MichalLytek
Copy link
Contributor

MichalLytek commented Sep 5, 2017

Why not? If the property is decorated with class-validator decorator (even @IsOptional()) we can just do:

const reflectedType = Reflect.getMetadata("design:type", UserViewModel.prototype, "company");
if (!reflectedType) {
    delete object["company"];
} else {
    if (object["company"].constructor !== reflectedType) {
        throw new PropertyConveringError();
    }
}

I would like to introduce this type of strictTransform function to class-transformer, which would require decorating each property to collect type but the gain is that we can validate the type of the property and strip additional ones + throw error when some of object properties are invalid (string expected, get boolean instead).

BTW, to get all decorated class property names we can even monkey-patch Reflect.defineMetadata to collect metadata into storage 😆

@pleerock
Copy link
Contributor

pleerock commented Sep 5, 2017

If you read comment above author would like to avoid decorating all properties. There is already support in class-transformer to serialize only decoratorated properties (using @Expose decorator).

@MichalLytek
Copy link
Contributor

Serialize yes, but deserialize? The problem is that the received body has additional properties and all that class-transformer do is assign object prototype, not check or strip additional properties, so when you want to store it in db you have to serialize and deserialize again? It makes no sense, it should be done by strictTransform function.

@pleerock
Copy link
Contributor

pleerock commented Sep 5, 2017

it works for both serialize and deserialize.

@MichalLytek
Copy link
Contributor

MichalLytek commented Sep 5, 2017

Oh rly?

image

For me, strictTransform should strip the additional property and leave only the ones declared (and decorated) in class + throw error when the type won't match (2 !== "2").

Having additional properties is an issue for storing object in documents but the horrible thing is that the types are not checked so you have completely no type safe after transforming, it's just methods attaching which isn't very helpful feature in DTO.

@pleerock
Copy link
Contributor

pleerock commented Sep 6, 2017

As I remember @Expose does not work until u mark class with @Exclude() decorator:

@Exclude()
export class Test {

  @Expose()
   property: number

}

Or alternatively

export class Test {

  @Expose()
   property: number

}

and

plainToClass(Test, { ... }, { strategy: "excludeAll" });

@MichalLytek
Copy link
Contributor

Ok, it works even without excludeAll strategy:

image

But the problem with no type-check is still there - if you don't mind, I will do the PR with strict option which would check types and throw error if doesn't match, as it's a breaking change.

@MichalLytek
Copy link
Contributor

MichalLytek commented Sep 6, 2017

In TypeScript its NOT POSSIBLE TO GET KNOWN WHAT PROPERTIES ARE DEFINED IN THE CLASS.

Here is my patch:

const _metadata = Reflect.metadata
Reflect.metadata = function(metadataKey: any, metadataValue: any) {
    if (metadataKey === "design:type") {
        return function decorator(target: any, propertyKey?: string | symbol): void {
            const designTypes = Reflect.getMetadata("design:types", target) || []
            designTypes.push({
                propertyKey,
                type: metadataValue,
            })
            Reflect.defineMetadata("design:types", designTypes, target);
            _metadata.call(Reflect, metadataKey, metadataValue)(target, propertyKey)
        }
    }
    return _metadata.apply(Reflect, arguments);
}

I will release it as reflect-metadata-types npm package, so the only thing to do is to register on top of entry file:

import "reflect-metatada";
import "reflect-metatada-types";

And then you can just simple do:

Reflect.getOwnMetadata("design:types", Test.prototype)

Which will return you an array of properties/methods with types. Example:

@ClassDec()
export class Test {
    @PropDec()
    one: string;
    @PropDec()
    two: number;
    @PropDec()
    three: boolean;
    @PropDec()
    method() {
        return "Hello"
    }
}
[
  { propertyKey: 'one', type: [λ: String] },​​​​​
​​​​​  { propertyKey: 'two', type: [λ: Number] },​​​​​
​​​​​  { propertyKey: 'three', type: [λ: Boolean] },​​​​​
​​​​​  { propertyKey: 'method', type: [λ: Function] }
]​​​​​

@pleerock
Copy link
Contributor

pleerock commented Sep 6, 2017

yeah we can get all properties if they marked with decorator even without any other dependency, but again its not an option. If someone needs such option it already exist in class-transformer

@MichalLytek
Copy link
Contributor

MichalLytek commented Sep 6, 2017

yeah we can get all properties if they marked with decorator even without any other dependency, but again its not an option

Not really. Can you get all properties names and types in class-transformer when the class is decorated only with typeorm/class-validator decorators? The types metadatas are emmited for each property but you can't access it by class type value. My workaround allows you to collect all properties with types if emmited metadata, not only the ones decorated with @Type or other class-transformer decorators when using it is not neccesary.

If someone needs such option it already exist in class-transformer

But this could be done without bloating the class markup. Currently it has to look like this:

@Exclude()
export class CustomClass {
    @Length(3, 255)
    @Expose()
    one: string;

    @IsInt()
    @Expose()
    two: number;

    @IsOptional()
    @Expose()
    isNice?: boolean = true;
}

But could look better:

class CustomClass {
    @Length(3, 255)
    one: string;

    @IsInt()
    two: number;

    @IsOptional()
    isNice?: boolean = true;
}

@pleerock
Copy link
Contributor

pleerock commented Sep 6, 2017

Same ideas and thoughts I had a whole time ago. Its not a solution.
Most of times class properties does not have any decorators and you will end up having:

class CustomClass {
    @Length(3, 255)
    one: string;

    @IsInt()
    two: number;

    @IsOptional()
    isNice?: boolean = true;

    @PropDec()
    a: string;

    @PropDec()
    b: string;
}

which ends up as a mess and non clear and very very implicit about what this @PropDec() does and why others dont have it.

@MichalLytek
Copy link
Contributor

which ends up as a mess and non clear and very very implicit about what this @propdec() does and why others dont have it.

Nope, for empty properties with no constraints you can use @IsDefined or @IsOptional decorator when class is used for validation purpose.

class City {
    @Length(3, 255)
    name: string;

    @IsOptional()
    isCaptial?: boolean = true;

    @IsDefined()
    density: number;
}

Other decorators like @Length or @IsInt implicitly have constraint that the prop @IsDefined().

Also, the empty @PropDec() could have better name, like @CollectType which would be described nicely how TS reflection works and when to use it.

@pleerock
Copy link
Contributor

pleerock commented Sep 6, 2017

Mess in anyway. You don't need validation in most models, no matter how propdec is called it will look dirty anyway, patching with is defined or is optional no matter decorators when you dont want to do such validation is dirty too. Mess and dirty.

@marshall007
Copy link

@19majkel94 @pleerock FYI, per microsoft/TypeScript#12437 it looks like the latest ES proposal states that class properties will be initialized with undefined values by default. In the future, we may be able to rely on that for looping over non-decorated class properties.

@NoNameProvided
Copy link
Member

In TypeScript its NOT POSSIBLE TO GET KNOWN WHAT PROPERTIES ARE DEFINED IN THE CLASS.

@pleerock Payloads are stateless so why we cant check it at the first conversion the properties on a dummy class?

// pseudo code below
function convert(type, obj) {
  if(!propertyMap[type]) {
    const instance new Type();
    propertyMap[type] = Object.getOwnPropertyNames(obj);
  }

  doStrictTransform(type, obj, propertyMap[type]);
}

@pleerock
Copy link
Contributor

pleerock commented Sep 7, 2017

@NoNameProvided what do you mean? Im talking to get properties of class, not objects.

@NoNameProvided
Copy link
Member

Im talking to get properties of class, not objects.

Classes are objects as well.

class TestClass {
  constructor() {
    this.x = undefined
  } 
}

Object.getOwnPropertyNames(TestClass);
// returns (3) ["length", "prototype", "name"]

But anyway I missed @marshall007 post about compilation of un-inited properties. So my approach wont work as

class TestClass {
  public x: string;
}

is compiled to

class TestClass {
  constructor() {  } 
}

and not

class TestClass {
  constructor() {
    this.x = undefined
  } 
}

@pleerock
Copy link
Contributor

pleerock commented Sep 7, 2017

right

@styxiak
Copy link

styxiak commented Dec 4, 2017

@pleerock your solution from comment breaks first test form basic-functionality.spec.ts.
When we heave User object:

      @Exclude()
      class User {
        @Expose()
        id: number;
        @Expose()
        firstName: string;
        @Expose()
        lastName: string;
        @Exclude()
        password: string;
      }
const fromExistUser = new User();
        fromExistUser.id = 1;

and plain object:

const fromPlainUser = {
            firstName: "Umed",
            lastName: "Khudoiberdiev",
            password: "imnosuperman",
            additional: "something we dont want"
        };

then

const fromExistTransformedUser = plainToClassFromExist(fromExistUser, fromPlainUser);

will produce

fromExistTransformedUser = {
  id = undefined,
  firstName: "Umed",
  lastName: "Khudoiberdiev"
}

I don't know is it expected behaviour, but if, then will be nice to have option to omit undefined properties.

@NoNameProvided
Copy link
Member

The latest release contains class-validator@0.8.1 which supports stripping unknown properties.

@NoNameProvided NoNameProvided added the type: documentation Issues related to improving the documentation. label Mar 5, 2018
@hgrewa
Copy link

hgrewa commented Oct 14, 2018

stripping unknown properties still do not address following
class A { a = 'a'; }
class B extends A { b = 'b'; }

object to be parsed
obj = const obj = { a: 'a', b: 'b', c: 'c' };

the desired result after the transform B is, { a: 'a', b: 'b }

plainToClass(obj, { strategy: "exposeAll" } will give { a: 'a', b: 'b', c: 'c' };
plainToClass(obj, { strategy: "excludeAll" } will give { b: 'b' };

@IRCraziestTaxi
Copy link

IRCraziestTaxi commented Mar 11, 2019

Just bumping this because I was also just searching for the solution to this. For instance, I am trying to use mediator pattern in my API, and if a user can just add a property in Postman, etc. that exists on the entity but not on the Command class, then without manual mapping of properties in every handler in order to ensure unwanted properties from the request are not persisted to the database, there is no way to ensure the user cannot do so.

Edit: I discovered that using the @Expose decorator on properties in my Command classes along with @Body({ transform: { strategy: "excludeAll" } }) to decorate the injected request body in my controller method will only include the properties in that class. I didn't realize at first that the comment by @hgrewa above me pertained to inheritance issues. In my case, the decorator and option I mentioned work for me.

Edit 2: Just found in the docs for class-transformer that the options passed into @Body can be replaced by an @Exclude decorator on the Command class, so that only the class properties (marked with @Expose are parsed from the request body. Much cleaner.

@vellengs
Copy link

Any news about this?

@ljobse
Copy link

ljobse commented Jun 21, 2019

For people who are using this in combination with class-validator and thus are decorating the objects with @IsString() for example anyway.

I wrote a little extension that uses those decorators to strip all unknown properties from the object.
https://gist.github.com/ljobse/a617ca62e7d0277a8da351479656f04f

The file just needs to be loaded and the plainToClass function will now strip all properties which are not decorated with anything.

@github-actions
Copy link

Stale issue message

@github-actions github-actions bot added the status: awaiting answer Awaiting answer from the author of issue or PR. label Feb 17, 2020
@NoNameProvided NoNameProvided added type: feature Issues related to new features. and removed enhancement labels Aug 2, 2020
@LukaszKuciel
Copy link

TBH, I don't get the point why plainToClass is not stripping objects off fields that are not defined in a given class. At least there should be an option to omit unknown values.
A very popular case for class-transformer is serialization. Let's say that you have many DTOs and if I understand correctly the only way now to have a properly serialized response according to DTO is to add @Expose() decorator to each field? This approach has no added value to it and it only makes developer life harder.
Please provide the option to skip fields that are not defined in the class definition.

@AlexanderFarkas
Copy link

Any updates on this?

@occultskyrong
Copy link

Time on 2021-12-27 13:47:37

just use excludeExtraneousValues like this :

import { plainToClass, Expose } from 'class-transformer';

export class UserViewModel {
  @Expose()
  name: string;

  @Expose()
  email: string;

  @Expose()
  password: string;

  @Expose()
  roles: Array<string>;
}

console.info(
  plainToClass(
    UserViewModel,
    {
      email: 'adnan11@gmail.com',
      name: 'test name',
      password: 'password4',
      company: 'My Company',
    },
    { excludeExtraneousValues: true },
  ),
);

it's the result for this:

微信截图_20211227134911

i think it been solved.

see the link : https://github.com/typestack/class-transformer#enforcing-type-safe-instance

@NoNameProvided
Copy link
Member

NoNameProvided commented Feb 17, 2022

As @occultskyrong said, this is implemented for some time class-transformer, you need to configure it in routing-controllers properly.

@NoNameProvided NoNameProvided removed status: awaiting answer Awaiting answer from the author of issue or PR. type: documentation Issues related to improving the documentation. labels Feb 17, 2022
@NoNameProvided NoNameProvided changed the title class transformer not stripping additional properties feature: class transformer should strip additional properties Feb 17, 2022
@github-actions
Copy link

This issue 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 Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: invalid Issues with no action to take. type: feature Issues related to new features.
Development

No branches or pull requests