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

Add Expose decorator to all fields on Pagination class #652

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThePooN
Copy link

@ThePooN ThePooN commented Dec 6, 2021

In my project, I use:

app.useGlobalInterceptors(
  new ClassSerializerInterceptor(app.get(Reflector), {
    excludeExtraneousValues: true,
  }),
);

excludeExtraneousValues: true is a secure practice that enforces the use of explicit @Expose() from class-transformer on every property that should be exposed on an API, thus eliminating any way to implicitly expose any field via HTTP.

This project-wide option is incompatible with nestjs-typeorm-paginate as @Expose() is missing on the Pagination class. I added it with no drawbacks for developers who don't use that option.

Looks like I made large edits to lock files; feel free to commit on my fork if you believe there's anything to fix on that front.

@bashleigh
Copy link
Collaborator

I get what you're trying to achieve but I think adding the decorators to the response object from this package is the wrong way around it. Few reasons and please feel free to prove me wrong. Adding class-transformer to this package adds another dependency version for every project. Imagine you'll have class-transformer 0.4.3 as a dep of this project say and nestjs validation method will need class-transformer 0.4.5. Just makes your deps larger. Sooo I would recommend perhaps a custom interceptor to handle this action for your specific case unless you think there's a higher perceptable of people using this method?
I personally would say I'd only use the class-transformer when needed and if I'm not going to use it, I don't want it as a dependency but that's just me. Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants