From 19bb72e7c740855ee8b02227a919e013899db013 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Fri, 29 Apr 2022 14:01:15 +1200 Subject: [PATCH] FIX Correctly remove relations with ManyManyThroughList::removeall Instead of just setting one side of the relation to null in the through list, remove the rows entirely. Remove only the relations which match the filters that have already been set on the list. This is consistent with the way ManyManyList works. Also some small tidy-up (removing an unnecessary line break and an unused "use" statement) --- src/ORM/ManyManyList.php | 1 - src/ORM/ManyManyThroughList.php | 26 ++++++--- src/ORM/ManyManyThroughQueryManipulator.php | 1 - tests/php/ORM/ManyManyThroughListTest.php | 58 +++++++++++++++++++++ tests/php/ORM/ManyManyThroughListTest.yml | 15 +++++- 5 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/ORM/ManyManyList.php b/src/ORM/ManyManyList.php index 56089e57024..11b7bfeb8e5 100644 --- a/src/ORM/ManyManyList.php +++ b/src/ORM/ManyManyList.php @@ -389,7 +389,6 @@ public function removeByID($itemID) */ public function removeAll() { - // Remove the join to the join table to avoid MySQL row locking issues. $query = $this->dataQuery(); $foreignFilter = $query->getQueryParam('Foreign.Filter'); diff --git a/src/ORM/ManyManyThroughList.php b/src/ORM/ManyManyThroughList.php index 3f2b41c911c..fcba3e61a3e 100644 --- a/src/ORM/ManyManyThroughList.php +++ b/src/ORM/ManyManyThroughList.php @@ -130,27 +130,37 @@ public function removeByID($itemID) // Find has_many row with a local key matching the given id $hasManyList = $this->manipulator->getParentRelationship($this->dataQuery()); $records = $hasManyList->filter($this->manipulator->getLocalKey(), $itemID); - $affectedIds = []; // Rather than simple un-associating the record (as in has_many list) // Delete the actual mapping row as many_many deletions behave. /** @var DataObject $record */ foreach ($records as $record) { - $affectedIds[] = $record->ID; $record->delete(); } - if ($this->removeCallbacks && $affectedIds) { - $this->removeCallbacks->call($this, $affectedIds); + if ($this->removeCallbacks && $itemID) { + $this->removeCallbacks->call($this, [$itemID]); } } public function removeAll() { - // Empty has_many table matching the current foreign key - $hasManyList = $this->manipulator->getParentRelationship($this->dataQuery()); - $affectedIds = $hasManyList->column('ID'); - $hasManyList->removeAll(); + // Get the IDs of records in the current list + $affectedIds = $this->limit(null)->column('ID'); + if (empty($affectedIds)) { + return; + } + + // Get the join records that apply for the current list + $records = $this->manipulator->getJoinClass()::get()->filter([ + $this->manipulator->getForeignIDKey() => $this->getForeignID(), + $this->manipulator->getLocalKey() => $affectedIds, + ]); + + /** @var DataObject $record */ + foreach ($records as $record) { + $record->delete(); + } if ($this->removeCallbacks && $affectedIds) { $this->removeCallbacks->call($this, $affectedIds); diff --git a/src/ORM/ManyManyThroughQueryManipulator.php b/src/ORM/ManyManyThroughQueryManipulator.php index 93ff393f4aa..6f3c1a8d362 100644 --- a/src/ORM/ManyManyThroughQueryManipulator.php +++ b/src/ORM/ManyManyThroughQueryManipulator.php @@ -3,7 +3,6 @@ namespace SilverStripe\ORM; -use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\Deprecation; diff --git a/tests/php/ORM/ManyManyThroughListTest.php b/tests/php/ORM/ManyManyThroughListTest.php index 04a3616b247..b73a48f0d9f 100644 --- a/tests/php/ORM/ManyManyThroughListTest.php +++ b/tests/php/ORM/ManyManyThroughListTest.php @@ -202,6 +202,64 @@ public function testRemove() ); } + public function testRemoveAll() + { + $first = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1'); + $first->Items()->add($this->objFromFixture(ManyManyThroughListTest\Item::class, 'child0')); + $second = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent2'); + + $firstItems = $first->Items(); + $secondItems = $second->Items(); + $initialJoins = ManyManyThroughListTest\JoinObject::get()->count(); + $initialItems = ManyManyThroughListTest\Item::get()->count(); + $initialRelations = $firstItems->count(); + $initialSecondListRelations = $secondItems->count(); + + $firstItems->removeAll(); + + // Validate all items were removed from the first list, but none were removed from the second list + $this->assertEquals(0, count($firstItems)); + $this->assertEquals($initialSecondListRelations, count($secondItems)); + + // Validate that the JoinObjects were actually removed from the database + $this->assertEquals($initialJoins - $initialRelations, ManyManyThroughListTest\JoinObject::get()->count()); + + // Confirm Item objects were not removed from the database + $this->assertEquals($initialItems, ManyManyThroughListTest\Item::get()->count()); + } + + public function testRemoveAllIgnoresLimit() + { + $parent = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1'); + $parent->Items()->add($this->objFromFixture(ManyManyThroughListTest\Item::class, 'child0')); + $initialJoins = ManyManyThroughListTest\JoinObject::get()->count(); + // Validate there are enough items in the relation for this test + $this->assertTrue($initialJoins > 1); + + $items = $parent->Items()->Limit(1); + $items->removeAll(); + + // Validate all items were removed from the list - not only one + $this->assertEquals(0, count($items)); + } + + public function testFilteredRemoveAll() + { + $parent = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1'); + $parent->Items()->add($this->objFromFixture(ManyManyThroughListTest\Item::class, 'child0')); + $items = $parent->Items(); + $initialJoins = ManyManyThroughListTest\JoinObject::get()->count(); + $initialRelations = $items->count(); + + $items->filter('Title:not', 'not filtered')->removeAll(); + + // Validate only the filtered items were removed + $this->assertEquals(1, $items->count()); + + // Validate the list only contains the correct remaining item + $this->assertEquals(['not filtered'], $items->column('Title')); + } + /** * Test validation * diff --git a/tests/php/ORM/ManyManyThroughListTest.yml b/tests/php/ORM/ManyManyThroughListTest.yml index c7c3c3c2a1e..f1549fe8935 100644 --- a/tests/php/ORM/ManyManyThroughListTest.yml +++ b/tests/php/ORM/ManyManyThroughListTest.yml @@ -1,7 +1,12 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject: parent1: Title: 'my object' + parent2: + Title: 'my object2' SilverStripe\ORM\Tests\ManyManyThroughListTest\Item: + # Having this one first means the IDs of records aren't the same as the IDs of the join objects. + child0: + Title: 'not filtered' child1: Title: 'item 1' child2: @@ -17,6 +22,14 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\JoinObject: Sort: 2 Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject.parent1 Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child2 + join3: + Title: 'join 3' + Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject.parent2 + Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child1 + join4: + Title: 'join 4' + Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject.parent2 + Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child2 SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyObjectA: obja1: Title: 'object A1' @@ -75,4 +88,4 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\FallbackLocale: mexico_argentina: Sort: 1 Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.mexico - Locale: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.argentina \ No newline at end of file + Locale: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.argentina