From 40fa74d89642a6b129eb37d64c56a79c878c0b6d Mon Sep 17 00:00:00 2001 From: prateek banga Date: Wed, 9 Aug 2023 22:33:53 +0530 Subject: [PATCH 1/5] add checking of nested docs id in comparison check when updating document --- src/Database/Database.php | 104 ++++++++++++++++++++++++++++++++++++-- tests/Database/Base.php | 6 +-- 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index c1fdc26f2..24be361b8 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2919,15 +2919,113 @@ 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; + $relationships[$relationshipKey] = $relationship; } // 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)) { + if (array_key_exists($key, $relationships)) { + $relationType = (string) $relationships[$key]['options']['relationType']; + $side = (string) $relationships[$key]['options']['side']; + switch($relationType) { + case DATABASE::RELATION_ONE_TO_ONE: { + $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); + if ( + (\is_null($old->getAttribute($key)) && !\is_null($value)) || + (!is_null($old->getAttribute($key)) && \is_null($value))) { + $shouldUpdate = true; + break; + } elseif (\is_string($value)) { + if ($oldValue !== $value) { + $shouldUpdate = true; + break; + } + } elseif ($oldValue !== $value->getId()) { + $shouldUpdate = true; + break; + } + break; + } + case Database::RELATION_MANY_TO_ONE: + case Database::RELATION_ONE_TO_MANY: + { + if ( + ($side === DATABASE::RELATION_SIDE_PARENT && $relationType === Database::RELATION_MANY_TO_ONE) || + ($side === DATABASE::RELATION_SIDE_CHILD && $relationType === Database::RELATION_ONE_TO_MANY) + ) { + if ( + (\is_null($old->getAttribute($key)) && !\is_null($value)) || + (!is_null($old->getAttribute($key)) && \is_null($value))) { + $shouldUpdate = true; + break; + } + $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); + if (\is_string($value)) { + if ($oldValue !== $value) { + $shouldUpdate = true; + break; + } + } elseif ($oldValue !== $value->getId()) { + $shouldUpdate = true; + break; + } + } else { + if ( + (\is_null($old->getAttribute($key)) && !\is_null($value)) || + (!is_null($old->getAttribute($key)) && \is_null($value))) { + $shouldUpdate = true; + break; + } + if (\count($value) !== \count($old->getAttribute($key))) { + $shouldUpdate = true; + break; + } + foreach ($value as $index=>$relation) { + $oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index]; + if (\is_string($relation)) { + if ($oldValue !== $relation) { + $shouldUpdate = true; + break; + } + } elseif ($oldValue !== $relation->getId()) { + $shouldUpdate = true; + break; + } + } + } + break; + } + case Database::RELATION_MANY_TO_MANY: { + if ( + (\is_null($old->getAttribute($key)) && !\is_null($value)) || + (!is_null($old->getAttribute($key)) && \is_null($value))) { + $shouldUpdate = true; + break; + } + if (\count($value) !== \count($old->getAttribute($key))) { + $shouldUpdate = true; + break; + } + foreach ($value as $index=>$relation) { + $oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index]; + if (\is_string($relation)) { + if ($oldValue !== $relation) { + $shouldUpdate = true; + break; + } + } elseif ($oldValue!== $relation->getId()) { + $shouldUpdate = true; + break; + } + } + break; + } + } + if ($shouldUpdate) { + break; + } continue; } diff --git a/tests/Database/Base.php b/tests/Database/Base.php index 1d77935c9..ded683268 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -3292,10 +3292,7 @@ public function testNoChangeUpdateDocumentWithRelationWithoutPermission(): void 'name'=>'Parent 1', '$collection' => 'parentRelationTest', 'childs' => [ - new Document([ - '$id' => 'child1', - '$collection' => 'childRelationTest' - ]), + 'child1', ] ])); @@ -11588,7 +11585,6 @@ public function testCollectionPermissionsRelationshipsUpdateWorks(array $data): Authorization::cleanRoles(); Authorization::setRole(Role::users()->toString()); - static::getDatabase()->updateDocument( $collection->getId(), $document->getId(), From 72454a2d9ff5d2fd26cc5a4a0424e857dd553c41 Mon Sep 17 00:00:00 2001 From: prateek banga Date: Wed, 9 Aug 2023 22:45:54 +0530 Subject: [PATCH 2/5] code quality check fix --- src/Database/Database.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 24be361b8..d32c7dd83 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2930,7 +2930,7 @@ public function updateDocument(string $collection, string $id, Document $documen $relationType = (string) $relationships[$key]['options']['relationType']; $side = (string) $relationships[$key]['options']['side']; switch($relationType) { - case DATABASE::RELATION_ONE_TO_ONE: { + case Database::RELATION_ONE_TO_ONE: { $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); if ( (\is_null($old->getAttribute($key)) && !\is_null($value)) || @@ -2952,8 +2952,8 @@ public function updateDocument(string $collection, string $id, Document $documen case Database::RELATION_ONE_TO_MANY: { if ( - ($side === DATABASE::RELATION_SIDE_PARENT && $relationType === Database::RELATION_MANY_TO_ONE) || - ($side === DATABASE::RELATION_SIDE_CHILD && $relationType === Database::RELATION_ONE_TO_MANY) + ($side === Database::RELATION_SIDE_PARENT && $relationType === Database::RELATION_MANY_TO_ONE) || + ($side === Database::RELATION_SIDE_CHILD && $relationType === Database::RELATION_ONE_TO_MANY) ) { if ( (\is_null($old->getAttribute($key)) && !\is_null($value)) || From 8579b57e22c51401ac6cdf4f47c4d768c26f88bd Mon Sep 17 00:00:00 2001 From: prateek banga Date: Thu, 10 Aug 2023 16:29:26 +0530 Subject: [PATCH 3/5] optimize if conditions check --- src/Database/Database.php | 104 ++++++++++++-------------------------- 1 file changed, 32 insertions(+), 72 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index d32c7dd83..96e90264a 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2933,16 +2933,11 @@ public function updateDocument(string $collection, string $id, Document $documen case Database::RELATION_ONE_TO_ONE: { $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); if ( - (\is_null($old->getAttribute($key)) && !\is_null($value)) || - (!is_null($old->getAttribute($key)) && \is_null($value))) { - $shouldUpdate = true; - break; - } elseif (\is_string($value)) { - if ($oldValue !== $value) { - $shouldUpdate = true; - break; - } - } elseif ($oldValue !== $value->getId()) { + (\is_null($oldValue) && !\is_null($value)) || + (!is_null($oldValue) && \is_null($value)) || + (\is_string($value) && $oldValue !== $value) || + ($value instanceof Document && $oldValue !== $value->getId()) + ) { $shouldUpdate = true; break; } @@ -2950,74 +2945,39 @@ public function updateDocument(string $collection, string $id, Document $documen } case Database::RELATION_MANY_TO_ONE: case Database::RELATION_ONE_TO_MANY: - { + case Database::RELATION_MANY_TO_MANY: { + if ( + ($relationType === Database::RELATION_MANY_TO_ONE && $side === Database::RELATION_SIDE_PARENT) || + ($relationType === Database::RELATION_ONE_TO_MANY && $side === Database::RELATION_SIDE_CHILD) + ) { + $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); if ( - ($side === Database::RELATION_SIDE_PARENT && $relationType === Database::RELATION_MANY_TO_ONE) || - ($side === Database::RELATION_SIDE_CHILD && $relationType === Database::RELATION_ONE_TO_MANY) + (\is_null($oldValue) && !\is_null($value)) || + (!is_null($oldValue) && \is_null($value)) || + (\is_string($value) && $oldValue !== $value) || + ($value instanceof Document && $oldValue !== $value->getId()) ) { - if ( - (\is_null($old->getAttribute($key)) && !\is_null($value)) || - (!is_null($old->getAttribute($key)) && \is_null($value))) { - $shouldUpdate = true; - break; - } - $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); - if (\is_string($value)) { - if ($oldValue !== $value) { - $shouldUpdate = true; - break; - } - } elseif ($oldValue !== $value->getId()) { - $shouldUpdate = true; - break; - } - } else { - if ( - (\is_null($old->getAttribute($key)) && !\is_null($value)) || - (!is_null($old->getAttribute($key)) && \is_null($value))) { - $shouldUpdate = true; - break; - } - if (\count($value) !== \count($old->getAttribute($key))) { - $shouldUpdate = true; - break; - } - foreach ($value as $index=>$relation) { - $oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index]; - if (\is_string($relation)) { - if ($oldValue !== $relation) { - $shouldUpdate = true; - break; - } - } elseif ($oldValue !== $relation->getId()) { - $shouldUpdate = true; - break; - } - } + $shouldUpdate = true; + break; } - break; - } - case Database::RELATION_MANY_TO_MANY: { - if ( - (\is_null($old->getAttribute($key)) && !\is_null($value)) || - (!is_null($old->getAttribute($key)) && \is_null($value))) { - $shouldUpdate = true; - break; - } - if (\count($value) !== \count($old->getAttribute($key))) { - $shouldUpdate = true; - break; - } - foreach ($value as $index=>$relation) { - $oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index]; - if (\is_string($relation)) { - if ($oldValue !== $relation) { + } else { + if ( + (\is_null($old->getAttribute($key)) && !\is_null($value)) || + (!is_null($old->getAttribute($key)) && \is_null($value)) || + \count($old->getAttribute($key)) !== \count($value) + ) { + $shouldUpdate = true; + break; + } + foreach ($value as $index=>$relation) { + $oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index]; + if ( + (\is_string($relation) && $oldValue!==$relation) || + ($relation instanceof Document && $oldValue !== $relation->getId()) + ) { $shouldUpdate = true; break; } - } elseif ($oldValue!== $relation->getId()) { - $shouldUpdate = true; - break; } } break; From 8cd9a5f31a02a1610596cc92b1dc2c5b4ee4618a Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 10 Aug 2023 17:59:58 -0400 Subject: [PATCH 4/5] Review fixes --- src/Database/Database.php | 89 +++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 45 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 96e90264a..5d6ab1ff1 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2920,82 +2920,82 @@ public function updateDocument(string $collection, string $id, Document $documen $documentSecurity = $collection->getAttribute('documentSecurity', false); foreach ($relationships as $relationship) { - $relationshipKey = $relationship->getAttribute('key'); - $relationships[$relationshipKey] = $relationship; + $relationships[$relationship->getAttribute('key')] = $relationship; } + // Compare if the document has any changes - foreach ($document as $key=>$value) { + foreach ($document as $key => $value) { // Skip the nested documents as they will be checked later in recursions. - if (array_key_exists($key, $relationships)) { + if (\array_key_exists($key, $relationships)) { $relationType = (string) $relationships[$key]['options']['relationType']; $side = (string) $relationships[$key]['options']['side']; + switch($relationType) { - case Database::RELATION_ONE_TO_ONE: { - $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); - if ( - (\is_null($oldValue) && !\is_null($value)) || - (!is_null($oldValue) && \is_null($value)) || - (\is_string($value) && $oldValue !== $value) || - ($value instanceof Document && $oldValue !== $value->getId()) - ) { + case Database::RELATION_ONE_TO_ONE: + $oldValue = $old->getAttribute($key) instanceof Document + ? $old->getAttribute($key)->getId() + : $old->getAttribute($key); + + if ((\is_null($value) !== \is_null($oldValue)) + || (\is_string($value) && $value !== $oldValue) + || ($value instanceof Document && $value->getId() !== $oldValue)) { $shouldUpdate = true; - break; } break; - } - case Database::RELATION_MANY_TO_ONE: case Database::RELATION_ONE_TO_MANY: - case Database::RELATION_MANY_TO_MANY: { + case Database::RELATION_MANY_TO_ONE: + case Database::RELATION_MANY_TO_MANY: if ( ($relationType === Database::RELATION_MANY_TO_ONE && $side === Database::RELATION_SIDE_PARENT) || ($relationType === Database::RELATION_ONE_TO_MANY && $side === Database::RELATION_SIDE_CHILD) ) { - $oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key); - if ( - (\is_null($oldValue) && !\is_null($value)) || - (!is_null($oldValue) && \is_null($value)) || - (\is_string($value) && $oldValue !== $value) || - ($value instanceof Document && $oldValue !== $value->getId()) - ) { + $oldValue = $old->getAttribute($key) instanceof Document + ? $old->getAttribute($key)->getId() + : $old->getAttribute($key); + + if ((\is_null($value) !== \is_null($oldValue)) + || (\is_string($value) && $value !== $oldValue) + || ($value instanceof Document && $value->getId() !== $oldValue)) { $shouldUpdate = true; - break; } - } else { - if ( - (\is_null($old->getAttribute($key)) && !\is_null($value)) || - (!is_null($old->getAttribute($key)) && \is_null($value)) || - \count($old->getAttribute($key)) !== \count($value) - ) { + break; + } + + if ((\is_null($old->getAttribute($key)) !== \is_null($value)) + || \count($old->getAttribute($key)) !== \count($value)) { + $shouldUpdate = true; + break; + } + foreach ($value as $index => $relation) { + $oldValue = $old->getAttribute($key)[$index] instanceof Document + ? $old->getAttribute($key)[$index]->getId() + : $old->getAttribute($key)[$index]; + + if ((\is_string($relation) && $relation !== $oldValue) + || ($relation instanceof Document && $relation->getId() !== $oldValue)) { $shouldUpdate = true; break; } - foreach ($value as $index=>$relation) { - $oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index]; - if ( - (\is_string($relation) && $oldValue!==$relation) || - ($relation instanceof Document && $oldValue !== $relation->getId()) - ) { - $shouldUpdate = true; - break; - } - } } break; - } } + if ($shouldUpdate) { break; } + continue; } - $oldAttributeValue = $old->getAttribute($key); + $oldValue = $old->getAttribute($key); + // If values are not equal we need to update document. - if ($oldAttributeValue !== $value) { + if ($value !== $oldValue) { $shouldUpdate = true; break; } } + if ($shouldUpdate && !$validator->isValid([ ...$collection->getUpdate(), ...($documentSecurity ? $old->getUpdate() : []) @@ -3006,8 +3006,6 @@ 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 @@ -3037,6 +3035,7 @@ public function updateDocument(string $collection, string $id, Document $documen $document = $this->decode($collection, $document); $this->purgeRelatedDocuments($collection, $id); + $this->cache->purge('cache-' . $this->getNamespace() . ':' . $collection->getId() . ':' . $id . ':*'); $this->trigger(self::EVENT_DOCUMENT_UPDATE, $document); From 9091665fd1c76e2851144529cef2d0660595b105 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 10 Aug 2023 18:00:35 -0400 Subject: [PATCH 5/5] Test fixes --- tests/Database/Base.php | 59 ++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/tests/Database/Base.php b/tests/Database/Base.php index ded683268..b2481a388 100644 --- a/tests/Database/Base.php +++ b/tests/Database/Base.php @@ -3221,15 +3221,14 @@ public function testWritePermissionsUpdateFailure(Document $document): Document return $document; } - /** * @depends testCreateDocument */ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): Document { - Authorization::setRole(Role::any()->toString()); - $document = static::getDatabase()->createDocument('documents', new Document([ + '$id' => ID::unique(), + '$permissions' => [], 'string' => 'text📝', 'integer' => 5, 'bigint' => 8589934592, // 2^33 @@ -3238,10 +3237,14 @@ public function testNoChangeUpdateDocumentWithoutPermission(Document $document): 'colors' => ['pink', 'green', 'blue'], ])); - Authorization::cleanRoles(); - $updatedDocument = static::getDatabase()->updateDocument('documents', $document->getId(), $document); + $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. + // 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; @@ -3254,17 +3257,9 @@ public function testNoChangeUpdateDocumentWithRelationWithoutPermission(): void return; } - Authorization::cleanRoles(); - Authorization::setRole(Role::user('a')->toString()); + static::getDatabase()->createCollection('parentRelationTest'); + static::getDatabase()->createCollection('childRelationTest'); - 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); @@ -3272,32 +3267,35 @@ public function testNoChangeUpdateDocumentWithRelationWithoutPermission(): void collection: 'parentRelationTest', relatedCollection: 'childRelationTest', type: Database::RELATION_ONE_TO_MANY, - id: 'childs' + id: 'children' ); // Create document with relationship with nested data $parent = static::getDatabase()->createDocument('parentRelationTest', new Document([ '$id' => 'parent1', + '$permissions' => [ + Permission::read(Role::any()), + ], 'name' => 'Parent 1', - 'childs' => [ + 'children' => [ [ '$id' => 'child1', + '$permissions' => [ + Permission::read(Role::any()), + ], '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' => [ - 'child1', - ] - ])); + + $this->assertEquals(1, \count($parent['children'])); + + $parent->setAttribute('children', ['child1']); + + $updatedParent = static::getDatabase()->updateDocument('parentRelationTest', 'parent1', $parent); $this->assertEquals($updatedParent->getUpdatedAt(), $parent->getUpdatedAt()); - $this->assertEquals($updatedParent->getAttribute('childs')[0]->getUpdatedAt(), $parent->getAttribute('childs')[0]->getUpdatedAt()); + $this->assertEquals($updatedParent->getAttribute('children')[0]->getUpdatedAt(), $parent->getAttribute('children')[0]->getUpdatedAt()); static::getDatabase()->deleteCollection('parentRelationTest'); static::getDatabase()->deleteCollection('childRelationTest'); @@ -3306,7 +3304,7 @@ public function testNoChangeUpdateDocumentWithRelationWithoutPermission(): void public function testExceptionAttributeLimit(): void { if ($this->getDatabase()->getLimitForAttributes() > 0) { - // load the collection up to the limit + // Load the collection up to the limit $attributes = []; for ($i = 0; $i < $this->getDatabase()->getLimitForAttributes(); $i++) { $attributes[] = new Document([ @@ -3320,7 +3318,8 @@ public function testExceptionAttributeLimit(): void 'filters' => [], ]); } - $collection = static::getDatabase()->createCollection('attributeLimit', $attributes); + + static::getDatabase()->createCollection('attributeLimit', $attributes); $this->expectException(LimitException::class); $this->assertEquals(false, static::getDatabase()->createAttribute('attributeLimit', "breaking", Database::VAR_INTEGER, 0, true));