-
Notifications
You must be signed in to change notification settings - Fork 150
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
Command handler dependency injection scope #60
Comments
So basically, the problem is that every |
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. |
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? |
@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('*') }):
Then you can just pull in http context anywhere else in your app and grab the state. It's stored in the current 'continuation'. |
@kamilmysliwiec: Could we support the request-scoped for command handler ? Our logging service is request-scoped, so can't use in in command handler :(( |
Let me reopen this. I'll think about what we can do as soon as I find a bit more time. |
@kamilmysliwiec : Thank for you quick response. I'm a big fan of NestJs 👍 |
Willing to work on this if @kamilmysliwiec thinks the design can be altered without too much performance issue |
@kamilmysliwiec: Is there any official way to resolve/get the instance by scope defined ? Seem the |
Hello @kamilmysliwiec thanks for your job on Nestjs. |
@kamilmysliwiec Any updates ? Thanks! |
waiting for solution. thanks! |
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 |
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. |
@kamilmysliwiec Any updates ? Thanks! |
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 The request handler (injectable) is resolved during runtime by using Here a code snippet to illustrate the solution.
// 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 |
@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 :) |
@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 We can start like this (me creating a new repo/library) and if it is useful to nestjs, it can be absorbed by them ( |
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. |
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:
I don't see why any of the above would limit us |
@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. |
@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. |
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. |
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 :) |
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.,
I imagine there is a new "observable bus" (and subscriptions) for each incoming request. |
What I did in my test yesterday was catch InvalidClassScopeException for |
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 |
@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. |
@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. 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. |
@xdave I had an idea of how to provide what you need. |
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:
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. |
@Farenheith here's what I've come up with as a workaround using stock @nestjs/cqrs: https://github.com/xdave/test-scoped-cqrs |
@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: 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. |
First, I did notice that the use of the
(the above applies to both the 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: The Transient Bus indeed makes this work -- but it does seem misnamed: it's not actually |
@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. |
The call to this.subject$.next(...) is missing in execute() |
@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. |
@xdave thank you, now I saw it! :D It's nice from you to notice it. I'll fix it right away |
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! |
Would be cool if you can take a look on it @kamilmysliwiec :) |
What is the expected date to merge or some feedback about #549? |
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? |
@kamilmysliwiec are there any plans to merge this branch into release? |
Bump! This would be fantastic if it could be merged! |
Bump! This is a really helpful feature. |
Bump! This would be very good! |
Bump! @kamilmysliwiec |
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.
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? |
@unckleg this may interest you https://github.com/xdave/nest-request-context |
@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. |
Bump! @kamilmysliwiec |
I think it should support scoped service. This issue is 2 years old :| |
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. |
@kamilmysliwiec ka |
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 ? 🙏🏽 |
I'm submitting a...
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
What is the motivation / use case for changing the behavior?
Environment
The text was updated successfully, but these errors were encountered: