Command handler dependency injection scope

[wergimity]

I'm submitting a...

[ ] Regression 
[x ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Command handler dependencies are being injected on module init. But since NestJS v6 there are provider scopes. And currently it's not possible to get request object in command handler.

Expected behavior

Command handlers should be using dependency injection container when executing commands. And not when command handlers are being registered.

Minimal reproduction of the problem with instructions

@CommandHandler(Test)
export class TestHandler implements ICommandHandler<Test> {
  @Inject(REQUEST)
  private request!: Request;

  public async execute(command: Test): Promise<any> {
    console.log(this.request); // undefined
  }
}

What is the motivation / use case for changing the behavior?

Environment

Nest version: 6.0.4

 
For Tooling issues:
- Node version: v10.15.0  
- Platform: Linux 

Others:

[kamilmysliwiec]

So basically, the problem is that every CqrsModule provider is statically scoped which mean that they cannot depend on request-scoped providers. Once you define your command handler as a request-scoped provider, either CommandBus or QueryBus won't be able to reference it. This is not an issue, but rather a design decision that sits behind the entire module.

[xdave]

I just came across this problem, as I needed to inject db repositories into the command handlers which are configured for request-scoped multi-tenancy.

The solution was to pass a repository factory as an argument to the command constructors when executing the commands from controllers or services.

Wish there was an easier way.

[grexlort]

Same problem here, were using request-scope for multi-tenancy (per tenant database connection injection) and CQRS module in this case will not work for us?

[xdave]

@grexlort @kamilmysliwiec started using https://www.npmjs.com/package/express-http-context for this. Uses something called "continuation-local storage" (kind of like thread-local storage, but for nodejs) to keep track of some state during a request.

I configure AppModule to (configure(consumer) { consumer.apply(...).forRoutes('*') }):

1. Load express-http-context middleware
2. Load my middleware that selects the tenant and sets the http context for that tenant
3. Connect to database, etc

Then you can just pull in http context anywhere else in your app and grab the state. It's stored in the current 'continuation'.

[cuongphamcxa]

@kamilmysliwiec: Could we support the request-scoped for command handler ? Our logging service is request-scoped, so can't use in in command handler :((

[kamilmysliwiec]

Let me reopen this. I'll think about what we can do as soon as I find a bit more time.

[cuongphamcxa]

@kamilmysliwiec : Thank for you quick response. I'm a big fan of NestJs 👍

[juicycleff]

Willing to work on this if @kamilmysliwiec thinks the design can be altered without too much performance issue

[cuongphamcxa]

@kamilmysliwiec: Is there any official way to resolve/get the instance by scope defined ?
I try to resolve my handler using ModuleRef but it require a contextId. I'm enable to resolve the handler instance by passing an undefined contextId but I can't inject the REQUEST into the handler.

Seem the router-explorer generate the context id but not store it in request[REQUEST_CONTEXT_ID]

[vinzcoco]

Hello @kamilmysliwiec thanks for your job on Nestjs.
Do you have a workaround to this problem?

[unckleg]

@kamilmysliwiec Any updates ? Thanks!

[shuramita]

waiting for solution. thanks!

[xdave]

In order for this to work, each command, query, and event handler would have to somehow know what request initiated it after a long running process (for example) even long after the controller has returned and the request is complete. This would require attaching that context to each command, query, and event or at least a request id that it can lookup later.

It seems like most people in the cqrs/es community tend to do this manually in the .NET world by just passing around a context object with some correlation identifiers and other useful info like what user initiated the request, a correlationId, timestamp, etc

[xdave]

What I'm thinking about doing is creating a custom parameter decorator with this info and then having another parameter on all of my command, query, and event constructors that takes this context. It would run, theoretically, after the middleware, interceptors, and guards to capture things like tenant db selection, auth, creating a correlationId, etc. Then I would store this context inside of all of the events that are persisted so that a long running, distributed transaction can complete properly.

[shajeeck]

@kamilmysliwiec Any updates ? Thanks!

[rafaelkallis]

for those interested, I have implemented a solution for this from scratch in a project of mine (closed source for the public, willing give access to @kamilmysliwiec if he wants to). The basic idea is to have a static map from Type<Request> to Type<RequestHandler>[].

The request handler (injectable) is resolved during runtime by using moduleRef.resolve(), rather than being registered during a module lifecycle event.

Here a code snippet to illustrate the solution.

Request is analogous to cqrs Command, Mediator is analogous to cqrs CommandBus.

// client

class Ping extends Request<void> {}

@RequestHandler(Ping, { scope: Scope.REQUEST })
class RequestScopedPingHandler extends AbstractRequestHandler<Ping, void> {
  constructor(
    private readonly requestScopedDependency: RequestScopedDependency,
  ) {}

  handle(command: Ping): void { /* ... */ }
}

@Injectable({ scope: Scope.REQUEST })
class RequestScopedService {
  constructor(
    private readonly mediator: Mediator,
    private readonly requestScopedDependency: RequestScopedDependency,
  ) {}

  func(): void {
    await mediator.send(new Ping());
  }
}

// request + request handler boilerplate
export abstract class Request<T> {}

export abstract class AbstractRequestHandler<TRequest extends Request<T>, T> {
  public abstract handle(request: TRequest): T | Promise<T>;
}

const staticRequestHandlerMap: Map<Type<Request<unknown>>, Type<AbstractRequestHandler<unknown, Request<unknown>>>[]> = new Map();

export function RequestHandler(requestType: Type<Request<unknown>>, scopeOptions?: ScopeOptions): ClassDecorator {
  return (target: Function): void => {
    if (!(target.prototype instanceof AbstractRequestHandler)) {
      throw new Error(`${target.name} is not a request handler, did you extend ${AbstractRequestHandler.name} ?`);
    } 
    const requestHandlerTypes = staticRequestHandlerMap.get(requestType) || [];
    staticRequestHandlerMap.set(requestType, [...requestHandlerTypes, target as Type<AbstractRequestHandler<unknown, Request<unknown>>>]);

    Injectable(scopeOptions)(target);
  };
}

export function getRequestHandlerType(requestType: Type<Request<unknown>>): Type<AbstractRequestHandler<Request<unknown>, unknown>>[] {
  return staticRequestHandlerMap.get(requestType) || [];
}

// mediator boilerplate
@Injectable()
export class Mediator {
  private readonly moduleRef: ModuleRef;
  private readonly httpRequest: any;

  public constructor(moduleRef: ModuleRef, @Inject(REQUEST) httpRequest: any) {
    this.moduleRef = moduleRef;
    this.httpRequest = httpRequest;
  }

  public async send<T, TRequest extends Request<T>>(request: TRequest): Promise<T> {
    const requestType = request.constructor as Type<TRequest>;
    const contextId = ContextIdFactory.getByRequest(this.httpRequest);
    const requestHandlerTypes = getRequestHandlerType(requestType);
    // there could be multiple handlers statically registered, but only one can be registered in nestjs container
    const resolvedRequestHandlers: AbstractRequestHandler<T, TRequest>[] = [];
    for (const requestHandlerType of requestHandlerTypes) {
      // MAGIC SAUCE
      const resolvedRequestHandler = await this.moduleRef.resolve(requestHandlerType, contextId, { strict: false });
      resolvedRequestHandlers.push(resolvedRequestHandler as AbstractRequestHandler<TRequest, T>);
    }
    if (resolvedRequestHandlers.length === 0) {
      throw new InternalServerErrorException(
        `No request handler registered for ${requestType.name}, did you apply @${RequestHandler.name}(${requestType.name}) to your request handler?`,
      );
    }
    if (resolvedRequestHandlers.length > 1) {
      throw new InternalServerErrorException(`Multiple request handlers registered for ${requestType.name}. Only one request handler can be registered per request.`);
    }
    return resolvedRequestHandlers[0].handle(request);
  }
}

EDIT: forgot to apply @RequestHandler(Ping) to class RequestScopedPingHandler

[xdave]

@rafaelkallis This is awesome! What kind of effort do you think it would take to refactor the existing busses (QueryBus, CommandBus, EventBus) to make it work like this and create a pull request?

If we could quantify the work, I might actually volunteer to help if @kamilmysliwiec thinks this is a good way forward on this issue.

Not sure what kind of 3rd party pkgs that depend on the current behavior would be impacted, either. Any insight is welcome :)

[rafaelkallis]

@xdave I am considering creating a library that exposes an abstract mediator (command bus) class, so that clients can create reusable concrete mediators. I am sure there are codebases beyond the cqrs module that would benefit from this. In fact this pattern is popular in other language stacks (most notably https://github.com/jbogard/MediatR).

In the context of @nestjs/cqrs, the CommandBus and QueryBys would be concrete subtypes of the abstract mediator class. The api should be backwards compatible.

We can start like this (me creating a new repo/library) and if it is useful to nestjs, it can be absorbed by them (@nestjs/mediator ?), happy to maintain the codebase as well to be honest.

[xdave]

I think figuring out how events and sagas fit into this would be helpful. While it would be useful to have 1:n request-scoped event handlers, it's probably not possible to scope them by request due to the nature of how the pattern works.

EDIT: I'm thinking about situations where an Event arrives from the wire through an external store or as a result of a timer expiring -- and then being instructed to execute a command inside of a saga or other event handler.

[rafaelkallis]

Event handlers shouldn't be a problem either, they are also part of mediatR actually, under the name "notifications" (https://github.com/jbogard/MediatR/wiki#notifications). I also assume sagas are easy to manage since they also make use of decorators.

I see two differences between event handlers and command/query handlers:

1. return type is void
2. multiple event handlers can be registered per event

I don't see why any of the above would limit us

[xdave]

@rafaelkallis I guess it wouldn't limit us, just curious to see what happens if a normally request-scoped command is executed by a saga or event handler outside of the request pipeline.

[rafaelkallis]

@xdave the answer is very simple: the mediator is request-scoped, therefore any mediator client becomes request-scoped :) so I can imagine sagas will also become request-scoped.

