Fix readonly attr stripping on write#10405
Conversation
📝 WalkthroughWalkthroughRemoves per-call unsets of server-managed/read-only document attributes and centralizes sanitization by changing Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.17)Note: Using configuration file /phpstan.neon. If the excluded path can sometimes exist, append (?) parameters: 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
123-128: Fix undefined index access for "$permissions".Accessing $data['$permissions'] without an existence check can raise notices. Validate only when provided.
- if ($data['$permissions']) { + if (isset($data['$permissions'])) { $validator = new Permissions(); if (!$validator->isValid($data['$permissions'])) { throw new Exception(Exception::GENERAL_BAD_REQUEST, $validator->getDescription()); } }
🧹 Nitpick comments (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
130-131: Centralized stripping: OK. Consider guarding against “empty-after-strip” payloads.If the client only sends read‑only/server fields, $data becomes empty and the update is a no‑op or may error downstream. Optionally reject empty post-strip payloads.
Example:
$data = $this->removeReadonlyAttributes($data); +if (empty($data)) { + throw new Exception($this->getMissingPayloadException()); +}src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
301-302: Sanitizing relation payloads: good. Also break the by-ref foreach to avoid lingering references.Unset the reference var after the loop to prevent accidental mutations later in scope.
- foreach ($relations as &$relation) { + foreach ($relations as &$relation) { $relation = $this->removeReadonlyAttributes($relation); // ... } + unset($relation); // break referencesrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)
207-208: Break by-ref foreach after relation sanitation.Avoid lingering reference from foreach ($relations as &$relation).
foreach ($relations as &$relation) { $relation = $this->removeReadonlyAttributes($relation); // ... } + unset($relation); // break referencesrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
31-32: Consider expanding removableAttributes; keep API-specific keys out of payloadsReordering is fine. Also remove '$collection' and '$tenant' if users submit them; they’re server-managed.
Apply:
- $this->removableAttributes = ['$sequence', '$databaseId', $contextId]; + $this->removableAttributes = [ + '$sequence', + '$databaseId', + $contextId, + '$collection', + '$tenant', + ];
📜 Review details
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.
📒 Files selected for processing (7)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php(4 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php(3 hunks)tests/e2e/Services/Databases/Legacy/DatabasesBase.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php
🧬 Code graph analysis (6)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(202-209)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(202-209)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (1)
tests/e2e/Client.php (1)
Client(8-328)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(202-209)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(202-209)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
369-371: Root payload sanitization timing: LGTM. Verify '$collection' is not listed as removable.Since '$collection' is set earlier and required by Utopia, confirm it’s not in removableAttributes to avoid stripping it.
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (1)
3047-3071: Good coverage of readonly semantics on PATCHThe assertions correctly verify that server-managed attributes cannot be overridden. LGTM.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (1)
164-165: Ensure removeReadonlyAttributes safely handles both arrays and Document instancesThese calls pass arrays (root $data) and possibly Utopia Document instances (relations). The current implementation in Action::removeReadonlyAttributes uses array unsets and will fatal if a Document slips through. Fix the helper to branch on type. See suggested patch in Action.php review.
Also applies to: 198-199
| protected function removeReadonlyAttributes(Document|array $document): Document|array | ||
| { | ||
| foreach ($this->removableAttributes as $attribute) { | ||
| $document->removeAttribute($attribute); | ||
| \var_dump('Removing attribute: ' . $attribute); | ||
| unset($document[$attribute]); | ||
| } | ||
| return $document; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix helper: support Document|array, no var_dump, no fatal on Document
- Using unset($document[$attribute]) on Utopia\Database\Document can fatal if ArrayAccess isn’t provided.
- var_dump leaks internals and breaks responses.
- Keep previous in-place mutation for Document; use unset for arrays; return the (possibly same) value.
Apply:
- protected function removeReadonlyAttributes(Document|array $document): Document|array
- {
- foreach ($this->removableAttributes as $attribute) {
- \var_dump('Removing attribute: ' . $attribute);
- unset($document[$attribute]);
- }
- return $document;
- }
+ protected function removeReadonlyAttributes(Document|array $document): Document|array
+ {
+ foreach ($this->removableAttributes as $attribute) {
+ if ($document instanceof Document) {
+ $document->removeAttribute($attribute);
+ } elseif (\is_array($document)) {
+ unset($document[$attribute]);
+ }
+ }
+ return $document;
+ }Run to spot any remaining debug prints and audit call sites:
🏁 Script executed:
#!/bin/bash
# 1) Any stray var_dump in src
rg -nP --type=php '\bvar_dump\s*\(' src || true
# 2) All removeReadonlyAttributes call sites and surrounding usage
rg -nP --type=php -C2 '->removeReadonlyAttributes\s*\(' src
# Expectation:
# - No var_dump matches.
# - Calls passing arrays should capture the return value; calls passing Document can ignore it.Length of output: 785
🏁 Script executed:
#!/bin/bash
# 1) Verify stray var_dump usage in src
rg -nP --type=php '\bvar_dump\s*\(' src || true
# 2) Find all call sites of removeReadonlyAttributes with context
rg -nP --type=php -C2 'removeReadonlyAttributes\s*\(' src || trueLength of output: 6268
Remove debug dumps and update removeReadonlyAttributes to handle Document safely
- In
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php(line 205) and.../Update.php(line 244), remove the\var_dump(...)calls leaking internals. - Replace
removeReadonlyAttributesbody with:
protected function removeReadonlyAttributes(Document|array $document): Document|array
{
- foreach ($this->removableAttributes as $attribute) {
- \var_dump('Removing attribute: ' . $attribute);
- unset($document[$attribute]);
- }
+ foreach ($this->removableAttributes as $attribute) {
+ if ($document instanceof Document) {
+ $document->removeAttribute($attribute);
+ } elseif (is_array($document)) {
+ unset($document[$attribute]);
+ }
+ }
return $document;
}- After updating, re-run
rg --type=php '\bvar_dump\s*\(' srcto ensure no stray debug prints remain. - Audit all
removeReadonlyAttributes(...)call sites to confirm array returns are captured and Document instances aren’t inadvertently re-wrapped.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function removeReadonlyAttributes(Document|array $document): Document|array | |
| { | |
| foreach ($this->removableAttributes as $attribute) { | |
| $document->removeAttribute($attribute); | |
| \var_dump('Removing attribute: ' . $attribute); | |
| unset($document[$attribute]); | |
| } | |
| return $document; | |
| } | |
| protected function removeReadonlyAttributes(Document|array $document): Document|array | |
| { | |
| foreach ($this->removableAttributes as $attribute) { | |
| if ($document instanceof Document) { | |
| $document->removeAttribute($attribute); | |
| } elseif (is_array($document)) { | |
| unset($document[$attribute]); | |
| } | |
| } | |
| return $document; | |
| } |
🤖 Prompt for AI Agents
In
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
around lines 202–209, remove the debugging \var_dump(...) calls and change
removeReadonlyAttributes so it does not assume the input is an array: iterate
removableAttributes and for each attribute, if $document is an array unset the
key, if $document is an instance of Document call the appropriate Document API
to remove that attribute (e.g. Document::removeAttribute or the project's
documented mutator) and leave other types untouched; ensure the method returns
the same type it received (Document or array). After editing, run rg --type=php
'\bvar_dump\s*\(' src to remove any remaining debug dumps and audit all call
sites of removeReadonlyAttributes to confirm callers correctly handle array
returns vs Document instances and do not re-wrap Document objects unnecessarily.
| foreach ($documents as $key => $document) { | ||
| $document = $this->removeReadonlyAttributes($document); | ||
| $documents[$key] = new Document($document); | ||
| } |
There was a problem hiding this comment.
Remove noisy debug output in removeReadonlyAttributes (can corrupt HTTP responses).
Calls here rely on a helper that currently var_dumps per attribute. This can leak into JSON responses and break clients. Replace the var_dump with a silent unset (or proper logger) in Action.php.
Apply in Action.php:
protected function removeReadonlyAttributes(Document|array $document): Document|array
{
foreach ($this->removableAttributes as $attribute) {
- \var_dump('Removing attribute: ' . $attribute);
- unset($document[$attribute]);
+ if (isset($document[$attribute])) {
+ unset($document[$attribute]);
+ }
}
return $document;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach ($documents as $key => $document) { | |
| $document = $this->removeReadonlyAttributes($document); | |
| $documents[$key] = new Document($document); | |
| } | |
| protected function removeReadonlyAttributes(Document|array $document): Document|array | |
| { | |
| foreach ($this->removableAttributes as $attribute) { | |
| if (isset($document[$attribute])) { | |
| unset($document[$attribute]); | |
| } | |
| } | |
| return $document; | |
| } |
🤖 Prompt for AI Agents
In
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php
around lines 102-105, the helper removeReadonlyAttributes used there currently
emits var_dump output which can leak into HTTP JSON responses; open Action.php
(where removeReadonlyAttributes is implemented), remove any var_dump/echo/print
statements, and instead silently unset the readonly keys from the document (e.g.
foreach readonly keys do unset($document[$key]) or unset($document->{$key}) as
appropriate) or replace those debug prints with a proper logger call
(logger->debug) that does not write to stdout, then return the cleaned document
so no debug text is emitted into responses.
| \var_dump($newDocument); | ||
|
|
There was a problem hiding this comment.
Remove debug output; var_dump corrupts HTTP responses and leaks data
var_dump will pollute JSON bodies and can expose sensitive info under load. Remove it.
Apply:
- \var_dump($newDocument);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| \var_dump($newDocument); |
🤖 Prompt for AI Agents
In
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
around lines 244-245, remove the debug statement "var_dump($newDocument)"
because it corrupts HTTP responses and can leak sensitive data; delete that line
and, if necessary, replace it with a proper logger call (e.g., use the existing
logger to log non-sensitive info) or nothing at all to avoid contaminating JSON
output.
| $data = $this->removeReadonlyAttributes($data); | ||
| $newDocument = new Document($data); | ||
| $operations = 0; |
There was a problem hiding this comment.
Remove var_dump in removeReadonlyAttributes to prevent response corruption.
Same concern as bulk upsert path—helper emits output. Replace with silent unset (or logger).
Patch in Action.php:
- \var_dump('Removing attribute: ' . $attribute);
- unset($document[$attribute]);
+ if (isset($document[$attribute])) {
+ unset($document[$attribute]);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php
around lines 173 to 175, the removeReadonlyAttributes helper currently emits
output (var_dump) which corrupts HTTP responses; update removeReadonlyAttributes
to stop printing—remove the var_dump and perform silent removal of readonly keys
(unset) or use the logger to record removed attributes instead, and ensure the
bulk upsert path uses the same silent behavior so no debug output is written to
the response.
|
|
||
| // Readonly attributes are ignored | ||
| $personNoPerm = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $person['body']['$id'] . '/documents/' . $newPersonId, array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), [ | ||
| 'data' => [ | ||
| '$id' => 'some-other-id', | ||
| '$collectionId' => 'some-other-collection', | ||
| '$databaseId' => 'some-other-database', | ||
| '$createdAt' => '2024-01-01T00:00:00Z', | ||
| '$updatedAt' => '2024-01-01T00:00:00Z', | ||
| 'library' => [ | ||
| '$id' => 'library3', | ||
| 'libraryName' => 'Library 3', | ||
| '$createdAt' => '2024-01-01T00:00:00Z', | ||
| '$updatedAt' => '2024-01-01T00:00:00Z', | ||
| ], | ||
| ], | ||
| ]); | ||
|
|
||
| $update = $personNoPerm; | ||
| $update['body']['$id'] = 'random'; | ||
| $update['body']['$sequence'] = 123; | ||
| $update['body']['$databaseId'] = 'random'; | ||
| $update['body']['$collectionId'] = 'random'; | ||
| $update['body']['$createdAt'] = '2024-01-01T00:00:00Z'; | ||
| $update['body']['$updatedAt'] = '2024-01-01T00:00:00Z'; | ||
|
|
||
| $upserted = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $person['body']['$id'] . '/documents/' . $newPersonId, array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| ], $this->getHeaders()), [ | ||
| 'data' => $update['body'] | ||
| ]); | ||
|
|
||
| $this->assertEquals(200, $upserted['headers']['status-code']); | ||
| $this->assertEquals($personNoPerm['body']['$id'], $upserted['body']['$id']); | ||
| $this->assertEquals($personNoPerm['body']['$collectionId'], $upserted['body']['$collectionId']); | ||
| $this->assertEquals($personNoPerm['body']['$databaseId'], $upserted['body']['$databaseId']); | ||
| $this->assertEquals($personNoPerm['body']['$sequence'], $upserted['body']['$sequence']); | ||
| $this->assertEquals($personNoPerm['body']['$createdAt'], $upserted['body']['$createdAt']); | ||
| $this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['$updatedAt']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen assertions; avoid brittle “echo body back” payload
- Add checks that nested relation readonly fields are also ignored.
- Using the entire previous response body as the next request data risks coupling tests to incidental fields (e.g., $permissions). Prefer a minimal payload containing only the readonly fields you’re asserting against.
Apply this diff to extend assertions (keeping your current flow):
$this->assertEquals(200, $upserted['headers']['status-code']);
$this->assertEquals($personNoPerm['body']['$id'], $upserted['body']['$id']);
$this->assertEquals($personNoPerm['body']['$collectionId'], $upserted['body']['$collectionId']);
$this->assertEquals($personNoPerm['body']['$databaseId'], $upserted['body']['$databaseId']);
$this->assertEquals($personNoPerm['body']['$sequence'], $upserted['body']['$sequence']);
$this->assertEquals($personNoPerm['body']['$createdAt'], $upserted['body']['$createdAt']);
$this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['$updatedAt']);
+ // Nested relation readonly attributes should also be ignored
+ $this->assertEquals('Library 3', $upserted['body']['library']['libraryName']);
+ $this->assertArrayHasKey('$createdAt', $upserted['body']['library']);
+ $this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['library']['$createdAt']);
+ $this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['library']['$updatedAt']);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Readonly attributes are ignored | |
| $personNoPerm = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $person['body']['$id'] . '/documents/' . $newPersonId, array_merge([ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| ], $this->getHeaders()), [ | |
| 'data' => [ | |
| '$id' => 'some-other-id', | |
| '$collectionId' => 'some-other-collection', | |
| '$databaseId' => 'some-other-database', | |
| '$createdAt' => '2024-01-01T00:00:00Z', | |
| '$updatedAt' => '2024-01-01T00:00:00Z', | |
| 'library' => [ | |
| '$id' => 'library3', | |
| 'libraryName' => 'Library 3', | |
| '$createdAt' => '2024-01-01T00:00:00Z', | |
| '$updatedAt' => '2024-01-01T00:00:00Z', | |
| ], | |
| ], | |
| ]); | |
| $update = $personNoPerm; | |
| $update['body']['$id'] = 'random'; | |
| $update['body']['$sequence'] = 123; | |
| $update['body']['$databaseId'] = 'random'; | |
| $update['body']['$collectionId'] = 'random'; | |
| $update['body']['$createdAt'] = '2024-01-01T00:00:00Z'; | |
| $update['body']['$updatedAt'] = '2024-01-01T00:00:00Z'; | |
| $upserted = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $person['body']['$id'] . '/documents/' . $newPersonId, array_merge([ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| ], $this->getHeaders()), [ | |
| 'data' => $update['body'] | |
| ]); | |
| $this->assertEquals(200, $upserted['headers']['status-code']); | |
| $this->assertEquals($personNoPerm['body']['$id'], $upserted['body']['$id']); | |
| $this->assertEquals($personNoPerm['body']['$collectionId'], $upserted['body']['$collectionId']); | |
| $this->assertEquals($personNoPerm['body']['$databaseId'], $upserted['body']['$databaseId']); | |
| $this->assertEquals($personNoPerm['body']['$sequence'], $upserted['body']['$sequence']); | |
| $this->assertEquals($personNoPerm['body']['$createdAt'], $upserted['body']['$createdAt']); | |
| $this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['$updatedAt']); | |
| } | |
| $this->assertEquals(200, $upserted['headers']['status-code']); | |
| $this->assertEquals($personNoPerm['body']['$id'], $upserted['body']['$id']); | |
| $this->assertEquals($personNoPerm['body']['$collectionId'], $upserted['body']['$collectionId']); | |
| $this->assertEquals($personNoPerm['body']['$databaseId'], $upserted['body']['$databaseId']); | |
| $this->assertEquals($personNoPerm['body']['$sequence'], $upserted['body']['$sequence']); | |
| $this->assertEquals($personNoPerm['body']['$createdAt'], $upserted['body']['$createdAt']); | |
| $this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['$updatedAt']); | |
| // Nested relation readonly attributes should also be ignored | |
| $this->assertEquals('Library 3', $upserted['body']['library']['libraryName']); | |
| $this->assertArrayHasKey('$createdAt', $upserted['body']['library']); | |
| $this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['library']['$createdAt']); | |
| $this->assertNotEquals('2024-01-01T00:00:00Z', $upserted['body']['library']['$updatedAt']); | |
| } |
🤖 Prompt for AI Agents
In tests/e2e/Services/Databases/Legacy/DatabasesBase.php around lines 2215-2258,
stop reusing the entire previous response body as the PUT payload and instead
send a minimal payload containing only the readonly fields you intend to test
(top-level readonly ids/sequence/timestamps and the nested relation readonly
fields under library), then assert that those readonly fields are ignored both
at top-level and inside the nested library object (compare
personNoPerm['body']['$id'], ['$collectionId'], ['$databaseId'], ['$sequence'],
['$createdAt'] and personNoPerm['body']['library']['$id'], ['libraryName' if
needed? — read-only fields only], ['$createdAt'], ['$updatedAt'] to the upserted
response), and also assert that incidental fields like $permissions are not
being relied upon or echoed back (i.e., not required in the request and
unchanged/absent in response). Replace the request payload with a small array of
only the readonly fields to be tested and add assertions verifying nested
relation readonly fields are ignored and updatedAt changes as before.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
300-329: Associative relations with $id skip sanitization and permissionsIf a relation is an associative array that already contains
$id, it never gets converted toDocument, so it bypasses bothremoveReadonlyAttributesand permission checks in this block. Convert any associative array toDocument(preserving an existing$id) to ensure consistent handling.Apply:
- foreach ($relations as &$relation) { - if ( - \is_array($relation) - && \array_values($relation) !== $relation - && !isset($relation['$id']) - ) { - $relation['$id'] = ID::unique(); - $relation = new Document($relation); - } + foreach ($relations as &$relation) { + // Normalize associative arrays into Document, preserving existing $id + if (\is_array($relation) && \array_values($relation) !== $relation) { + if (!isset($relation['$id'])) { + $relation['$id'] = ID::unique(); + } + $relation = new Document($relation); + }
📜 Review details
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.
📒 Files selected for processing (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
355-371: removeReadonlyAttributes only unsets the API‐specific attributes defined in setHttpPath ($sequence,$databaseId, and the context ID ) and never removes$id,$collection,$createdAt, or$updatedAt. No changes required.Likely an incorrect or invalid review comment.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (1)
234-235: Remove debug output; var_dump corrupts responses and leaks dataDelete these lines; printing
$newDocumentbreaks JSON output and may expose sensitive fields. If needed, use structured logging with redaction.- \var_dump($newDocument);src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
212-226: Don’t unset on Document; use removeAttribute for safety (also keep array support)Unset on Utopia\Database\Document is unsafe; use the Document API. Keep unset for arrays; return the (possibly same) value. Also ensure callers capture the return when passing arrays.
Apply:
- foreach ($this->removableAttributes['*'] as $attribute) { - unset($document[$attribute]); - } + foreach ($this->removableAttributes['*'] as $attribute) { + if ($document instanceof Document) { + $document->removeAttribute($attribute); + } elseif (\is_array($document)) { + unset($document[$attribute]); + } + } - if (!$privileged) { - foreach ($this->removableAttributes['privileged'] ?? [] as $attribute) { - unset($document[$attribute]); - } - } + if (!$privileged) { + foreach ($this->removableAttributes['privileged'] ?? [] as $attribute) { + if ($document instanceof Document) { + $document->removeAttribute($attribute); + } elseif (\is_array($document)) { + unset($document[$attribute]); + } + } + }#!/bin/bash # 1) Ensure no remaining var_dump in src (debug leaks) rg -nP --type=php '\bvar_dump\s*\(' src || true # 2) Audit call sites capture return when passing arrays rg -nP --type=php -C3 '->removeReadonlyAttributes\s*\(' src # Manually verify: when an array is passed, result is assigned, e.g.: # $data = $this->removeReadonlyAttributes($data, ...);
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (2)
179-184: Harden against undefined index on removableAttributes['*']Guard the loop to avoid notices if the key is missing.
- foreach ($this->removableAttributes['*'] as $attribute) { + foreach (($this->removableAttributes['*'] ?? []) as $attribute) {
189-194: Same guard when stripping non-requested attributesMirror the null-coalescing fallback here too.
- foreach ($this->removableAttributes['*'] as $attribute) { + foreach (($this->removableAttributes['*'] ?? []) as $attribute) {
📜 Review details
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.
📒 Files selected for processing (9)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php(2 hunks)src/Appwrite/Platform/Workers/Migrations.php(0 hunks)tests/e2e/Services/Databases/Legacy/DatabasesBase.php(3 hunks)
💤 Files with no reviewable changes (1)
- src/Appwrite/Platform/Workers/Migrations.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/Services/Databases/Legacy/DatabasesBase.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
🧬 Code graph analysis (5)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-112)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(212-226)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(212-226)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-112)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (2)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-112)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(212-226)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
removeReadonlyAttributes(212-226)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-112)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
[error] 1-1: Pint PSR-12: braces_position violation detected in Action.php. Lint command 'vendor/bin/pint --test --config pint.json' failed with exit code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (13)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
130-130: Good: centralized sanitization with correct privilege scopeUsing
privileged: trueis consistent with ADMIN/KEY-only access for bulk update. No further changes needed.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (3)
154-156: LGTM: sanitize payload pre-Document constructionPrivilege-aware stripping before building
$newDocumentis correct and aligns with the new helper signature.
159-166: LGTM: propagate privilege context into relation walkerCapturing
$isAPIKey/$isPrivilegedUserin the closure maintains consistent sanitization behavior in nested relations.
198-201: LGTM: sanitize related Document objectsApplying
removeReadonlyAttributesto each relatedDocumentkeeps nested writes consistent.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (4)
165-166: LGTM: sanitize root payload with privilege awarenessPre-construction sanitization matches Update/Create behavior.
168-175: LGTM: pass privilege context into relation traversalKeeps nested upserts consistent with top-level rules.
207-213: LGTM: sanitize each related Document before lookupPrevents clients from smuggling read-only attrs into nested writes.
357-359: LGTM: sanitize per-document in bulk mapRight place in the pipeline; maintains consistent behavior in bulk upserts.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (3)
257-259: LGTM: carry privilege context into permission-check walkerEnsures nested permission checks and sanitization use the same rules.
310-311: LGTM: sanitize nested relations before auth/existence checksPrevents read-only fields from influencing permission decisions.
357-359: LGTM: sanitize before instantiating DocumentKeeps single-doc create aligned with bulk and upsert paths.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (2)
1-1: Fix applied: PSR-12 braces_position
Pint auto-fix aligned the function’s opening brace with its declaration per the project’s pint.json; this file now passes Pint.
31-41: No action needed:$removableAttributesalready declared in parentThe
protected array $removableAttributes = [];declaration exists insrc/Appwrite/Platform/Action.php(line 34), so the child class inherits the properly typed property—no redeclaration required.Likely an incorrect or invalid review comment.
| $data['$id'] = $documentId; | ||
| $data['$permissions'] = $permissions ?? []; | ||
| $data = $this->removeReadonlyAttributes($data, $isAPIKey || $isPrivilegedUser); | ||
| $newDocument = new Document($data); | ||
| $operations = 0; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify no lingering debug prints in helper
Past commits showed var_dump inside removeReadonlyAttributes. Please confirm none remain.
Also applies to: 207-208, 357-357
🏁 Script executed:
#!/usr/bin/env bash
# Scan the repo for accidental debug prints
rg -n 'var_dump\(|print_r\(' src/Appwrite/Platform/Modules/Databases/Http/Databases/ -C2
# Confirm helper implementation
rg -nPU '(?s)protected\s+function\s+removeReadonlyAttributes.*?\{' src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php -C5Length of output: 1493
Remove lingering debug print in Update.php
Delete the stray \var_dump($newDocument); at line 234 in
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php.
No other var_dump or print_r calls remain under src/Appwrite/Platform/Modules/Databases/Http/Databases.
🤖 Prompt for AI Agents
In
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
around line 234, there is a stray debug call `\var_dump($newDocument);` that
must be removed; delete that line and run a quick grep to ensure no other
`var_dump` or `print_r` calls remain under
src/Appwrite/Platform/Modules/Databases/Http/Databases, keeping behavior
unchanged and no additional logging added.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (1)
2216-2263: Stop echoing entire response body back as PUT payload; assert nested readonly tooReusing the previous response body as the next request payload is brittle and hides what you’re testing. Also missing: assertions that nested relation readonly fields are ignored. Replace with a minimal payload and strengthen assertions. (Same feedback shared earlier.)
Apply:
- // Readonly attributes are ignored - $personNoPerm = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $person['body']['$id'] . '/documents/' . $newPersonId, array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), [ - 'data' => [ - '$id' => 'some-other-id', - '$collectionId' => 'some-other-collection', - '$databaseId' => 'some-other-database', - '$createdAt' => '2024-01-01T00:00:00Z', - '$updatedAt' => '2024-01-01T00:00:00Z', - 'library' => [ - '$id' => 'library3', - 'libraryName' => 'Library 3', - '$createdAt' => '2024-01-01T00:00:00Z', - '$updatedAt' => '2024-01-01T00:00:00Z', - ], - ], - ]); - - $update = $personNoPerm; - $update['body']['$id'] = 'random'; - $update['body']['$sequence'] = 123; - $update['body']['$databaseId'] = 'random'; - $update['body']['$collectionId'] = 'random'; - $update['body']['$createdAt'] = '2024-01-01T00:00:00.000+00:00'; - $update['body']['$updatedAt'] = '2024-01-01T00:00:00.000+00:00'; - - $upserted = $this->client->call(Client::METHOD_PUT, '/databases/' . $databaseId . '/collections/' . $person['body']['$id'] . '/documents/' . $newPersonId, array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], $this->getHeaders()), [ - 'data' => $update['body'] - ]); + // Readonly attributes are ignored — use minimal payload (no incidental fields like $permissions) + $prevCreatedAt = $personNoPerm['body']['$createdAt']; + $prevUpdatedAt = $personNoPerm['body']['$updatedAt']; + $readonlyPayload = [ + 'data' => [ + // top-level readonly attempts + '$id' => 'some-other-id', + '$collectionId' => 'some-other-collection', + '$databaseId' => 'some-other-database', + '$sequence' => 123, + '$createdAt' => '2024-01-01T00:00:00.000+00:00', + '$updatedAt' => '2024-01-01T00:00:00.000+00:00', + // nested relation: readonly attempts + a regular writable field + 'library' => [ + '$id' => 'library3', + 'libraryName' => 'Library 3', + '$createdAt' => '2024-01-01T00:00:00.000+00:00', + '$updatedAt' => '2024-01-01T00:00:00.000+00:00', + ], + ], + ]; + + $upserted = $this->client->call( + Client::METHOD_PUT, + '/databases/' . $databaseId . '/collections/' . $person['body']['$id'] . '/documents/' . $newPersonId, + array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), + $readonlyPayload + ); $this->assertEquals(200, $upserted['headers']['status-code']); $this->assertEquals($personNoPerm['body']['$id'], $upserted['body']['$id']); $this->assertEquals($personNoPerm['body']['$collectionId'], $upserted['body']['$collectionId']); $this->assertEquals($personNoPerm['body']['$databaseId'], $upserted['body']['$databaseId']); $this->assertEquals($personNoPerm['body']['$sequence'], $upserted['body']['$sequence']); if ($this->getSide() === 'client') { - $this->assertEquals($personNoPerm['body']['$createdAt'], $upserted['body']['$createdAt']); - $this->assertNotEquals('2024-01-01T00:00:00.000+00:00', $upserted['body']['$updatedAt']); + // createdAt unchanged; updatedAt changed (but not to client-supplied value) + $this->assertEquals($prevCreatedAt, $upserted['body']['$createdAt']); + $this->assertNotEquals($prevUpdatedAt, $upserted['body']['$updatedAt']); + $this->assertNotEquals('2024-01-01T00:00:00.000+00:00', $upserted['body']['$updatedAt']); + // Nested relation readonly attributes should also be ignored + $this->assertEquals('Library 3', $upserted['body']['library']['libraryName']); + $this->assertArrayHasKey('$createdAt', $upserted['body']['library']); + $this->assertNotEquals('2024-01-01T00:00:00.000+00:00', $upserted['body']['library']['$createdAt']); + $this->assertNotEquals('2024-01-01T00:00:00.000+00:00', $upserted['body']['library']['$updatedAt']); } else { $this->assertEquals('2024-01-01T00:00:00.000+00:00', $upserted['body']['$createdAt']); $this->assertEquals('2024-01-01T00:00:00.000+00:00', $upserted['body']['$updatedAt']); }
🧹 Nitpick comments (2)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (2)
3053-3082: Strengthen PATCH readonly checks by comparing to prior valuesBefore issuing the readonly-field PATCH, capture current createdAt/updatedAt and assert invariants against them. Avoid only checking against the literal payload strings.
- // Test readonly attributes are ignored + // Test readonly attributes are ignored + // Capture current timestamps to assert invariants + $current = $this->client->call( + Client::METHOD_GET, + '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $id, + array_merge(['content-type' => 'application/json','x-appwrite-project' => $this->getProject()['$id']], $this->getHeaders()) + ); + $prevCreatedAt = $current['body']['$createdAt']; + $prevUpdatedAt = $current['body']['$updatedAt']; $response = $this->client->call(Client::METHOD_PATCH, '/databases/' . $databaseId . '/collections/' . $data['moviesId'] . '/documents/' . $id, array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-timestamp' => DateTime::formatTz(DateTime::now()), ], $this->getHeaders()), [ 'data' => [ '$id' => 'newId', '$sequence' => 9999, '$collectionId' => 'newCollectionId', '$databaseId' => 'newDatabaseId', '$createdAt' => '2024-01-01T00:00:00.000+00:00', '$updatedAt' => '2024-01-01T00:00:00.000+00:00', 'title' => 'Thor: Ragnarok', ], ]); @@ if ($this->getSide() === 'client') { - $this->assertNotEquals('2024-01-01T00:00:00.000+00:00', $response['body']['$createdAt']); - $this->assertNotEquals('2024-01-01T00:00:00.000+00:00', $response['body']['$updatedAt']); + $this->assertEquals($prevCreatedAt, $response['body']['$createdAt']); + $this->assertNotEquals($prevUpdatedAt, $response['body']['$updatedAt']); + $this->assertNotEquals('2024-01-01T00:00:00.000+00:00', $response['body']['$updatedAt']); } else { $this->assertEquals('2024-01-01T00:00:00.000+00:00', $response['body']['$createdAt']); $this->assertEquals('2024-01-01T00:00:00.000+00:00', $response['body']['$updatedAt']); }
4344-4347: Use stronger invariants for client expectationsAlso assert createdAt remains the original and updatedAt actually changes relative to the last update.
- $this->assertEquals($document['body']['title'], 'Again Updated Date Test'); - $this->assertNotEquals($document['body']['$createdAt'], DateTime::formatTz('2022-08-01 13:09:23.040')); - $this->assertNotEquals($document['body']['$updatedAt'], DateTime::formatTz('2022-08-01 13:09:23.050')); + $this->assertEquals($document['body']['title'], 'Again Updated Date Test'); + $this->assertEquals($createdAt, $document['body']['$createdAt']); // createdAt must persist + $this->assertNotEquals($updatedAtSecond, $document['body']['$updatedAt']); // updatedAt should change + $this->assertNotEquals(DateTime::formatTz('2022-08-01 13:09:23.040'), $document['body']['$createdAt']); + $this->assertNotEquals(DateTime::formatTz('2022-08-01 13:09:23.050'), $document['body']['$updatedAt']);
📜 Review details
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.
📒 Files selected for processing (4)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php(4 hunks)tests/e2e/Services/Databases/Legacy/DatabasesCustomClientTest.php(0 hunks)tests/e2e/Services/Databases/TablesDB/DatabasesBase.php(1 hunks)tests/e2e/Services/Databases/TablesDB/DatabasesCustomClientTest.php(0 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/Services/Databases/Legacy/DatabasesCustomClientTest.php
- tests/e2e/Services/Databases/TablesDB/DatabasesCustomClientTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (1)
tests/e2e/Client.php (1)
Client(8-328)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (1)
tests/e2e/Services/Databases/Legacy/DatabasesBase.php (1)
1700-1700: No-op whitespace changeNothing to review here.
| $this->assertNotEquals($row['body']['$createdAt'], DateTime::formatTz('2022-08-01 13:09:23.040')); | ||
| $this->assertNotEquals($row['body']['$updatedAt'], DateTime::formatTz('2022-08-01 13:09:23.050')); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen invariants for readonly timestamps on client updates
Add explicit checks that createdAt remains unchanged and updatedAt advances, and assert 200 status to catch regressions.
- $this->assertNotEquals($row['body']['$createdAt'], DateTime::formatTz('2022-08-01 13:09:23.040'));
- $this->assertNotEquals($row['body']['$updatedAt'], DateTime::formatTz('2022-08-01 13:09:23.050'));
+ $this->assertEquals(200, $row['headers']['status-code']);
+ $this->assertNotEquals($row['body']['$createdAt'], DateTime::formatTz('2022-08-01 13:09:23.040'));
+ $this->assertNotEquals($row['body']['$updatedAt'], DateTime::formatTz('2022-08-01 13:09:23.050'));
+ // Ensure immutability and auto-update behavior
+ $this->assertEquals($createdAt, $row['body']['$createdAt']);
+ $this->assertNotEquals($updatedAtSecond, $row['body']['$updatedAt']);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->assertNotEquals($row['body']['$createdAt'], DateTime::formatTz('2022-08-01 13:09:23.040')); | |
| $this->assertNotEquals($row['body']['$updatedAt'], DateTime::formatTz('2022-08-01 13:09:23.050')); | |
| } else { | |
| $this->assertEquals(200, $row['headers']['status-code']); | |
| $this->assertNotEquals($row['body']['$createdAt'], DateTime::formatTz('2022-08-01 13:09:23.040')); | |
| $this->assertNotEquals($row['body']['$updatedAt'], DateTime::formatTz('2022-08-01 13:09:23.050')); | |
| // Ensure immutability and auto-update behavior | |
| $this->assertEquals($createdAt, $row['body']['$createdAt']); | |
| $this->assertNotEquals($updatedAtSecond, $row['body']['$updatedAt']); | |
| } else { |
🤖 Prompt for AI Agents
In tests/e2e/Services/Databases/TablesDB/DatabasesBase.php around lines
4273-4275, strengthen the readonly-timestamp assertions after the client update:
ensure the HTTP response status is asserted as 200, assert that
$row['body']['$createdAt'] equals the original createdAt value (so createdAt did
not change), and assert that $row['body']['$updatedAt'] is strictly greater than
the original updatedAt (so updatedAt advanced); use DateTime::formatTz for
consistent formatting when comparing timestamps and add these explicit
assertions in place of or in addition to the existing assertNotEquals checks.
1 participant
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist