feat: add a useResponseClassTransformer global option #329

epiphone · 2017-11-22T14:23:01Z

Hi, first of all thanks for a great library!

By default response values are coerced to plain objects with class-transformer. When returning large objects (#226) or values with complex serialization logic (e.g. Mongoose documents #149) you might opt for the default toJSON handler instead.

This PR adds a global useResponseClassTransformer option parameter for toggling response transformation. It defaults to true, and is overridden by classTransformer: false.

Also related to #179.

MichalLytek · 2017-11-24T12:47:58Z

src/driver/BaseDriver.ts

@@ -129,14 +135,14 @@ export abstract class BaseDriver {

    protected transformResult(result: any, action: ActionMetadata, options: Action): any {
        // check if we need to transform result
-        const shouldTransform = (this.useClassTransformer && result != null) // transform only if enabled and value exist
+        const shouldTransform = (this.useClassTransformer && this.useResponseClassTransformer && result != null) // transform only if enabled and value exist


not updated comment

MichalLytek · 2017-11-24T12:49:25Z

Isn't that the feature that @NoNameProvided promised to implement a few months ago? 😉

NoNameProvided · 2017-11-24T13:39:28Z

Yes, it was. Shame I haven't had the time. I haven't reviewed this either because I think we had agreed on a slightly different approach, but I am not sure, and I don't have time to read it back now. I have a lot of thing on my table now, so I struggle to find any time to spend on my hobbies (including this lib) right now.

epiphone · 2017-12-04T16:15:25Z

Hi, I updated the PR to better match with @NoNameProvided's solution outlined here, i.e. added transformRequest and transformResponse options to controller and route decorators. Gotta agree this approach seems more future-proof 😄

What do you think?

MichalLytek

Please refactor your test entities to make sure that class-transformer option works - use @Exclude and @Expose decorator on class properties instead of toJSON.

MichalLytek · 2017-12-04T21:24:20Z

src/decorator-options/ControllerOptions.ts

+/**
+ * Extra options that apply to each controller action.
+ */
+export interface ControllerOptions extends TransformationOptions {


I think that you should implement TransformationOptions, not extending it - it would be ready for future extension/changes because you can't extend multiple interfaces.

MichalLytek · 2017-12-04T21:30:42Z

test/functional/action-options.spec.ts

+        assertRequest([3001, 3002], "post", "default", { firstName: "Umed", lastName: "Khudoiberdiev" }, response => {
+            expect(initializedUser).to.be.instanceOf(User);
+            expect(response).to.have.status(200);
+            expect(response.body.lastName).to.be.defined;


You've set transformResponse: false but you expect lastName to be defined even if lastName is excluded when class-transformer is disabled... I don't get it 😕

MichalLytek · 2017-12-04T21:32:19Z

test/functional/controller-options.spec.ts

+        }
+
+        @JsonController("/transform", {transformRequest: true, transformResponse: true})
+        class NoResponseTransformController {


NoResponseTransformController? No? 😆

MichalLytek · 2017-12-04T21:35:36Z

test/functional/controller-options.spec.ts

+        assertRequest([3001, 3002], "post", "default", { firstName: "Umed", lastName: "Khudoiberdiev" }, response => {
+            expect(initializedUser).to.be.instanceOf(User);
+            expect(response).to.have.status(200);
+            expect(response.body.lastName).to.be.defined;


default: ...lastName).to.be.defined;

enable: ...lastName).not.to.be.undefined;
Please be consistent with assertion, don't negate the negative expectation 😉

MichalLytek · 2017-12-04T21:43:08Z

test/functional/action-options.spec.ts

+        assertRequest([3001, 3002], "post", "override", { firstName: "Umed", lastName: "Khudoiberdiev" }, response => {
+            expect(initializedUser).not.to.be.instanceOf(User);
+            expect(response).to.have.status(200);
+            expect(response.body.lastName).to.be.defined;


Shouldn't you use .exist here? I can't see defined in chai documentation 😕

…/action options

epiphone · 2017-12-05T16:36:19Z

Thanks for the feedback @19majkel94, lots of good points in there 👍 . I refactored the tests, let me know if there's anything else you'd like to see changed.

I did have some trouble with class-transformer's global metadata storage: calling defaultMetadataStorage.clear() (here for example) deletes the class-transformer metadata of all defined classes, so I had to move the User class definitions inside a before-block and assign it to a variable in the outer scope. The end result is still reasonably clean, I think, but let me know if you'd have a better solution.

MichalLytek · 2017-12-05T17:53:13Z

Yeah, it clears because you call clear 😆 But I don't see the need to do change this - pleerock's configuration worked well, all metadata was keeped, only after all tests the storage was cleared. Why you had to change it?

epiphone · 2017-12-06T09:29:31Z

Because the way it was set up, after a test module finished it would not only clear the metadata of its own test classes, but also of all the classes declared in the following tests. For example after class-transformer-options.ts finished, it wiped out User's metadata in global-options.spec.ts which would break the test. That's why I ended up delaying the class declarations until the before block; I'd be happy to refactor it though if you have an alternative solution in mind.

MichalLytek · 2017-12-07T19:33:51Z

So the problem was that pleerock uses the class-transformer only in one test so clearing metadata wasn't affect other tests but now you have tests also in other files so it clears the metadata of all tests, right? 😄

epiphone · 2017-12-07T22:17:44Z

Exactly 👍 . I took a peek at class-transformer tests, seems like they're handling things in a similar way, declaring classes in the upper describe and it blocks.

MichalLytek · 2017-12-09T20:30:14Z

Ok, you have user.lastName || "default"; in your tests but I can't see a test case that is using it. Please make a one more test case that have transformRequest: true and transformResponse: false to see that it's working and we are ready to merge 😉

…est controllers

epiphone · 2017-12-26T11:28:46Z

Hey, sorry for the delay, got kinda busy over the holidays! I added the test case and removed user.lastName = "default"; where it's unused.

MichalLytek

LGTM!

MichalLytek · 2018-01-05T21:48:36Z

I'm not sure if @pleerock will like the options object inside decorator so I will wait for his review before merging this PR 😉

vekexasia · 2018-07-13T10:38:21Z

Hello, i'm jumping here to understand if this was holded back due to an alternative way to use both JSON controllers and "Raw" controllers that returns buffers

epiphone · 2018-07-27T08:49:18Z

I'm also curious about what's holding this back. Care to comment @NoNameProvided ?

f4z3k4s · 2019-01-17T15:53:09Z

@pleerock, @NoNameProvided could you please have a look at this? This is still an existing issue :(

kunokdev · 2019-01-21T23:06:35Z

Is there anything happening regarding this?

CSLTech · 2019-06-29T00:29:55Z

Is there still something holding this back @NoNameProvided @pleerock ?

github-actions · 2020-02-12T00:00:51Z

Stale pull request message

MohammedSiddiqui · 2020-04-03T10:45:00Z

What's the update on this ? 😅

hallsamuel90 · 2020-05-09T22:25:46Z

Also curious about the status of this?

jotamorais · 2020-05-12T21:58:53Z

@hallsamuel90, @MohammedSiddiqui @epiphone, @CSLTech, @kunokdev, @f4z3k4s, @vekexasia, @MichalLytek.

I'm planning to merge this PR and release this tomorrow night among some other fixes:

fix(Interceptors): order of global interceptors #543 fix(Interceptors): order of global interceptors
Input-validation bypass vulnerability #518 Input-validation bypass vulnerability
Fix incorrect handling of rejected promises from Middleware.use() #438 class-validator arbitrary bypass vulnerability
feat: add support for wildcard "all" routes #536 Add support for wildcard "all" routes
chore: add another example for using the response directly #546 chore: add another example for using the response directly
feat: add a useResponseClassTransformer global option #329 Add a useResponseClassTransformer global option
Performance Issue with big json result #226 Performance Issue with big json result
Problem with mongoose model serialization #149 Problem with mongoose model serialization
use classTransformer only for input #179 use classTransformer only for input
feat: controller inheritance #301 Controller inheritance
Controller Inheritance support #147 Controller Inheritance support

yurkagon · 2020-07-07T15:24:36Z

Still waiting :)

Add a useResponseClassTransformer global option

dc50de6

MichalLytek suggested changes Nov 24, 2017

View reviewed changes

Aleksi Pekkala added 3 commits November 24, 2017 15:43

Update comment, remove redundant null check

2346d51

Merge remote-tracking branch 'upstream/master'

0fbe188

Implement route/controller-specific transform options

be17c80

MichalLytek suggested changes Dec 4, 2017

View reviewed changes

Aleksi Pekkala added 2 commits December 5, 2017 12:30

Implement TransformationOptions instead of extending it in controller…

778ea0b

…/action options

Fix inconsistencies in transform options tests

c408966

Aleksi Pekkala added 2 commits December 26, 2017 12:55

Merge

0d9ebae

Test action only transforming requests, cleanup default values from t…

19a931a

…est controllers

MichalLytek approved these changes Jan 5, 2018

View reviewed changes

MichalLytek requested review from pleerock and NoNameProvided January 5, 2018 21:47

github-actions bot added the no-pr-activity label Feb 12, 2020

jotamorais added this to the 0.8.x release milestone Feb 12, 2020

jotamorais merged commit cd26ae1 into typestack:master Jul 21, 2020

NoNameProvided changed the title ~~Add a useResponseClassTransformer global option~~ feat: add a useResponseClassTransformer global option Aug 9, 2020

typestack locked as resolved and limited conversation to collaborators Aug 9, 2020

feat: add a useResponseClassTransformer global option #329

feat: add a useResponseClassTransformer global option #329

Uh oh!

Conversation

epiphone commented Nov 22, 2017

Uh oh!

MichalLytek Nov 24, 2017

Choose a reason for hiding this comment

Uh oh!

MichalLytek commented Nov 24, 2017

Uh oh!

NoNameProvided commented Nov 24, 2017

Uh oh!

epiphone commented Dec 4, 2017

Uh oh!

MichalLytek left a comment

Choose a reason for hiding this comment

Uh oh!

MichalLytek Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

MichalLytek Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

MichalLytek Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

MichalLytek Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalLytek Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

epiphone commented Dec 5, 2017

Uh oh!

MichalLytek commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epiphone commented Dec 6, 2017

Uh oh!

MichalLytek commented Dec 7, 2017

Uh oh!

epiphone commented Dec 7, 2017

Uh oh!

MichalLytek commented Dec 9, 2017

Uh oh!

epiphone commented Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichalLytek left a comment

Choose a reason for hiding this comment

Uh oh!

MichalLytek commented Jan 5, 2018

Uh oh!

vekexasia commented Jul 13, 2018

Uh oh!

epiphone commented Jul 27, 2018

Uh oh!

f4z3k4s commented Jan 17, 2019

Uh oh!

kunokdev commented Jan 21, 2019

Uh oh!

CSLTech commented Jun 29, 2019

Uh oh!

github-actions bot commented Feb 12, 2020

Uh oh!

MohammedSiddiqui commented Apr 3, 2020

Uh oh!

hallsamuel90 commented May 9, 2020

Uh oh!

jotamorais commented May 12, 2020

Uh oh!

yurkagon commented Jul 7, 2020

Uh oh!

Uh oh!

MichalLytek Dec 4, 2017 •

edited

Loading

MichalLytek commented Dec 5, 2017 •

edited

Loading

epiphone commented Dec 26, 2017 •

edited

Loading