Skip to content
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

No error/warning for duplicate command handler #1756

Closed
azzazzel opened this issue Apr 13, 2021 · 9 comments
Closed

No error/warning for duplicate command handler #1756

azzazzel opened this issue Apr 13, 2021 · 9 comments
Assignees
Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.

Comments

@azzazzel
Copy link
Contributor

Basic information

Steps to reproduce

Create an Aggregate that handles the same command in a constructor and in a method:

@CommandHandler
public MyAggregate(MyCommand myCommand) {
   ...
}

@CommandHandler
public void update(MyCommand myCommand) {
   ...
}

and start the application.

Expected behaviour

The framework prints "A duplicate command handler was found for command ... " error / warning message

Actual behaviour

No information about the duplicate handler

@azzazzel azzazzel added the Type: Bug Use to signal issues that describe a bug within the system. label Apr 13, 2021
@azzazzel azzazzel added this to the Release 4.5.1 milestone Apr 13, 2021
@smcvb smcvb added the Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. label Apr 13, 2021
@smcvb smcvb modified the milestones: Release 4.5.1, Release 4.5.2 May 12, 2021
@smcvb smcvb modified the milestones: Release 4.5.2, Release 4.5.3 Jun 14, 2021
@leechedan
Copy link

@smcvb @azzazzel well, is just logging in warn is ok in this scene? or when should open the window like DuplicateCommandHandlerResolver

@smcvb
Copy link
Member

smcvb commented Jul 12, 2021

I'd wager the DuplicateCommandHandlerResolver would be an ideal solution in this case too.
The default instance would indeed log, as throwing an exception out of the dark right now is definitely a breaking change.

@leechedan
Copy link

I'd wager the DuplicateCommandHandlerResolver would be an ideal solution in this case too.
The default instance would indeed log, as throwing an exception out of the dark right now is definitely a breaking change.

well, how about the name of DuplicateCommandHandlingMemberResolver

@smcvb
Copy link
Member

smcvb commented Jul 12, 2021

Although technically the same, conceptually a Command Handler can only exist at one spot. Otherwise, there's no guarantee that there's a single aggregate or component that is in charge of the command.

So the current class is called DuplicateCommandHandlerResolver to closely align with that conceptual point.

Added, renaming DuplicateCommandHandlerResolver to DuplicateCommandHandlingMemberResolver would be a breaking change at this point in time. As such it is not something we can do for Axon version 4.x. As soon as we start with Axon 5 we would be able to do.

@leechedan
Copy link

i mean create a new DuplicateCommandHandlingMemberResolver interface and configure bean in AxonAutoConfiguration

@smcvb
Copy link
Member

smcvb commented Jul 13, 2021

i mean create a new DuplicateCommandHandlingMemberResolver interface and configure bean in AxonAutoConfiguration

Gotcha. I do not think it makes sense to construct an identical class as the DuplicateCommandHandlerResolver. Or, another object that serves the same purpose for that matter.
It's best to reuse DuplicateCommandHandlerResolver as that is exactly the problem that needs to be resolved on the Aggregate.

@smcvb smcvb added the Status: In Progress Use to signal this issue is actively worked on. label Jul 13, 2021
@smcvb
Copy link
Member

smcvb commented Jul 13, 2021

Note that the solution for this problem is threefold:

  1. In the AnnotatedAggregateModel, log if there is another command handler with the same name.
  2. In the AnnotatedAggregateModel, throw an AggregateModellingException if there are two command handlers with the same name and set of parameters (as this does indicate a duplicate).
  3. Expand the test fixtures to validate duplicate command handlers within the Aggregate under test.

So, not direct reuse or copy of the DuplicateCommandHandlerResolver, but a different way to tackle the situation.
This follows from the requirement that an Aggregate can have two Command Handlers for the same command, which can be fine given the parameters are different.
Two (or more) command handlers with the same command name but different resolvable parameters can be a form to switch between different handling solutions based on metadata, for example.
As this is currently supported, that should stay within the framework.

@smcvb smcvb removed the Type: Bug Use to signal issues that describe a bug within the system. label Jul 15, 2021
@smcvb smcvb modified the milestones: Release 4.5.3, Release 4.6.0 Jul 15, 2021
@smcvb smcvb added the Type: Enhancement Use to signal an issue enhances an already existing feature of the project. label Jul 15, 2021
@smcvb
Copy link
Member

smcvb commented Jul 16, 2021

We consider this not as a bug, but as an enhancement over the current solution.
Since we prefer enhancements and features for minor releases, we adjusted the milestone of this issue to 4.6.0.

@smcvb smcvb removed the Status: In Progress Use to signal this issue is actively worked on. label Apr 22, 2022
@CodeDrivenMitch CodeDrivenMitch self-assigned this Apr 25, 2022
@close-label close-label bot added the Status: Resolved Use to signal that work on this issue is done. label Apr 28, 2022
@smcvb smcvb removed this from the Release 4.6.0 milestone Aug 3, 2022
@smcvb smcvb added this to the Release 4.5.10 milestone Aug 3, 2022
@smcvb
Copy link
Member

smcvb commented Aug 3, 2022

Closing this issue as it's resolved in pull request #2207.

@smcvb smcvb closed this as completed Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Development

No branches or pull requests

4 participants