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 56089e570..11b7bfeb8 100644 --- a/src/ORM/ManyManyList.php +++ b/src/ORM/ManyManyList.php @@ -389,7 +389,6 @@ class ManyManyList extends RelationList */ 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 3f2b41c91..fcba3e61a 100644 --- a/src/ORM/ManyManyThroughList.php +++ b/src/ORM/ManyManyThroughList.php @@ -130,27 +130,37 @@ class ManyManyThroughList extends RelationList // 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 93ff393f4..6f3c1a8d3 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 04a3616b2..b73a48f0d 100644 --- a/tests/php/ORM/ManyManyThroughListTest.php +++ b/tests/php/ORM/ManyManyThroughListTest.php @@ -202,6 +202,64 @@ class ManyManyThroughListTest extends SapphireTest ); } + 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 c7c3c3c2a..f1549fe89 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