Skip to content

Loading…

DDC-130: Cascading and @ManyToMany associations is broken #1913

Closed
doctrinebot opened this Issue · 20 comments

2 participants

@doctrinebot

Jira issue originally created by user nicokaiser:

I have two Entities: Users and Alerts. They are associated with @ManyToMany, the assiciation table should be "user_alert".

Entities:
http://pastebin.com/m7bca724a

Sample code:
http://pastebin.com/m4e530f42

The "remove" command in the sample code deletes the user entry and alert entry, but not the user_alert entry (which was automatically created though). This leaves an orphan entry (or the DBMS will complain because of FK constraints).

INSERT INTO users (name) VALUES (?)
array(1) {
  [1]=>
  string(3) "Bob"
}
INSERT INTO alert (name) VALUES (?)
array(1) {
  [1]=>
  string(9) "Testalert"
}
INSERT INTO user_alert (userId, alertId) VALUES (?, ?)
array(2) {
  [0]=>
  int(1)
  [1]=>
  int(1)
}
DELETE FROM users WHERE id = ?
array(1) {
  [0]=>
  int(1)
}
DELETE FROM alert WHERE id = ?
array(1) {
  [0]=>
  int(1)
}

The useralert table should be automatically updated (on creation _and removal)...

@doctrinebot

Comment created by romanb:

I think in most cases the entry in the association table should be deleted by the FK constraint. However I think that is currently not the default. You can force it by using onDelete="CASCADE" on the @JoinColumn definitions inside the @JoinTable. That should probably be set by default on join columns on an association table.

I'm not sure its worth supporting manual deletion of association table entries through Doctrine as all databases support proper foreign key constraints and this is the most effective way to delete these entries and also enforces the integrity on the database side.

So I'm rather voting for making onDelete="CASCADE" the default for join columns of association tables unless specified otherwise.

@doctrinebot

Comment created by romanb:

It appears that this doesnt work in JPA(2) either, or well, it depends on the implementation provider. But cascade=REMOVE on @ManyToMany is not allowed per the spec.

This issue here is probably one of the reasons for this. Meaning there might be no easy way for the ORM to deal with entries in association tables transparently on cascade=REMOVE.

We have the following options:

1) Disallow cascade=REMOVE on ManyToManyMapping (throw a MappingException)

2) Allow it but clearly document that entries in association tables are not removed by Doctrine, so that you need to apply onDelete="CASCADE" on the @JoinColumn definitions. Are there any more caveats with this I can not think of currently?

3) Someone has an idea for transparently deleting entries in association tables on cascade=REMOVE. Solutions would need to be performant and not too cumbersome to implement.

Waiting for feedback.

@doctrinebot

Comment created by @beberlei:

Hm after thinking about it the problem is that the collection has to be either:

  1. loaded into memory completely and put through to regular remove a single entity process. This would be required to mark entities for deleted that are also hydrated but attached to another many to many collection of this type.
  2. selected for all their ids and there should be a many to many deletion mechanism that works based on id, however then also finds also hydrated entities based on their id.
@doctrinebot

Comment created by nicokaiser:

Hm, I don't know if my report was unclear... What I meant was automatic update of the join table ("useralert" in this example). When I delete e.g. a User, I don't want to have to care about updates in the "useralert" table - this table was automatically generated (by the @JoinTable statement), so it should be automatically updated, e.g. when I delete either a User or an Alert...

@doctrinebot

Comment created by alex:

How about removing a relation between a user and an alert?

Meaning the user and alert both remain in the database but they are not connected anymore (think about users with multiple roles).

@doctrinebot

Comment created by romanb:

@Alexander: Just like in normal OOP. If a user as a collection of alerts and an alert a collection of users (bidirectional many-many) and you want to remove an alert from a user you simply remove the alert from the collection and the user from the collection of the corresponding alert. This is very clean, the only problem with this is the efficiency because in order to remove the elements from the collections, the collections must be loaded. (Strictly speaking, you only need to adjust the "owning side", so only 1 collection needs to be loaded). Of course you can always use some custom SQL that directly manipulates the intermediate table. See also DDC-128. Also there is DDC-45 but I dont think thats doable.

Collections of entities always only represent the association of the entity that has the collection and the contained entities. If you remove an entity from a collection, the association is removed, not the entity itself. "The association is removed" means that either the foreign key is NULLed out (in the case of one-to-many) or that the entry in the association table is removed.

@doctrinebot

Comment created by alex:

@Roman: All sounds very simple. But in practice this doesn't seem to work.
I have the following:

$uRep = $em->getRepository('System_User');
$user = $uRep->findOneById(1);
unset($user->roles[0]);
$em->persist($user);
$em->flush();

Now, when I clear the manager and reload the user:

  • If the user had 1 role -> it still has one role
  • If the user had more roles -> it still has the removed role and all others are duplicated.
@doctrinebot

Comment created by romanb:

@Alexander: Please open a separate issue for this. Thanks.

@doctrinebot

Comment created by mjh_ca:

Pastebin entries have expired and this bug is not attached to a specific version so it is difficult to tell the status. Nico, is this still a problem with the current release?

@doctrinebot

Comment created by romanb:

My currently proposed solution to this would be to simply make onDelete="CASCADE" the default for join columns in join tables. Anyone has a strong objection or better idea?

@doctrinebot

Comment created by @beberlei:

sounds good to me

@doctrinebot

Comment created by @beberlei:

Other tools actually delete the related records manually, no cascading involved. Since Sqlite doesn't even support foreign keys we should probably do the same instead of relying on "onDelete"="cascade".

Whats your take roman?

@doctrinebot

Comment created by @beberlei:

We have to support this anyways for the following reason:

Doctrine 2 gives you absolutely no access to the many-to-many join table, i.e. to be working with cascade it should not only be the default, but the only option (since other options don't work). We need to extend the way "delete" works inside each persister to also clean up many-to-many tables.

We need this mechanism anyways for Element Collections to delete all the related entries.

The option onDelete="cascade|noaction" is therefore only a hint for the persisters to decide if they perform this action themselves, or let the database vendor perform it.

@doctrinebot

Comment created by @beberlei:

I pushed my proposed changes to a feature branch: http://github.com/doctrine/doctrine2/tree/[DDC-130](http://www.doctrine-project.org/jira/browse/DDC-130)

There are definatly refactorings that need to be done, however in that state its currently doing its job very well.

@doctrinebot

Comment created by @beberlei:

What about DQL DELETE?

DELETE FROM Boo WHERE bar

For each join table:

DELETE FROM join_foo WHERE (SELECT id FROM foo WHERE bar)

Can this be done determinstically?

@doctrinebot

Comment created by @beberlei:

The DQL stuff is just way to much to add. Foreign Key constraints should fail in these cases imho

@doctrinebot

Comment created by romanb:

I think we have to do this on bulk deletion then, too.

Examples from EclipseLink:

em.createQuery("delete from User2 u where u.id=1").executeUpdate();
=>
DELETE FROM USER2*GROUP2 WHERE EXISTS(SELECT ID FROM USER2 WHERE (ID = ?) AND ID = USER2_GROUP2.User2*ID)
DELETE FROM USER2 WHERE (ID = ?)
em.createQuery("delete from User2 u").executeUpdate();
=>
DELETE FROM USER2_GROUP2
DELETE FROM USER2
@doctrinebot

Comment created by @beberlei:

Fixed in master using the following semantics:

  • If a many-to-many join table is to be found not using onDelete="CASCADE" execute this manually
  • In self-referential relationships delete both possible pairs.
  • Use Database onDelete="CASCADE" if present on both foreign keys of the join table.

Tested against normal entities, self-referential, CTI and STI, from the owning and the inverse side.

@doctrinebot

Issue was closed with resolution "Fixed"

@beberlei beberlei was assigned by doctrinebot
@doctrinebot doctrinebot added this to the 2.0-BETA3 milestone
@doctrinebot doctrinebot closed this
@doctrinebot doctrinebot added the Bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.