Added an option to create a fixture for a state stored aggregate

[sascha-eisenmann]

Adds an option to explicitly create an AggregateTestFixture for a state stored aggregate.

This enables the usage of givenCommands() in connection with a constructor handling the command for state stored aggregates.

P.S. Since this is my first PR to this repository, I would be grateful for any feedback or comments.

[CLAassistant]

All committers have signed the CLA.

[smcvb]

Hey @sascha-eisenmann, happy to see your first contribution!
I was wondering, have you tried doing the following to achieve the same:

fixture.givenState(() -> new SampleAggregate())
         .andGivenCommands(...)

I understand your solution allows direct usage of the givenCommands() method, which definitely has value on its own.
If we would provide something like this, I'd more so see it as an additional method like useStateStorage() instead of providing a parameter to the constructor.
This would resemble this method towards all the other registration options the fixture provides, which would be used inside a setUp() method any how.

Well, let me know what you think, and whether you've used the andGivenCommands option too!

[sascha-eisenmann]

Hi @smcvb,

I already tried the combination of givenState() and andGivenCommands().
For me this results in the following Exception, because I want to use the constructor as command handler.

java.lang.IllegalStateException: Creating an Aggregate while one is already stored. Test fixtures do not allow multiple instances to be stored.

I could probably use the command handling constructor in the givenState() method, but then I couldn't use the ParameterResolver for injecting services into the command handler.

It would be possible, but it would not be as nice as it is for event sourced aggregates. :)

I really like the idea of using an additional method like useStateStorage() for the configuration, so I changed my PR accordingly.

Let me know what you think! 

[smcvb]

Hi @smcvb,

I already tried the combination of givenState() and andGivenCommands().
For me this results in the following Exception, because I want to use the constructor as command handler.

java.lang.IllegalStateException: Creating an Aggregate while one is already stored. Test fixtures do not allow multiple instances to be stored.

I could probably use the command handling constructor in the givenState() method, but then I couldn't use the ParameterResolver for injecting services into the command handler.

It would be possible, but it would not be as nice as it is for event sourced aggregates. :)

I really like the idea of using an additional method like useStateStorage() for the configuration, so I changed my PR accordingly.

Let me know what you think!

Thanks for this reply @sascha-eisenmann.
Given your explanation, I agree to have a method to define a state-stored aggregate is used, is important towards the ease-of-use of the AggregateTestFixture.
That without this you're incapable of using Axon's ParameterResolvers on constructor command handlers in such aggregates is simply a missed.

I've left a comment on your changes by the way.
If you could have a look at that, then I think we can proceed quickly with this.

And, thanks for providing this pull request by the way!
Any (type of) contribution is always very much appreciated. :-)

[smcvb]

A couple of comments to go over still. But, looking good so far!

[smcvb]

The change you did on the givenNoPriorActivity method actually let me validate the JavaDoc more thoroughly.

As you can read on the givenNoPriorActivity method, it is intended to define a start point where no events have been provided yet.
This makes it an event-sourcing specific operation, not something to be used in conjunction with a state-stored aggregate.
Hence, I would like to ask you to keep the givenNoPriorActivity as is, thus event-sourcing specific.

Sadly enough, my eye didn't immediately fall on the givenCommands/andGivenCommands early enough, but they state a similar point as on the givenNoPriorActivity.
Granted, that is what you're adjusting, which thus warrants an adjustment in the JavaDoc of those methods too.
Furthermore, we'd need to check whether the current implementation of the andGivenCommands(List<?>) method actually does what you want.

Right now all it does is dispatch the given commands, check if command handling was successful and store the events as the "given events"-set.
This makes it seem like it doesn't do what we need it to do, which is constructing the initial state.
I am pretty confident it does do the trick right now, but it does warrant a little extra investigation in that area if you ask me.

In all, I thus still have some adjustments to ask of you.
So, let me know what you think!

[sascha-eisenmann]

I removed the call to andGiven() from givenNoPriorActivity(), because it would result in an exception if givenNoPriorActivity() would be called with useStateStorage set to true.
But the givenNoPriorActivity() method is also needed for testing state stored aggregates, if the constructor is used to initialize the aggregate. (like in testCreateStateStoredAggregateWithCommand())

Do you think it would be reasonable to update the JavaDoc for givenNoPriorActivity(), because the method does not only indicate no prior events, it also indicates that no prior state exists in the repository?

[smcvb]

I removed the call to andGiven() from givenNoPriorActivity(), because it would result in an exception if givenNoPriorActivity() would be called with useStateStorage set to true.
But the givenNoPriorActivity() method is also needed for testing state stored aggregates, if the constructor is used to initialize the aggregate. (like in testCreateStateStoredAggregateWithCommand())

Do you think it would be reasonable to update the JavaDoc for givenNoPriorActivity(), because the method does not only indicate no prior events, it also indicates that no prior state exists in the repository?

Hmm, wouldn't hurt indeed actually. It does clarify the process when using the AggregateTextFixture for state storage without any prior info.
Sure, give it a try, I would like to see that change.
That would thus require a change in the FixtureConfiguration#givenNoPriorActivity JavaDoc and a switch in the impl. in the AggregateTextFixture#givenNoPriorActivity.
Simply checking the added boolean and switch between given (for event sourcing) and givenState (for state-storage) would be sufficient I think.

