From 3fe3bc7d51f0ca94236ad4d62f044161e34cb469 Mon Sep 17 00:00:00 2001 From: Prateek Banga Date: Mon, 17 Jul 2023 22:50:44 +0530 Subject: [PATCH 1/8] added logic for throwing no exception when no change in document in update document --- src/Database/Database.php | 23 +++++++++++++++++------ tests/Database/Base.php | 5 ++--- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index e9bd5b1b6..deb44ec60 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2791,7 +2791,6 @@ public function updateDocument(string $collection, string $id, Document $documen } $time = DateTime::now(); - $document->setAttribute('$updatedAt', $time); $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)); @@ -2801,7 +2800,17 @@ public function updateDocument(string $collection, string $id, Document $documen if ($collection->getId() !== self::METADATA) { $documentSecurity = $collection->getAttribute('documentSecurity', false); - if (!$validator->isValid([ + $skipPermission = true; + foreach ($document as $key=>$value) { + if ($old->getAttribute($key) instanceof Document) { + continue; + } + if ($old->getAttribute($key) !== $value) { + $skipPermission = false; + break; + } + } + if (!$skipPermission && !$validator->isValid([ ...$collection->getUpdate(), ...($documentSecurity ? $old->getUpdate() : []) ])) { @@ -2809,6 +2818,8 @@ public function updateDocument(string $collection, string $id, Document $documen } } + $document->setAttribute('$updatedAt', $time); + // Check if document was updated after the request timestamp $oldUpdatedAt = new \DateTime($old->getUpdatedAt()); if (!is_null($this->timestamp) && $oldUpdatedAt > $this->timestamp) { @@ -3010,16 +3021,16 @@ private function updateDocumentRelationships(Document $collection, Document $old $removedDocuments = \array_diff($oldIds, $newIds); foreach ($removedDocuments as $relation) { - $relation = $this->skipRelationships(fn () => $this->getDocument( + $relation = $this->skipRelationships(fn () => Authorization::skip(fn () => $this->getDocument( $relatedCollection->getId(), $relation - )); + ))); - $this->skipRelationships(fn () => $this->updateDocument( + $this->skipRelationships(fn () => Authorization::skip(fn () =>$this->updateDocument( $relatedCollection->getId(), $relation->getId(), $relation->setAttribute($twoWayKey, null) - )); + ))); //removing relationship from 10 id document in collection B by updating it. } foreach ($value as $relation) { diff --git a/tests/Database/Base.php b/tests/Database/Base.php index 83f8b3027..c0716f047 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -2729,7 +2729,7 @@ public function testWritePermissionsUpdateFailure(Document $document): Document Permission::delete(Role::any()), ], 'string' => 'textđź“ť', - 'integer' => 5, + 'integer' => 6, 'bigint' => 8589934592, // 2^33 'float' => 5.55, 'boolean' => true, @@ -10597,7 +10597,6 @@ public function testCollectionPermissionsUpdateWorks(array $data): array public function testCollectionPermissionsUpdateThrowsException(array $data): void { [$collection, $document] = $data; - Authorization::cleanRoles(); Authorization::setRole(Role::any()->toString()); @@ -10605,7 +10604,7 @@ public function testCollectionPermissionsUpdateThrowsException(array $data): voi $document = static::getDatabase()->updateDocument( $collection->getId(), $document->getId(), - $document->setAttribute('test', 'ipsum') + $document->setAttribute('test', 'lorem') ); } From 18786780e3cc3510e26ff0717e7e0f962a758ed0 Mon Sep 17 00:00:00 2001 From: Prateek Banga Date: Tue, 18 Jul 2023 00:34:56 +0530 Subject: [PATCH 2/8] removed unnecessary changes --- src/Database/Database.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index deb44ec60..9d4a9273e 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -3021,16 +3021,16 @@ private function updateDocumentRelationships(Document $collection, Document $old $removedDocuments = \array_diff($oldIds, $newIds); foreach ($removedDocuments as $relation) { - $relation = $this->skipRelationships(fn () => Authorization::skip(fn () => $this->getDocument( + $relation = $this->skipRelationships(fn () => $this->getDocument( $relatedCollection->getId(), $relation - ))); + )); - $this->skipRelationships(fn () => Authorization::skip(fn () =>$this->updateDocument( + $this->skipRelationships(fn () => $this->updateDocument( $relatedCollection->getId(), $relation->getId(), $relation->setAttribute($twoWayKey, null) - ))); //removing relationship from 10 id document in collection B by updating it. + )); } foreach ($value as $relation) { From 657cbc177bd8292de5390c9bef3ad27d568a9535 Mon Sep 17 00:00:00 2001 From: Prateek Banga Date: Tue, 18 Jul 2023 15:01:06 +0530 Subject: [PATCH 3/8] Added test case for no authorization exception when no change to update Document --- src/Database/Database.php | 4 +++- tests/Database/Base.php | 41 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 9d4a9273e..9638857dc 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2791,8 +2791,8 @@ public function updateDocument(string $collection, string $id, Document $documen } $time = DateTime::now(); - $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)); $validator = new Authorization(self::PERMISSION_UPDATE); @@ -2801,7 +2801,9 @@ public function updateDocument(string $collection, string $id, Document $documen $documentSecurity = $collection->getAttribute('documentSecurity', false); $skipPermission = true; + //compare if the document any changes foreach ($document as $key=>$value) { + //skip the nested documents as they will be checked later in recursions. if ($old->getAttribute($key) instanceof Document) { continue; } diff --git a/tests/Database/Base.php b/tests/Database/Base.php index c0716f047..f5860c10d 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -2739,6 +2739,47 @@ public function testWritePermissionsUpdateFailure(Document $document): Document return $document; } + + /** + * @depends testCreateDocument + */ + public function testNoChangeUpdateDocumentWithoutPermission(Document $document): Document + { + Authorization::cleanRoles(); + Authorization::setRole(Role::any()->toString()); + + + $document = static::getDatabase()->createDocument('documents', new Document([ + 'string' => 'textđź“ť', + 'integer' => 5, + 'bigint' => 8589934592, // 2^33 + 'float' => 5.55, + 'boolean' => true, + 'colors' => ['pink', 'green', 'blue'], + ])); + Authorization::cleanRoles(); + //no changes in document + $documentToUpdate = new Document([ + '$id' => ID::custom($document->getId()), + 'string' => 'textđź“ť', + 'integer' => 5, + 'bigint' => 8589934592, // 2^33 + 'float' => 5.55, + 'boolean' => true, + 'colors' => ['pink', 'green', 'blue'], + '$collection' => 'documents', + ]); + $documentToUpdate->setAttribute('$createdAt', $document->getAttribute('$createdAt')); + $documentToUpdate->setAttribute('$updatedAt', $document->getAttribute('$updatedAt')); + $updatedDocument = static::getDatabase()->updateDocument('documents', $document->getId(), $documentToUpdate); + + + // Document should be updated without any problem and without any authorization exception as there is no change in document. + $this->assertEquals(true, $updatedDocument->getUpdatedAt() > $document->getUpdatedAt()); + + return $document; + } + public function testExceptionAttributeLimit(): void { if ($this->getDatabase()->getLimitForAttributes() > 0) { From 95b0f94fe851e4e53de88f525cc28f53af0cf62c Mon Sep 17 00:00:00 2001 From: Prateek Banga <30731059+fanatic75@users.noreply.github.com> Date: Wed, 19 Jul 2023 01:23:51 +0530 Subject: [PATCH 4/8] Format issues Co-authored-by: Steven <1477010+stnguyen90@users.noreply.github.com> --- src/Database/Database.php | 4 ++-- tests/Database/Base.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 9638857dc..97bddf6c0 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2801,9 +2801,9 @@ public function updateDocument(string $collection, string $id, Document $documen $documentSecurity = $collection->getAttribute('documentSecurity', false); $skipPermission = true; - //compare if the document any changes + // Compare if the document has any changes foreach ($document as $key=>$value) { - //skip the nested documents as they will be checked later in recursions. + // Skip the nested documents as they will be checked later in recursions. if ($old->getAttribute($key) instanceof Document) { continue; } diff --git a/tests/Database/Base.php b/tests/Database/Base.php index f5860c10d..1223d43fc 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -2758,7 +2758,7 @@ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): 'colors' => ['pink', 'green', 'blue'], ])); Authorization::cleanRoles(); - //no changes in document + // No changes in document $documentToUpdate = new Document([ '$id' => ID::custom($document->getId()), 'string' => 'textđź“ť', From 8bb7a0084f9cd8374926aaa6da06f877f6c09d5f Mon Sep 17 00:00:00 2001 From: Prateek Banga Date: Wed, 19 Jul 2023 13:58:16 +0530 Subject: [PATCH 5/8] Not updating updateAt when no change in document --- src/Database/Database.php | 6 ++++-- tests/Database/Base.php | 8 +++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 97bddf6c0..6ec4093a8 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2796,11 +2796,11 @@ public function updateDocument(string $collection, string $id, Document $documen $collection = $this->silent(fn () => $this->getCollection($collection)); $validator = new Authorization(self::PERMISSION_UPDATE); + $skipPermission = true; if ($collection->getId() !== self::METADATA) { $documentSecurity = $collection->getAttribute('documentSecurity', false); - $skipPermission = true; // Compare if the document has any changes foreach ($document as $key=>$value) { // Skip the nested documents as they will be checked later in recursions. @@ -2820,7 +2820,9 @@ public function updateDocument(string $collection, string $id, Document $documen } } - $document->setAttribute('$updatedAt', $time); + if(!$skipPermission){ + $document->setAttribute('$updatedAt', $time); + } // Check if document was updated after the request timestamp $oldUpdatedAt = new \DateTime($old->getUpdatedAt()); diff --git a/tests/Database/Base.php b/tests/Database/Base.php index 1223d43fc..00571e74f 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -78,9 +78,10 @@ public function testCreateExistsDelete(): void public function testCreatedAtUpdatedAt(): void { $this->assertInstanceOf('Utopia\Database\Document', static::getDatabase()->createCollection('created_at')); - + static::getDatabase()->createAttribute('created_at','title', Database::VAR_STRING, 100, false); $document = static::getDatabase()->createDocument('created_at', new Document([ '$id' => ID::custom('uid123'), + '$permissions' => [ Permission::read(Role::any()), Permission::create(Role::any()), @@ -2774,8 +2775,8 @@ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): $updatedDocument = static::getDatabase()->updateDocument('documents', $document->getId(), $documentToUpdate); - // Document should be updated without any problem and without any authorization exception as there is no change in document. - $this->assertEquals(true, $updatedDocument->getUpdatedAt() > $document->getUpdatedAt()); + // Document should not be updated as there is no change. It should also not throw any authorization exception without any permission because of no change. + $this->assertEquals($updatedDocument->getUpdatedAt(),$document->getUpdatedAt()); return $document; } @@ -3526,6 +3527,7 @@ public function testCreatedAtUpdatedAtAssert(): void $document = static::getDatabase()->getDocument('created_at', 'uid123'); $this->assertEquals(true, !$document->isEmpty()); sleep(1); + $document->setAttribute('title','new title'); static::getDatabase()->updateDocument('created_at', 'uid123', $document); $document = static::getDatabase()->getDocument('created_at', 'uid123'); From 92dddab6bcc5020428b5d0bfded0592534a9e5a4 Mon Sep 17 00:00:00 2001 From: Prateek Banga Date: Wed, 19 Jul 2023 22:34:31 +0530 Subject: [PATCH 6/8] fix lint issues --- src/Database/Database.php | 2 +- tests/Database/Base.php | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 6ec4093a8..dc53f8730 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2820,7 +2820,7 @@ public function updateDocument(string $collection, string $id, Document $documen } } - if(!$skipPermission){ + if (!$skipPermission) { $document->setAttribute('$updatedAt', $time); } diff --git a/tests/Database/Base.php b/tests/Database/Base.php index 00571e74f..2070c5f95 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -78,7 +78,7 @@ public function testCreateExistsDelete(): void public function testCreatedAtUpdatedAt(): void { $this->assertInstanceOf('Utopia\Database\Document', static::getDatabase()->createCollection('created_at')); - static::getDatabase()->createAttribute('created_at','title', Database::VAR_STRING, 100, false); + static::getDatabase()->createAttribute('created_at', 'title', Database::VAR_STRING, 100, false); $document = static::getDatabase()->createDocument('created_at', new Document([ '$id' => ID::custom('uid123'), @@ -2776,7 +2776,7 @@ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): // Document should not be updated as there is no change. It should also not throw any authorization exception without any permission because of no change. - $this->assertEquals($updatedDocument->getUpdatedAt(),$document->getUpdatedAt()); + $this->assertEquals($updatedDocument->getUpdatedAt(), $document->getUpdatedAt()); return $document; } @@ -3527,7 +3527,7 @@ public function testCreatedAtUpdatedAtAssert(): void $document = static::getDatabase()->getDocument('created_at', 'uid123'); $this->assertEquals(true, !$document->isEmpty()); sleep(1); - $document->setAttribute('title','new title'); + $document->setAttribute('title', 'new title'); static::getDatabase()->updateDocument('created_at', 'uid123', $document); $document = static::getDatabase()->getDocument('created_at', 'uid123'); From 246e6033f9d1ff8c65817041ae90887efbd0484b Mon Sep 17 00:00:00 2001 From: prateek banga Date: Wed, 26 Jul 2023 11:03:16 +0530 Subject: [PATCH 7/8] proper variable naming and fixes lint issues --- src/Database/Database.php | 8 ++++---- tests/Database/Base.php | 19 ++----------------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index dc53f8730..885f283d9 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2796,7 +2796,7 @@ public function updateDocument(string $collection, string $id, Document $documen $collection = $this->silent(fn () => $this->getCollection($collection)); $validator = new Authorization(self::PERMISSION_UPDATE); - $skipPermission = true; + $shouldUpdate = false; if ($collection->getId() !== self::METADATA) { $documentSecurity = $collection->getAttribute('documentSecurity', false); @@ -2808,11 +2808,11 @@ public function updateDocument(string $collection, string $id, Document $documen continue; } if ($old->getAttribute($key) !== $value) { - $skipPermission = false; + $shouldUpdate = true; break; } } - if (!$skipPermission && !$validator->isValid([ + if ($shouldUpdate && !$validator->isValid([ ...$collection->getUpdate(), ...($documentSecurity ? $old->getUpdate() : []) ])) { @@ -2820,7 +2820,7 @@ public function updateDocument(string $collection, string $id, Document $documen } } - if (!$skipPermission) { + if ($shouldUpdate) { $document->setAttribute('$updatedAt', $time); } diff --git a/tests/Database/Base.php b/tests/Database/Base.php index 2070c5f95..1631b82c3 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -2746,10 +2746,8 @@ public function testWritePermissionsUpdateFailure(Document $document): Document */ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): Document { - Authorization::cleanRoles(); Authorization::setRole(Role::any()->toString()); - $document = static::getDatabase()->createDocument('documents', new Document([ 'string' => 'textđź“ť', 'integer' => 5, @@ -2758,22 +2756,9 @@ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): 'boolean' => true, 'colors' => ['pink', 'green', 'blue'], ])); - Authorization::cleanRoles(); - // No changes in document - $documentToUpdate = new Document([ - '$id' => ID::custom($document->getId()), - 'string' => 'textđź“ť', - 'integer' => 5, - 'bigint' => 8589934592, // 2^33 - 'float' => 5.55, - 'boolean' => true, - 'colors' => ['pink', 'green', 'blue'], - '$collection' => 'documents', - ]); - $documentToUpdate->setAttribute('$createdAt', $document->getAttribute('$createdAt')); - $documentToUpdate->setAttribute('$updatedAt', $document->getAttribute('$updatedAt')); - $updatedDocument = static::getDatabase()->updateDocument('documents', $document->getId(), $documentToUpdate); + Authorization::cleanRoles(); + $updatedDocument = static::getDatabase()->updateDocument('documents', $document->getId(), $document); // Document should not be updated as there is no change. It should also not throw any authorization exception without any permission because of no change. $this->assertEquals($updatedDocument->getUpdatedAt(), $document->getUpdatedAt()); From 68206dc5a5ba349a29904f42430462b6ca6be9fd Mon Sep 17 00:00:00 2001 From: prateek banga Date: Wed, 26 Jul 2023 21:23:09 +0530 Subject: [PATCH 8/8] refactor of a getAttribute call --- src/Database/Database.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 885f283d9..cdb6badb6 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2804,10 +2804,11 @@ public function updateDocument(string $collection, string $id, Document $documen // 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 ($old->getAttribute($key) instanceof Document) { + $oldAttributeValue = $old->getAttribute($key); + if ($oldAttributeValue instanceof Document) { continue; } - if ($old->getAttribute($key) !== $value) { + if ($oldAttributeValue !== $value) { $shouldUpdate = true; break; }