Cascade all on bidirectional relation, when the parent is not an aggragete root

[deschilter]

Description

Imagine this: Entity A and B are both not aggregate roots.

class EntityA {
    /**
    * @ORM\OneToMany(...,  orphanRemoval=true, cascade={"persist"})
   */
   protected $entityBs;
}

class EntityB {
    /**
    * @ORM\ManyToOne()
   */
   protected $entityA;
}

Now if an instance of EntityB is removed the parent, an instance of EntityA, gets remove too. That is because in https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Persistence/Doctrine/Mapping/Driver/FlowAnnotationDriver.php#L664 "cascade all" is added to the mapping when the targetEntity (EntityA) is not an aggregate root or the cascade option is not already set.

There is no "cascade none" option in Doctrine therefore the only way to work around this behavior is to make EntityA an aggregate root (=> creating a Repository for it).

Steps to Reproduce

1. Create a model like the one shown above
2. Delete a child entity

Expected behavior

Deleting the child entity does not delete the parent entity, when the cascade option to provoke this behavior is not set. My guess is that the "cascade all" is set for the use-case when the source-entity is the aggregate root. So a possible solution would be to check for that?

https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Persistence/Doctrine/Mapping/Driver/FlowAnnotationDriver.php#L664:

$this->isAggregateRoot($mapping['targetEntity'], $className) === false && $this->isAggregateRoot($className, $className)

Actual behavior

The parent entity gets deleted without the explicit annotation.

[albe]

Hey, thanks a lot for the detailed description!

There is no "cascade none" option in Doctrine

You can annotate it like this to have "none": @ORM\xToy(..., cascade={})

I think the current implementation comes from following PoV (which might be wrong in details):

Every non-aggregate entity is at some depth a part of one aggregate (in the case given, some entity C exists which is an aggregate root (AR) and has a relation to either A or B for example). The whole aggregate acts as a single unit, hence removal of any entity, should automatically cascade to all (non-AR) entities it owns, so that removal of the AR cascades down into all entities inside.

Here comes the twist - currently this "ownership" assumption is basically: "every entity owns every other non-AR entity it is in any way related to" 🙅‍♂ instead of a clear line of ownership.

I think in the case of ManyToOne specifically (and probably ManyToMany too), this assumption about ownership, is definitely wrong. The Many side can not be the (single) owner here, because, obviously, there are many of them. I can not cascade a delete to an entity, which might still be referenced (owned) by other entities. I can however remove the entity, once all owners are removed (orphanRemoval). This holds true, even if the owning entity is an AR (in a clean modelling aproach, an AR would never contain a @ORM\ManyToX relation). So I would go even further and say:

• we should revise the "ownership" assumption logic in place in the AnnotationDriver
• we probably need to put new methods (annotations? @Flow\RelationOwner?) in place, that can make ownership requirements explicit on a higher level (doctrine annotations are very implicit there, i.e. the Many side of a OneToMany is the owner, in bidirectional relations ownership is denoted by mappedBy/inversedBy)
• document that logic down on a high level, so it can act as a reference to decide if a change in the code makes sense

The golden rule(s) in place as of now:

• nothing can implicitly persist/delete an AR (i.e. no cascade/orphanRemoval towards an AR)
• deleting an AR will delete all contained entities (except for non-embedded VOs) - i.e. the AR privately owns all related entities

Some cases I think aren't fully thought through:

• multiple ARs relating to a single non-AR (true multiple ownership - as stated, this is not a sane model, but we currently don't deal with that at all)
• deep aggregate trees in general (non-AR relations, as uncovered in this issue)
• ownership on VOs - especially considering the difference of non-embedded/embedded (the former implicating required multiple ownership, while the later effectively preventing it)
• VOs not being allowed to own anything else (except other VOs)

See also https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/working-with-associations.html#orphan-removal and https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/working-with-associations.html#transitive-persistence-cascade-operations

[deschilter]

I put some thought about determining the owner of a relationship. I don't really like the idea of another Annotation, but maybe i just don't have enough insight.

My assumptions:

• The owner of a relationship is only relevant to select the right cascade option (or none in some cases)
• This is only for default behavior, when no explicit cascade options is set on the property, and can easily be overruled by setting it

I think we can just compare the types of source/target entity to determine the owner. I made some sort of mapping by source/target:

The column is the the source and the row is the target. RE => regular Entity

OneToOne
	RE	AR	VO
RE	none	source	target
AR	target	none	target
VO	source	source	none
OneToMany
	RE	AR	VO
RE	source	source	none
AR	none	source	none
VO	source	source	source
ManyToOne
	RE	AR	VO
RE	target	target	target
AR	target	target	target
VO	none	none	target
ManyToMany
RE	AR	VO
RE	none	source	target
AR	target	none	target
VO	source	source	none

Some of the cases covered by these tables should not happen. For example the case where a VO owns anything other than another VO.

If the source entity is the owner then cascade persist is set. The cases where no owner is determined or the target is the owner no cascade option is set.

Now if someone wants to explicitly set the owner, they can just set the cascade option. This would work in cases where no owner is determined. It would be more troublesome for cases where the ownership should be inverted, but i can't really think of a proper reason for that. Anyway they would have to use persist={} on the original owner.

About the case of multiple ARs owning an entity. If i understand correctly this is only an issue when orphanRemoval is set to true. Cascade remove seems to check if an entity is reused by another. Right now orphanRemoval is set when the target entity is a regular entity in OneToOne and OneToMany relations. I think it would be better to remove this and leave the orphanRemoval completely for explicit configuration.

Does this make any sense or have i missed the point?

[sorenmalling]

In my mind related to @albe work on #2056