inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
11.34k stars 719 forks source link

Issues with @RequestParam @Request @Response #527

Closed remojansen closed 7 years ago

remojansen commented 7 years ago

So I'm testing the 3.5.0 release of inversify-express-utils and I have the following Controller:

@injectable()
@Controller(
    authServerPaths.server.roleFeature.name,
    authorize({ feature: FEATURE.APP_ACCESS_APP_PREFERENCES })
)
class RoleFeatureController {

    @repositoryOfRoleFeature private readonly _roleFeatureRepository: framework.Repository<entities.RoleFeature>;
    @logger private readonly _logger: logging.Logger;

    @Get(authServerPaths.server.roleFeature.endpoints.getById)
    public async getById(
        @RequestParam("id") roleId: string,
        @Request() req: express.Request,
        @Response() res: express.Response
    ) {
        try {
            this._logger.info(`HTTP ${req.method} ${req.url}`);
            return await this._roleFeatureRepository.readMany([{ key: "role_id", val: roleId }]);
        } catch (e) {
            this._logger.error(`HTTP ERROR ${req.method} ${req.url}`, e.message);
            res.status(500).json([]);
        }
    }

    @Post(authServerPaths.server.roleFeature.endpoints.post)
    public async post(
        @RequestBody() roleFeature: entities.RoleFeature,
        @Request() req: express.Request,
        @Response() res: express.Response
    ) {
        try {
            this._logger.info(`HTTP ${req.method} ${req.url}`);
            return await this._roleFeatureRepository.create(roleFeature);
        } catch (e) {
            this._logger.error(`HTTP ERROR ${req.method} ${req.url}`, e.message);
            res.status(500).json({});
        }
    }

    @Delete(authServerPaths.server.roleFeature.endpoints.delete)
    public async delete(
        @RequestParam("role_id") roleId: string,
        @RequestParam("feature_id") featureId: string,
        @Request() req: express.Request,
        @Response() res: express.Response
    ) {
        try {
            this._logger.info(`HTTP ${req.method} ${req.url}`);
            return await this._roleFeatureRepository.delete([
                { key: "role_id", val: roleId },
                { key: "feature_id", val: featureId }
            ]);
        } catch (e) {
            this._logger.error(`HTTP ERROR ${req.method} ${req.url}`, e.message);
            res.status(500).json({ affectedRows: 0 });
        }
    }

}

The @Get() method getById works as expected and it is binding @RequestParam("id"), @Request() and @Response() correctly. However, the @Delete() methoi is not binded correctly:

It looks like it is just not doing any kind of binding. The same problem takes place on the @Post() method.

Any ideas of what can be wrong @AltekkeE? Thanks!

dcavanagh commented 7 years ago

@remojansen it looks like you are getting the default behavior which adds req, res, next to the end of the arguments array that is passed to the RoleFeatureController.delete method. That

what is the value of "authServerPaths.server.roleFeature.endpoints.delete"? what is the value of request.params? The request.params object must contain an "role_id" and "feature_id" property

it is working for me using this code.

@Delete('/:id/:feature_id')
    public deleteJob(@RequestParam('id') roleId: string,
        @RequestParam('feature_id') featureId: string,
        @Request() req: express.Request,
        @Response() res: express.Response): Promise<Message> {
        return  this.jobService.deleteJob(roleId);
    }
remojansen commented 7 years ago

I have:

@Delete(authServerPaths.server.roleFeature.endpoints.delete)
    public async delete(
        @RequestParam("role_id") roleId: string,
        @RequestParam("feature_id") featureId: string,
        @Request() req: express.Request,
        @Response() res: express.Response
    ) {

and authServerPaths.server.roleFeature.endpoints.delete is /role_feature/:role_id/:feature_id.

I'm debugging and looks like there is something wrong with here. The else is not invoking Reflect.defineMetadata ? I'm trying a few things...

remojansen commented 7 years ago

Fixed! these two lines need to be moved out of the if/else :tada:

dcavanagh commented 7 years ago

@remojansen I'm still having trouble reproducing the problem

On Thu, Apr 6, 2017 at 12:00 PM, Remo H. Jansen notifications@github.com wrote:

Closed #527 https://github.com/inversify/InversifyJS/issues/527 via inversify/inversify-express-utils@bd16e7a https://github.com/inversify/inversify-express-utils/commit/bd16e7a751a7f51e5969163a78d7ccd8870808b9 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/inversify/InversifyJS/issues/527#event-1032497651, or mute the thread https://github.com/notifications/unsubscribe-auth/AKvP2Lz_btk9Vrny7v5qLIhuq2JhaGW8ks5rtQwtgaJpZM4M1iM- .

remojansen commented 7 years ago

I think you need a controller with multiple methods and multiple annotations in the arguments, to reproduce the issue one method is not enough. Did you try that? I released that change and it is working fine at work for me :)

dcavanagh commented 7 years ago

Ok.. I will try that. I just wanted to reproduce it so I could understand why it was going wrong.

remojansen commented 7 years ago

I added a test case. The test case needed to hit this else.

The original code was:

if (!Reflect.hasOwnMetadata(METADATA_KEY.controllerParameter, target.constructor)) {
    parameterMetadataList.unshift(parameterMetadata);
    metadataList[methodName] = parameterMetadataList; // !
    Reflect.defineMetadata(METADATA_KEY.controllerParameter, metadataList, target.constructor); // !
} else {
    metadataList = Reflect.getOwnMetadata(METADATA_KEY.controllerParameter, target.constructor);
    if (metadataList.hasOwnProperty(methodName)) {
        parameterMetadataList = metadataList[methodName];
    }
    parameterMetadataList.unshift(parameterMetadata);
}

The if will generate some metadata thanks to the Reflect.defineMetadata call but the else case is not generating any metadata because it is missing the Reflect.defineMetadata call.

The fixed code is:

if (!Reflect.hasOwnMetadata(METADATA_KEY.controllerParameter, target.constructor)) {
    parameterMetadataList.unshift(parameterMetadata);
} else {
    metadataList = Reflect.getOwnMetadata(METADATA_KEY.controllerParameter, target.constructor);
    if (metadataList.hasOwnProperty(methodName)) {
        parameterMetadataList = metadataList[methodName];
    }
    parameterMetadataList.unshift(parameterMetadata);
}
metadataList[methodName] = parameterMetadataList; // !
Reflect.defineMetadata(METADATA_KEY.controllerParameter, metadataList, target.constructor); // !
dcavanagh commented 7 years ago

@remojansen Looks great! I was able to reproduce the problem. Sorry for all the trouble.

remojansen commented 7 years ago

No, problem thanks for your awesome PR :+1: