Question: one controller instance per request

[maghis]

Is there a way to create an instance of a controller per request?

This would allow providing context about the request to all the methods of the controller (ie: currentUser, logger (with request metadata), requestId, ...), without having to pass the context around via method parameters.

[NoNameProvided]

To share date between calls, you can use ctx in Koa. In Express you can attach it to the req object.

[maghis]

@NoNameProvided that's exactly what I wanted to avoid, because it's harder to get type information and you still have to pass the req around.
Makes it harder to break an action implementation in multiple methods and it's harder to reuse with inheritance.

[NoNameProvided]

I am not sure I understand what do you want then.Do you want a special param, like an injectable Context type?

@Get(/item/:id)
public async function(@Context() context: Context) {
  // ... 
}

In this case I dont see the difference between injecting a @Req() req: Request object or this context object.

Or do you want Context Attached to the actual request controllers?

@Get(/item/:id)
public async function(@Context() context: Context) {
  this.context.mySpecifiedParam;
}

If you want this, how would you specify it's type? How will they override each other (if speaking about inheritance)

In theory it would be possible. Currently we register a single global route handler to handle requests. So we can initialize a new instance of a controller for the matched route, but this would need a rewrite of the drivers to work this way. Also initializing controllers, and all needed dependency on every request probably will have a performance impact on the response time, so this should be made optional if we implement it and I am not sure if this can be done a nice way.

But this is a kind of question we should ask @pleerock about

[maghis]

What I really would like is to be able to register a Controller with a factory method that then returns the controller instance to handle the request.

Quick example of a use case:

class MyController {
    constructor(private logger: Logger, private somethingModel: SomethingModel) { }

    @Get("/foo")
    public async getSomething() {
        this.logger.log("I'm about to get something")
        this.checkSomething();

        return await this.somethingModel.get();
    }

    private checkSomething() {
        this.logger.log("I'm checking!");
    }
}

const app = createExpressServer({
    controllers: [req => new MyController(Logger.fromRequest(req), new SomethingModel("production"))]
});

This lets you log with contextual info from any helper method (checkSomething() in the example) and makes it easy to test controllers:

// test!
const testController = new MyController(consoleLogger, modelMock);
await testController.getSomething();

Just an example of a use case.

[idchlife]

Basically you want controllers as services as I believe. If I'm not mistaken there is not such functionality right now in routing controllers.
Proper way (even in frameworks where controllers as services are option, like symfony(php)) is to create separate service using typedi and test it instead. Like test service in unit tests, but controllers should be tested in functional/acceptance tests.

[NoNameProvided]

Basically you want controllers as services as I believe.

No, services are singletons injected by typedi (if you use typedi), what @maghis wants is a different instance of controllers for every request.

From the Logger.fromRequest(req) line and his comments I think he simply doesn't want to pass id-s and that's kind of stuff around. So he doesnt have to write
this.logger.log("I'm checking!", req.contextId); but simply this.logger.log("I'm checking!"); because Logger.fromRequest(req) would return an instance of the logger with the id of the request pre-set.

But I don't see much value in adding such feature. Context is easily creatable in a before middleware already. Attaching a logger to the request via req.logger = Logger.fromRequest(req) and then using it in the controllers like req.logger.log('xy') is perfectly can solve his problem. So I still don't understand why it's so important to have a different instance for every request.

Like test service in unit tests, but controllers should be tested in functional/acceptance tests.

You can already test them in unit tests if you want. You can inject whatever dummy class you want for your controller. They are just simple classes after all.

[pleerock]

routing-controllers allows you to declare routes and functions that will be executed on user's request on that route. Using decorators and class methods.

What you are asking is just complexity - don't do things this way. Simply handle your actions on controller methods level and if you have something to reusable extract functionality into services and reuse services.

[idchlife]

@NoNameProvided my bad, I meant to say something about "you want to have one request instance and ability to work with it like in "request - process spawn -response - process died" way. Like you work with requests in PHP. There is exactly one instance of controller per request :D

The thing is, it's not so easy to get used to thinking "hey, this service/controller/class is not for user who I got from request, it's for everyone, so it can have my app parameters, but not some user's parameters." and passing params everytime and everywhere. If you are from different background, tho

[maghis]

@idchlife it's actually one of the requirement of the MVC design pattern, for the controller to be stateless on a request basis. Allows you to do a lot of code reuse by inheriting from common base controllers and guarantees that there is no state "leak" between requests (state persistence across requests should be handled by models).

