Merge pull request #600 from utopia-php/fix-upsert-mismatch

Fix upsert duplicate IDs

Author: abnegate

src/Database/Adapter/MariaDB.php

@@ -1445,12 +1445,14 @@ public function createOrUpdateDocuments(
                     if (!empty($toRemove)) {
                         $removeQueries[] = "(
                             _document = :_uid_{$index}
-                            {$this->getTenantQuery($collection, tenantCount: \count($toRemove))}
+                            " . ($this->sharedTables ? " AND _tenant = :_tenant_{$index}" : '') . "
                             AND _type = '{$type}'
                             AND _permission IN (" . \implode(',', \array_map(fn ($i) => ":remove_{$type}_{$index}_{$i}", \array_keys($toRemove))) . ")
                         )";
                         $removeBindValues[":_uid_{$index}"] = $document->getId();
-                        $removeBindValues[":_tenant_{$index}"] = $document->getTenant();
+                        if ($this->sharedTables) {
+                            $removeBindValues[":_tenant_{$index}"] = $document->getTenant();
+                        }
                         foreach ($toRemove as $i => $perm) {
                             $removeBindValues[":remove_{$type}_{$index}_{$i}"] = $perm;
                         }

src/Database/Database.php

@@ -4920,7 +4920,7 @@ public function createOrUpdateDocumentsWithIncrease(
         $time = DateTime::now();
         $created = 0;
         $updated = 0;
-
+        $seenIds = [];
         foreach ($documents as $key => $document) {
             if ($this->getSharedTables() && $this->getTenantPerDocument()) {
                 $old = Authorization::skip(fn () => $this->withTenant($document->getTenant(), fn () => $this->silent(fn () => $this->getDocument(
@@ -5028,12 +5028,19 @@ public function createOrUpdateDocumentsWithIncrease(
                 $document = $this->silent(fn () => $this->createDocumentRelationships($collection, $document));
             }
 
+            $seenIds[] = $document->getId();
+
             $documents[$key] = new Change(
                 old: $old,
                 new: $document
             );
         }
 
+        // Required because *some* DBs will allow duplicate IDs for upsert
+        if (\count($seenIds) !== \count(\array_unique($seenIds))) {
+            throw new DuplicateException('Duplicate document IDs found in the input array.');
+        }
+
         foreach (\array_chunk($documents, $batchSize) as $chunk) {
             /**
              * @var array<Change> $chunk

tests/e2e/Adapter/Scopes/DocumentTests.php

@@ -735,6 +735,75 @@ public function testUpsertDocumentsNoop(): void
         $this->assertEquals(0, $count);
     }
 
+    public function testUpsertDuplicateIds(): void
+    {
+        $db = static::getDatabase();
+        if (!$db->getAdapter()->getSupportForUpserts()) {
+            $this->expectNotToPerformAssertions();
+            return;
+        }
+
+        $db->createCollection(__FUNCTION__);
+        $db->createAttribute(__FUNCTION__, 'num', Database::VAR_INTEGER, 0, true);
+
+        $doc1 = new Document(['$id' => 'dup', 'num' => 1]);
+        $doc2 = new Document(['$id' => 'dup', 'num' => 2]);
+
+        try {
+            $db->createOrUpdateDocuments(__FUNCTION__, [$doc1, $doc2]);
+            $this->fail('Failed to throw exception');
+        } catch (\Throwable $e) {
+            $this->assertInstanceOf(DuplicateException::class, $e, $e->getMessage());
+        }
+    }
+
+    public function testUpsertMixedPermissionDelta(): void
+    {
+        $db = static::getDatabase();
+        if (!$db->getAdapter()->getSupportForUpserts()) {
+            $this->expectNotToPerformAssertions();
+            return;
+        }
+
+        $db->createCollection(__FUNCTION__);
+        $db->createAttribute(__FUNCTION__, 'v', Database::VAR_INTEGER, 0, true);
+
+        $d1 = $db->createDocument(__FUNCTION__, new Document([
+            '$id' => 'a',
+            'v' => 0,
+            '$permissions' => [
+                Permission::update(Role::any())
+            ]
+        ]));
+        $d2 = $db->createDocument(__FUNCTION__, new Document([
+            '$id' => 'b',
+            'v' => 0,
+            '$permissions' => [
+                Permission::update(Role::any())
+            ]
+        ]));
+
+        // d1 adds write, d2 removes update
+        $d1->setAttribute('$permissions', [
+            Permission::read(Role::any()),
+            Permission::update(Role::any())
+        ]);
+        $d2->setAttribute('$permissions', [
+            Permission::read(Role::any())
+        ]);
+
+        $db->createOrUpdateDocuments(__FUNCTION__, [$d1, $d2]);
+
+        $this->assertEquals([
+            Permission::read(Role::any()),
+            Permission::update(Role::any()),
+        ], $db->getDocument(__FUNCTION__, 'a')->getPermissions());
+
+        $this->assertEquals([
+            Permission::read(Role::any()),
+        ], $db->getDocument(__FUNCTION__, 'b')->getPermissions());
+    }
+
     public function testRespectNulls(): Document
     {
         static::getDatabase()->createCollection('documents_nulls');