Passing read models (value objects representing state) / domain service to aggregate methods

[webdevilopers]

Came from:

Given a Rule spanning aggregates: existing contract periods should not overlap new period. How much „logic“ in repository? 1) query all contracts and service class checks overlap 2) query and check overlap by passing new period (logic in SQL)

• https://twitter.com/webdevilopers/status/1315638157514354688

/cc @AntonStoeckl @plalx @BrunoRommens

After the discussion I moved my entire logic from an extra service and repository methods to the aggregate.

A few things to consider: - Logic in infrastructure implies integration tests verification. - Checking in service leaks BL outside the model -- I'd start trying: 1. contract.changePeriod(period, otherPeriods) 2. contract.changePeriod(period, overlapDetector). -- Refactor...

As suggested by @plalx.

Event-sourced aggregate root (A+ES)

final class ServiceContract extends AggregateRoot
{
    public static function enter(
        ServiceContractId $anId, PersonId $anPersonId, PersonalData $aPersonalData,
        AssignmentPeriod $anAssignmentPeriod, WorkweekDays $aWorkweekDays, WeeklyWorkingHours $aWeeklyWorkingHours,
        JobFunctionId $aJobFunctionId, string $aJobFunctionName,
        AgencyId $anAgencyId, string $anAgencyName,
        WorkplaceId $aWorkplaceId, string $aWorkplaceName,
        array $enteredContracts
    ): ServiceContract
    {
        Assert::notEmpty($aJobFunctionName);
        Assert::notEmpty($anAgencyName);
        Assert::notEmpty($aWorkplaceName);

        $self = new self();
        $self->detectConflicts($anId, $enteredContracts, $anAssignmentPeriod);
        $self->recordThat(ServiceContractEntered::withData(
            $anId, $anPersonId, $aPersonalData,
            $anAssignmentPeriod, $aWorkweekDays, $aWeeklyWorkingHours,
            $aJobFunctionId, $aJobFunctionName, $anAgencyId, $anAgencyName,
            $aWorkplaceId, $aWorkplaceName, new DateTimeImmutable()
        ));

        return $self;
    }

    private function detectConflicts(ServiceContractId $id, array $enteredContracts, AssignmentPeriod $period): void
    {
        Assert::allIsInstanceOf($enteredContracts, Term::class);

        foreach ($enteredContracts as $enteredContract) {
            if ($id->sameValueAs(ServiceContractId::fromString($enteredContract->contractId()))) {
                continue; // Used only when editing the assignment period, not for actual creation of the contract
            }

            if ($period->overlapsWith($enteredContract->assignmentPeriod())) {
                throw new AssignmentPeriodOverlapsException();
            }
        }
    }
}

Application Service Command Handler

final class EnterServiceContractHandler
{
    public function __invoke(EnterServiceContract $command): void
    {
        $person = $this->personDetailsRepository->ofPersonId($command->personId()->toString());

        $enteredContracts = $this->contractTermRepository->ofPersonId($command->personId()->toString());

        $contract = ServiceContract::enter(
            $command->contractId(), $command->personId(),
            ... personal data from the person etc. ...
            $enteredContracts
        );

        $this->contractCollection->save($contract);
    }
}

As @BrunoRommens suggested the enteredContracts are no full details read models. They are just value objects holding the actual assignment periods of the other contracts entered by the person.

The "Available periods" aggregate contains only "Period" value objects. Maybe the tradeoff (against nature in classical DDD), is to implement this type of aggregate as an interface in the domain with an implementation in infrastructure handling available periods finding operation

Unit test