Design of other popular MVC frameworks:
ASP.net MVC: https://stackoverflow.com/questions/5425920/asp-net-mvc-is-controller-created-for-every-request
Ruby on Rails: https://stackoverflow.com/questions/14172986/why-does-rails-create-a-controller-for-every-request
ASP.net Web API: https://stackoverflow.com/questions/25175626/asp-net-web-api-why-are-controllers-created-per-request

Maybe I'm old school, but I'm working on a large number of microservices that need to share common tracing/logging/perf measurement infrastructure and this is currently a requirement.
I really like the project, I'll see if I can reuse the controller description/metadata part.

Thanks for the help!

[pleerock]

it's actually one of the requirement of the MVC design pattern, for the controller to be stateless on a request basis

actually MVC design pattern is quite abstract does not force you to make such design decisions. But yeah, controllers should be stateless on a request basis and nothing should be stored by a user in a controller, it should be a rule for everyone.

In routing-controllers controllers are stateless until you set a container. If you set a container - they are services. If you make them services. You can store in such services something request-based or not, but generally its a bad idea and everyone should avoid doing that.

share common tracing/logging/perf measurement

You can do that using services. If you need a request object in your services simply pass them as a method parameter, e.g. this.logger.log(req, "I'm checking!");. The only problem is to send req object each time. So, is it the problem for you?

[maghis]

@pleerock yep, I would like to avoid that, having to pass around req objects continuously makes it harder to test.

I'll try using services that way and maybe give a shot at implementing a driver.

Thanks!

[pleerock]

okay I've got your idea and request, probably something like this is implementable:

@JsonController({
     factory: (request, response) => {
          const logger = new Logger(request);
          return new UserController(logger);
     }
})
export class UserController {

      constructor(private logger: Logger) {
      }

}

so, if factory is defined then it will use it instead of getting controller from a service container.

[marshall007]

@pleerock in that case you would no longer be able to use @Inject() and other DI utilities. You'd be responsible for resolving all dependencies of the controller yourself, correct?

[pleerock]

right he would need to do something like this:

@JsonController({
     factory: (request, response) => {
          const logger = new Logger(request);
          const userRepository = Container.get(UserRepository);
          return new UserController(logger, userRepository);
     }
})
export class UserController {

      constructor(private logger: Logger, private userRepository: UserRepository) {
      }

}

[MichalLytek]

Wouldn't it be simplier if TypeDI (or an other configured container) could create new instance of Controller class with injected services/repository (also shared or newed)? There would only be an option in decorator to mark controller to be 1 instance per 1 request and the injecting boilerplate would happen under the hood.

[pleerock]

lets say if we do such thing in typedi then how do you see it should work, provide more details. How integration should look like and how request/response objects shall come from routing-controllers to typedi

[MichalLytek]

how request/response objects shall come from routing-controllers to typedi

I was answering to one controller instance per request problem.

And I really don't get the idea of Logger.fromRequest(req), why some class has to be instantied using whole req object instead of explicit params? Even if there is a use case for this kind of weird things, it can be done now easily with custom param decorator which can inject Logger.fromRequest(req), so you can use the logger associated with request in your action handler. And if you don't call the logger explicit in handle but it performs autologging, it should be extracted to a middleware and attached before route.

[pleerock]

I was answering to one controller instance per request problem.

yeah, I was talking about same.

I really don't get the idea

Imagine you want to log each user action and add to logs request data (browser, ip, etc.). To do so right now you have two inconvenient solutions:

@JsonController()
export class UserController {

     constructor(private logger: Logger) {
     }

     @Get("/users")
     all(@Req() request: Request) {
         this.logger.log("message", req);
     }

     @Post("/users")
     save(@Req() request: Request) {
         this.logger.log("message", req);
     }
}

or

@JsonController()
export class UserController {

     @Get("/users")
     all(@MyLogger() logger: Logger) {
         logger.log("message");
     }

     @Post("/users")
     save(@MyLogger() logger: Logger) {
         this.logger.log("message");
     }
}

With first approach inconvenience is that you have to pass req into logger each time, with second approach inconvenience is that you have to inject your logger each time

[MichalLytek]

with second approach inconvenience is that you have to inject your logger each time

This is the same problem as discussed in #147 - you have to decorate each action in each controller explicit instead of implicit inherit from base controller and in every route there is a boilerplate to fetch entity from db based on id from params, etc. And we claimed that it's better to do it explicitly rather than jumping through all the files.

