From ca3f3a7832a292023b683a345871c58623188b44 Mon Sep 17 00:00:00 2001 From: prateek banga Date: Tue, 1 Aug 2023 23:08:20 +0530 Subject: [PATCH 1/3] fix comparison check in updateDocument Previously the comparison check between old document and new document in updateDocument was not able to check if the key is a relationship key or not so it was creating issues when updating document with relationships --- src/Database/Database.php | 15 ++++++++++ tests/Database/Base.php | 61 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index b4fed64c9..285a267d0 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2881,6 +2881,9 @@ public function updateDocument(string $collection, string $id, Document $documen $old = Authorization::skip(fn () => $this->silent(fn () => $this->getDocument($collection, $id))); // Skip ensures user does not need read permission for this $collection = $this->silent(fn () => $this->getCollection($collection)); + $relationships = \array_filter($collection->getAttribute('attributes', []), function ($attribute) { + return $attribute['type'] === Database::VAR_RELATIONSHIP; + }); $validator = new Authorization(self::PERMISSION_UPDATE); $shouldUpdate = false; @@ -2888,13 +2891,23 @@ public function updateDocument(string $collection, string $id, Document $documen if ($collection->getId() !== self::METADATA) { $documentSecurity = $collection->getAttribute('documentSecurity', false); + $relationshipKeys = []; + foreach ($relationships as $relationship) { + $relationshipKey = $relationship->getAttribute('key'); + $relationshipKeys[$relationshipKey] = $relationshipKey; + } // Compare if the document has any changes foreach ($document as $key=>$value) { + // Skip the nested documents as they will be checked later in recursions. + if (array_key_exists($key, $relationshipKeys)) { + continue; + } // Skip the nested documents as they will be checked later in recursions. $oldAttributeValue = $old->getAttribute($key); if ($oldAttributeValue instanceof Document) { continue; } + // If values are not equal we need to update document. if ($oldAttributeValue !== $value) { $shouldUpdate = true; break; @@ -2910,6 +2923,8 @@ public function updateDocument(string $collection, string $id, Document $documen if ($shouldUpdate) { $document->setAttribute('$updatedAt', $time); + } else { + $document->setAttribute('$updatedAt', $old->getUpdatedAt()); } // Check if document was updated after the request timestamp diff --git a/tests/Database/Base.php b/tests/Database/Base.php index 313985065..d9f61338d 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -3064,6 +3064,65 @@ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): return $document; } + public function testNoChangeUpdateDocumentWithRelationWithoutPermission(): void + { + if (!static::getDatabase()->getAdapter()->getSupportForRelationships()) { + $this->expectNotToPerformAssertions(); + return; + } + + Authorization::cleanRoles(); + Authorization::setRole(Role::user('a')->toString()); + + static::getDatabase()->createCollection('parentRelationTest', [], [], [ + Permission::read(Role::user('a')), + Permission::create(Role::user('a')), + ]); + static::getDatabase()->createCollection('childRelationTest', [], [], [ + Permission::read(Role::user('a')), + Permission::create(Role::user('a')), + ]); + static::getDatabase()->createAttribute('parentRelationTest', 'name', Database::VAR_STRING, 255, false); + static::getDatabase()->createAttribute('childRelationTest', 'name', Database::VAR_STRING, 255, false); + + static::getDatabase()->createRelationship( + collection: 'parentRelationTest', + relatedCollection: 'childRelationTest', + type: Database::RELATION_ONE_TO_MANY, + id: 'childs' + ); + + // Create document with relationship with nested data + $parent = static::getDatabase()->createDocument('parentRelationTest', new Document([ + '$id' => 'parent1', + 'name' => 'Parent 1', + 'childs' => [ + [ + '$id' => 'child1', + 'name' => 'Child 1', + ], + ], + ])); + $this->assertEquals(1, \count($parent['childs'])); + $updatedParent = static::getDatabase()->updateDocument('parentRelationTest', 'parent1', new Document([ + '$id' => 'parent1', + 'name'=>'Parent 1', + '$collection' => 'parentRelationTest', + 'childs' => [ + new Document([ + '$id' => 'child1', + '$collection' => 'childRelationTest' + ]), + ] + ])); + + $this->assertEquals($updatedParent->getUpdatedAt(), $parent->getUpdatedAt()); + $this->assertEquals($updatedParent->getAttribute('childs')[0]->getUpdatedAt(), $parent->getAttribute('childs')[0]->getUpdatedAt()); + + static::getDatabase()->deleteCollection('parentRelationTest'); + static::getDatabase()->deleteCollection('childRelationTest'); + } + public function testExceptionAttributeLimit(): void { if ($this->getDatabase()->getLimitForAttributes() > 0) { @@ -11216,7 +11275,7 @@ public function testCollectionPermissionsRelationshipsUpdateThrowsException(arra $document = static::getDatabase()->updateDocument( $collection->getId(), $document->getId(), - $document->setAttribute('test', 'ipsum') + $document->setAttribute('test', $document->getAttribute('test').'new_value') ); } From 6bc2a415fafa173b0899da5b395eb399bd8a5e55 Mon Sep 17 00:00:00 2001 From: prateek banga Date: Tue, 1 Aug 2023 23:40:06 +0530 Subject: [PATCH 2/3] removes unnecessary check --- src/Database/Database.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 285a267d0..dd4aa9c5f 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2902,11 +2902,8 @@ public function updateDocument(string $collection, string $id, Document $documen if (array_key_exists($key, $relationshipKeys)) { continue; } - // Skip the nested documents as they will be checked later in recursions. + $oldAttributeValue = $old->getAttribute($key); - if ($oldAttributeValue instanceof Document) { - continue; - } // If values are not equal we need to update document. if ($oldAttributeValue !== $value) { $shouldUpdate = true; From 0d9a6482775a3bcf05c51e6b6248ca6a995e9456 Mon Sep 17 00:00:00 2001 From: prateek banga Date: Thu, 3 Aug 2023 12:15:07 +0530 Subject: [PATCH 3/3] fix lint issues --- src/Database/Database.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index dd4aa9c5f..fd4fdcd73 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2894,7 +2894,7 @@ public function updateDocument(string $collection, string $id, Document $documen $relationshipKeys = []; foreach ($relationships as $relationship) { $relationshipKey = $relationship->getAttribute('key'); - $relationshipKeys[$relationshipKey] = $relationshipKey; + $relationshipKeys[$relationshipKey] = $relationshipKey; } // Compare if the document has any changes foreach ($document as $key=>$value) {