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. :)

[rafaelkallis]

some more thoughts:

custom scopes

A low level api for custom scopes exists actually. ContextIdFactory.create() is the fundamental building block for custom scopes, one only has to provide an abstraction for it that makes sense for a use case.

What smells to me is that the mediator has knowledge about a request, and needs that request to resolve other services in a scope subtree. Perhaps having an abstraction over moduleRef that allows clients to resolve services in a subtree without supplying a contextId would allow for a more elegant solution. A resolve() without a contextId.

[xdave]

Seems like such an abstraction already exists & could be further extended, ie. ExecutionContext?

[rafaelkallis]

No, ExecutionContext is not an abstraction over moduleRef. Let me illustrate with an example:

// scope abstraction, can originate from requests, timers, cron, amqp consumers, etc.
@Injectable()
class Scope {
  // wraps contextId ?
}

// moduleRef abstraction, service locator pattern
@Injectable()
class ServiceLocator {
  private readonly scope: Scope;
  constructor(scope: Scope) {
    this.scope = scope;
  }

  // locate service from current scope, implementation unknown
  getService<T>(typeOrToken: Type<T> | string | symbol): Promise<T>;
}

@Injectable()
class Mediator {
  private readonly serviceLocator: ServiceLocator;
  constructor(serviceLocator: ServiceLocator) {
    this.serviceLocator = serviceLocator;
  }

  async send(request) {
    const handlerType = getRequestHandlerType(request);
    // is it request scoped? is it singleton? Mediator doesn't know
    const handler = await this.serviceLocator.getService(handlerType);
    return await handler.handle(request);
  }
}

So with such an abstraction we can address the mediator not having to worry about scope. The implementation you have suggested (try catch) is a candidate solution, but preferably we use an alternative that does not rely on try catch for control flow.

[xdave]

@rafaelkallis I like this idea.

[rafaelkallis]

I implemented the following locally and having no problems so far:

@Injectable()
export class ServiceLocator {
  private readonly moduleRef: ModuleRef;
  private readonly contextId: ContextId;

  public constructor(moduleRef: ModuleRef, @Inject(REQUEST) request: Request) {
    this.moduleRef = moduleRef;
    this.contextId = ContextIdFactory.getByRequest(request);
  }

  public async getService<T>(type: Abstract<T> | Type<T>): Promise<T> {
    return this.moduleRef.resolve(type as any, this.contextId, {
      strict: false,
    });
  }
}

Note that it lives in the request scope (works for my mediator use case)

[xdave]

FYI you can avoid manually assigning variables in the constructor with the following...

Plus all class properties & methods are public by default, there's no need for the public visibility specifier.

@Injectable()
export class ServiceLocator {
  private readonly contextId: ContextId;

  constructor(
    private readonly moduleRef: ModuleRef,
    @Inject(REQUEST) request: Request,
  ) {
    this.contextId = ContextIdFactory.getByRequest(request);
  }

 async getService<T>(type: Abstract<T> | Type<T>): Promise<T> {
    return this.moduleRef.resolve(type as any, this.contextId, {
      strict: false,
    });
  }
}

[xdave]

So, how do I make this work for 1:n event handlers?

[rafaelkallis]

based on earlier code:

@Injectable()
export class Mediator {
  /* ... */
  public async publish(notification: Notification): Promise<void> {
    const notificationType = notification.constructor as Type<Notification>;
    const notificationHandlerTypes = getNotificationHandlerTypes(notificationType);
    for (const notificationHandlerType of notificationHandlerTypes) {
      const handler = await this.serviceLocator.getService(notificationHandlerType);
      if (!handler) { continue; }
      await handler.handle(notification);
    }
  }
}

getNotificationHandlerTypes() is left as exercise for the reader ;) (read my previous posts to get an idea of how it looks like)

[diego-rangel]

@kamilmysliwiec Any updates ? Thanks!

[xdave]

So, I started using the @nestjs/microservices package instead of this package to get what I want.

• The controllers of the microservice can serve the same purpose as Command/Query handlers -- they are (by design) request-scoped.
• I have a wrapper class around client proxy that copies request-context-like stuff to each Command/Query
• The AggregateRoot domain models have mutation methods that accept a "reason" parameter, which is the Command of the mutation. This contains all of the request context which apply() then copies to events before they're published (ie. emitted using the client proxy).
• Repositories pull the events directly out of the AggregateRoot using getUncommittedEvents() and publish from there
• I had to re-implement the Sagas functionality from @nestjs/cqrs so that I can fan-out events inside of each service and make them send commands using the client proxy -- allowing me to also copy request context to the resulting commands from the events -- thereby establishing a complete audit chain for any given series of related operations.
• Can use NATS for the Transport, which allows load-balancing the queues so I can scale up any of the services if need be.

It was a lot to figure out, but it's way cleaner, and I never have a static/request/transient injection problem now.

[lukaszwilisowski]

@xdave can you share the code for the solution you just described? I am thinking about advantages / drawbacks of this approach compared to custom cqrs based on RxJS and Mediator pattern (@rafaelkallis approach).

@kamilmysliwiec can you shortly describe the rationale for making CQRS handlers singleton-scoped?

Current cqrs module in nest.js is great, but has its limitations. My initial conclusions are the following:

1. You cannot inject request-scoped dependencies into handlers. What you can do instead is to inject them in a controller and then pass them onto the handler.

For example: to get currently authenticated user from DB - instead of this:

  @Get('current')
  async getCurrentUser(): Promise<IUser> {
    return this.queryBus.execute(new GetCurrentUserQuery());
  }