So with the problem with logger related to single request and one controller instance:
In the app code we should have clear separation of shared objects and request-scoped objects. Query/path params, session/jwt, current user and others are injected as action params because they are related with a single request. Shared (between requests) service like entities repositories or hashing service can be singletons, so they are injected in controller constructor. Of course we can create new instance of controller for every request (and waste memory), create new connection to db and we can even spawn new thread 😆. But node.js is asynchronous, single-thread and event-loop based so we shouldn't map features from other languages/frameworks which are multi-threaded based. We have to change thinking and abandon patterns we learned before to benefit the best from node.js 😉

[adnan-kamili]

Currently, all services are singletons, may be you can allow users to create services with different lifetimes at the time of creation, something similar is done in Asp.Net Core:

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection#service-lifetimes-and-registration-options

Transient
Transient lifetime services are created each time they are requested. This lifetime works best for lightweight, stateless services.

Scoped
Scoped lifetime services are created once per request.

Singleton
Singleton lifetime services are created the first time they are requested (or when ConfigureServices is run if you specify an instance there) and then every subsequent request will use the same instance.

[emallard]

Sorry for the long post, but I think per request injection can lead to very succinct code. Even if the injection hierarchy is complex.
Example:

@JsonController()
export class UserController {

    // So finally something very simple but with complex hierarchy of dependencies
    // friends -> currentUser -> request
    // friends -> db
    // clients -> currentUser -> request
    // clients -> db
    // debt -> clients

     @Get("/friendsClientsAndDebts")
     all(friendsService: FriendsService,
         clientsService: ClientsService,
         debtService: DebtService) {
        return {
            friends: friendsService.getFriends();
            clients: clientsService.getClients();
            debt : debtService.getDebt();
        }
     }
}

Explanations of dependencies :

// Db is a singleton
@Singleton()
export class Db {
    users:collection;
    clients:collection;
    friends:collection;
}

@RequestScope()
export class CurrentUserService
{
    currentUser:User = null;
    constructor(
        private db:Db,
        private req:Request) {
    }

    getUser() {
        if (this.currentUser == null)
            this.currentUser = this.db.users.find(this.req.session.userId);
        return this.currentUser;
    }
}

@RequestScope()
export class FriendsService {
    constructor(
        private db:Db,
        currentUser:CurrentUserService) {}

    getFriends()
    {
        if (this.friends == null)
            this.friends = db.friends.find(currentUser.getUser());
        return this.friends;
    }
}

@RequestScope()
export class ClientsService {
    constructor(
        private db:Db,
        currentUser:CurrentUserService) {}

    getClients()
    {
        if (this.clients == null)
            db.clients.find(currentUser.getUser());
        return this.clients;
    }
}

@RequestScope()
export class DebtService {
    constructor(
        private clientsService:ClientsService) {}

    debt()
    {
        // use clientsService
    }
}

[dmikov]

I think controller per request is very important to have, if you are using DI. All examples here deal with a simplified use case of logger, that is too simplistic to base design decision on, it has no dependencies and only couple of methods. Sure in that case you can pass req in the call. But what if you have a DI topography of 3 classes down, DBLocator->DBService->SearchSearvice-> and it's DBService that needs user info to check permissions and set createdBy automatically? Now do you have to put extra object on every method in DBService and SearchService to pass req through to actual one function that does it? Or wouldn't it be simpler to let DI initialize SearchService, cascading to DBService which will have Zones or Context injected in it and have it as this.userId for the duration of request. So no matter which method you call as long as DBService references this.userId you fine. I implemented this architecture thinking I am in good old MVC world and it worked. Then it stopped, because it only stored first user request during initial creating other users were ignored. That's pretty fragile and confusing. I would definitely appreciate stateless controllers that can be initialized per request, to avoid this problem.

[MichalLytek]

Sorry for the long post, but I think per request injection can lead to very succinct code. Even if the injection hierarchy is complex.

The whole code can be also succinct using the functional approach:

export class Db {
    users: collection;
    clients: collection;
    friends: collection;
    debts: collection;
}

export class FriendsService {

    constructor(
        private db:Db,
    ) {}

    getFriends(user: User) {
        if (this.friends == null) {
            this.friends = this.db.friends.find(user);
        }
        return this.friends;
    }
}

export class ClientsService {

    constructor(
        private db:Db,
    ) {}

    getClients(user: User) {
        if (this.clients == null) {
            this.db.clients.find(user);
        }
        return this.clients;
    }
}

export class DebtService {

    constructor(
        private db:Db,
    ) {}

    getDebt(user: User) {
        this.db.debts.getAccountState(user)
    }
}


@JsonController()
export class UserController {