final class ServiceContractTest extends EventSourcedAggregateRootTestCase
{
    /** @test */
    public function signingDetectsConflictWhenExistingPrecedingContractIsStillActive(): void
    {
        $this->expectException(AssignmentPeriodOverlapsException::class);

        $enteredContracts = [
            Term::fromArray([
                'contract_id' => 'a391c713-2cf7-4500-b997-e37ea24c5889',
                'person_id' => '71efb6b7-6eb1-4b17-b1d5-a3a47bac78f1',
                'start_date' => '2019-02-01',
                'end_date' => '2020-01-31',
            ])
        ];

        ServiceContract::enter(
            ServiceContractId::fromString('b88f676e-221d-4807-97c9-8f549b13425a'),
            PersonId::fromString('71efb6b7-6eb1-4b17-b1d5-a3a47bac78f1'),
            $this->getPersonalData(),
            AssignmentPeriod::with(
                new DateTimeImmutable('2020-01-01'),
                new DateTimeImmutable('2020-12-31'),
            ),
            WorkweekDays::fromArray(["monday","tuesday","wednesday","thursday","friday"]),
            WeeklyWorkingHours::fromFloat(40),
            JobFunctionId::fromInteger(4), 'Qualitätsprüfer/in',
            AgencyId::fromString('904c0436-4226-4725-afbe-14526906afdb'), 'Office People',
            WorkplaceId::fromString('1e522e02-b28d-44d2-a3fb-4efbdacec462'), 'Bratislava',
            $enteredContracts
        );
    }
}

[webdevilopers]

The alternative approaches were using a Domain Service to ensure that the developer could not pass "incorrectly determined" entered contracts.

A+ES

final class ServiceContract extends AggregateRoot
{
    public static function enter(
        ServiceContractId $anId, PersonId $anPersonId, PersonalData $aPersonalData,
        AssignmentPeriod $anAssignmentPeriod, WorkweekDays $aWorkweekDays, WeeklyWorkingHours $aWeeklyWorkingHours,
        JobFunctionId $aJobFunctionId, string $aJobFunctionName,
        AgencyId $anAgencyId, string $anAgencyName,
        WorkplaceId $aWorkplaceId, string $aWorkplaceName,
        OverlapDetectorServiceInterface $overlapDetector
    ): ServiceContract
    {
        Assert::notEmpty($aJobFunctionName);
        Assert::notEmpty($anAgencyName);
        Assert::notEmpty($aWorkplaceName);

        $overlapDetector->ofPersonId($anPersonId, $anId, $anAssignmentPeriod);

        $self = new self();
        $self->recordThat(ServiceContractEntered::withData(
            $anId, $anPersonId, $aPersonalData,
            $anAssignmentPeriod, $aWorkweekDays, $aWeeklyWorkingHours,
            $aJobFunctionId, $aJobFunctionName, $anAgencyId, $anAgencyName,
            $aWorkplaceId, $aWorkplaceName, new DateTimeImmutable()
        ));

        return $self;
    }

Service implementation

final class OverlapDetector implements OverlapDetectorServiceInterface
{
    public function ofPersonId(PersonId $personId, ServiceContractId $currentContractId, AssignmentPeriod $currentPeriod): array
    {
        $enteredContracts = $this->contractTermRepository->ofPersonId($personId()->toString());

        foreach ($enteredContracts as $enteredContract) {
            if ($currentPeriod->overlapsWith($enteredContract->assignmentPeriod())) {
                throw new AssignmentPeriodOverlapsException();
            }
        }
    }
}

Alternatively the service could return a bool and the aggregate could throw the exception. For testing different implementations could be used e.g. "HasConflictDetector" or "HasNoConflictDetector".

Still I find this harder to test and as @plalx mentioned it leaks business logic outside of the aggregate.

A few things to consider: - Logic in infrastructure implies integration tests verification. - Checking in service leaks BL outside the model The additional argument (data/service) communicates there's a rule to check at least and you can't just set the period to whatever you want. The service may be safer, but compromises the model's purity. If you go with an infrastructure-based impl. I prefer providing the service. That looks good: hard to misuse the AR method and easy to test the collab between the AR and the detector. The AR is unpure and the detector may not be unit testable, but you can't win everywhere.

[webdevilopers]

Yet another solution would be to go full event-driven.

As @BrunoRommens suggests:

What are design implications with this ? Maybe one new aggregate "Available periods" providing a "Reserve period" command execution, raising "Period reserved" or "Period not reserved" events. The "Contract" aggregate factory would tell command exec and create or not the contract.

Mabye a Contract should not be entered through itself but through Person. The Person could hold the enteredContracts value objects. The overlapping could be checked. In addition all the personalData mentioned above can come from there too.

final class Person extends AggregateRoot
{
    private $personId;