you can do this:

  @Get('current')
  async getCurrentUser(@CurrentUser() user: ICurrentUser): Promise<IUser> {
    return this.queryBus.execute(new GetUserByIdQuery(user.id));
  }

The downsides of this are redundancy and lesser security. If you want to prevent developers from making simple mistakes, you would like to differentiate between functions invoked in current-user-context vs administrative-context. It is generally better to inject context into handler directly (final execution point).

2. As far as I understand you cannot add guards to CQRS and queries handlers. This would be useful because some queries call other queries and granular approach to security is much safer. The workaround is to use standard controller guards, but we lose modularity features (other modules can extend functionality of existing routes), leading to higher redundancy.

3. You cannot easily extend the logic of commandBus and queryBus. It would be nice to have general commandHandler and queryHandler that can be decorated the same way as I commented here: https://github.com/nestjs/nest/issues/4786. This would form a nice abstraction layer: to be able to add custom logging / authentication / other logic to all handlers (but not controllers). Unless I am missing something, in which case feel free to correct me.

So now I am more inclined to try the microservice architecture (although it seems a little bit far-fetched for my requirements) and if not, consider reimplementing the whole CQRS module.

Let me know what you think.

[xdave]

@lukaszwilisowski I ended up re-implementing/adding everything I needed from the CqrsModule:

• ICommand, IQuery, IEvent, ICommandHandler, IQueryHandler
• AggregateRoot
• Saga processing

I may not have needed to do this, but I wanted to customize things a lot further than you generally can with the CqrsModule. I add things like:

• hostname,
• clientIp,
• correlationId,
• causationId, and
• timestamp

to each Command, Query, and Event as they're created, because my use cases require it for things like multi-tenancy and auditing. The saga processor needed to be re-implemented to handle copying the request context stuff over to the commands, to maintain the audit trail.

Here's some example code: https://github.com/xdave/cqrs-es-test

[xdave]

With the above (if it is split out into multiple services), it should be possible to do things like:

• verify a JWT using another service in an API Guard
• attach the current userId to the Request Context
• forward the context (ie, the command/query) to the respective service it's for
• use an interceptor to query the user by id and attach it to the RPC Context
• pull it in using a decorator directly on the ICommandHandler

But, for the use case you stated above, most of this dance wouldn't be necessary, because we'd have a userId directly on the command/query/event in this case.

[lukaszwilisowski]

Thanks, I will check it out and let you know my thoughts.

[Farenheith]

@kamilmysliwiec I'm also in need of such feature for the command handler and, regardless I could do some workarounds to achieve what I needed, I thought that would be nice for the command bus to support request scoped handlers.

So, I forked the repo and tried to change it myself. https://github.com/nestjs/cqrs/compare/master...Codibre:master

The solution already worked locally :) As soon as I get some time I'll complete the change with unit tests and send a PR for the project!

In the meantime, with anyone can take a look at the change to give some opinions it'd greatly help me with my anxiety lol

[pedrosodre]

I'm working together with @Farenheith on the changes to accept handlers with request scope, I think it should be ready soon :)

[koolamusic]

@pedrosodre thanks for taking this up, really appreciated.

[xdave]

@Farenheith @pedrosodre excellent!

I tried this out, and it appears to resolve handlers that are request-scoped; however, when doing so, the handlers do not resolve their dependencies. They are undefined.

[xdave]

@Farenheith @pedrosodre OK, if I put { command: SomeCommand, scope: Scope.REQUEST } in the @CommandHandler() decorator, the dependencies resolve, but I cannot @Inject(REQUEST) in the same class -- the request object is undefined.

[Farenheith]

@xdave thanks for taking your time to test our try! I added some unit test just to assure the request scope and singleton injection for handlers, and everything is working as we expected! You can take a look at the tests in the command-bus.spec.ts and query-bus.spec.ts files, but I can already say to you that there are three ways to inject a request scope handler that I know of:

• Through provider specification: Take the handler

  @QueryHandler(MyQuery)
  class MyHandler implements IQueryHandler<MyQuery> {
  ... some code ...
  }

  And the provider:

  {
  provide: MyHandler,
  useClass: MyHandler,
  scope: Scope.REQUEST,
  }

• Through Injectable decorator:

  @QueryHandler(MyQuery)
  @Injectable({ scope: Scope.REQUEST })
  class MyHandler implements IQueryHandler<MyQuery> {
  ... some code ...
  }

• Through CommandHandler/QueryHandler/EventHandler decorator, that @pedrosodre coded in our fork:

  @QueryHandler({ command: MyQuery, scope: Scope.REQUEST })
  class MyHandler implements IQueryHandler<MyQuery> {
  ... some code ...
  }

I let some example running in the tests I talked above the three options , you can take a look :) About the request scope object injected in the handler, the only scenario I found where this is a problem is when the Handler is singleton, but I think this is an expected behavior. Have you tested these scenarios in our fork? If so, Take a look at the tests to see if there is something different from what you tried out and let me know.

Also, try it in our updated fork, because in the first version I uploaded I did have this injection problem

[xdave]

@Farenheith hi, thanks for the update. I have pulled the latest from that fork, and I am still having the same problem. The request is undefined when injected into the handler.

[Farenheith]

@xdave I need to see your code to understand what the problem is. Could you upload a repo in github simulating the issue?

[xdave]

@Farenheith Here's a repo that demonstrates the problem: https://github.com/xdave/test-scoped-cqrs I have installed your fork by cloning it into a separate directory, and running: npm link ../cqrs @nestjs/cqrs in this repo dir.

[Farenheith]

@xdave nice, I'll try it right away. Thank you