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

Allow routes to have a unique IoC container per request #779

Closed
2 of 4 tasks
alxwrd opened this issue Aug 25, 2020 · 5 comments · Fixed by #803
Closed
2 of 4 tasks

Allow routes to have a unique IoC container per request #779

alxwrd opened this issue Aug 25, 2020 · 5 comments · Fixed by #803
Labels
good first issue This issue could be an easy PR for those looking to help contribute help wanted

Comments

@alxwrd
Copy link
Contributor

alxwrd commented Aug 25, 2020

I'm submitting a ...

  • bug report
  • feature request
  • support request

I confirm that I

  • used the search to make sure that a similar issue hasn't already been submitted.

Description

I'm currently working on a project where I am migrating from inversify-express-utils to tsoa, and I'm looking to replicate some functionality.

In inversify-express-utils, each request gets it's own IoC container. This allows data (in my case an ID) to be bound to the container and then injected into services.

With tsoa, there is only one IoC container which is loaded from the user-defined IoC module and used in the routes:

const controller: any = iocContainer.get<SomeController>(SomeController);

With inversify-express-utils, the Express app is built with middleware that adds a child container to each request using Reflect and reflect-metadata.

this._app.all("*", (
    req: express.Request,
    res: express.Response,
    next: express.NextFunction
) => {
    (async () => {
        const httpContext = await _self._createHttpContext(req, res, next);
        Reflect.defineMetadata(
            METADATA_KEY.httpContext,
            httpContext,
            req
        );
        next();
    })();
});

https://github.com/inversify/inversify-express-utils/blob/bc40bb7671e0d4d8183266b3b3d563bb62056618/src/server.ts#L102

This is then extracted when handling the request and used to load the controller.

const httpContext = this._getHttpContext(req);
httpContext.container.bind<interfaces.HttpContext>(TYPE.HttpContext)
    .toConstantValue(httpContext);

// invoke controller's action
const value = await httpContext.container.getNamed<any>(TYPE.Controller, controllerName)[key](...args);

https://github.com/inversify/inversify-express-utils/blob/bc40bb7671e0d4d8183266b3b3d563bb62056618/src/server.ts#L248

Currently, I've implemented the middleware that adds the container to the request and I'm using a modified template to load that container out of the request so it can be used as the IoC container.

Is this something tsoa would support?

If so I'm happy to get a pull request started. Below is the minimal change in my project in order to support this, but more work would need to be done for tsoa.

diff --git a/express.hbs b/routes-template.hbs
index 8956053..311086d 100755
--- a/express.hbs
+++ b/routes-template.hbs
@@ -14,6 +14,8 @@ import { {{name}} } from '{{modulePath}}';
 import { expressAuthentication } from '{{authenticationModule}}';
 {{/if}}
 {{#if iocModule}}
+import { METADATA_KEY } from './core/constant/keys';
+import { Container } from 'inversify';
 import { iocContainer } from '{{iocModule}}';
 {{/if}}
 import * as express from 'express';
@@ -76,7 +78,8 @@ export function RegisterRoutes(app: express.Express) {
             }

             {{#if ../../iocModule}}
-            const controller: any = iocContainer.get<{{../name}}>({{../name}});
+            const container: Container = Reflect.getMetadata(METADATA_KEY.httpContext, request).container
+            const controller: any = container.get<{{../name}}>({{../name}});
             if (typeof controller['setStatus'] === 'function') {
                 controller.setStatus(undefined);
             }
@WoH
Copy link
Collaborator

WoH commented Sep 2, 2020

I assume this would break other DI containers, don't think we can do that without a proper strategy.

@alxwrd
Copy link
Contributor Author

alxwrd commented Sep 2, 2020

I don't think it would. From the tsoa side, the container would just be loaded out of the request (if it's available). Nothing has really changed, except for the source of the container.

{{#if ../../iocModule}}
let container: any = iocContainer;

if (Reflect.hasOwnProperty('getMetadata')) {
    let meta = Reflect.getMetadata('tsoa:request-meta', request);
    if (meta.container) {
        container = meta.container;
    }
}

const controller: any = container.get<{{../name}}>({{../name}});
if (typeof controller['setStatus'] === 'function') {
    controller.setStatus(undefined);
}
{{else}}

And then, from the user side, documentation would need to be added to show that adding some metadata will enable this functionality.

A container per request

To enable a separate IoC container for each request, you will need to install the package reflect-metadata. Then you can add a global handler to add the container to each request using Reflect.defineMetadata().

app.all("*",
    (req, res, next) => {(async () => {
        Reflect.defineMetadata('tsoa:request-meta', {container: container.createChild()}, req);
        next();
    })();
});

@celebro
Copy link

celebro commented Sep 3, 2020

I also have a use case that needs to take request object into consideration when resolving controllers from the iocContainer, but I propose an opt-in option that would add the request object as a second parameter to iocContainer.get<{{../name}}>({{../name}}, request);. It's then up to library user to provide a wrapper iocContainer that implements request specific ioc logic.

@alxwrd
Copy link
Contributor Author

alxwrd commented Sep 3, 2020

@celebro I like that better. I removes all implementation details away from tsoa.

@WoH WoH added good first issue This issue could be an easy PR for those looking to help contribute help wanted labels Sep 26, 2020
@alxwrd
Copy link
Contributor Author

alxwrd commented Oct 1, 2020

Hi @WoH, I've just seen you've labelled this. I will look at putting something together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This issue could be an easy PR for those looking to help contribute help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants