Conversation
📝 WalkthroughWalkthroughAdds four string-based types (varchar, text, mediumtext, longtext) across the platform. New Response model classes for Attribute* and Column* types were introduced. New create/update HTTP action handlers were added for Collections API and TablesDB (attributes/columns), type-to-model mapping was extended, models registered in init/registry files, deprecation metadata added to legacy string endpoints, and comprehensive end-to-end tests for both Collections and TablesDB were added. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@app/init/models.php`:
- Around line 27-31: The import statements for the attribute models are not
sorted to PSR-12 order; reorder the use statements for AttributeLongtext,
AttributeMediumtext, AttributeText, AttributeURL, and AttributeVarchar (and the
similar block containing the same symbols around the second occurrence) into
canonical alphabetical order (or run Pint) so the imports are consistently
ordered and the linter passes.
In
`@src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Varchar/Create.php`:
- Line 65: Update the parameter description for the size option in the Create
endpoint: locate the param call that defines 'size' (the ->param('size', null,
new Range(1, 16381, Validator::TYPE_INTEGER), '...') invocation) and change the
human-readable string to reference "varchar attributes" (or "varchar attribute")
instead of "text attributes" so the description correctly reads that the size is
for varchar attributes and states the maximum of 16381 characters.
In `@src/Appwrite/Platform/Modules/Databases/Services/Registry/Legacy.php`:
- Around line 31-40: The added use statements for CreateTextAttribute,
UpdateTextAttribute, CreateURLAttribute, UpdateURLAttribute,
CreateVarcharAttribute, UpdateVarcharAttribute, CreateMediumtextAttribute,
UpdateMediumtextAttribute, CreateLongtextAttribute, and UpdateLongtextAttribute
need to be alphabetically ordered to satisfy PSR-12 ordered_imports; open
Legacy.php and reorder those use lines (and any nearby use imports) so they are
in ascending alphabetical order by full import string (or simply run
./vendor/bin/pint to auto-fix), ensuring symbols like CreateLongtextAttribute,
CreateMediumtextAttribute, CreateTextAttribute, CreateURLAttribute,
CreateVarcharAttribute and their corresponding Update* imports appear in the
correct alphabetical sequence.
In `@src/Appwrite/Platform/Modules/Databases/Services/Registry/TablesDB.php`:
- Around line 34-43: Reorder the import statements so they satisfy PSR-12
ordered_imports: alphabetize the added column class imports (CreateLongtext,
CreateMediumtext, CreateText, CreateURL, CreateVarchar, UpdateLongtext,
UpdateMediumtext, UpdateText, UpdateURL, UpdateVarchar) among the existing use
statements in TablesDB.php; you can either manually sort those use lines or run
./vendor/bin/pint to auto-fix the import ordering.
In `@tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php`:
- Around line 170-252: The duplicate-key assertion in
testCreateVarcharAttributeFailures assumes an attribute named 'varchar_field'
exists; modify the test so it explicitly depends on or creates that attribute
first: either add a dependency annotation to require the test/method that
creates the 'varchar_field' attribute (or collection attribute creation test) or
insert a setup step inside testCreateVarcharAttributeFailures that calls the
same API to create 'varchar_field' (expecting 201) before issuing the duplicate
create (expecting 409); reference the test name
testCreateVarcharAttributeFailures and the POST endpoint used to create varchar
attributes to locate where to add the dependency or setup call.
In `@tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php`:
- Around line 461-462: Replace the fixed sleep(3) and sleep(2) calls with
polling using assertEventually() from the Async trait: for the "Wait for columns
to be created" case (where sleep(3) is used) wrap the condition that verifies
the columns exist in assertEventually() with a sensible timeout and poll
interval; do the same for the other sleep(2) instance—replace it with
assertEventually() checking the exact expected state instead of a fixed delay.
Use the existing assertEventually() helper and the same condition expressions
used after each sleep to determine success, and tune timeout/interval to match
CI reliability.
🧹 Nitpick comments (2)
tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php (1)
632-660: Add assertions for mediumtext/longtext defaults.
Defaults for those types are updated earlier; asserting them here would lock the behavior.✅ Suggested assertions
$this->assertEquals(201, $row['headers']['status-code']); // Check that default values are applied $this->assertEquals('updated default', $row['body']['varchar_with_default']); $this->assertEquals('Updated text default value', $row['body']['text_with_default']); + $this->assertEquals('Updated mediumtext default', $row['body']['mediumtext_with_default']); + $this->assertEquals('Updated longtext default', $row['body']['longtext_with_default']);tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php (1)
461-462: Replace fixed sleeps with eventual polling to reduce flakiness.Consider using
assertEventually(available onScope) to wait for attribute availability instead of fixed delays; apply the same pattern in both locations.♻️ Example for the first sleep (apply similarly to the second)
- // Wait for attributes to be created - sleep(3); + $this->assertEventually(function () use ($databaseId, $collectionId) { + $attr = $this->client->call( + Client::METHOD_GET, + '/databases/' . $databaseId . '/collections/' . $collectionId . '/attributes/varchar_field', + [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ] + ); + $this->assertEquals(200, $attr['headers']['status-code']); + });Also applies to: 587-588
...Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Varchar/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Services/Registry/Legacy.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Services/Registry/TablesDB.php
Outdated
Show resolved
Hide resolved
✨ Benchmark results
⚡ Benchmark Comparison
|
Co-Authored-By: Claude Opus 4.5 <[email protected]>
The test tried to create multiple VARCHAR attributes in the same collection, including a 16381-character varchar. The cumulative row width exceeded MariaDB's 65535 byte row limit, causing the test to fail with a 400 error. Calculation: 1067 (base) + 1021 (255*4+1) + 401 (100*4+1) + 201 (50*4+1) + 20 (array) + 65526 (16381*4+2) = 68236 bytes > 65535 Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php`:
- Around line 755-776: The test method testDeleteStringTypeAttributes attempts
to delete an attribute named varchar_max which is never created, causing the
delete to return 404; update the test to target an existing attribute (for
example change the DELETE and subsequent GET paths from 'varchar_max' to
'varchar_renamed') or reintroduce creation of a safe-size attribute named
'varchar_max' earlier in the test setup so that the calls in
testDeleteStringTypeAttributes operate on a real attribute; adjust the
Client::METHOD_DELETE and Client::METHOD_GET calls in
testDeleteStringTypeAttributes accordingly to match the chosen attribute name.
In `@tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php`:
- Around line 755-776: The testDeleteStringTypeColumns method is deleting a
non-existent column varchar_max; update the test to target an existing column or
create the column first: either change the DELETE and subsequent GET requests to
use the existing column name varchar_renamed, or insert a safe creation step for
varchar_max earlier in testDeleteStringTypeColumns (or shared setup) so
varchar_max exists before attempting deletion; ensure the assertions remain
checking for 204 on delete and 404 on subsequent GET.
- Around line 153-238: The duplicate-key assertion in
testCreateVarcharColumnFailures uses 'varchar_field' which is created in
testCreateVarcharColumn but this test only depends on testCreateTable; update
testCreateVarcharColumnFailures to depend on testCreateVarcharColumn (add
"@depends testCreateVarcharColumn") or alternatively create the 'varchar_field'
column inline before the duplicate-key check so the duplicate constraint is
guaranteed; locate the test method testCreateVarcharColumnFailures and add the
dependency or inline column creation around the duplicate-key section
referencing 'varchar_field'.
♻️ Duplicate comments (2)
tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php (1)
447-448: Replace fixed sleeps with polling (assertEventually).
Same feedback as earlier review: fixed delays can be flaky; useassertEventually()for readiness checks.Also applies to: 573-574
tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php (1)
153-238: Ensure the duplicate-key test depends on attribute creation.
The duplicate-key check relies onvarchar_fieldexisting; make the test depend ontestCreateVarcharAttribute(or create the attribute inline) to avoid ordering flakiness.🔧 Suggested change
- /** - * `@depends` testCreateCollection - */ + /** + * `@depends` testCreateVarcharAttribute + */ public function testCreateVarcharAttributeFailures(array $data): void
🧹 Nitpick comments (1)
tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php (1)
447-448: Prefer polling over fixed sleeps for eventual consistency.
sleep(3)andsleep(2)can be flaky on slower CI. Consider usingassertEventually()to poll for attribute availability before updates and document creation.Also applies to: 573-574
The size field in the attributes collection is stored as a signed 32-bit integer (VAR_INTEGER). The longtext size of 4294967295 (2^32-1) exceeds the maximum value of 2147483647 (2^31-1), causing attribute creation to fail with a 400 error (document_invalid_structure). Changed the longtext size to 2147483647 which is the maximum value that fits within the signed 32-bit integer constraint of the schema. Co-Authored-By: Claude Opus 4.5 <[email protected]>
01b5c12 to
760f065
Compare
The string types feature requires the dev-feat-string-types branch of utopia-php/database which includes support for VARCHAR, TEXT, MEDIUMTEXT, and LONGTEXT types in the updateAttribute method. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@composer.json`:
- Line 55: Replace the unstable alias dependency entry for utopia-php/database
in composer.json ("utopia-php/database": "dev-feat-string-types as 4.7") with
the stable released constraint that contains string-types support: change it to
"utopia-php/database": "^4.6" (remove the dev branch alias and use the released
version); update composer.lock by running composer update afterward to ensure
the lockfile and installed packages reflect the stable v4.6+ release.
The updateAttribute method in utopia-php/database does not yet support VARCHAR, TEXT, MEDIUMTEXT, and LONGTEXT types. These tests are skipped until the upstream library adds support for updating these attribute types. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php`:
- Around line 573-576: The `@depends` on testUpdateLongtextAttribute in the
testCreateDocumentWithStringTypes method causes this test to be skipped whenever
updates are skipped; change the dependency to the last create-phase test instead
so create/GET/delete coverage still runs (e.g. replace `@depends`
testUpdateLongtextAttribute with `@depends` testCreateLongtextAttribute or the
actual final create test in the file), updating the docstring on
testCreateDocumentWithStringTypes accordingly; ensure you keep the method name
testCreateDocumentWithStringTypes and only modify the `@depends` annotation.
- Around line 626-654: The test testCreateDocumentWithDefaultValues assumes
downstream "update" steps ran and asserts updated defaults; instead make the
assertions validate the create-time defaults so the test is decoupled from
skipped updates: in testCreateDocumentWithDefaultValues update the checks
against document['body']['varchar_with_default'] and
document['body']['text_with_default'] to expect the collection's original
default values (or derive them from the collection/schema fixture/constants)
rather than "updated default" / "Updated text default value", or alternatively
fetch the collection schema defaults dynamically and assert those values.
In `@tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php`:
- Around line 576-579: The downstream tests are skipping because they depend on
testUpdateLongtextColumn; change those dependencies to depend on the last create
test instead so create/GET/delete coverage still runs when updates are skipped:
locate tests that have a `@depends` annotation referencing
testUpdateLongtextColumn (and any tests that list it in their signature) and
replace that dependency with testCreateRowWithStringTypes (the last create
test), updating method signatures and `@depends` docblocks accordingly so they no
longer transitively skip when update tests are skipped.
- Around line 626-654: The test testCreateRowWithDefaultValues currently asserts
for "updated" defaults but relies on previous skipped update tests; change it to
be deterministic by either (a) asserting the create-time default values defined
in the table schema for the fields varchar_with_default and text_with_default
instead of "updated default"/"Updated text default value", or (b) explicitly
provide values for varchar_with_default and text_with_default in the POST
payload's 'data' so the test does not depend on prior update runs; update the
assertions in testCreateRowWithDefaultValues accordingly.
♻️ Duplicate comments (4)
tests/e2e/Services/Databases/Legacy/DatabasesStringTypesTest.php (2)
153-156: Make duplicate-key failure depend on varchar creation.
The duplicate-key case assumesvarchar_fieldexists, but this test only depends on collection creation.🔧 Suggested fix
/** - * `@depends` testCreateCollection + * `@depends` testCreateVarcharAttribute */ public function testCreateVarcharAttributeFailures(array $data): voidAlso applies to: 226-238
763-784: Delete targets a non-existent attribute.
varchar_maxis never created in this class, so the delete will likely 404.🔧 Suggested fix (delete an existing attribute)
- $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/databases/' . $databaseId . '/collections/' . $collectionId . '/attributes/varchar_max', [ + $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/databases/' . $databaseId . '/collections/' . $collectionId . '/attributes/varchar_with_default', [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]); $this->assertEquals(204, $deleteVarchar['headers']['status-code']); // Verify deletion - $getDeleted = $this->client->call(Client::METHOD_GET, '/databases/' . $databaseId . '/collections/' . $collectionId . '/attributes/varchar_max', [ + $getDeleted = $this->client->call(Client::METHOD_GET, '/databases/' . $databaseId . '/collections/' . $collectionId . '/attributes/varchar_with_default', [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]);tests/e2e/Services/Databases/TablesDB/DatabasesStringTypesTest.php (2)
153-156: Make duplicate-key failure depend on column creation.
The duplicate-key case assumesvarchar_fieldexists, but this test only depends on table creation.🔧 Suggested fix
/** - * `@depends` testCreateTable + * `@depends` testCreateVarcharColumn */ public function testCreateVarcharColumnFailures(array $data): voidAlso applies to: 226-238
763-784: Delete targets a non-existent column.
varchar_maxis never created in this class, so the delete will likely 404.🔧 Suggested fix (delete an existing column)
- $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_max', [ + $deleteVarchar = $this->client->call(Client::METHOD_DELETE, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_with_default', [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]); $this->assertEquals(204, $deleteVarchar['headers']['status-code']); // Verify deletion - $getDeleted = $this->client->call(Client::METHOD_GET, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_max', [ + $getDeleted = $this->client->call(Client::METHOD_GET, '/tablesdb/' . $databaseId . '/tables/' . $tableId . '/columns/varchar_with_default', [ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]);
Co-authored-by: abnegate <[email protected]>
Co-authored-by: abnegate <[email protected]>
Co-authored-by: abnegate <[email protected]>
Co-authored-by: abnegate <[email protected]>
Co-authored-by: abnegate <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Create.php`:
- Line 7: Update the deprecation metadata for the string column creator: change
the replaceWith value from "createTextAttribute" to "createTextColumn" so it
points at the correct TablesDB API; locate the deprecation declaration in the
Create class for string columns (look for the Deprecated metadata and the
replaceWith property currently set to createTextAttribute) and set it to
"tablesDB.createTextColumn" (consistent with Text/Create.php and other tablesDB
creators).
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Create.php
Show resolved
Hide resolved
# Conflicts: # composer.lock
… feat-string-types # Conflicts: # src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Create.php # src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Update.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Update.php`:
- Around line 56-59: The Deprecated annotation in
Tables/Columns/String/Update.php uses since: '1.8.0' which is inconsistent with
the paired String/Create.php deprecation; update the Deprecated(...) invocation
in the Update endpoint to use since: '1.9.0' so both String column create and
update endpoints share the same deprecation version (keep replaceWith:
'tablesDB.updateTextColumn' unchanged).
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Update.php
Show resolved
Hide resolved
# Conflicts: # composer.lock
commit: |
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