fix: class-level Expose/Exclude is ignored for child classes

[jbjhjm]

Description

Minimal code-snippet showcasing the problem

@Exclude()
class BaseModel {}

class ChildModel extends BaseModel {
  @Expose()
  value1:string;
		
  unexposed:string;
}

Expected behavior

ChildModel should be transformed using excludeAll strategy, because it inherits the Exclude strategy from BaseModel.

Actual behavior

MetadataStorage.getStrategy does not check for ancester decorators, resulting in ChildModel being transformed using configurated strategy which defaults to exposeAll strategy.

Workaround is to decorate all Models with an own @Exclude decorator. But this may easily be forgotten. Which will then result in potential data leaks.

I added this as a todo on my roadmap because I really don't want to clutter my main repo with more workarounds. Will open a PR then which I hope someone takes care of.

[deviprsd]

I disagree, for one being that explicit is better than being implicit.

Second, what if there is a use case where you want to expose all on your inherited class? Implementing this would require additional APIs to counter the said solution such an @UnExclude. This could be a breaking change for many who use this as a tool, and must go through their classes to implement @UnExclude.

Further options can be added to plainToInstance to solve your need as well but that is an increase in library size and other complexities.

Third, is the perception the Exclude works for the class it is defined on. For your minimal case, it means I want to Exclude all unexposed fields in BaseModel. This is a stretch but more than likely someone is going to see the Exclude not included and assume that all the fields are exposed if they don't look deeply enough. Which again circle backs to the first point.

All In all, this should be a viable change if most users of this package, use it by default the way you are suggesting.

[jbjhjm]

I agree on explicit being better than being implicit. But I have to add: enforced general behavior patterns are better than granular/optional/forgettable ones. And also: framework behaviour should be consistent.

---

Let's go into detail and first talk about your stated points.

Point 2 -- reverting exclusion on a child of a base class that enforces opposite behavior would be misleading and should not be possible. The base class is there to ensure all child classes implement and follow this behavior.

Further options can be added to plainToInstance -- I disagree on this being a good solution. Options should enforce a global/general behavior, not one depending on the class being passed. Adding custom logic for changing options depending on the inherited parent class is not a fix, but a workaround.

Point 3 -- class-level Expose/Exclude decorator is not there to work on props , but to set the general exposeAll / excludeAll switch for this class. (it wouldn't even be possible to do so because on class-level the decorator cannot know which props will be known/exposed.)

About your summary: I doubt this is a good conclusion. "Users do use it the way it is" doesn't mean it works correctly or as expected. Users may not even be aware, may be expecting it to work the way I described, and have leaking data without knowing. Personally I only realized it doesn't work because I was writing many tests for a class working closely with class-transformer.

---

Further statement

class-transformer is known for taking parent class metadata into account. It WILL take parent class metadata into account. For everything, but class-level Expose/Exclude configuration. This clearly is inconsistent behavior to me.

This is a single exception to the general behavior. Iit is unexpected, it can be misleading, it is not documented. It can become a security risk.

Furthermore, I am not even sure if it was conciously implemented this way. With exposeAll/excludeAll being the only class-level metadata, it is possible that noone thought about inheritance when implementing this. But that's just speculation.

But even if this is expected behavior, it should urgently be noted in documentation for all the reasons mentioned above.

[jbjhjm]

Oh I just found that this is a duplicate of an issue from 2017, still open. https://github.com/typestack/class-transformer/issues/95