[xdave]

The problem with this is that any provider that is request scoped is automatically garbage collected after the request is complete.

This prevents the Mediator from running continuously in the background to listen for incoming events that may be triggered by outside forces, timers, or from code that is being executed outside of the entire request pipeline.

This becomes a real problem if I want to have an Observable bus. The Observable, being tied to a request-scoped provider, will only live as long as that instance of Mediator exists. In order for the Observable stream to remain open across all requests, it will be locked-in to the request context of the first request that created it until that Observable is "completed" (stream end).

Any new requests that come in while that instance still exists will create new Observables instead of using the first one.

[xdave]

Quite frankly, I'm OK with that, but I'm still not sure having request-scope event/notification listeners is going to be useful.

Prove me wrong :)

[rafaelkallis]

This prevents the Mediator from running continuously in the background to listen for incoming events that may be triggered by outside forces, timers, or from code that is being executed outside of the entire request pipeline.

Correct, but perhaps we could get around this by modifying the mediator implementation to detect the scope and adjust the handler resolution strategy accordingly (e.g., this.httpRequest === null implies scope is singleton, therefore use moduleRef.get(Foo) instead of resolve() ?). I am not sure I like doing this, instead I would prefer the framework would provide a Scoped scope (like in .Net), or custom scopes, so that there can exist other short-lived transactional scopes other than request scopes.

This becomes a real problem if I want to have an Observable bus. The Observable, being tied to a request-scoped provider, will only live as long as that instance of Mediator exists. In order for the Observable stream to remain open across all requests, it will be locked-in to the request context of the first request that created it until that Observable is "completed" (stream end).

I imagine there is a new "observable bus" (and subscriptions) for each incoming request.

[xdave]

What I did in my test yesterday was catch InvalidClassScopeException for resolve() and then use get() in the catch block. This prevents you from having to explicitly use { scope: Scope.REQUEST } on the Handlers. As for custom scopes, this is a question for @kamilmysliwiec -- would be nice to get an idea what he thinks of all of this. :)

[xdave]

@Farenheith

Manually passing request-related information along with the command/query is exactly what I wanted to avoid, and defeats the purpose (I think) of doing all of this.

I realize that I should be keeping "expressed-related" things out of my application, but I need to be able to access the request in order to keep track of context that has information FROM the request -- without littering my commands with that information. For example, I need to keep track of the currently selected database in a multi-tenant situation, which means my repositories need to access that USING the request, which the Command handler depends on.

May I suggest, that when you are injecting the handlers, check for REQUEST scope metadata and then do something like the following?:

// Pseudo code
let handlerInstance: ICommandHandler<ICommand>;
if (handlerIsRequestScoped) {
  const req = await moduleRef.resolve(REQUEST, undefined, { strict: false });
  const contextId = ContextIdFactory.getByRequest(req);
  handlerInstance = await moduleRef.resolve(HandlerType, contextId, { strict: false });
} else {
  handlerInstance = moduleRef.get(HandlerType, { strict: false });
}

I don't know if this exact code will work, but from what I can gather, you need to use the contextId to properly scope the handlers.

[Farenheith]

@xdave It can't be done unless the commandBus is request scoped, otherwise it'll not have access to the REQUEST that a request scoped controller received, and we really disagree on that point.
The idea of a request scoped bus may create a heavy overload for each request and the handler must not have access to the express request directly.
To be honest, I realized with your question that our PR don't aim to solve the question made in this thread, but a need we had to have scoped request handlers. I'll remove from the PR to reference to this issue

[Farenheith]

@xdave I know you don't want to pollute your command with request information, but I really think that, in your case, it makes sense to put, not request, but multi-tenant information in it. Any information derived from the entry point transporter must be obtained in the entry point, and the command must have every request information necessary for the command execution.

Using what we're proposing to the nestjs team and having a request-scoped helper class, you can achieve access for multi-tenant information at every point of your application.
For example, with the following line

 async execute(command: SomethingCommand): Promise<any> {
    this.req.tenantId = command.tenantId;
    this.service.do(command.message);
  }

Assuming req is an instance of a request-scoped service and the handler is also request-scoped, you can guarantee access to the tenantId at every request-scoped service with it, without having to pass it through every method of your stack.

[Farenheith]

@xdave I had an idea of how to provide what you need.
Take a look at https://github.com/Farenheith/test-scoped-crqs.
The problem is that you'll be unable to test it through npm link due to the uniqueness of the internal @nestj/core REQUEST_CONTEXT_ID. So, you will need to copy the dist folder of the branch request-command-bus of our fork inside the folder node_modules/@nestjs/crqs.

[xdave]

I do not consider this to be a solution. I have projects with hundreds of commands and queries that do not pass extra information inside.

Instead, we use continuation local storage to keep track of request-specific information starting in middleware, and then used in places like repositories and analytics. It's a hack, but it works. This keeps infrastructure-related details out of our application code.

Additionally, I anticipate that this PR will not be merged upstream in its current form, because of two problems:

1. The contextId of the request itself is not being used to scope the providers,
2. It seems that you have removed usage of the command publisher inside of the command bus

I don't think there is a real solution to this until perhaps the registration of handlers is changed to be more dynamic. Normally, the cqrs module instantiates each handler in the Bootstrap phase, and this requires the busses to be static.

If the registration could occur separately from the busses, the busses could be provided in any scope (with the exception of the event bus).

