Axonframework: More semantically correct alternative to markDeleted

[vab2048]

Enhancement Description

A more 'semantically correct' alternative to markDeleted.

_Of course what is more 'correct' is always up to interpretation. I hope I can convince you of what I mean and am suggesting in the rest of the issue below._

Current Behaviour

Currently, we model aggregates as if they have a lifecycle i.e. they have a start state, bunch of intermediate states, and an end state. Once the end state is reached the lifecycle is over.

This aligns with the style mentioned by @abuijze in the podcast here: https://www.youtube.com/watch?v=zhzR1WiHQqE (apologies I don't remember the exact minute he mentions it but its there somewhere!).

Let's take the example from @idugalic's state machine demo project. We have an aggregate with the following lifecycle:

Currently to transition to the [END] state we need to publish an event which triggers the following event handler:

    @EventSourcingHandler
    void on(AbstractOrderDeletedEvent event) {
        markDeleted();
    }

Everything works fine and as expected. However, I think that calling markDeleted() kind of throws me here. The documentation on the method states:

Marks this aggregate as deleted, instructing a repository to remove that aggregate at an appropriate time.
Note that different repository implementations may react differently to aggregates marked for deletion.
Typically, Event Sourced Repositories will ignore the marking and expect deletion to be provided as 
part of Event information.

The problem here is that I believe we are overloading the term 'delete' to mean something which makes the code less understandable. The use of markDeleted conveys that we want to delete the aggregate which in actual fact is completely incorrect in this case - we simply want to transition it to the END state so it rejects future commands. _This is confusing._

When our intent is to transition the aggregate to the [END] state we are having to use the markDeleted() method as kind of a workaround.

The confusion about markDeleted() has led me to ask a question on the discuss site to get further clarification in the past. Other people will most likely end up in the same boat. It would be better to have an explicit method which does not cause confusion.

Wanted Behaviour

This to me is confusing and I would like to actually have a separate method whose semantics are not ambiguous and would make the code much more readable such as: transitionToEndState(..) / markAsLifecycleEnded() / (or whatever name makes the most sense).

Possible Workarounds

1. As mentioned above, the current way of using markDeleted() works but creates confusion.
2. As mentioned by @OLibutzki in a response you could model the [END] state explicitly.

Other notes

This may relate to https://github.com/AxonFramework/AxonFramework/issues/1415

[vab2048]

As an addendum, it would also be nice to have a declarative exception handler to deal with the case when after the aggregate lifecycle has ended (i.e. it has transitioned to its [END] state) AND a command is received for that aggregate. Currently we get the following (illustrative) exception message logged:

2020-12-20 15:50:47.002  WARN 15336 --- [ault-executor-2] o.a.c.gateway.DefaultCommandGateway      : Command 'pro.idugalic.axonstatemachine.command.api.command.MarkOrderAsCancelledCommand' resulted in org.axonframework.commandhandling.CommandExecutionException(Aggregate with identifier [0e9727f7-1d08-469d-9a7c-1d28a86a7e47] not found. It has been deleted.)

Placing this on the aggregate has no effect:

    @ExceptionHandler(resultType = AggregateDeletedException.class)
    public void handle(AggregateDeletedException exception) {
        log.info("In the exception handler to deal with a command being sent to an aggregate which has been deleted", exception);
        // deal with the fact that the aggregate lifecycle has ended but we have received a command.
       // ....
    }

org.axonframework.eventsourcing.EventSourcingRepository::doLoadWithLock has the following in it:

    protected EventSourcedAggregate<T> doLoadWithLock(String aggregateIdentifier, Long expectedVersion) {
        // ..... snipping method body to show only relevant part
        if (aggregate.isDeleted()) {
            throw new AggregateDeletedException(aggregateIdentifier);
        }
        // ..... snipping method body to show only relevant part
   }

Perhaps a separate special if statement could be added to the effect of (pseudo-code below):

       if (aggregate.lifecycleHasEnded()) {
            if(aggregate.hasLifecycleEndedExceptionHandler()) {
                     aggregate.invokeLifecycleHasEndedExceptionHandler()
            }
           // If no specialised exception handler exists for this situation throw the generic exception
            throw new AggregateLifecycleHasEndedException(aggregateIdentifier);
        }
      // ..... 

Just sharing my thoughts.... hopefully you find them useful.
Thanks

[OLibutzki]

What about having an explicit end state? You never mark the aggregate as deleted but just update the state. Yes, you have to check the state in each CommandHandler, but don't you have to do this anyway in order to ensure that only valid state transitions are performed?

Nevertheless, I agree that _markDeleted_ is not the best term for expressing that an aggregate has reached its lifecycle end.

[idugalic]

Hi, @vab2048. I am the creator of https://github.com/idugalic/axon-statemachine-demo. My idea was to force us to think about our models in terms of behavior. The consequence is that we tend to design better systems when we use the Finite-State-Machine pattern.

I believe that @OLibutzki has a good point here: "markDeleted is not the best term for expressing that an aggregate has reached its lifecycle end". This is domain-specific, and (looking from this perspective, today) I would rather use custom validation (as @OLibutzki described) in my aggregates than marking them deleted.

As this is a domain-specific problem, I (personally) do not see a general solution by having Axon Fremwerk introducing markAsLifecycleEnded method(s) TBH.

Hopefully, this makes sense to you as well. Please let us know what do you think?
Thank you very much for opening this discussion!

[vab2048]

Thanks @OLibutzki and @idugalic for your comments.

@OLibutzki - you are of course absolutely correct - having an explicit [END] state would work.
I'll edit my original post to add this to the workarounds list.

You also mention:

Yes, you have to check the state in each CommandHandler, but don't you have to do this anyway in order to ensure that only valid state transitions are performed?

If an aggregate has reached [END] then no state transition is a valid one, right?
And so my idea was rather than having to write the boiler plate "to check the state in each CommandHandler" - the framework
would handle it for you (if the aggregate is marked as having its lifecycle ended it would know to reject the command).

@idugalic - I've learned a lot about how to use the framework from your projects - thanks for making them open source!

(looking from this perspective, today) I would rather use custom validation (as @OLibutzki described) in my aggregates than marking them deleted.

I think this is a good recommendation which I will also follow.

As this is a domain-specific problem, I (personally) do not see a general solution by having Axon Fremwerk introducing markAsLifecycleEnded method(s) TBH.

I think you may be right - I was mainly just hoping that the framework could help remove the boiler plate (as I mentioned above). But as it currently stands this would probably be low priority on the delivery path for the framework... and custom validation in each command handler is obviously the most flexible route.

I guess for now a simple pattern would be to have something like the following within an aggregate:

private boolean lifecycleEnded;

@CommandHandler
void on(Cmd cmd) {
  Assert.isTrue(!lifecycleEnded, () -> "The aggregate's lifeycle has ended. It is no longer accepting commands.");
  // ....
}

Closing this issue in favour of the custom validation route is probably the best way forward... :)

[smcvb]

Worthwhile discussion everybody, thanks for that.

As pointed out by @vab2048, this issue will be closed for now.
Feel free to keep providing insights here though.
Who knows, we might just reopen it when new insights turn up.