Delay event uuid generation until it is requested or external generator supplied

[Xerkus]

I would like to throw in an idea for improvement around event ids.
Currently, if you want to employ different uuid generation strategy, for example COMB, ramsey uuid lib has to be configured statically beforehand.

I think it would be beneficial to delay event uuid generation (currently happens in DomainMessage::init()) until it is requested by something, which should not occur before extractor is run in most cases.

That would allow us to explicitly supply uuid generator in a clean way from infrastructure with a proper dependency injection:

   public function extractPendingStreamEvents($anEventSourcedAggregateRoot): array
   {
       if (null === $this->pendingEventsExtractor) {
           $this->pendingEventsExtractor = function (): array {
               return $this->popRecordedEvents();
           };
       }
       $events =  $this->pendingEventsExtractor->call($anEventSourcedAggregateRoot);
       if ($this->uuidGenerator) {
           foreach ($events as $event) {
               $event->applyUuidGenerator($this->uuidGenerator);
           }
       }
       return $events;
   }

public function applyUuidGenerator(UuidFactoryInterface $factory) : void
{
    if (! $this->uuid) {
        $this->uuid = $factory->uuid4();
    }
    // noop otherwise
}

[codeliner]

interesting approach and I agree that a custom uuid generator could be useful.
But I'm not sure if we should solve it this way. It feels a bit hacky, because events are immutable and should have a uuid at the moment they are created. Also changing the behavior of DomainMessage::init() would be a BC break because one could rely on the generation of the UUID. That's the purpose of method: If information is not provided in another way the defaults kick in.

So I think the solution can work but we should not include it in the core. DomainMessage explicitly allows that one can override the init method and tweak it. That would be my preferred way to add the functionality if needed. A short example in the docs would be nice, tough.

@prolic your thoughts?
@Xerkus Is that ok for you?

[Xerkus]

@codeliner I agree that it should not be used by default. It is a decision for the event implementers. However, I think following would be useful in the core:

• Introduce interface for event.
• Add support for interface in extractors.
• Make sure core is using uuid getter and not property directly, eg in toArray()
• May be trait for the interface that would in addition move uuid fallback generation to uuid getter and remove from init()
• docs

That is if this approach is sensible at all. I can't think of a use case for event uuid in the aggregate at this time but I might be missing something.

[Xerkus]

To clarify about immutability: in the code example I provided existence of uuid is explicitly checked and results in a noop if one is set exactly for that reason: to maintain immutability contract.

Also, I think event interface would belong here, together with extractors, not in common.

[codeliner]

Sounds like a good plan. I need to think about it and let's see what @prolic says about it.

One thing that bothers me: It feels complex or too much. Do you know what I mean? Instead of providing one solution the user has to decide now. We don't touch the default, ok. But more classes, more opportunities and maybe more confusion than necessary. Not sure, maybe I need to see it in action first. Do you want to provide a PR even without definit OK?

[Xerkus]

It feels complex or too much.

Yeah, there is that. Plus uuid optimisations are usually needed way down the line, when most of the stuff is already there and working.

Do you want to provide a PR even without definit OK?

Probably not until august.

[fritz-gerneth]

The problems I see with setting the IDs so late are:

• Either inconsistent with how commands / queries have their ID generated or this would need to be changed everywhere
• The entity dispatching the event/command/query has no access to the respective ID. For events this might not be an issue, for commands & queries this is (correlation id).
• To pass back the generated ID back to the one publishing the event/command/query the storage would need to return the new events, through the (possibly distributed) message bus.
• This would at least life on the message bus, not the event store itself. Not all events are AggregateChanged events
• (DDD) Events should always be in a valid state. They should protect their invariants to always only allow valid states. Not entirely sure though if the event ID should be seen as part of this invariant yet.

Imho, if you need a custom event ID it should be provided on event creation (so set it explicitly there by whomever creates the event). If implemented, I think this should go to the message bus instead. Still the issue remains to return the ID to the one submitting it (for correlation tracking ... ).

[prolic]

I think the ID is part of the event / query / command. Therefore the ID should not be added lately by some magic that's hidden somewhere else.
Actually we have an ID interface, it's this: https://github.com/ramsey/uuid/blob/master/src/UuidInterface.php. I know, this is not part of our library but "only" a dependency, but that doesn't change much. If you want to use prooph with a custom uuid implementation, go ahead and implement ramseys's uuid interface (build a bridge, proxy methods, whatever). It's the same amount of work as implementing "our" ID interface.
We have an interface for messages, you you are completely free to use any uuid implementation and even add the ID late if you want to.
I don't see the real benefit in accepting this change, but maybe I am missing something?

[Xerkus]

There is one case I didn't account for here: cloning event without uuid will result in different values when it is finally created on access.
Implementing lazy decorator factory using interface from ramsey lib is the only valid approach.
And on translator side it is better to implement translator decorator as well.
There is nothing to be done in prooph.