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

Refactor aggregate module #424

Merged
merged 18 commits into from
Dec 28, 2024
Merged

Refactor aggregate module #424

merged 18 commits into from
Dec 28, 2024

Conversation

dgafka
Copy link
Member

@dgafka dgafka commented Dec 28, 2024

Why is this change proposed?

  • This removes multiple abstractions for different way of saving (It was complicating reasoning about system)
  • Introduces generic way of saving Aggregate and introduce Aggregate Definition (holding all information about Aggregate configuration)
  • Refactor lower test that were bound to existing design to use Ecotone Lite (was testing implementation, not end user behaviour and automatically broke with refactor)

Description of Changes

Pull Request Contribution Terms

  • I have read and agree to the contribution terms outlined in CONTRIBUTING.


}

public function process(Message $message): Message|null
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New implementation able to handle resolving aggregate instance, storing it in repo and publishing events.

/**
* licence Apache-2.0
*/
final class AggregateResolver
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class resolve aggregates that are needed to be saved. No need for RESULT_AGGREGATE header anymore, as it will cover different scenarios to find out what is to be saved based on incoming message.

@dgafka dgafka merged commit d211546 into main Dec 28, 2024
65 checks passed
@dgafka dgafka deleted the refactor/simplify-aggregate-module branch December 28, 2024 19:01
@lifinsky
Copy link
Contributor

Will this somehow affect current implementations? When, for example, built-in repository with doctrine entity manager are used.

We also use repository interface additionally to in-built event sourcing repository. For, example:

interface CardRepositoryInterface
{
    #[Repository]
    public function getById(Uuid $id): Card; // event sourcing aggregate
}

Will this still work?
@dgafka

@lifinsky
Copy link
Contributor

lifinsky commented Jan 23, 2025

@dgafka after update to latest version this code stopped working (used it for testing the issue with outbox and amqp):

    #[CommandHandler(self::PREPARE_TICKET_TICKET)]
    public static function prepare(PrepareTicket $command): array
    {
        return [new TicketWasPrepared($command->ticketId ?: Uuid::uuid4()->toString(), $command->ticketType, $command->description)];
    }

    #[CommandHandler(self::CANCEL_TICKET)]
    public function cancel(): array
    {
        if ($this->isCancelled) {
            return [];
        }

        return [new TicketWasCancelled($this->ticketId)];
    }

    #[EventSourcingHandler]
    public function applyTicketWasPrepared(TicketWasPrepared $event): void
    {
        $this->ticketId    = $event->ticketId;
        $this->isCancelled = false;
        $this->isAssigned  = false;
    }

    #[Asynchronous('backoffice_db_amqp')] // combined channel
    #[EventHandler(endpointId: 'ticket.wasPrepared')]
    public function cancelAfterPrepare(TicketWasPrepared $event, #[Reference] CommandBus $commandBus): void
    {
        $commandBus->sendWithRouting(Ticket::CANCEL_TICKET, metadata: ['aggregate.id' => $event->ticketId]);
    }

In amqp consumer get exception:
Pure event sourced aggregate should return iterable of events, but got string in AggregateResolver

@lifinsky
Copy link
Contributor

@dgafka after update to latest version this code stopped working (used it for testing the issue with outbox and amqp):

    #[CommandHandler(self::PREPARE_TICKET_TICKET)]
    public static function prepare(PrepareTicket $command): array
    {
        return [new TicketWasPrepared($command->ticketId ?: Uuid::uuid4()->toString(), $command->ticketType, $command->description)];
    }

    #[CommandHandler(self::CANCEL_TICKET)]
    public function cancel(): array
    {
        if ($this->isCancelled) {
            return [];
        }

        return [new TicketWasCancelled($this->ticketId)];
    }

    #[EventSourcingHandler]
    public function applyTicketWasPrepared(TicketWasPrepared $event): void
    {
        $this->ticketId    = $event->ticketId;
        $this->isCancelled = false;
        $this->isAssigned  = false;
    }

    #[Asynchronous('backoffice_db_amqp')] // combined channel
    #[EventHandler(endpointId: 'ticket.wasPrepared')]
    public function cancelAfterPrepare(TicketWasPrepared $event, #[Reference] CommandBus $commandBus): void
    {
        $commandBus->sendWithRouting(Ticket::CANCEL_TICKET, metadata: ['aggregate.id' => $event->ticketId]);
    }

In amqp consumer get exception: Pure event sourced aggregate should return iterable of events, but got string in AggregateResolver

it still works with this code:

    #[Asynchronous('backoffice_outbox')]
    #[EventHandler(endpointId: 'ticket.wasPrepared')]
    public function cancelAfterPrepare(TicketWasPrepared $event): array
    {
        return $this->cancel();
    }

But it seems to me dangerous if something that previously worked stops working.

@dgafka
Copy link
Member Author

dgafka commented Jan 24, 2025

@lifinsky that was not expected behaviour to use with Pure ES. It did work but not by intention.
But it make sense to make it possible, so that's unblocked and covered with test scenario to avoid B/C now (#430)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants