From f49fa56a7404bc0e60e127535fd3af98d69ffce8 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 2 May 2023 14:27:45 +1200 Subject: [PATCH 1/3] Fix cascading deletes across multiple levels --- src/Database/Database.php | 172 +++++++++++++++++++++++++++++++------- tests/Database/Base.php | 65 ++++++++++++++ 2 files changed, 207 insertions(+), 30 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index e9bd5b1b6..7c8f62175 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -265,6 +265,7 @@ class Database protected bool $resolveRelationships = true; private int $relationshipFetchDepth = 1; + private int $relationshipDeleteDepth = 1; /** * Stack of collection IDs when creating or updating related documents @@ -273,10 +274,15 @@ class Database private array $relationshipWriteStack = []; /** - * @var array> + * @var array */ private array $relationshipFetchMap = []; + /** + * @var array + */ + private array $relationshipDeleteMap = []; + /** * @param Adapter $adapter * @param Cache $cache @@ -2237,11 +2243,9 @@ private function populateDocumentRelationships(Document $collection, Document $d { $attributes = $collection->getAttribute('attributes', []); - $relationships = \array_filter( - $attributes, - fn ($attribute) => - $attribute['type'] === Database::VAR_RELATIONSHIP - ); + $relationships = \array_filter($attributes, function ($attribute) { + return $attribute['type'] === Database::VAR_RELATIONSHIP; + }); foreach ($relationships as $relationship) { $key = $relationship['key']; @@ -2881,7 +2885,6 @@ private function updateDocumentRelationships(Document $collection, Document $old if ($stackCount >= Database::RELATION_MAX_DEPTH - 1 && $this->relationshipWriteStack[$stackCount - 1] !== $relatedCollection->getId()) { $document->removeAttribute($key); - continue; } @@ -3377,6 +3380,7 @@ public function deleteDocument(string $collection, string $id): bool /** * @throws Exception + * @throws Throwable */ private function deleteDocumentRelationships(Document $collection, Document $document): Document { @@ -3396,6 +3400,13 @@ private function deleteDocumentRelationships(Document $collection, Document $doc $onDelete = $relationship['options']['onDelete']; $side = $relationship['options']['side']; + $relationship->setAttribute('collection', $collection->getId()); + $relationship->setAttribute('document', $document->getId()); + + if ($this->relationshipDeleteDepth >= Database::RELATION_MAX_DEPTH) { + return $document; + } + switch ($onDelete) { case Database::RELATION_MUTATE_RESTRICT: $this->deleteRestrict($relatedCollection, $document, $value, $relationType, $twoWay, $twoWayKey, $side); @@ -3404,7 +3415,51 @@ private function deleteDocumentRelationships(Document $collection, Document $doc $this->deleteSetNull($collection, $relatedCollection, $document, $value, $relationType, $twoWay, $twoWayKey, $side); break; case Database::RELATION_MUTATE_CASCADE: - $this->deleteCascade($collection, $relatedCollection, $document, $key, $value, $relationType, $twoWay, $twoWayKey, $side); + foreach ($this->relationshipDeleteMap as $processedRelationship) { + $existingKey = $processedRelationship['key']; + $existingCollection = $processedRelationship['collection']; + $existingRelatedCollection = $processedRelationship['options']['relatedCollection']; + $existingTwoWayKey = $processedRelationship['options']['twoWayKey']; + $existingSide = $processedRelationship['options']['side']; + + // If this relationship has already been fetched for this document, skip it + $reflexive = $processedRelationship == $relationship; + + // If this relationship is the same as a previously fetched relationship, but on the other side, skip it + $symmetric = $existingKey === $twoWayKey + && $existingTwoWayKey === $key + && $existingRelatedCollection === $collection->getId() + && $existingCollection === $relatedCollection->getId() + && $existingSide !== $side; + + // If this relationship is not directly related but relates across multiple collections, skip it. + // + // These conditions ensure that a relationship is considered transitive if it has the same + // two-way key and related collection, but is on the opposite side of the relationship (the first and second conditions). + // + // They also ensure that a relationship is considered transitive if it has the same key and related + // collection as an existing relationship, but a different two-way key (the third condition), + // or the same two-way key as an existing relationship, but a different key (the fourth condition). + $transitive = (($existingKey === $twoWayKey + && $existingCollection === $relatedCollection->getId() + && $existingSide !== $side) + || ($existingTwoWayKey === $key + && $existingRelatedCollection === $collection->getId() + && $existingSide !== $side) + || ($existingKey === $key + && $existingTwoWayKey !== $twoWayKey + && $existingRelatedCollection === $relatedCollection->getId() + && $existingSide !== $side) + || ($existingKey !== $key + && $existingTwoWayKey === $twoWayKey + && $existingRelatedCollection === $relatedCollection->getId() + && $existingSide !== $side)); + + if ($reflexive || $symmetric || $transitive) { + break 2; + } + } + $this->deleteCascade($collection, $relatedCollection, $document, $key, $value, $relationType, $twoWayKey, $side, $relationship); break; } } @@ -3414,9 +3469,17 @@ private function deleteDocumentRelationships(Document $collection, Document $doc /** * @throws Exception + * @throws Throwable */ - private function deleteRestrict(Document $relatedCollection, Document $document, mixed $value, string $relationType, bool $twoWay, string $twoWayKey, string $side): void - { + private function deleteRestrict( + Document $relatedCollection, + Document $document, + mixed $value, + string $relationType, + bool $twoWay, + string $twoWayKey, + string $side + ): void { if ($value instanceof Document && $value->isEmpty()) { $value = null; } @@ -3443,6 +3506,7 @@ private function deleteRestrict(Document $relatedCollection, Document $document, return; } + $this->skipRelationships(fn () => $this->updateDocument( $relatedCollection->getId(), $related->getId(), @@ -3465,8 +3529,16 @@ private function deleteRestrict(Document $relatedCollection, Document $document, } } - private function deleteSetNull(Document $collection, Document $relatedCollection, Document $document, mixed $value, string $relationType, bool $twoWay, string $twoWayKey, string $side): void - { + private function deleteSetNull( + Document $collection, + Document $relatedCollection, + Document $document, + mixed $value, + string $relationType, + bool $twoWay, + string $twoWayKey, + string $side + ): void { switch ($relationType) { case Database::RELATION_ONE_TO_ONE: if (!$twoWay && $side === Database::RELATION_SIDE_PARENT) { @@ -3553,28 +3625,55 @@ private function deleteSetNull(Document $collection, Document $relatedCollection } } - private function deleteCascade(Document $collection, Document $relatedCollection, Document $document, string $key, mixed $value, string $relationType, bool $twoWay, string $twoWayKey, string $side): void - { + /** + * @throws AuthorizationException + * @throws ConflictException + * @throws Throwable + */ + private function deleteCascade( + Document $collection, + Document $relatedCollection, + Document $document, + string $key, + mixed $value, + string $relationType, + string $twoWayKey, + string $side, + Document $relationship + ): void { switch ($relationType) { case Database::RELATION_ONE_TO_ONE: if ($value !== null) { - $this->skipRelationships(fn () => + $this->relationshipDeleteDepth++; + $this->relationshipDeleteMap[] = $relationship; + $this->deleteDocument( $relatedCollection->getId(), $value->getId() - )); + ); + + $this->relationshipDeleteDepth--; + \array_pop($this->relationshipDeleteMap); } break; case Database::RELATION_ONE_TO_MANY: if ($side === Database::RELATION_SIDE_CHILD) { break; } + + $this->relationshipDeleteDepth++; + $this->relationshipDeleteMap[] = $relationship; + foreach ($value as $relation) { - $this->skipRelationships(fn () => $this->deleteDocument( + $this->deleteDocument( $relatedCollection->getId(), $relation->getId() - )); + ); } + + $this->relationshipDeleteDepth--; + \array_pop($this->relationshipDeleteMap); + break; case Database::RELATION_MANY_TO_ONE: if ($side === Database::RELATION_SIDE_PARENT) { @@ -3586,12 +3685,19 @@ private function deleteCascade(Document $collection, Document $relatedCollection Query::limit(PHP_INT_MAX) ]); + $this->relationshipDeleteDepth++; + $this->relationshipDeleteMap[] = $relationship; + foreach ($value as $relation) { - $this->skipRelationships(fn () => $this->deleteDocument( + $this->deleteDocument( $relatedCollection->getId(), $relation->getId() - )); + ); } + + $this->relationshipDeleteDepth--; + \array_pop($this->relationshipDeleteMap); + break; case Database::RELATION_MANY_TO_MANY: $junction = $this->getJunctionCollection($collection, $relatedCollection, $side); @@ -3601,20 +3707,25 @@ private function deleteCascade(Document $collection, Document $relatedCollection Query::limit(PHP_INT_MAX) ]); + $this->relationshipDeleteDepth++; + $this->relationshipDeleteMap[] = $relationship; + foreach ($junctions as $document) { - $this->skipRelationships(function () use ($document, $junction, $relatedCollection, $key, $side) { - if ($side === Database::RELATION_SIDE_PARENT) { - $this->deleteDocument( - $relatedCollection->getId(), - $document->getAttribute($key) - ); - } + if ($side === Database::RELATION_SIDE_PARENT) { $this->deleteDocument( - $junction, - $document->getId() + $relatedCollection->getId(), + $document->getAttribute($key) ); - }); + } + $this->deleteDocument( + $junction, + $document->getId() + ); } + + $this->relationshipDeleteDepth--; + \array_pop($this->relationshipDeleteMap); + break; } } @@ -3625,6 +3736,7 @@ private function deleteCascade(Document $collection, Document $relatedCollection * @param string $collection * * @return bool + * @throws Exception */ public function deleteCachedCollection(string $collection): bool { diff --git a/tests/Database/Base.php b/tests/Database/Base.php index 83f8b3027..cca1848ba 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -10328,6 +10328,71 @@ public function testManyToManyRelationshipKeyWithSymbols(): void $this->assertEquals($doc1->getId(), $doc2->getAttribute('$symbols_coll.ection8')[0]->getId()); } + public function testCascadeMultiDelete(): void + { + if (!static::getDatabase()->getAdapter()->getSupportForRelationships()) { + $this->expectNotToPerformAssertions(); + return; + } + + static::getDatabase()->createCollection('cascadeMultiDelete1'); + static::getDatabase()->createCollection('cascadeMultiDelete2'); + static::getDatabase()->createCollection('cascadeMultiDelete3'); + + static::getDatabase()->createRelationship( + collection: 'cascadeMultiDelete1', + relatedCollection: 'cascadeMultiDelete2', + type: Database::RELATION_ONE_TO_MANY, + twoWay: true, + onDelete: Database::RELATION_MUTATE_CASCADE + ); + + static::getDatabase()->createRelationship( + collection: 'cascadeMultiDelete2', + relatedCollection: 'cascadeMultiDelete3', + type: Database::RELATION_ONE_TO_MANY, + twoWay: true, + onDelete: Database::RELATION_MUTATE_CASCADE + ); + + $root = static::getDatabase()->createDocument('cascadeMultiDelete1', new Document([ + '$id' => 'cascadeMultiDelete1', + '$permissions' => [ + Permission::read(Role::any()), + Permission::delete(Role::any()) + ], + 'cascadeMultiDelete2' => [ + [ + '$id' => 'cascadeMultiDelete2', + '$permissions' => [ + Permission::read(Role::any()), + Permission::delete(Role::any()) + ], + 'cascadeMultiDelete3' => [ + [ + '$id' => 'cascadeMultiDelete3', + '$permissions' => [ + Permission::read(Role::any()), + Permission::delete(Role::any()) + ], + ], + ], + ], + ], + ])); + + $this->assertCount(1, $root->getAttribute('cascadeMultiDelete2')); + $this->assertCount(1, $root->getAttribute('cascadeMultiDelete2')[0]->getAttribute('cascadeMultiDelete3')); + + $this->assertEquals(true, static::getDatabase()->deleteDocument('cascadeMultiDelete1', $root->getId())); + + $multi2 = static::getDatabase()->getDocument('cascadeMultiDelete2', 'cascadeMultiDelete2'); + $this->assertEquals(true, $multi2->isEmpty()); + + $multi3 = static::getDatabase()->getDocument('cascadeMultiDelete3', 'cascadeMultiDelete3'); + $this->assertEquals(true, $multi3->isEmpty()); + } + public function testCollectionUpdate(): Document { $collection = static::getDatabase()->createCollection('collectionUpdate', permissions: [ From 6594bae765c442750ee29c3337e42f7c1cae7d92 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 16 May 2023 17:28:50 +1200 Subject: [PATCH 2/3] Rename db instance vars to more clearly reflect usage --- src/Database/Database.php | 48 +++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 7c8f62175..7514c644a 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -276,12 +276,12 @@ class Database /** * @var array */ - private array $relationshipFetchMap = []; + private array $relationshipFetchStack = []; /** * @var array */ - private array $relationshipDeleteMap = []; + private array $relationshipDeleteStack = []; /** * @param Adapter $adapter @@ -2268,7 +2268,7 @@ private function populateDocumentRelationships(Document $collection, Document $d $relationship->setAttribute('document', $document->getId()); $skipFetch = false; - foreach ($this->relationshipFetchMap as $fetchedRelationship) { + foreach ($this->relationshipFetchStack as $fetchedRelationship) { $existingKey = $fetchedRelationship['key']; $existingCollection = $fetchedRelationship['collection']; $existingRelatedCollection = $fetchedRelationship['options']['relatedCollection']; @@ -2325,12 +2325,12 @@ private function populateDocumentRelationships(Document $collection, Document $d } $this->relationshipFetchDepth++; - $this->relationshipFetchMap[] = $relationship; + $this->relationshipFetchStack[] = $relationship; $related = $this->getDocument($relatedCollection->getId(), $value, $queries); $this->relationshipFetchDepth--; - \array_pop($this->relationshipFetchMap); + \array_pop($this->relationshipFetchStack); $document->setAttribute($key, $related); break; @@ -2342,12 +2342,12 @@ private function populateDocumentRelationships(Document $collection, Document $d } if (!\is_null($value)) { $this->relationshipFetchDepth++; - $this->relationshipFetchMap[] = $relationship; + $this->relationshipFetchStack[] = $relationship; $related = $this->getDocument($relatedCollection->getId(), $value, $queries); $this->relationshipFetchDepth--; - \array_pop($this->relationshipFetchMap); + \array_pop($this->relationshipFetchStack); $document->setAttribute($key, $related); } @@ -2359,7 +2359,7 @@ private function populateDocumentRelationships(Document $collection, Document $d } $this->relationshipFetchDepth++; - $this->relationshipFetchMap[] = $relationship; + $this->relationshipFetchStack[] = $relationship; $relatedDocuments = $this->find($relatedCollection->getId(), [ Query::equal($twoWayKey, [$document->getId()]), @@ -2368,7 +2368,7 @@ private function populateDocumentRelationships(Document $collection, Document $d ]); $this->relationshipFetchDepth--; - \array_pop($this->relationshipFetchMap); + \array_pop($this->relationshipFetchStack); foreach ($relatedDocuments as $related) { $related->removeAttribute($twoWayKey); @@ -2387,12 +2387,12 @@ private function populateDocumentRelationships(Document $collection, Document $d break; } $this->relationshipFetchDepth++; - $this->relationshipFetchMap[] = $relationship; + $this->relationshipFetchStack[] = $relationship; $related = $this->getDocument($relatedCollection->getId(), $value, $queries); $this->relationshipFetchDepth--; - \array_pop($this->relationshipFetchMap); + \array_pop($this->relationshipFetchStack); $document->setAttribute($key, $related); break; @@ -2408,7 +2408,7 @@ private function populateDocumentRelationships(Document $collection, Document $d } $this->relationshipFetchDepth++; - $this->relationshipFetchMap[] = $relationship; + $this->relationshipFetchStack[] = $relationship; $relatedDocuments = $this->find($relatedCollection->getId(), [ Query::equal($twoWayKey, [$document->getId()]), @@ -2417,7 +2417,7 @@ private function populateDocumentRelationships(Document $collection, Document $d ]); $this->relationshipFetchDepth--; - \array_pop($this->relationshipFetchMap); + \array_pop($this->relationshipFetchStack); foreach ($relatedDocuments as $related) { @@ -2436,7 +2436,7 @@ private function populateDocumentRelationships(Document $collection, Document $d } $this->relationshipFetchDepth++; - $this->relationshipFetchMap[] = $relationship; + $this->relationshipFetchStack[] = $relationship; $junction = $this->getJunctionCollection($collection, $relatedCollection, $side); @@ -2455,7 +2455,7 @@ private function populateDocumentRelationships(Document $collection, Document $d } $this->relationshipFetchDepth--; - \array_pop($this->relationshipFetchMap); + \array_pop($this->relationshipFetchStack); $document->setAttribute($key, $related); break; @@ -3415,7 +3415,7 @@ private function deleteDocumentRelationships(Document $collection, Document $doc $this->deleteSetNull($collection, $relatedCollection, $document, $value, $relationType, $twoWay, $twoWayKey, $side); break; case Database::RELATION_MUTATE_CASCADE: - foreach ($this->relationshipDeleteMap as $processedRelationship) { + foreach ($this->relationshipDeleteStack as $processedRelationship) { $existingKey = $processedRelationship['key']; $existingCollection = $processedRelationship['collection']; $existingRelatedCollection = $processedRelationship['options']['relatedCollection']; @@ -3645,7 +3645,7 @@ private function deleteCascade( case Database::RELATION_ONE_TO_ONE: if ($value !== null) { $this->relationshipDeleteDepth++; - $this->relationshipDeleteMap[] = $relationship; + $this->relationshipDeleteStack[] = $relationship; $this->deleteDocument( $relatedCollection->getId(), @@ -3653,7 +3653,7 @@ private function deleteCascade( ); $this->relationshipDeleteDepth--; - \array_pop($this->relationshipDeleteMap); + \array_pop($this->relationshipDeleteStack); } break; case Database::RELATION_ONE_TO_MANY: @@ -3662,7 +3662,7 @@ private function deleteCascade( } $this->relationshipDeleteDepth++; - $this->relationshipDeleteMap[] = $relationship; + $this->relationshipDeleteStack[] = $relationship; foreach ($value as $relation) { $this->deleteDocument( @@ -3672,7 +3672,7 @@ private function deleteCascade( } $this->relationshipDeleteDepth--; - \array_pop($this->relationshipDeleteMap); + \array_pop($this->relationshipDeleteStack); break; case Database::RELATION_MANY_TO_ONE: @@ -3686,7 +3686,7 @@ private function deleteCascade( ]); $this->relationshipDeleteDepth++; - $this->relationshipDeleteMap[] = $relationship; + $this->relationshipDeleteStack[] = $relationship; foreach ($value as $relation) { $this->deleteDocument( @@ -3696,7 +3696,7 @@ private function deleteCascade( } $this->relationshipDeleteDepth--; - \array_pop($this->relationshipDeleteMap); + \array_pop($this->relationshipDeleteStack); break; case Database::RELATION_MANY_TO_MANY: @@ -3708,7 +3708,7 @@ private function deleteCascade( ]); $this->relationshipDeleteDepth++; - $this->relationshipDeleteMap[] = $relationship; + $this->relationshipDeleteStack[] = $relationship; foreach ($junctions as $document) { if ($side === Database::RELATION_SIDE_PARENT) { @@ -3724,7 +3724,7 @@ private function deleteCascade( } $this->relationshipDeleteDepth--; - \array_pop($this->relationshipDeleteMap); + \array_pop($this->relationshipDeleteStack); break; } From 0f3f93ac52684016224d7cda551324ff0529f800 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Tue, 16 May 2023 17:43:04 +1200 Subject: [PATCH 3/3] Don't check depth for cascading deletes --- src/Database/Database.php | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 7514c644a..0093663f3 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -265,7 +265,6 @@ class Database protected bool $resolveRelationships = true; private int $relationshipFetchDepth = 1; - private int $relationshipDeleteDepth = 1; /** * Stack of collection IDs when creating or updating related documents @@ -3403,10 +3402,6 @@ private function deleteDocumentRelationships(Document $collection, Document $doc $relationship->setAttribute('collection', $collection->getId()); $relationship->setAttribute('document', $document->getId()); - if ($this->relationshipDeleteDepth >= Database::RELATION_MAX_DEPTH) { - return $document; - } - switch ($onDelete) { case Database::RELATION_MUTATE_RESTRICT: $this->deleteRestrict($relatedCollection, $document, $value, $relationType, $twoWay, $twoWayKey, $side); @@ -3644,7 +3639,6 @@ private function deleteCascade( switch ($relationType) { case Database::RELATION_ONE_TO_ONE: if ($value !== null) { - $this->relationshipDeleteDepth++; $this->relationshipDeleteStack[] = $relationship; $this->deleteDocument( @@ -3652,7 +3646,6 @@ private function deleteCascade( $value->getId() ); - $this->relationshipDeleteDepth--; \array_pop($this->relationshipDeleteStack); } break; @@ -3661,7 +3654,6 @@ private function deleteCascade( break; } - $this->relationshipDeleteDepth++; $this->relationshipDeleteStack[] = $relationship; foreach ($value as $relation) { @@ -3671,7 +3663,6 @@ private function deleteCascade( ); } - $this->relationshipDeleteDepth--; \array_pop($this->relationshipDeleteStack); break; @@ -3685,7 +3676,6 @@ private function deleteCascade( Query::limit(PHP_INT_MAX) ]); - $this->relationshipDeleteDepth++; $this->relationshipDeleteStack[] = $relationship; foreach ($value as $relation) { @@ -3695,7 +3685,6 @@ private function deleteCascade( ); } - $this->relationshipDeleteDepth--; \array_pop($this->relationshipDeleteStack); break; @@ -3707,7 +3696,6 @@ private function deleteCascade( Query::limit(PHP_INT_MAX) ]); - $this->relationshipDeleteDepth++; $this->relationshipDeleteStack[] = $relationship; foreach ($junctions as $document) { @@ -3723,9 +3711,7 @@ private function deleteCascade( ); } - $this->relationshipDeleteDepth--; \array_pop($this->relationshipDeleteStack); - break; } }