Bug #29416

Aggregate root detection for Person vs AbstractParty is broken

Added by Karsten Dambekalns almost 10 years ago. Updated almost 10 years ago.

Status:
Resolved
Priority:
Must have
Category:
Persistence
Start date:
2011-09-01
Due date:
% Done:

100%

Estimated time:
PHP Version:
Has patch:
Complexity:

Description

The association from Account to Person is cascaded during persist, because the fact that Person has a PersonRepository is not detected from the type hint to AbstractParty in the Account class.


Related issues

Related to TYPO3.Flow - Task #29543: Clarifiy persistence behavior for inheritance in entities / aggregate rootsResolvedKarsten Dambekalns2011-09-07

Actions
#1

Updated by Karsten Dambekalns almost 10 years ago

  • Status changed from Accepted to Needs Feedback

What actually happens

  1. Account has an association to AbstractParty
  2. the metadata mapping is done statically and does not find AbstractParty to be an aggregate root
  3. cascade={"all"} is added by default
  4. during runtime the associated party is actually a Person, which is an aggregate root
  5. changes are (falsely) persisted, because of the metadata mapping

What should happen

Depending on what actually is the associated party, operations should cascade or not

The problem

The metadata mapping is static, Doctrine does no runtime type inspection or something. Thus we cannot "simply add a runtime check in the persistence code" or something like that. Doctrine expects the information in the mapping to be "final and correct". If an association to some hierarchy is configured, Doctrine would expect all instances to behave the same way, i.e. cascade options apply to all instances the same way.

The solution?

"Easy"

We can "solve" this problem by making the same assumption: If AbstractParty is an aggregate root, then all concrete subclasses are aggregate roots - if not, none are.

"Complex as hell"

Find a place to hook in some runtime checking code. That code would require persist operations to be configured non-cascading and would then add persist/remove/add calls as needed if an actual associated instance would need it.

#2

Updated by Christopher Hlubek almost 10 years ago

I think it should depend on the actual type declaration of a property (the "easy" solution). So for example we could have an AbstractParty that is not an aggregate root and a property declaration with Person, which would be an aggregate root:

AbstractParty -> Person

PersonRepository
  ...

MyAccount:
  party: Person

In this case the party would not be persisted (since it's an aggregate root).

We should forbid this declaration, since you could mix aggregate roots and non aggregate roots for the party:

AbstractParty -> Person

PersonRepository
  ...

OtherAccount:
  party: AbstractParty

But I would not infer the aggregate root information from the start of the type hierarchy, which would circumvent modeling of your domain objects. It should be rather that once a class is an aggregate root, all subclasses should also be aggregate roots.

#3

Updated by Robert Lemke almost 10 years ago

  • Target version changed from 1.0 beta 2 to 1.0.0
#4

Updated by Karsten Dambekalns almost 10 years ago

  • Status changed from Needs Feedback to Accepted

Here is what will happen:

  • hierarchies need to be consistent - either all members are aggregate root, or none
  • if violated (as it is currently the case for AbstractParty and Person), an exception will be thrown

This way we can safely rely on what the "tip" of the hierarchy is and the problem is solved.

#5

Updated by Mr. Hudson almost 10 years ago

  • Status changed from Accepted to Under Review

Patch set 1 of change I5538230b42624629a6746ebff5e9e5dae6e93859 has been pushed to the review server.
It is available at http://review.typo3.org/5450

#6

Updated by Mr. Hudson almost 10 years ago

Patch set 2 of change I5538230b42624629a6746ebff5e9e5dae6e93859 has been pushed to the review server.
It is available at http://review.typo3.org/5450

#7

Updated by Karsten Dambekalns almost 10 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

Also available in: Atom PDF