bug: body is undefined in customParamDecorators

[NoNameProvided]

When running the createParamDecorator function, the bodyparser is not yet called on the request object, so request.body is undefined.

[MichalLytek]

The problem is here:
https://github.com/pleerock/routing-controllers/blob/master/src/metadata/ActionMetadata.ts#L186

this.isBodyUsed = !!this.params.find(param => param.type === "body" || param.type === "body-param");

which is used here:
https://github.com/pleerock/routing-controllers/blob/master/src/driver/express/ExpressDriver.ts#L80

if (action.isBodyUsed) {
    if (action.isJsonTyped) {
        defaultMiddlewares.push(this.loadBodyParser().json(action.bodyExtraOptions));
    } else {
        defaultMiddlewares.push(this.loadBodyParser().text(action.bodyExtraOptions));
    }
}

@pleerock shall we just add custom-converter to the list or maybe expose the isBodyUsed prop as the decorator option?

BTW, !!this.params.find is awful, you can use ES6 features:

this.isBodyUsed = this.params.some(param => ["body", "body-param"].includes(param));

[pleerock]

shall we just add custom-converter to the list or maybe expose the isBodyUsed prop as

Does not look as a good solution. I don't know what @NoNameProvided is doing in his decorators, but if he does not use @Body decorator then maybe he should control this by his own in his decorator and do .use(bodyParser) manually?

BTW, !!this.params.find is awful,

agree, but I don't think includes is supported in node v4 which is minimal supported version.

[pleerock]

hmmmm waaaat, "body" || "body-param" always returns "body"

[MichalLytek]

hmmmm waaaat

The most epic "copy from clipboard" fail 😆 I've deleted it to not be so ashamed.
I just wanted to paste indexOf solution:

this.isBodyUsed = this.params.some(param => ["body", "body-param", "custom-converter"].indexOf(param.type) >= 0);

Or even better is to use this one-line polyfill for this ES2016 feature as it's really useful:

Array.prototype.includes = Array.prototype.includes || function(element) {
  return this.indexOf(element) >= 0
}

But anyway, going back to the question - @NoNameProvided what is your use case? What are you trying to with the body in decorator function?

he should control this by his own in his decorator and do .use(bodyParser) manually?

I think we should create a mechanism to registering middlewares to run before decorator value extraction function call. I have @JWT decorator which just extract from req.user property but I have to turn the express-jwt middleware globally or @UseBefore on every route which use @JWT.

[NoNameProvided]

This will sounds strange, but to be honest I don't remember anymore. 😄 I saw it was not working so I opened an issue and gave upon on the approach for now, but I don't remember what I tried to accomplish.

[NoNameProvided]

I don't know what @NoNameProvided is doing in his decorators, but if he does not use @Body decorator

But CustomParamDecorators is not for getting data from your request the way you want it? So it would make sense to allow reading from body. What was the original goal for it then?

[MichalLytek]

Yeah, but it should just extract it from property, like you use express-jwt middleware and want to inject req.token property. Don't put there any logic connecting to the db or sth, do it on the middleware level.

[NoNameProvided]

Don't put there any logic connecting to the db or sth, do it on the middleware level.

Hmm, but I have implementations like @CurrentUser (I know there is an official implementation for this), and other custom param decorator where I load dynamic data. Because I found it better for some very common stuff to use . It's more convenient to write

@Get('xy')
xy(@xy) {
  this.someService.startUseItRightAway(xy)
}

vs

@Get('xy')
xy(x) {
  let xy = this.xyService.getXy(x);
  this.someService.startUseItRightAway(xy)
}

[MichalLytek]

Decorators in action should be used only to inject something from request - param, body, cookie, header, etc. but not preloading entity from db or getting data from service.

CustomParam was introduced to allow extracting properties from 3rd party middlewares like express-jwt or something.

[NoNameProvided]

Then I am abusing decorators af. 😄

[NoNameProvided]

But still it should allow to read from body not?

[pleerock]

wait wait its absolutely legal in custom decorators to read from body and do some operations on it, or to load user from the database before controller method execution. Why not? - they are designed to work like this. So this:

@Get('')
xy(@xy) {
  this.someService.startUseItRightAway(xy)
}

can be totally better then

@Get('')/xy()
xy(@xy) {
  let xy = this.xyService.getXy();
  this.someService.startUseItRightAway(xy)
}

@CurrentUser is an example of such scenario - it can take user id from the request headers it performs database query to load that user and returns it directly into the controller.

[NoNameProvided]

Okay, so this (reading data from body) should be fixed right? To be honest I don't have a whole overview of the working parts in createParamDecorator but I will dig in and hopefully be able to create a PR for it. :)

[MichalLytek]

Ping @NoNameProvided - decide if you need to create a PR for this or close the issue.

I think that custom decorator should mount/call middlewares which it use by itself. But I'm not sure as this may introduce conflict as some middlewares might be called twice.
Of course we can make an option field to mark that it need body parser turned on but we would then have to do this also for multer or session or other decorators/middlewares.

[NoNameProvided]

I will look into this.