    private $personalData;

    private $enteredContracts; // From applying "ServiceContractEntered" event from the `ServiceContract` A+ES

    public function enterServiceContract(ServiceContractId $serviceContractId, AssignmentPeriod $period, ...other arguments...): ServiceContract
    {
        foreach ($this->enteredContracts as $enteredContract) {
            if ($period->overlapsWith($enteredContract->assignmentPeriod())) {
                throw new AssignmentPeriodOverlapsException();
            }
        }

        return ServiceContract::enter(
            $serviceContractId
            $this->personId,
            $this->personalData,
            $period,
            WorkweekDays::fromArray(["monday","tuesday","wednesday","thursday","friday"]),
            WeeklyWorkingHours::fromFloat(40),
            JobFunctionId::fromInteger(4), 'Qualitätsprüfer/in',
            AgencyId::fromString('904c0436-4226-4725-afbe-14526906afdb'), 'Office People',
            WorkplaceId::fromString('1e522e02-b28d-44d2-a3fb-4efbdacec462'), 'Bratislava'
        );
    }
}

final class ServiceContract extends AggregateRoot
{
    public static function enter(
        ServiceContractId $anId, PersonId $anPersonId, PersonalData $aPersonalData,
        AssignmentPeriod $anAssignmentPeriod, WorkweekDays $aWorkweekDays, WeeklyWorkingHours $aWeeklyWorkingHours,
        JobFunctionId $aJobFunctionId, string $aJobFunctionName,
        AgencyId $anAgencyId, string $anAgencyName,
        WorkplaceId $aWorkplaceId, string $aWorkplaceName
    ): ServiceContract
    {
        Assert::notEmpty($aJobFunctionName);
        Assert::notEmpty($anAgencyName);
        Assert::notEmpty($aWorkplaceName);

        $self = new self();
        $self->recordThat(ServiceContractEntered::withData(
            $anId, $anPersonId, $aPersonalData,
            $anAssignmentPeriod, $aWorkweekDays, $aWeeklyWorkingHours,
            $aJobFunctionId, $aJobFunctionName, $anAgencyId, $anAgencyName,
            $aWorkplaceId, $aWorkplaceName, new DateTimeImmutable()
        ));

        return $self;
    }

But does this not move the logic of overlapping far away from the developer? He could easily ignore the Person factory method. Or is this just a matter of communication?

At least the true invariants are still protected inside the ServiceContract. The spanning aggregate rules are protected by the factory on another aggregate (Person).

[webdevilopers]

Thanky you very much to @BrunoRommens who draw these diagrams to understand the suggested solutions:

• https://twitter.com/BrunoRommens/status/1320073246667059205

[webdevilopers]

This 3rd solution is not exactly what I meant initialy. I think the "non overlaping contract assignment periods" rule deserves its own aggregate, because it only applies to "Service contract assignment periods" and not on other "Person" data. Something like period reservation. I think it enforces the transactionality of the rule. Checking and choosing a non overlaping assignment period has to be done sequentially. But maybe it is overdesign in your user context (ex: no risk of concurrent calls). Moreover it will need synchronization. Something like a policy handling the "Assignement period reserved for Service contract" and triggering the "Enter (Service contract)" on the "Service contract Factory". Ps : more precisely on my previous writing : Checking and choosing a non overlaping assignment period has to be done "atomistically and non concurrently" (in place of "sequentially")

@BrunoRommers

[webdevilopers]

I like the idea of making the AssignmentPeriodAvailability an AR. Didin't consider it at first b/c I thought there could be thousands or more taken periods, but that's less likely given the segregation by Person. If scalability isin't an issue then perhaps both ARs could be modified in the same tx to avoid eventual consistency. Domain events could still be leveraged to keep things decoupled (in-memory bus). Eventual consistency can be implemented later if needed without mod. AR bounds.

@plalx

[webdevilopers]

Here is a more complex example for merging contracts.

final class ServiceContract extends AggregateRoot
{
    public function mergeWith(array $mergeWithContractIds, array $enteredContracts): void
    {
        Assert::minCount($mergeWithContractIds, 2);

        /** @var Details $contractsToMerge */
        $contractsToMerge = [];
        $surroundingAssignmentPeriod = $this->assignmentPeriod;

        foreach ($mergeWithContractIds as $mergeWithContractId) {
            if (!key_exists($mergeWithContractId->toString(), $enteredContracts)) {
                throw new InvalidServiceContractProvidedForMergingException();
            }

            if ($this->isMergedWith($mergeWithContractId)) {
                throw new AlreadyMergedWithServiceContractException();
            }

            $contractToMerge = $enteredContracts[$mergeWithContractId->toString()];

            // Calculate surrounding period for all contracts.
            $surroundingAssignmentPeriod = $surroundingAssignmentPeriod->mergeWith($contractToMerge->assignmentPeriod());

            if ($this->id->sameValueAs(ServiceContractId::fromString($contractToMerge->contractId()))) {
                continue;
            }

            $contractsToMerge[] = $contractToMerge;
        }

        if ($this->assignmentPeriod->startDate()->format('Y-m-d') !== $surroundingAssignmentPeriod->startDate()->format('Y-m-d')) {
            // This is not the initial contract
            throw new InvalidInitialServiceContractForMergingException();
        }

        $overlappingEnteredContractIds = [];

        foreach ($enteredContracts as $enteredContract) {
            if ($enteredContract->assignmentPeriod()->overlapsWith($surroundingAssignmentPeriod)) {
                // Handle overlapping contracts only.
                if (!in_array(ServiceContractId::fromString($enteredContract->contractId()), $mergeWithContractIds)) {
                    // Register overlapping contracts that have not been marked for merging.
                    $overlappingEnteredContractIds[] = $enteredContract->contractId();
                }
            }
        }

        if (0 !== count($overlappingEnteredContractIds)) {
            throw new ServiceContractMergeConflictDetectedException();
        }

        foreach ($contractsToMerge as $contractToMerge) {
            $this->recordThat(MergedWithServiceContract::with(
                $this->id,
                ServiceContractId::fromString($contractToMerge->contractId()),
                $surroundingAssignmentPeriod,
                WorkweekDays::fromArray($contractToMerge->workweekDays()),
                WeeklyWorkingHours::fromFloat($contractToMerge->weeklyWorkingHours()),
                JobFunctionId::fromInteger($contractToMerge->jobFunctionId()),
                $contractToMerge->jobFunctionName(),
                AgencyId::fromString($contractToMerge->agencyId()),
                $contractToMerge->agencyName(),
                WorkplaceId::fromString($contractToMerge->workplaceId()),
                $contractToMerge->workplaceName(),
                new DateTimeImmutable()
            ));
        }
    }
}

While merging per se is a complex process this feels a little bit overloaded for the aggregate root.

That is why I suggested to use the Person aggregate for the overlap detection and then enter the valid contract from there.

There is a similar approach in "Implement #DDDesign" by Vaughn Vernon:

The "Forum -> Discussion" example analogous to my "Person- > Contract" example:

The question here is: Is Discussion an event-sourced aggregate-root? Since Forum pops the event it does not seem be.