Creating aggregates and enforcing invariants spanning multiple aggregates

webdevilopers commented 4 years ago

Given is an event-sourced EmploymentContract holding an EmploymentPeriod with a start and end date. It is then possible to "deploy employees of a contract to other work places". But the DeploymentPeriod must be inside the EmploymentPeriod.

We want to keep our aggregates small and decided to make Deployment a separated event-sourced aggregate root (A+ES) too.

In a future solution we would create a separate microservice and make them event-driven. Then the could suggest a deployment, check the business rule and eventually fire a `DeploymentRejected" event or similar.

For now we are using CQRS and a deployEmployeeToWorkplace is handled by the command handler application service.

Currently we are thinking of three approaches ("Deployment" and "Solution" 1-3):

https://gist.github.com/webdevilopers/1357abd0d639337d5e7e56346f64ad67

Currently we prefer solution 1 which creates the Deployment thought a factory method on the EmploymentContract which already will hold the EmploymentPeriod. All rules - actually they are not invariants since this does not concern the "transaction" of the Deployment A+ES - can be validated here and return the Deployment.

The only thing we ask ourselves: Could and should we prevent other team developers from skipping the factory method and calling the named constructor on Deployment directly?

final class Deployment1
{
    public static function with(DeploymentId $anId, EmploymentContractId $anEmploymentContractId,
                                DeploymentPeriod $aPeriod): Deployment
    {
        // No period range validation, final period was already validated and passed
    }
}

Should this be done in your code base in general or should these constructors just be available as is and the rules explained in pair-programming. What are your thoughts?

Final thoughts: Whatever is the best solution, we are thinking of moving the responsibility away from the handler into a domain factory.

Similar to the questions asked here:

With an example of the red book by @VaughnVernon:

    public Discussion startDiscussionFor(
            ForumIdentityService aForumIdentityService,
            Author anAuthor,
            String aSubject,
            String anExclusiveOwner) {

        if (this.isClosed()) {
            throw new IllegalStateException("Forum is closed.");
        }

        Discussion discussion =
                new Discussion(
                    this.tenant(),
                    this.forumId(),
                    aForumIdentityService.nextDiscussionId(),
                    anAuthor,
                    aSubject,
                    anExclusiveOwner);

        return discussion;
}

https://github.com/VaughnVernon/IDDD_Samples/blob/master/iddd_collaboration/src/main/java/com/saasovation/collaboration/domain/model/forum/Forum.java#L153-L173

yourwebmaker commented 4 years ago

Nice topic... I have a few questions tho:

In a future solution, we would create a separate microservice and make them event-driven.

Why? Are they not part of the same bounded context?

`DeploymentRejected" event or similar.

It is not a constraint?

Regardless your answers, that's how I see your problem being solved so far:

<?php
namespace Webdevilopers\Employment
{
    class EmploymentContract
    {
        private EmploymentPeriod $employmentPeriod;

        public function deployTo(Workplace $workplace, DeploymentPeriod $deploymentPeriod) : Deployment
        {
            if (!$deploymentPeriod->isInside($this->employmentPeriod)) {
                throw new \InvalidArgumentException('DeploymentPeriod must be inside the EmploymentPeriod.');
            }

            //... In case Deployment is part of the same context, just return
            //return new Deployment($workplace, $deploymentPeriod);

            //... In case Deployment is part or another context (Microservice) send and event
            //$this->record(new DeploymentCreatedEvent())
        }
    }

    class EmploymentPeriod{}

    class Workplace{}

    class DeploymentPeriod
    {
        public function isInside(EmploymentPeriod $employmentPeriod) : bool
        {
            return true; //or false
        }
    }

    class Deployment
    {
        public function __construct(Workplace $workplace, DeploymentPeriod $deploymentPeriod) {}
    }

    class UseCase // Application
    {
        public function handleSameContext(DeployEmployeeCommand $command) : void
        {
            $contract = $this->contractRepository->get($command->contractId);
            $workPlace = $this->workplaceRepository->get($command->workplaceId);
            $deployment = $contract->deployTo($workPlace, new DeploymentPeriod($command->from, $command->to));

            $this->deploymentRepository->save($deployment);
        }

        public function handleMicroservice(DeployEmployeeCommand $command) : void
        {
            $contract = $this->contractRepository->get($command->contractId);
            $workPlace = $this->workplaceRepository->get($command->workplaceId);
            $contract->deployTo($workPlace, new DeploymentPeriod($command->from, $command->to));

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

webdevilopers commented 4 years ago

Thank you so much for this detailled feedback @yourwebmaker. It almost feels like playing poker and you read my hand because the code suggestion is very close to the current solution.

Why? Are they not part of the same bounded context?

I guess this is an important point. And I think I often miss it. "Deployment" is a subdomain, just like "Contracting". But yes, I really think they belong together in a single bounded context.

return new Deployment($workplace, $deploymentPeriod);

Should become:

return new Deployment($contractId, $workplace, $deploymentPeriod);

I guess my greatest fear is that another developer could use this method to create an "invalid" Deployment e.g. inside a migration console command. The invariants may be protected. But there is no guarantee that the period was checked against an EmploymentPeriod. But does it matter really?

I often tend to force the developer pass an interface e.g. EmploymentContractReadModel to the Deployment constructor - then there is no way around it. But it feels strange and maybe that is over-complication.

Another approach I'm playing with is making the Deployment become a collection of value objects inside EmploymentContract. There will only be few deployments per contract. No performance issues here.

But as mentioned this feels strange since I think they belond to different subdomains. On the other hand - remember all the "Order & order lines" examples out there... sigh

yourwebmaker commented 4 years ago

e.g. inside a migration console command

IMHO migrations should not be aware of domain objects. But this is totally out of the scope of this discussion.

If they belong on the same context, just use the returning/factory and persist it inside the command handler. Aggregates generating new aggregates is not an issue.

And keep in mind: All models are wrong, some are useful. Yours seem to be really useful for me.

I understood, it's testable, it's simple. So just go for it. In case it does not work later, refactor to something better. It's part of DDD process to try out models.

webdevilopers commented 4 years ago

At the bottom line what we devs do is offering other devs a single-point-of-entrance via command handler and a factory (method) to create the aggregate.

But we don't have to ensure that he skips using our "guidance" and possible spanning business-rules by directly creating the aggregate?

We deliver "useful" tools and hope they use them correctly.

webdevilopers / php-ddd

Creating aggregates and enforcing invariants spanning multiple aggregates #39