Events are different, because the events may fire later, after a long running async process and after the request cycle is complete... so, scoping event handlers is not reasonably possible. In this case, it is necessary to manually store some contextual information inside of the events. This can be done trivially by using a custom event publisher.

[xdave]

@Farenheith here's what I've come up with as a workaround using stock @nestjs/cqrs: https://github.com/xdave/test-scoped-cqrs

[Farenheith]

@xdave Actually I did not remove any usage of the publisher from the busses, I took a look at the untouched master and it appears that they're being used in the same places as before. About the contextId, maybe you're right.

The thing is that I see scoped handlers and the request itself as two different things. The request scope concept, in my understanding, refers to the request of a root object to the moduleRef (maybe because I came from inversify), and a scoped handler can make sense even for events when you see it that way. For example:
Assuming a scoped Handler, let's say you have an event/command/query that, when handled, a few classes are injected in the injection chain, with something like this:

Imagine that each class that uses the repository may query the same information, you don't want to do a new database roundtrip for each one, but you also don't want to cache it in a redis or something because others request must no use this cache. If your handler is scoped, it's quite simple to solve this: you just need to store the promise of the first call the repository got and respond it for the other ones. When the request ends, everything dies and you're good to go.
You may also want to collect different statistics information at many points of your chain to log it at the end of your request, again, with a scoped handler, you can just inject a collector at each point you want, collect it and log it all at once in the end. It's quite simple and quite decoupled to do so.

Even so, I see the value in your need and I did a transient command bus that would work for you, so I'd like for you take a look if you can (I think you didn't saw my last message). I didn't put it in the PR branch because I see it as a different problem from the one I was aiming at the beginning, as I realized talking to you, that my need was similar but no the same of this issue and I misunderstood it at the beginning.

[xdave]

@Farenheith

First, I did notice that the use of the ICommandPublisher and IQueryPublisher are indeed missing -- see:

1. https://github.com/nestjs/cqrs/pull/549/files#diff-ba3b51a6452723e5c5932802cf32882082f3f0c0acbae54cd4e9f3ae4e5314b6L44
2. https://github.com/nestjs/cqrs/pull/549/files#diff-077f9c78eb3526c090ed295ae929fd52c7a9348c6b858566daafab0dc2bc70e3L51

(the above applies to both the feature-request-scope-handlers and master branches)

Second, I like the idea of the Transient bus, this does give the desired effect. A common pattern in (private) projects I maintain is (simplified version):

Nest starts-up/boostrap -> Master DB Connection established, and at some point later...

A GET/POST/PUT, etc request is received:
-> Tenant lookup/selection from Master DB using Request information -> Establish Tenant DB Connection
-> Command/Query -> Handler -> Repository uses Tenant DB to read/write data

The Transient Bus indeed makes this work -- but it does seem misnamed: it's not actually Scope.TRANSIENT because it directly depends on the REQUEST, which is of Scope.REQUEST.

[Farenheith]

@xdate, man, I really don't see where there is this missing Publisher you're pointing out. Every reference to publish, publishAll and the Publishers are identical.
About the name TransientCommandBus I think you're right and I changed it.
But thank you for your feedback :). Let's hope the nestjs team take a look in the PR @pedrosodre and I did and, if they accept it (with fix requests or not), then the idea of a request scope command bus can also be implemented. Or maybe they think in a better solution.

[xdave]

The call to this.subject$.next(...) is missing in execute()

[xdave]

@Farenheith See the lines I highlighted in the links above. Line 44 on the left-hand-side for the command bus, and line 51 on the left hand side for the query bus.

[Farenheith]

@xdave thank you, now I saw it! :D It's nice from you to notice it. I'll fix it right away

[altick]

When could we expect this to be merged? I'm already using the fork and it's working pretty well. Btw thank you @Farenheith and @xdave for the great job!

[pedrosodre]

Would be cool if you can take a look on it @kamilmysliwiec :)

[viniciuslk]

What is the expected date to merge or some feedback about #549?

