tests/e2e/Services/Databases/TablesDB/DatabasesBase.php (1)

1696-1724: Improve idempotent-upsert test: assert true no-op via $updatedAt and exact permissions

The existing test only checks that the second PUT returns status 200, that the title remains “Thor: Ragnarok,” and that there are three permissions. To guarantee a true no-op, we should:

• Capture the row’s $updatedAt and $permissions immediately before resubmitting.
• After the second PUT, assert that $updatedAt is unchanged and that the permission sets are identical (not merely equal in count).
• (Optional) Construct the upsert payload from the persisted row to avoid mismatches between null vs. [] defaults.

Locations to update:

• tests/e2e/Services/Databases/TablesDB/DatabasesBase.php, around lines 1696–1724.

Proposed diff:

@@ tests/e2e/Services/Databases/TablesDB/DatabasesBase.php
         $this->assertEquals('Thor: Ragnarok', $row['body']['title']);

         /**
          * Resubmit same document, nothing to update
          */
+        // Capture current state to verify true no-op behavior
+        $before = $this->client->call(
+            Client::METHOD_GET,
+            '/tablesdb/' . $databaseId . '/tables/' . $data['moviesId'] . '/rows/' . $rowId,
+            array_merge([
+                'content-type' => 'application/json',
+                'x-appwrite-project' => $this->getProject()['$id'],
+            ], $this->getHeaders())
+        );
+        $prevUpdatedAt = $before['body']['$updatedAt'];
+        $prevPermissions = $before['body']['$permissions'];
@@
         $row = $this->client->call(Client::METHOD_PUT, '/tablesdb/' . $databaseId . '/tables/' . $data['moviesId'] . '/rows/' . $rowId, array_merge([
             'content-type' => 'application/json',
             'x-appwrite-project' => $this->getProject()['$id'],
         ], $this->getHeaders()), [
             'data' => [
                 'title' => 'Thor: Ragnarok',
                 'releaseYear' => 2000,
                 'integers' => [],
                 'birthDay' => null,
                 'duration' => null,
                 'starringActors' => [],
                 'actors' => [],
                 'tagline' => '',
                 'description' => '',
             ],
             'permissions' => [
                 Permission::read(Role::users()),
                 Permission::update(Role::users()),
                 Permission::delete(Role::users()),
             ],
         ]);

         $this->assertEquals(200, $row['headers']['status-code']);
         $this->assertEquals('Thor: Ragnarok', $row['body']['title']);
+        $this->assertEquals(2000, $row['body']['releaseYear']);
+        // Prove no-op: timestamps and permissions unchanged
+        $this->assertEquals(
+            $prevUpdatedAt,
+            $row['body']['$updatedAt'],
+            'Upsert with identical data should not bump $updatedAt'
+        );
+        $this->assertEqualsCanonicalizing(
+            $prevPermissions,
+            $row['body']['$permissions'],
+            'Permissions should remain identical on no-op'
+        );

Optional safer payload construction to mirror persisted shape:

// Build payload from persisted row to avoid [] vs. null drift
$writableKeys = ['title','releaseYear','integers','birthDay','duration','starringActors','actors','tagline','description'];
$sameData = array_intersect_key($before['body'], array_flip($writableKeys));
// Then PUT with 'data' => $sameData

tests/e2e/Services/Databases/Legacy/DatabasesBase.php (2)

1756-1794: PUT with array payload under “data” is schema-invalid; clarify intent and avoid false positives

This PUT sends an array for “data” where the API expects an object. The test then asserts 200 and that the resource is unchanged. If the goal is to validate server-side input rejection, assert a 400. If the goal is to validate idempotent behavior when payload is invalid/ignored, make it explicit and compare against a snapshot from the previous step. Also, drop the debug dump.

Option A (preferred: treat invalid shape as client error):

