[inversify-express-utils] BaseMiddleware bindings unique to HTTP request

[simplegsb]

Expected Behavior

From the README:

Middleware extending BaseMiddleware is capable of re-binding services in the scope of a HTTP request.

Bindings made using BaseMiddleware.bind should be scoped to the HTTP request and not be available outside of that request.

Current Behavior

BaseMiddleware.bind is binding/rebinding to the Container passed to InversifyExpressServer's constructor and so the bound service will override any previous value(s) and be available to all HTTP requests running at the time (and in the future).

Possible Solution

A child container could be created and attached to the HttpContext in InversifyExpressServer._createHttpContext. Then, BaseMiddleware.bind can bind to that child container. The child container should also replace the one created in InversifyExpressServer.handlerFactory and used to get the Controller.

Steps to Reproduce (for bugs)

The following example reproduces the problem:

// Required by inversify
import 'reflect-metadata';

import * as express from 'express';
import { Container, inject, optional } from 'inversify';
import { BaseHttpController, BaseMiddleware, controller, httpGet, InversifyExpressServer } from 'inversify-express-utils';

let TYPES =
{
  Transaction: Symbol.for('Transaction'),
  TransactionMiddleware: Symbol.for('TransactionMiddleware')
};

class TransactionMiddleware extends BaseMiddleware
{
  private count: number = 0;

  public handler(req: express.Request, res: express.Response, next: express.NextFunction): void
  {
    this.bind(TYPES.Transaction).toConstantValue(`I am transaction #${++this.count}\n`);
    next();
  }
}

@controller('/test')
class Controller extends BaseHttpController
{
  constructor(@inject(TYPES.Transaction) @optional() private transaction: string)
  {
    super();
  }

  @httpGet('/1', TYPES.TransactionMiddleware)
  public get1(): string { return this.transaction; }

  @httpGet('/2') // <= No middleware!
  public get2(): string { return this.transaction; }
}

let container = new Container();
container.bind<TransactionMiddleware>(TYPES.TransactionMiddleware).to(TransactionMiddleware);

new InversifyExpressServer(container).build().listen(3000);

Then send requests via curl:

$ curl localhost:3000/test/1 && curl localhost:3000/test/2
I am transaction #1
I am transaction #1

Context

I was trying to bind some contextual information in my middleware e.g. a database transaction for the HTTP request. This transaction is not used directly by the Controller but by it's dependent services. I do not want these dependent services to be aware of the HTTP request as they need to work in contexts outside of a web server as well.

Your Environment

• Version used: [email protected], [email protected]
• Environment name and version (e.g. Chrome 39, node.js 5.4): node.js 8.12.0
• Operating System and version (desktop or mobile): Ubuntu 18.04.1 LTS
• Link to your project: Sorry, private...

[simplegsb]

Example commit: https://github.com/simplegsb/inversify-express-utils/commit/cad4bac8dbc1b4848c2f91d5d323b468df620072

[csuich2]

I pulled in inversify-express-utils for the same reason - to create a per request DB transaction and am hitting the same issue. My dependencies are being reused by the next request to come in and thus the DB transaction is attempting to be reused.

@simplegsb Did you find a workaround for this?

@remojansen Any thoughts on supporting this? Or are we missing something that should allow this to work?

[simplegsb]

I just added steps to reproduce in my initial comment.

@csuich2 I implemented my proposed solution in the commit mentioned in my previous comment and it is working fine. Child containers support this feature quite easily and were already used to inject the HttpContext itself. I have a branch in my fork that can be used as an NPM dependency if you want to try it:

"dependencies": {
  ...
  "inversify-express-utils": "https://github.com/simplegsb/inversify-express-utils.git#Issue#962_lib",
  ...
}

[csuich2]

@simplegsb I didn't even think to use your sample commit as a package. Pulling it in seems to have done the trick! Have you thought about just opening your changes as a PR? Maybe @remojansen or @codyjs could take a closer look if they see a PR with the tests all passing.

[dcavanagh]

@simplegsb Please submit it as a PR! Do you have unit tests showing that your changes fix the problem? Thanks

[simplegsb]

@csuich2 I made a branch that has the lib and dts folders committed, that's why it can be used as an NPM dependency :)

@dcavanagh I've just created a PR here: https://github.com/inversify/inversify-express-utils/pull/235. I've amended an existing test and added another based on the repro steps above.

Glad I could help. I'm popping my open source cherry!

[rcollette]

It looks like there was a merge to fix this. Can this issue be closed?