[joseaquino]

Friendly bump on this request, it would definitely bring more flexibility to the CQRS handlers and the PR looks to be working great 😀 any possibility of merging it in @kamilmysliwiec?

[k-jarzyna]

@kamilmysliwiec are there any plans to merge this branch into release?

[flashpaper12]

Bump! This would be fantastic if it could be merged!

[renanz]

Bump! This is a really helpful feature.

[skript53]

Bump! This would be very good!

[unckleg]

Bump! @kamilmysliwiec

[unckleg]

The bump I've posted 2+ years ago above was due to same challenge I've had earlier. Multi-Teancy + Dynamic DB Runtime Connecting (Implementing Microservices based on CQRS + Nats + MongoDB)

We managed to overcome these limitations by monkey-patching here and there some stuff to make Nest.js DI Container happy, path of implementation was far from easy but we came with an "Elegant solution".

Must mention digging into Nest.js internals/core we released bunch of hacks, kind of dirty implementations that we/you cannot see when you are just "Consumer" of the framework, suggesting for those who are interested and have time to go through the Nest.js core codebase 🗡️

That being said, I'm posting some bullet-points how we plumbed up everything to work with CQRS.

• Scope.Request: WE DON'T USE IT anywhere NOR WE CAN, NOR WE WILL in the future since we don't wont to trade App performance for our goal because of poor IoC implementation and app behaviour under the hood.
• We came up with using/implementing our global interceptor for all microservices that Intercepts consumer Nats.io events
  • There we are using node async-hooks for context setting/switching (tenant-id, requestId)
  • Created our "DBConnectionManager" that is retrieving tenant-id and connecting tenant to database if one is not already being established by the same.
  • Monkey-patched all dependencies that are using Nest.js InjectModel decorator/method in our Query/Command handlers in how we called it "TenantAwareModule" by providing mock Models as Nest.js providers so we can bypass STATIC flagged modules and their IoC/DI during compile-time. Now we have mocks that are nothing than pure Proxy classes that are calling our DBConnectionManager to retrieve model/connection during runtime (Nats.io message retrieved/interceptor) by getting context data with async-hooks mentioned above.

You can get some inspiration from here nestjs-context-poc but as I said we are using our global interceptor (Nothing related to Nest.js interceptors) instead of serialization/deserialization implemented in that repo.

Will post more info about our findings.

And for now we are not planning to use CQRS + DI Scope.Request feature once there is one in Nest.js core. Why?
My answer to that is that performance of application when you have hundreds of command/handlers would be terrible, also I'm sure that implementation for the issue above and proposed solutions will never arrive in that shape because core team is aware of what I've mentioned as performance issues above, and that's why for 2+ years there has been no solution to this.

[xdave]

@unckleg this may interest you https://github.com/xdave/nest-request-context

[unckleg]

@xdave Hi 👋

If you read my comment carefully you will see that we ended up with the same solution as in your repo:

"As a solution, this uses node's AsyncLocalStorage from async_hooks to wrap the class in a Proxy that allows you to set/get fields on the class that are scoped any way that you want. Generally speaking, you'll want it scoped to the current Request."

We are using cls-hooked 👀🙂

I'm just sorry for the lost time implementing and trying to get everything working, on the other hand there is satisfaction again because as I see we are on the same page.

[UncleVic]

Bump! @kamilmysliwiec

[redplane]

I think it should support scoped service.
Passing the request information to command & queries is not a good idea :(.

This issue is 2 years old :|

[9hnd5]

Check it out https://www.npmjs.com/package/nestjs-mediator I hope this is suitable for you. This package support injection of any provider with a scope request.

[huuhoa0506]

@kamilmysliwiec ka
I hope this issue will be resolved soon. It is really helpful for everyone.

[koblaymerve]

Is there any update about this issue? Looks like there is a PR for it already. Could you please give us an update, on when the PR merge @kamilmysliwiec ? 🙏🏽