@@
-        $document = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $documentId, array_merge([
+        $invalid = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $documentId, array_merge([
             'content-type' => 'application/json',
             'x-appwrite-project' => $this->getProject()['$id'],
         ], $this->getHeaders()), [
             'data' => [
                 [
                     'title' => 'Thor: Ragnarok1',
@@
                 ]
             ],
             'permissions' => [
                 Permission::read(Role::users()),
                 Permission::update(Role::users()),
                 Permission::delete(Role::users()),
             ],
         ]);
-var_dump($document);
-        $this->assertEquals(200, $document['headers']['status-code']);
-        $this->assertEquals('Thor: Ragnarok', $document['body']['title']);
-        $this->assertCount(3, $document['body']['$permissions']);
+        $this->assertEquals(400, $invalid['headers']['status-code']);

Option B (if the API intentionally ignores array payloads and returns the existing document): assert explicitly that nothing changed relative to the previous snapshot.

@@
-        $document = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $documentId, array_merge([
+        $unchanged = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $documentId, array_merge([
             'content-type' => 'application/json',
             'x-appwrite-project' => $this->getProject()['$id'],
         ], $this->getHeaders()), [
             'data' => [
                 [
@@
             ],
         ]);
-var_dump($document);
-        $this->assertEquals(200, $document['headers']['status-code']);
-        $this->assertEquals('Thor: Ragnarok', $document['body']['title']);
-        $this->assertCount(3, $document['body']['$permissions']);
+        $this->assertEquals(200, $unchanged['headers']['status-code']);
+        $this->assertEquals($prev['body']['title'], $unchanged['body']['title']);
+        $this->assertEquals($prev['body']['releaseYear'], $unchanged['body']['releaseYear']);
+        $this->assertEqualsCanonicalizing($prev['body']['$permissions'], $unchanged['body']['$permissions']);

Follow-up: do you want this negative test split into its own method (e.g., testUpsertDocumentRejectsArrayPayload) for clearer intent?

---

1791-1794: Strengthen permission assertions to verify exact set, not just count

Counting to 3 won’t catch wrong roles/types. Assert canonical equality against the expected permissions.

-        $this->assertCount(3, $document['body']['$permissions']);
+        $this->assertCount(3, $document['body']['$permissions']);
+        $this->assertEqualsCanonicalizing(
+            [
+                Permission::read(Role::users()),
+                Permission::update(Role::users()),
+                Permission::delete(Role::users()),
+            ],
+            $document['body']['$permissions']
+        );

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)

100-103: Reject any top-level array payload for "data" and use InvalidStructure, not MissingPayload

Allowing a single-item list passes a malformed structure downstream (mixing numeric index 0 with $id when you later set $data['$id']), which can yield subtle corruption. Semantically, this is not a “missing payload” but an invalid structure. Enforce object-only payloads.

Apply this diff:

-        if (\array_is_list($data) && \count($data) > 1) { // Allow 1 associated array
-            throw new Exception($this->getMissingPayloadException());
-        }
+        // Reject top-level JSON arrays; document 'data' must be an object
+        if (\is_array($data) && \array_is_list($data)) {
+            throw new Exception(
+                $this->getInvalidStructureException(),
+                'Payload "data" must be a JSON object, not an array.'
+            );
+        }

tests/e2e/Services/Databases/Legacy/DatabasesBase.php (1)

1731-1759: Strengthen the “no-op upsert” test to actually prove idempotence and avoid unintended mutations

The current payload adds attributes that may not exist on the document yet (e.g., actors, integers), which can cause a real update. Capture the pre-state, send only truly unchanged fields, and assert that metadata like $updatedAt, $sequence, and $permissions remain stable.

Apply this diff:

         $this->assertEquals('Thor: Ragnarok', $document['body']['title']);
 
-        /**
-         * Resubmit same document, nothing to update
-         */
-        $document = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $documentId, array_merge([
+        /**
+         * Resubmit same document, nothing to update (true no-op)
+         */
+        $prev = $document; // snapshot GET state before no-op PUT
+        $document = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $documentId, array_merge([
             'content-type' => 'application/json',
             'x-appwrite-project' => $this->getProject()['$id'],
         ], $this->getHeaders()), [
             'data' => [
                 'title' => 'Thor: Ragnarok',
-                'releaseYear' => 2000,
-                'integers' => [],
-                'birthDay' => null,
-                'duration' => null,
-                'starringActors' => [],
-                'actors' => [],
-                'tagline' => '',
-                'description' => '',
+                'releaseYear' => 2000
             ],
             'permissions' => [
                 Permission::read(Role::users()),
                 Permission::update(Role::users()),
                 Permission::delete(Role::users()),
             ],
         ]);
 
         $this->assertEquals(200, $document['headers']['status-code']);
         $this->assertEquals('Thor: Ragnarok', $document['body']['title']);
         $this->assertCount(3, $document['body']['$permissions']);
+        // Prove idempotence: critical fields unchanged
+        $this->assertEquals($prev['body']['$id'], $document['body']['$id']);
+        $this->assertEquals($prev['body']['releaseYear'], $document['body']['releaseYear']);
+        $this->assertEquals($prev['body']['$sequence'], $document['body']['$sequence']);
+        $this->assertEquals($prev['body']['$updatedAt'], $document['body']['$updatedAt'], 'updatedAt must not change on a no-op PUT');
+        $this->assertEqualsCanonicalizing($prev['body']['$permissions'], $document['body']['$permissions']);

Notes:

• Removing empty arrays/nullable fields avoids converting “attribute absent” into “attribute present but empty,” which would mutate the document and defeat the intent of a no-op test.

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)

271-275: Make the fallback fetch authorization-safe and fail fast if still empty

If the callback doesn’t emit a document, the immediate fetch should bypass permissions (the caller may have just removed their own read permission) and we should guard against an unexpected empty result instead of proceeding.

Apply this diff:

-        if (empty($upserted[0])) {
-            $upserted[0] = $dbForProject->getDocument('database_' . $database->getSequence() . '_collection_' . $collection->getSequence(), $documentId);
-        }
+        if (empty($upserted[0])) {
+            $upserted[0] = Authorization::skip(
+                fn () => $dbForProject->getDocument(
+                    'database_' . $database->getSequence() . '_collection_' . $collection->getSequence(),
+                    $documentId
+                )
+            );
+            // Defensive: if still empty, surface a clear error instead of continuing with an empty document
+            if ($upserted[0]->isEmpty()) {
+                throw new Exception(Exception::DOCUMENT_NOT_FOUND);
+            }
+        }

Follow-up:

• Can createOrUpdateDocuments(..., onNext: ...) ever legitimately yield no items for a single-document upsert? If not, consider asserting on $upserted size and removing the fallback entirely to simplify the code path.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.