Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DDC-601: UnitOfWork commit order problem #5109

Closed
doctrinebot opened this issue May 19, 2010 · 12 comments
Closed

DDC-601: UnitOfWork commit order problem #5109

doctrinebot opened this issue May 19, 2010 · 12 comments

Comments

@doctrinebot
Copy link

@doctrinebot doctrinebot commented May 19, 2010

Jira issue originally created by user exxbrain:

The next critical case doesn't work, because of Duplicate entry error:

$obj = $repository->findOneByUniqueKey($key);

if ($obj != null) {
     $em->remove($obj);
}
$obj = new Object();
$obj->setUniqueKey($key);
$em->persist($obj);

$em->flush();

Instead of that i must use the next construction:

$obj = $repository->findOneByUniqueKey($key);

if ($obj != null) {
     $em->remove($obj);
     $em->flush();      //  the possible data inconsistency cause
}
$obj = new Object();
$obj->setUniqueKey($key);
$em->persist($obj);

$em->flush();
@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented May 19, 2010

Comment created by romanb:

Hi. The order of operations is mainly designed for a) efficiency and b) preserving foreign key constraints, but not for such kinds of unique constraints.

The order is:

  1. all entity inserts (topological order)
  2. all entity updates (topological order)
  3. all collection deletions
  4. all collection updates
  5. all entity deletions (reverse toplogical order)

Where the topological order is dictated by the foreign key constraints.

The unique constraint is not the primary key, is it? In either case, I think this is a strange thing to do, even more if this is the primary key, as a primary key should never be updated or reused by other objects.

You can still easily do it, just use an extra flush like you did. If you want both flushes to execute in a single transaction, simply use your own transaction demarcation:

$em->getConnection()->beginTransaction();
try {
    if ($obj != null) {
        $em->remove($obj);
        $em->flush();
    } 
    $obj = new Object();
    $obj->setUniqueKey($key);
    $em->persist($obj);
    $em->flush();
    $em->getConnection()->commit();
} catch (Exception $e) {
   $em->getConnection()->rollback();
   $em->close();
   throw $e;
}

See: http://www.doctrine-project.org/projects/orm/2.0/docs/reference/transactions-and-concurrency/en#transaction-demarcation

I don't think we can change the order by putting deletions first without breaking other things. At least Hibernate has a similar order (inserts first) and probably for a reason.

Here are some related discussions:

http://opensource.atlassian.com/projects/hibernate/browse/HHH-2801
http://blog.krecan.net/2008/01/05/flushing-hibernate/
https://forum.hibernate.org/viewtopic.php?p=2421199

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented May 19, 2010

Comment created by romanb:

I tried this with eclipselink (jpa 2 reference implementation, http://www.eclipse.org/eclipselink/ ) as well, same unique constraint violation (inserts first).

That makes me not very optimistic that we can do any better.

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented Jul 4, 2010

Comment created by romanb:

Some more references on why deletes are performed last:

http://old.nabble.com/EntitEntityManager.remove%3A-Why-does-it-care-about-nullable%3Dfalse--td22942681.html
http://forums.oracle.com/forums/thread.jspa?messageID=3805444

We may consider doing the deletes first since we always have a topological order and most of the mentioned issues might not apply to our architecture or might not be common cases but I am not certain yet.

In any case, like I mentioned in my first comment, you can just flush() after remove() to force the deletion first.

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented Aug 25, 2010

Issue was closed with resolution "Won't Fix"

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented Dec 5, 2011

Comment created by jean-gui:

Calling flush() after remove() is not always possible when using Doctrine with Symfony, because bindRequest does everything under the hood.
A function "setShouldPerformDeletesFirst" as described in http://docs.oracle.com/cd/B31017_01/web.1013/b28218/uowadv.htm#i1137484 would be very useful in those cases.
Without it, I need to mess with detach() and merge() to keep only the things I want to remove() attached, flush() and then merge() everything back before a second flush().

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented Aug 10, 2015

Comment created by magnetik:

I'm facing the exact same issue 4 years later. Is ther any thing new to help with this situation?

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented Aug 10, 2015

Comment created by @Ocramius:

[~magnetik] the issue is marked as won't fix

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented Sep 15, 2015

Comment created by deatheriam:

When there is a complicated system of flush event handlers that involves sending requests to independent systems, having multiple flushes to solve this particular issue creates a lot of other issues on its own. Also it goes against Doctrine philosophy to issue as few flushes as possible.

I checked one of two links (the other one is dead) posted above that supposedly explains why deletes should go last in a transaction, and did not find any meaningful explanation, a quick Google search also yielded a few discussions without a clear answer, for example this one: https://forum.hibernate.org/viewtopic.php?t=934483.

@marco, could you re-state why this cannot be achieved without sending any future readers of this ticket to some articles?

@doctrinebot

This comment has been minimized.

Copy link
Author

@doctrinebot doctrinebot commented Sep 15, 2015

Comment created by deatheriam:

At least give us, users an option to use this approach:

{quote}Using the Unit of Work setShouldPerformDeletesFirst Method

By default, TopLink does insert and update operations first, before delete operations, to ensure that referential integrity is maintained. This is the preferred approach.

If you are forced to replace an object with unique constraints by deleting it and inserting a replacement, you may cause a constraint violation if the insert operation occurs before the delete operation. In this case, call setShouldPerformDeletesFirst to perform the delete operation before the insert operation.{quote}

@BenMorel

This comment has been minimized.

Copy link
Contributor

@BenMorel BenMorel commented Apr 21, 2016

I'd like to emphasize that this really is a pain when you deal with collections, that you want to clear() and add() to again. You can usually not flush() in between, as the whole operation happens in your model, with no knowledge of the EntityManager. So if your table contains unique constraints, you're screwed because the inserts are performed before the deletes. I've had this problem on pretty much every project I've used Doctrine on.

The explanation of the current order of operations lies in this link:

Deletes are general performed last, and an update may be required in order to allow deletion because of constraints.

One could argue that you could then do UPDATE, DELETE, then INSERT. But if an UPDATE needs an INSERT, you're screwed.

So there is no one-size-fits-all solution to this problem.

What would be good is, indeed, a configuration option to force DELETE before INSERT, at least in a collection.

@ureimers

This comment has been minimized.

Copy link
Contributor

@ureimers ureimers commented Dec 2, 2016

Even though this whole issue relates to the edge case of replacing entities with unique constraints with one another, "won't fix" won't make it go away. The current workarounds look horrifying, to say the least, and there really should be a better way to solve this problem.

Theoretically:
As Doctrine knows about the unique constraints of entities it could automatically check if the current unit of work contains an entity (A) with unique constraints that gets replaced by another entity (B) of the same class and with the same values in the constrained fields. In that case it needs to delete A before inserting B and everything should be fine. Or am I missing something?

@mastir

This comment has been minimized.

Copy link

@mastir mastir commented Feb 6, 2020

3 years gone and this is not solved. To "replace" entity in my project i do DBAL delete query before persist (in my case no need to load entity). ORM is simple unable to handle it. This is fine for simple entities, but when it comes from cascade persist combined with orphan remove it is so messed and hard to debug.

So there is no one-size-fits-all solution to this problem.

Actually there is one, not so simple but real solution: do not split INSERT / UPDATE / DELETE operations, but build operation order based on requirements. First run queries that has no need in chained operations. Then loop next queries that has all requirements solved and continue till all work is done. And for sure circular dependencies can be detected and solved by splitting into two INSERT/UPDATE queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.