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

[#2689] Support Snapshotting for Polymorphic Aggregates #2753

Merged
merged 7 commits into from Jun 16, 2023

Conversation

smcvb
Copy link
Member

@smcvb smcvb commented Jun 15, 2023

This pull request adjust the SpringPrototypeAggregateFactory to expect a fixed aggregateType instead of retrieving it from the ApplicationContext.
This is necessary as abstract classes, as we recommend for the root of a polymorphic aggregate, and due to this, they are simply not a part of the ApplicationContext.

This causes the returned type from AggregateFactory#getAggregateType to be faulty for polymorphic aggregates.
The only spot where this hurts is during snapshot creation; hence meriting this fix.

Combined with the SpringPrototypeAggregateFactory, we need to ensure the SpringAggregateLookup correctly constructs said instance without dropping the root type name.

In doing the above, this pull request resolves #2689.

Add test cases for polymorphic aggregate snapshotting, both during
aggregate construction and subsequent command handling. The former
inserts the aggregate type based on the model, whereas the latter
retrieves the type from a domain event. Due to this, the paths are
slightly different.

#2689
Whenever we're dealing with a polymorphic aggregate, the provided
prototypeBeanName can not reference any existing bean. This stems from
the point that root aggregate types are intended to be abstract, causing
 them not to end up in the Application Context. Due to this, polymorphic
 aggregate snapshotting currently defaults to the first concrete class,
 resulting in incorrect aggregate types in the snapshot events.
 Furthermore, it causes snapshot creation during aggregate creation to
 simply not work.

#2689
The approach towards defining the root/top aggregate name should change
to accommodate for the fact an abstract @aggregate annotated root class
does not occur in the ApplicationContext. Due to this, the aggregate
hierarchy is essentially wrong. We should instead default to a
reasonable root aggregate bean name for our own purposes, and provide
this name to the SpringPrototypeAggregateFactory

#2689
@smcvb smcvb added Type: Feature Use to signal an issue is completely new to the project. Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: In Progress Use to signal this issue is actively worked on. labels Jun 15, 2023
@smcvb smcvb added this to the Release 4.8.0 milestone Jun 15, 2023
@smcvb smcvb requested a review from a team June 15, 2023 15:34
@smcvb smcvb self-assigned this Jun 15, 2023
@smcvb smcvb requested review from gklijs and CodeDrivenMitch and removed request for a team June 15, 2023 15:34
Drop subtype prototype bean validation, as there's no guarantee all
subtypes are annotated with @aggregate. Users are only inclined to mark
the root and children of a polymorphic aggregate, so nodes in the middle
 are by nature not prototype beans. Furthermore, open post-processing a
 bean name for the root type should be returned too. To that end we need
  to check whether the provided aggregate (to post-process) equals the
  configured aggregateType. When true, we return the prototypeBeanName.
When false, the type should by a subtype by definition.

#2689
Although the chances are extremely slim users are working with the
deprecated SpringAxonAutoConfigurer directly, it should support the
adjusted factory construction regardless.

#2689
The SpringPrototypeAggregateFactoryTest deserves an update, as it is
based on old ideas of aggregate prototype bean wiring (with a xml file,
 for example). Adjust it so that it mirrors how Axon's Spring Boot
 autoconfiguration works, without relying on Spring Boot.

#2689
@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

80.6% 80.6% Coverage
0.0% 0.0% Duplication

Clean test case by using isTrue/isFalse i.o. isEqualTo(true)/isEqualTo
(false)

#2680
Copy link
Member

@CodeDrivenMitch CodeDrivenMitch left a 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

@smcvb smcvb merged commit a978a1a into master Jun 16, 2023
5 of 6 checks passed
@smcvb smcvb deleted the feature/2689-polymorphic-aggregate-snapshot-support branch June 16, 2023 13:46
@smcvb 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 16, 2023
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: Feature Use to signal an issue is completely new to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce Polymorphic Aggregate Snapshotting auto-configuration
2 participants