cqrs icon indicating copy to clipboard operation
cqrs copied to clipboard

feat: add command interceptors

Open V3RON opened this issue 1 year ago • 3 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [x] Feature
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Other... Please describe:

What is the current behavior?

None. It's not possible to intercept commands. Things like logging must be handled by extension of CommandBus class in the application code.

What is the new behavior?

Additional actions can be executed both before and after the command is handled. This feature enhances the flexibility and extensibility of command processing. For instance, one can implement ValidationCommandInterceptor, run validation on commands.

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

V3RON avatar Feb 01 '24 15:02 V3RON

Apologies for the delay in reviewing this PR, and thank you for your contribution!

I just wanted to confirm that I haven’t missed anything here—wouldn't it be simpler to create your own abstraction over the CommandBus? For example:

@Injectable()
class MyCommandBus {
  constructor(private readonly commandBus: CommandBus) {}

  async execute<T extends ICommand>(command: T): Promise<T> {
    console.log('Before command execution');
    const result = await this.commandBus.execute(command);
    console.log('After command execution');
    return result;
  }
}

I'm a bit hesitant about introducing another abstraction here. If we merge this PR, we’d likely need to add similar annotations for queries and possibly even events—introducing a total of three new annotations to achieve something that appears manageable with the approach above.

Please let me know if there’s something I might be overlooking—whether it’s something currently too complex, requires too much overhead, or isn’t feasible at the moment but would be resolved by merging this PR.

Thank you again!

kamilmysliwiec avatar Dec 03 '24 11:12 kamilmysliwiec

@kamilmysliwiec the problem is that the bus has no middleware system allowing developers to perform activity around command execution (like transaction management). Every bus implementation in others languages (PHP/Messenger, C#/Mediatr, Java/Pipelinr) has this kind of mechanism. Related to #1865 .

The Observable isn't enough to implement this capability.

Of course a simple solution would be to wrap the CommandBus, but this should be a default capability of any Message bus implementation.

ancyrweb avatar Jan 26 '25 04:01 ancyrweb

I completely agree with you, @ancyrweb.

Allowing middleware to be set on query, command, and event buses would be a significant improvement for this library. The event bus, in particular, is not very extensible due to the current implementation with Observable. This would address many needs (logging, error handling, transactions) without requiring direct implementations within the library itself or systematically extending buses classes. For example, the unhandledExceptionBus could be managed externally through an interceptor.

I'm a bit hesitant about introducing another abstraction here. If we merge this PR, we’d likely need to add similar annotations for queries and possibly even events—introducing a total of three new annotations to achieve something that appears manageable with the approach above.

I agree, this should be done on 3 buses (event/query/command) for consistency

lucas-gregoire avatar Feb 15 '25 19:02 lucas-gregoire