[sascha-eisenmann]

I don't think that any more changes to the code will be necessary. Calling andGiven() would not change the behavior, because it would only result in iterating over an empty list.
Calling the givenState() method would counteract the idea of the givenNoPriorActivity(), because it would populate the repository with some value. Also, we would need to pass a producer function from givenNoPriorActivity() to givenState() for this to work.

In my opinion the only thing to do is updating the JavaDoc of givenNoPriorActivity().

[smcvb]

I don't think that any more changes to the code will be necessary. Calling andGiven() would not change the behavior, because it would only result in iterating over an empty list.
Calling the givenState() method would counteract the idea of the givenNoPriorActivity(), because it would populate the repository with some value. Also, we would need to pass a producer function from givenNoPriorActivity() to givenState() for this to work.

In my opinion the only thing to do is updating the JavaDoc of givenNoPriorActivity().

I shouldn't be free-wheeling on my mental picture of the implementation and should've instead looked at the changes you made...
You're right, how you've altered the flow of givenNoPriorActivity is sufficient I think.
Nonetheless, if you could make the JavaDoc changes, that would be great of course :-)

[smcvb]

One small nit, and then we should be done.

[smcvb]

My concerns have been addressed, hence approving.

[lfgcampos]

LGTM

[sascha-eisenmann]

I'm not entirely sure, but I think the build is failing at the Sonar scan.

java.lang.IllegalStateException: You're not authorized to run analysis. Please contact the project administrator.

Any ideas what is causing this problem?

[smcvb]

I'm not entirely sure, but I think the build is failing at the Sonar scan.

java.lang.IllegalStateException: You're not authorized to run analysis. Please contact the project administrator.

Any ideas what is causing this problem?

That's the current set up of the GitHub Actions I'm afraid.
The secrets used to validate with Sonar and push changes for snapshots aren't shared with contributors outside of the AxonIQ team (for security reasons).
We're thinking of an approach to fine-tune this to at least make it friendlier, by the way.

For now, though, I'll base myself on the fact that the JDK 8 build succeeds, as that's the Java version used by Axon for releases still.

[smcvb]

Thanks for the contribution on this @sascha-eisenmann!
Hoping to see more of those in the future if you find anything to improve or add to Axon :-)

[ahmetavc]

Hi @smcvb, I can't see this feature in the v4.5.1 release but it is the master branch. Is this PR reverted somehow in the release?

[smcvb]

That's correct and intended @ahmetavc. The milestone for this issue is targeted towards 4.6.0.
This makes it a feature that will be part of the framework as soon as we release 4.6.0.
Furthermore, master is where we construct the minor releases (so, 4.6.0, and 4.7.0 in the future).
Bug releases are done on axon-4.5.x, and thus axon-4.6.x once 4.6.0 is out.

The intent for placing this under 4.6.0 and not under 4.5.x., is because this is a feature and not a bug.
As you can read in the conversation, though, this feature only solves a relatively small issue.
It enables the givenCommands to be used in combination with the AggregateTestFixture for State-Stored Aggregates.

Another way to test a State-Stored Aggregate is by using the givenState method.
With this operation, you can provide the initial state of the aggregate before invoking the "when".

[sallowdish]

Hello Steven @smcvb,

Sorry that I have some difficulties to understand the approach you suggested about using Fixture.givenState(..) for State-stored Aggregates. My use case is exactly like @sascha-eisenmann described, a command handler annotated on top of the Agregate's constructor.

Since AggregateTestFixture can only hold one instance of Aggregate. The goal reduces to instantiating a fixture object with no aggregate using Fixture.givenState(..) as our start point. So that we can pass some CreateComamnd to the chained call to .when() in order to test the handling part.

So the sample you shared in earlier would result in complaint goes
java.lang.IllegalStateException: Creating an Aggregate while one is already stored
as it create an instance of Aggregate at the very first place:

After checking the implementation of Fixture.givenState(...), I come up with the idea of overriding the logic (GenericJPA)Repository::newInstance method to do nothing. But after registering to customized repository to the fixture instance, I notice that the 'doNothing' logic would also impacting the handling of CreateCommand. I feel that I have dug myself into a really deep hole and cannot help but pulling my hairs out.

Could you share some insight or sample on how to setup the fixture properly please? Any advice will be greatly appreciated. Thanks a lot.

[smcvb]

Firstly @sallowdish, I'd like to point to our forum as the place to open threads for questions.
Although I understand sticking to this thread is easy, it deviates from the intent we've set for GitHub: purely about issues.

Nonetheless, pointing you "to go away" without an answer is bonkers.
So I'll try to provide some guidance for now, anticipating seeing your questions in the future on the forum.

The testing use case you have is exactly what's not implemented yet.
Testing the aggregate constructor with the givenCommands operation is what @sascha-eisenmann provided support for.
That means that current versions of Axon Framework do not provide the solution described in this pull request.

I can tell you, though, that the Creating an Aggregate while one is already stored exception is only thrown if the AggregateTestFixture is not set up anew for every test case.
Granted, this could be more elegant by clearing out the old storedAggregate instance from the AggregateTestFixture.InMemoryRepository whenever a "given..." method is invoked.
It might be a nice enhancement to construct, actually. Maybe you're up for providing this?

At any point, I hope this gives some guidance @sallowdish!