[#29] Feature/aggregate with immutable identifier

[zambrovski]

Just to make the development visible to AxonIQ guys, see #29

Implemented :

• AggregateFactory (in kotlin module)
• AggregateIdentifierConverter (in kotlin module)

This can be used to manually configure Aggregates.

• AggregateRepository (in kotlin module)
• SpringConfiguration for auto configuration (in kotlin-autoconfigure module)

This is used to detect the Aggregates automatically and configure them using the AggregateRepository, AggregateFactory and AggregateIdentifierConverter.

• Example Project (in example module) with two aggregates (identifier: UUID and custom data class)

[zambrovski]

Ok, so I merged the changes from master in to this branch, which made it a lot easier!

Please have a look on the solution, even if WIP. Currently, the tests for the spring-autoconfigure are missing and the starter is empty (just passing the dependency, but not bundling it together with other deps).

Still, the provided solution allows to use data classes with immutable identifier as aggregates, which is actually not the first step in the Scala challenge we had with Jan-Hendrik but its fulfillment. The manual configuration of such an aggregate is inside the example code, but is currently commented out.

In addition (Jan-Hendrik don't have it at all), I implemented automatic configuration of such aggregates, their aggregate factories and aggregate repository (including the identifier converter) and put it into a spring configuration. Again, check the example project for this, especially BankAccount and BankAccountAdvanced having the configurations with converters, the service sending commands, the aggregate itself.

You can actually also run the example application and will see the stuff working.

I'd like to get any feedback from you guys, before I implement all JUnit tests for the autoconfigure module and finalize all this.

[sandjelkovic]

Thank you for this PR Simon, this looks great! In the next days, I'll go through the whole PR and do the initial review and provide you with needed feedback.

In the meantime, I also changed set this as a Draft PR. Feel free to remove the draft status whenever it's ready.

[smcvb]

Obviously it's a WIP, but regardless I still have to much questions to approve this as it is.

Added, I feel things like the AggregateIdentifierConverter and the entire Spring Boot auto configuration are, although nice, not needed for a PR about a new AggregateFactory. I'd rather see those back in a different PR so we keep focus on what's aimed for with feature #29.

[zambrovski]

@smcvb Thank you for such a comprehensive feedback, I'll answer change the PR accordingly. I'll need some time for this...

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

[CLAassistant]

All committers have signed the CLA.

[zambrovski]

@smcvb What do you think about this PR?
Should I try to separate it in two (one for the Aggregate Factory only) and the second one for the Spring / Spring-Auto-Config support?

[smcvb]

@zambrovski, honestly, I thought a lot about this issue, together with @sandjelkovic too.
We've just been too swamped with looking back at the last months.
Furthermore, I'd instead not make excuses, but I don't know how to put this otherwise...my apologies.

To come around to our idea on this PR, though, let me break it down:

• The sample is excellent, but we'd rather see a different pull request for this to minimize the scope of the pull request
• We are not overly convinced on the dedicated annotation (read: @AggregateWithImmutableIdentifier) to automate this process. On any note, it's an upgrade from the original idea, so I think it might come out better in a separate PR as well.
• To further separate and simplify the pull request, it indeed wouldn't hurt to extract the Spring Boot Configuration too.
• Lastly, and most importantly maybe: I don't see why this is a dedicated solution for the Kotlin Extension. Sure, Kotlin makes immutability easier, but several components in this pull request have merit within Axon Framework instead of this Extension

So, that's my thought on this PR.
Let me know whether this makes sense in your book, @zambrovski.

And, as always, we appreciate the effort you've put in so far!

[zambrovski]

Great to get some movement in it...

1. Fully understandable - let slice it into several smaller features: example, spring-boot config...
2. Let us decide something on @Aggregate vs. @AggregateWithImmutableIdentifier issue. I think this will become clearer if discussed alone
3. If I understand you correctly, we'll move AggregateFactory which is capable of working with Aggregates taking the aggregate identifier as a constructor parameter to Axon Framework then? This is actually very interesting idea.. This kind of immutability is even possible in Java indeed - making it a required final field.

Ok - I'll try to slice the original issue to multiple issues and handle it then...

By the way - I personally don't like long discussions in issues. GH offers "discussions" for this... Is it worth to activate those to run those code-related discussions not in PRs or in Issues?

Thank you for the answer, I'll try to finish Kafka Upcasters first and then will switch over back to this one....

[smcvb]

Ok - I'll try to slice the original issue to multiple issues and handle it then...

Awesome, happy to hear your replies @zambrovski.

Let us decide something on @aggregate vs. @AggregateWithImmutableIdentifier issue. I think this will become clearer if discussed alone

Agreed, a form of discussion on this would be good.
Speaking about that:

By the way - I personally don't like long discussions in issues. GH offers "discussions" for this... Is it worth to activate those to run those code-related discussions not in PRs or in Issues?

I've seen it come by, but haven't used it yet. We are internally discussing the idea though.
I'll keep you posted, as always.

Thank you for the answer, I'll try to finish Kafka Upcasters first and then will switch over back to this one....

Great! I'll see the adjusted PR's flying by as soon as your time allows it. :-)

[smcvb]

Removing the milestone as this is still under discussion, while the release is intended to come soon. Will add this to the following release milestone for further discussions.