    constructor(
        private friendsService: FriendsService,
        private clientsService: ClientsService,
        private debtService: DebtService,
    ) {}

    @Get("/friendsClientsAndDebts")
    all(@CurrentUser() user: User) {
        return {
            friends: this.friendsService.getFriends(user),
            clients: this.clientsService.getClients(user),
            debt: this.debtService.getDebt(user),
        }
    }
}

So your services are stateless and they get the data from outside - it's passed to them by action handler which extract the data from the request.

It doesn't need shared state and new instances for request but only some changes in app architecture and different thinking as it's single threaded, event-loop based Node.js 😉

[MichalLytek]

But what if you have a DI topography of 3 classes down, DBLocator->DBService->SearchSearvice-> and it's DBService that needs user info to check permissions and set createdBy automatically?

This is node.js ecosystem. We don't have 5 layers of abstraction, 10 interfaces to implements and use 3 different design patterns like in C#/Java world to make the things works nice 😆

Now do you have to put extra object on every method in DBService and SearchService to pass req through to actual one function that does it? Or wouldn't it be simpler to let DI initialize SearchService, cascading to DBService which will have Zones or Context injected in it and have it as this.userId for the duration of request.

Don't overcomplicate things because when they don't have to be. Keep it simple, stupid 😜

I implemented this architecture thinking I am in good old MVC world and it worked.

Node.js with single threaded and event-loop needs different way of thinking. It might be hard for senior C# developers to switch thinking but this requires it. This is not .NET, we don't need overcomplicated MVC implementations. You can't map the way you always create your apps to the node.js world because you would kill all the advantages it has thanks to its nature. If it's hard to you, go back to C# 😉

[pleerock]

I think what @19majkel94 want to say (and what I say for everyone who come from java/c#) is that in node.js and javascript/typescript we must think more javascript/typescript-ish way. Most of design patterns were developed for languages like java because of limitation those language have. I don't tell they are bad, they have their own pros and cons (thats why typescript exist - it took pros of another languages and merge them into javascript), but anyway you need to think new way. Thats what I think @19majkel94 want to say and thats what I personally learned in javascript world after years of programming in java.

But node.js can have 5 layers of abstraction, why not? Okay, personally I hate people who overcomplicate things and make useless abstractions (one of the biggest mistakes of "pro"-s), but this can be real.

And I can admit that its not pleasure to pass data from controller to all downside levels. Its really not a big pleasure to do so. And I would like to fix this problem if there is a good fix.

So what is your solution guys? Can you propose some really really good solution that fits routing-controllers, typedi and nodejs ecosystems?

One thing I can propose is container-per request that may work, however Im not sure what other problems it may bring

[steven166]

Because Node.js is a single threaded and event-loop, it makes it difficult to maintain the scope variables across its callbacks and promises (async/await). C# (founded the async/await syntax) uses per request containers to easily maintain the scope of the request.

It will also make it easier to forward request information to downstream services: extract for example an opentracing id in a request interceptor, store it in a request scoped container and use it when making a request to a downstream service.

Besides that it will prevent bugs when a programmer stores (accidentally) request variables as a property of a Service. When 2 requests are using that same variable at the 'same time' it will have ugly results. On the developer pc it will work fine, but not in production...

I don't really see a down side for per request containers. Maybe a solution like I proposed here will be sufficient.

[dvlsg]

Just wanted to chime in with a real-world item that scoped lifetime (per request) services would solve in node.

Say you have a LogService which you want to decorate with per-request information, such as the username of the user making the http call. Something like the following:

class LogService {
  private _identity: Identity;
  public log(msg) {
    console.log(`${this._identity.username}: ${msg}`);
  }
}

With a singleton service, that won't work. Sure, you can pass around the user's Identity to each call to LogService.log(), but that gets super tedious. Essentially clutters up a ton of function interfaces.

You could technically have a singleton which was a factory which makes a new LogService when you call it and provide it with an Identity, but that feels like sidestepping the problem and losing a lot of the benefits that DI containers provide.

If you had a scoped lifetime, which is to say scoped to the context of the http call, it's much easier to construct something like the LogService above, and then you wouldn't have to explicitly track the user's Identity by passing it through each relevant service's functions.

[MichalLytek]

Multiple instances is not the solution. Continuation-Local Storage is. Right now you can use bare req/ctx object to pass request-related data between services. etc. We will think how to implement request-scoped context in the future.

[github-actions]

Stale issue message

[komanton]

@MichalLytek cls is a good decisions. I have already use it instead of passing req object everywhere.

[nickjtch]

Any progress on this issue?