Fix commit order #8349
Fix commit order #8349
Conversation
This fixes a commit order when persisting many entities with cyclic associations within one flush. Before the fix, when multiple cycles were presented in the precedence graph and more than two classes existed in within a cycle the calculated commit order was likely to be wrong, resulting in not null constraint violation when saving to a database. An example of such situation is provided in CommitOrderCalculatorTest::testCommitOrdering4() - this is a test case with won’t pass without the introduced fix. Prior to the fix the weight of an edge in a class precedence graph was compared only to the weight of the subsequent edges. Therefore, in case of cycles containing more than two classes the result was unpredictable. The fixed code uses also the weight of further nodes in the precedence graph. It is achieved by adding minimum weight argument to the visitor function of CommitOrderCalulator – edges having weight below the minimum weight are ignored. The minimum weight is increased each time a node is revisited, i. e. a cycle is detected. It guarantees that edges of highest priority representing not nullable relationships are processed first.
It may happen that an entity class A has multiple many-to-one associations with the same target entity B. These associations may differ in “nullable” parameter. In such situation CommitOrderCalculator::addDependency() is called multiple times for the same pair of $fromHash and $toHash, with different value of $weight argument. Before the fix the subsequent call to CommitOrderCalculator::addDependency() for the same $fromHash and $toHash overrode $weight set at the previous call. It caused unpredictable commit order of entities which resulted in not null constraint violation when saving to a database. Fixed code preserves the highest value of weight argument. If at subsequent call the weight is lower than previously established weight the addition of edge to the dependency list is skipped.
greg0ire
force-pushed
the
2.7
branch
3 times, most recently
from
e531738
to
66c95a6
Compare
|
This PR fixes #7866. @tomaszgaw: if you are interested in adding a test for this, it can be found here: https://gist.github.com/jhony1104/3c0793be7f1ee2fb9b933e1ee3734782 What is the status of this PR? |
Well, it has been open for a while. If it's still relevant, it needs a rebase and a functional test that reproduces the issues that it tries to fix. |
|
We also have a bunch of commit order related PRs already. Its very complex algo with all rhe side effects and requirements, therefore it requires lots of maintainer time to review and trust to merge. |
|
The product I am working on runs in a lof of issues with the commit order calculator. Because of this we are using a modified fork of doctrine wich changes the commit order calculator.These changes were made to 2.9.6. A few weeks ago I war updating doctrine and since the plain doctrine was still having the same issues and our old patch was also no longer working as expeted I tried some of the PRs on this repos. I am sorry to report, that this PR can - in some cases - still calculate the wrong commit order. In fact, I am not sure if the algorithm used (modified topolocical sorting with weights) can - without larger changes than modifying weights - ever reliably calculate the correct commit order. For the software I am working on, I did create a new patch that treats all edges as required (not nullable) and on detecting a loop backtracks to the last optional (nullable) edge, skips it an continues. This is of course much less performant, but should allways be correct. It can be found here. I am not sure if I should create a PR for it (performance, larger changes, ...). |
|
@jhony1104 and @tomaszgaw you might be valuable reviewing #10547. |
Fixes visit method of CommitOrderCalculator.
Without the fix the insertion order is not properly calculated when a cycle within the graph involves more than 2 classes, leading to a violation of not null constraints on the owning side of ManyToOne relation.
With the fix when a cycle is detected a node is revisited ignoring edges of weight 0.