-
Notifications
You must be signed in to change notification settings - Fork 784
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
[#2242] Correctly support null-identifier and no-event scenarios from Command Handling constructors, Always
, and Create-If-Missing
creation policies
#2248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
During the commit phase of an Aggregate, the identifier is expected to be present. Otherwise, no events or state-stored aggregate can be inserted. However, if the identifier is not present at this point that means the command handler decided not to create the aggregate. As such, no commit action is necessary. Validate this behavior with a test case targeted towards a command handling constructor without publishing any events. #2242
If the AggregateNotFoundException is thrown within the test fixture and the identifier is null, that means the identifier hasn't been set by the command handling constructor. #2242
If the PessimisticLockFactory throws a dedicated exception instead of an IllegalArgumentException than the AlwaysCreateAggregateCommandHandler can deal with this scenario by defining the result as null instead of rethrowing the exception. This covers for the scenario when an ALWAYS creation policy decides not to publish any events or sets state. #2242
smcvb
added
Type: Bug
Use to signal issues that describe a bug within the system.
Priority 1: Must
Highest priority. A release cannot be made if this issue isn’t resolved.
Status: In Progress
Use to signal this issue is actively worked on.
labels
Jun 8, 2022
Except new NullLockIdentifierException instead of the previous IllegalArgumentException #2242
abuijze
requested changes
Jun 8, 2022
messaging/src/main/java/org/axonframework/common/lock/PessimisticLockFactory.java
Show resolved
Hide resolved
...ing/src/main/java/org/axonframework/modelling/command/AggregateAnnotationCommandHandler.java
Outdated
Show resolved
Hide resolved
Instead of throwing a specific exception to handled in the case when the ALWAYS creation policy does not set the aggregateIdentifier, we'll instead construct a NoOpLock. This is safe and specific enough, since the AbstractRepository ensure we do not commit whenever the aggregate identifier is null. #2242
YvonneCeelie
approved these changes
Jun 14, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
abuijze
reviewed
Jun 20, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just one minor comment on a Javadoc.
messaging/src/main/java/org/axonframework/common/lock/PessimisticLockFactory.java
Outdated
Show resolved
Hide resolved
abuijze
approved these changes
Jun 20, 2022
smcvb
added
Status: Resolved
Use to signal that work on this issue is done.
and removed
Status: In Progress
Use to signal this issue is actively worked on.
labels
Jun 20, 2022
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Priority 1: Must
Highest priority. A release cannot be made if this issue isn’t resolved.
Status: Resolved
Use to signal that work on this issue is done.
Type: Bug
Use to signal issues that describe a bug within the system.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is perfectly valid for an aggregate creating command handler to decide not to publish any events, nor if it sets the aggregate identifier.
This scenario did not work for command handling constructors, the Create-if-missing, and Always creation policies.
For constructors, this is a bug that sneaked into the framework as part of release 4.5.3.
The constructor predicament is resolved by not entering the prepare-for-commit phase of the
UnitOfWork
when the aggregate does not contain an identifier.The create-if-missing policy had an issue in the
AggregateTestFixture
, which incorrectly assumed this to be a faulty state.We solved this by checking whether the identifier is null when the aggregate couldn't be found during assertion.
The always policy rethrew the
IllegalStateException
of thePessimisticLockFactory
that tried to obtain a lock for anull
identifier.By throwing a dedicated exception (the
NullLockIdentifierException
), the always creation policy command handler can react to this accordingly, assuming the scenario the identifier isn't set.Note that issue #2242 only references the Create-If-Missing policy.
Nonetheless, it will resolve #2242 accordingly.