ecotoneframework / ecotone-dev

Ecotone Framework Development - This is Monorepo which contains all official public modules
https://docs.ecotone.tech
Other
37 stars 16 forks source link

The built-in repository for event sourcing cannot be used if there are custom repositories (more than one) for state aggregates #318

Closed lifinsky closed 6 months ago

lifinsky commented 6 months ago

Ecotone version(s) affected: 1.220.0

Description
The current implementation of RepositoryStorage depends on the number of implemented repositories, and when the total number of repositories exceeds two, it does not allow the use of the built-in repository for event sourcing, EventSourcingRepository.

How to reproduce
Create more than one custom repository for state aggregates with the Repository attribute. At the same time, RepositoryStorage uses the following code:

foreach ($this->aggregateRepositories as $repository) {
    if ($repository->canHandle($this->aggregateClassName)) {
        return $this->returnRepository($repository);
    }
}

Method canHandle in EventSourcingRepository always returns false. Because EventSourcingRepositoryBuilder::compile() method always passes an empty list of event-sourcing aggregate classes.

Possible Solution
Call method EventSourcingRepositoryBuilder::withAggregateClassesToHandle in EventSourcingModule passes an array of classes that have the EventSourcingAggregate attribute but for which there are no other implementations of custom repositories. This issue can be resolved by using the built-in repository for event-sourcing aggregates at the end of the repository list in RepositoryStorage, giving preference to more specific implementations.

Context
Exception: There is no repository available for aggregate: Domain\Entity\Card in RepositoryStorage::getRepository() method.

lifinsky commented 6 months ago

@dgafka ASAP)

lifinsky commented 6 months ago

There is also a potentially serious issue with the ManagerRegistryRepository::canHandle() method, which, in the default configuration (DbalConfiguration::createWithDefaults()->withDoctrineORMRepositories(true)), passes an event-sourcing aggregate to the repository for state aggregates, allowing code to be executed in a repository that does not implement the EventSourcedRepository interface.

dgafka commented 6 months ago

@lifinsky can you provide a PR with test which will reproduce problem?

lifinsky commented 6 months ago

RepositoryStorage::aggregateRepositories:

\Infra\Doctrine\Repository\CardBatchIssuingSagaRepository \Infra\Doctrine\Repository\CardBinRepository \Infra\Doctrine\Repository\MarketingPlatformRepository \Infra\Doctrine\Repository\ProviderAccountRepository ***\Infra\Doctrine\Repository\ProviderRepository Ecotone\Dbal\ObjectManager\ManagerRegistryRepository Ecotone\EventSourcing\EventSourcingRepository

lifinsky commented 6 months ago

custom repositories for state aggregates

lifinsky commented 6 months ago

@dgafka We'll likely be able to make such a pull request next week.

lifinsky commented 6 months ago
    #[ServiceContext]
    public function persistenceStrategy(): array
    {
        $configuration = EventSourcingConfiguration::createWithDefaults()
            ->withSingleStreamPersistenceStrategy()
        ;

        return [
            $configuration,
            EventSourcingRepositoryBuilder::create($configuration)
                ->withAggregateClassesToHandle([
                    Card::class,
                ]),
        ];
    }

@dgafka current solution (but not documented)

lifinsky commented 6 months ago

I think event sourcing aggregates list should be managed by Ecotone with automatic class lookup by EventSourcingAggregate attribute

lifinsky commented 6 months ago

Also use case: create own state aggregate repository and business interface for the event sourcing aggregate.

#[Repository]
final class UserRepository implements StandardRepository
{
    public function __construct(private readonly EntityManagerInterface $entityManager)
    {
    }

    public function canHandle(string $aggregateClassName): bool
    {
        return $aggregateClassName === User::class;
    }

    public function findBy(string $aggregateClassName, array $identifiers): ?object
    {
        return $this->entityManager->getRepository(User::class)
            ->find($identifiers['userId']);
    }

    public function save(array $identifiers, object $aggregate, array $metadata, ?int $versionBeforeHandling): void
    {
        $this->entityManager->persist($aggregate);
    }
}
interface TicketRepositoryInterface
{
    #[Repository]
    public function find(string $id): ?Ticket;
}

Exception: Registered standard repository for event sourced aggregate App\Domain\Ticket\Ticket

RepositoryStorage::aggregateRepositories:

lifinsky commented 6 months ago

After add:

    #[ServiceContext]
    public function persistenceStrategy(): array
    {
        $configuration = EventSourcingConfiguration::createWithDefaults();

        return [
            $configuration,
            EventSourcingRepositoryBuilder::create($configuration)
                ->withAggregateClassesToHandle([
                    Ticket::class,
                ]),
        ];
    }

RepositoryStorage::aggregateRepositories:

As a result, everything works correctly.

lifinsky commented 6 months ago

@dgafka Why order and logic depends on service configuration for aggregates that have specific attributes of the aggregate variety?

dgafka commented 6 months ago

@lifinsky the current behaviour is expected. Ecotone does not divine Repositories for inbuilt / not inbuilt, this naming is only used for simplifying understanding by end users. Ecotone, when single Event Sourcing Repository is registered will use it in all cases, the same about Standard Repository. Above simplicity comes from the fact, we basically have no other repositories therefore if we use specific Aggregate it has to belong to that repo.

When we have two Repositories of the same type, this simplification is no longer valid. Introducing something like inbuilt being default wouldn't be explicit as in theory there may be more than one inbuilt repo of given type. Therefore if we register custom repositories, we need to take over the process. I will make it explicit in documentation, as indeed that was missing.

lifinsky commented 6 months ago

We do not have custom event sourcing repositories, why default is not working? Custom state repositories are applicable for concrete state aggregate class. This is bug.

lifinsky commented 6 months ago

Event sourcing repository is difficult and if other want to change default repository then maybe can create repository with concrete canHandle method

lifinsky commented 6 months ago

It’s strange to list classes for an event sourcing default repository in service configuration without having other repositories for them

dgafka commented 6 months ago

It’s strange to list classes for an event sourcing default repository in service configuration without having other repositories for them

True, make sense. If there is only single Event Sourcing Repository and multiple Standard ones, then for ES Aggregates we should fallback to that single one.

dgafka commented 6 months ago

Fixed with 1.221.0 :)