#6499 commit order must consider non-nullability of join columns as prioritised over nullable join columns #6533
Conversation
| /** | ||
| * DDC6499A constructor. | ||
| */ | ||
| public function __construct() |
There was a problem hiding this comment.
Can you drop all API that isn't strictly needed? Getters, setters, etc
| */ | ||
| public function __construct() | ||
| { | ||
| $this->bs = new ArrayCollection(); |
There was a problem hiding this comment.
Use = [] as default property value (test is simple enough) ;-)
| /** | ||
| * Test for the bug described in issue #6499. | ||
| */ | ||
| public function testIssue() |
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| protected function setUp() |
| $this->_em->flush(); | ||
|
|
||
| // Issue #6499 will result in a Integrity constraint violation before reaching this point | ||
| $this->assertEquals(true, true); |
There was a problem hiding this comment.
This should be dropped - instead, try a find after a clear, and verify that the collection and the association are correctly populated. Put the comment in the assertion message
| private $id; | ||
|
|
||
| /** | ||
| * @OneToMany(targetEntity="DDC6499B", mappedBy="a", cascade={"persist", "remove"}, orphanRemoval=true) |
There was a problem hiding this comment.
Drop all cascade operations, orphan removals and properties/mappings that aren't strictly related to the tests
| @@ -1154,7 +1154,7 @@ private function getCommitOrder(array $entityChangeSet = null) | |||
|
|
|||
| $joinColumns = reset($assoc['joinColumns']); | |||
|
|
|||
| $calc->addDependency($targetClass->name, $class->name, (int)empty($joinColumns['nullable'])); | |||
| $calc->addDependency($targetClass->name, $class->name, (int) ! ( ! isset($joinColumns['nullable']) || $joinColumns['nullable'] === true)); | |||
There was a problem hiding this comment.
Lots of negations. Also, this is only checking the first join column... Possibly requiring to add tests around this area. :-\
There was a problem hiding this comment.
I think it makes sense to also cover this on the unit level (UnitOfWorkTest)
There was a problem hiding this comment.
Let's keep it on integration tests for now - unit testing the UoW is a bit more messy, and we can add that just before merging, rather than early during the fixing process
There was a problem hiding this comment.
I just fixed the other requested changes, causing the example to be a lot smaller. This proposed change fixes our broken test, but consequently breaks two other tests. I will look a bit further into this when I have some spare time.
@Ocramius Do you agree that there is a logic inconsistency in the the original line in UnitOfWork.php?
|
I'll adjust the above suggestions, fix Scrutinizer improvements and run some more tests locally. |
-- Processed Ocramius' feedback.
|
Just for the record: the file's history shows that the current logic
originates from 27e9b49#diff-6e8c1c1e78b054ba05e20ea09d877865L1158 as a simplification of
(actually with an intermediate variable so the present PR is like saying that the
or alternatively
all being equivalent to the currently proposed fix just without the Now, as you already noted, this is unfortunately causing exceptions in an existing test, and the original logic was precisely a fix for the affected test. |
|
@guilliamxavier Thanks for clarifying the file's history. As said, I'm not that familiar with the codebase and when digging into it I also could not find out what causes the other two tests to fail. It might be that this proposed fix isn't correct for this issue, but then the question remains: what causes issue #6499 to arise? Or is #6499 intended behavior? |
|
Any news on this issue? |
|
@triasinformatica-gabe adding it to my bucket of stuff to check |
|
@Ocramius thanks. If there's anything we (me or my colleagues) can do to help, please let us know. But currently we're unfamiliar with the history of this part of the doctrine codebase. |
… of what happened at persistence-level
…er persistence in some edge cases
…r makes it *SOMEWHAT* more readable (no miracles)
Ocramius
changed the title
|
@triasinformatica-gabe I manually merged after some cleanups and moving stuff around. @lcobucci I couldn't find a proper way to test this in isolation, as it's deep in UnitOfWork internals, sadly. This patch can only land in |
|
@triasinformatica-gabe sorry, I had to revert the merge - this patch introduces errors when deleting foreign key referenced data. I think this is much more complex than what it looks. To be clear, I know as much as you do about this issue. The problem area is exactly the one you worked on, except that the commit order isn't simply "reversible" for deletes (current approach). |
|
@bendavies see #6533 (comment) - this was reverted |
|
@Ocramius yeah thanks. I got confused as this PR was still open. was |
|
@bendavies no, we are lacking tests about cascade-deleted data, from what I remember (sorry, mind is also a bit clouded over here) |
|
@bendavies It's passing the tests, but only in SQLite, which is ignoring the foreign keys. If you run the test set in MySQL or another database that does enforce this, some other tests will fail. There seems to be a non-trivial issue in building up the relationship dependency graph for cascades. I didn't have time so far to look into it, but it's still somewhere on my list. The fix this PR originates from was a bit too rigid and therefore not solving the issue. |
|
I ran against mysql and postres. They passed!
…On Mon, 5 Feb 2018, 18:51 Gabe van der Weijde, ***@***.***> wrote:
@bendavies <https://github.com/bendavies> It's passing the tests, but
only in SQLite, which is ignoring the foreign keys. If you run the test set
in MySQL or another database that does enforce this, some other tests will
fail. There seems to be a non-trivial issue in building up the relationship
dependency graph for cascades. I didn't have time so for to look into it,
but it's still somewhere on my list.
The fix this PR originates from was a bit to rigid and therefore not
solving the issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6533 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAmK8NGk2Bqc3AsLb455sPz8DVX3tO03ks5tR03NgaJpZM4OJHmg>
.
|
|
Then maybe some other fix has fixed this issue; I’ll look into it tomorrow morning if I have time and post back my findings. Should save Ocramius some time.
|
|
hey @triasinformatica-gabe. Any chance to look at this yet? |
|
@bendavies Sorry for the late response, haven't had time to feedback on this before since I was abroad for a while. Any way; I did run all the tests on top of the latest changes and for me all the same tests keep failing the same way as they did last summer, so I'm not sure how you managed to get it working running Postgress and MySQL. Could you elaborate? |
|
Hmm, I'll try again. |
|
@triasinformatica-gabe which test is failing for you please? |
@Ocramius can you give more context about that? Maybe creating an issue for us to track these tests (specially considering the changes in |
The problem is:
I also don't remember the details. |
@Ocramius I BELIEVE IN YOU, you can do it!!!! Didn't help, did it? HAHAHA alright, thanks! |
|
@triasinformatica-gabe trying to pick this up again....which test fails for you? |
@Frikkle was the first to propose these tests in doctrine#6533. @rvanlaak followed up in doctrine#8703, making some adjustments.
@Frikkle was the first to propose these tests in doctrine#6533. @rvanlaak followed up in doctrine#8703, making some adjustments. Co-authored-by: Gabe van der Weijde <[email protected]> Co-authored-by: Richard van Laak <[email protected]> Co-authored-by: Grégoire Paris <[email protected]>
As suggested by @lcobucci, I created a PR with a minimal test example.
While working on the test example I looked into the code and found the (probable) cause for this issue and proposed a fix for it in a second commit.
The issue seems to originate from the part of the code in UnitOfWork which calculates the weight for new dependencies in the graph based on whether or not the
nullableclause on join columns is set and to which value.In particular the part
(int)empty($joinColumns['nullable'])which to me seems incorrect. With the current implementation inmasterthe case where thenullableproperty on the join column is set to false (so, the relation is mandatory) yields the same weight as when thenullableproperty is not defined. But when thenullableproperty is not defined, the documention states that the default would benullable === true.Below, you find a simple truth table breakdown of the specific part of the code (for the possible values for
$joinColumns['nullable'].(int)empty( $joinColumns['nullable'] )I tried to keep the (in-line) fix as compact as possible. All test keep running and the added test for this issue also succeeds after applying the fix. Before applying the fix, it throws a ConstraintViolationException and tries to insert null for a mandatory foreign key.
Since I'm not completely familiar with the codebase, please look into whether this does not influence other parts of the code. If there's anything I can do to assist, let me know.