-
Notifications
You must be signed in to change notification settings - Fork 4.8k
1.8.x #9985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* updated test cases regarding internal id
…zy-load-relationships
exposed internal id as a part of auto increment id
updated errro for the string encryption
# Conflicts: # app/controllers/api/functions.php # app/controllers/api/project.php # app/controllers/api/proxy.php # app/controllers/api/storage.php # app/controllers/api/vcs.php # app/controllers/general.php # app/controllers/shared/api.php # app/init/database/filters.php # composer.json # composer.lock # src/Appwrite/Migration/Migration.php # src/Appwrite/Migration/Version/V15.php # src/Appwrite/Migration/Version/V16.php # src/Appwrite/Migration/Version/V17.php # src/Appwrite/Migration/Version/V18.php # src/Appwrite/Migration/Version/V19.php # src/Appwrite/Migration/Version/V20.php # src/Appwrite/Migration/Version/V21.php # src/Appwrite/Platform/Tasks/Migrate.php # src/Appwrite/Platform/Workers/Builds.php # src/Appwrite/Platform/Workers/Deletes.php # src/Appwrite/Platform/Workers/Functions.php # src/Appwrite/Platform/Workers/StatsResources.php # src/Appwrite/Platform/Workers/StatsUsage.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
app/controllers/api/databases.php (2)
281-288
:⚠️ Potential issueBroken conditionals due to misplaced parentheses
getAttribute(('type') !== $type)
andgetAttribute(('filter') !== $filter)
evaluate the comparison before the call, so you end up executing
getAttribute(true|false)
on a non-existent boolean key.- if ($attribute->getAttribute(('type') !== $type)) { + if ($attribute->getAttribute('type') !== $type) { ... - if ($attribute->getAttribute('type') === Database::VAR_STRING && $attribute->getAttribute(('filter') !== $filter)) { + if ( + $attribute->getAttribute('type') === Database::VAR_STRING + && $attribute->getAttribute('filter') !== $filter + ) {
2848-2853
:⚠️ Potential issueInvalid named argument
optional
– will fatal in PHP 8
App::param()
doesn’t declare a parameter called$optional
; passing a named
argument that doesn’t exist triggers a runtime error.- ->param('lengths', [], new ArrayList(new Nullable(new Integer()), APP_LIMIT_ARRAY_PARAMS_SIZE), 'Length of index. Maximum of ' . APP_LIMIT_ARRAY_PARAMS_SIZE, optional: true) + ->param('lengths', [], new ArrayList(new Nullable(new Integer()), APP_LIMIT_ARRAY_PARAMS_SIZE), 'Length of index. Maximum of ' . APP_LIMIT_ARRAY_PARAMS_SIZE, true)
♻️ Duplicate comments (2)
app/controllers/api/databases.php (2)
297-299
: Parameter shadowing still hurts readability
$collectionId
is overwritten with an internal path, resurrecting the same
confusion flagged in earlier reviews. Use a distinct variable
(e.g.$internalCollectionPath
) instead of mutating the original identifier.
1354-1362
: 🛠️ Refactor suggestionNull-size input not handled for encrypted string attributes
If
size
is omitted (null
), the comparison$size < APP_DATABASE_ENCRYPT_SIZE_MIN
implicitly castsnull
to0
, throwing a misleading “size too small” error instead of “size is required”.
Add an explicit null-check:- if ($encrypt && $size < APP_DATABASE_ENCRYPT_SIZE_MIN) { + if ($encrypt && (is_null($size) || $size < APP_DATABASE_ENCRYPT_SIZE_MIN)) { throw new Exception( Exception::GENERAL_BAD_REQUEST, "Size too small. Encrypted strings require a minimum size of " . APP_DATABASE_ENCRYPT_SIZE_MIN . " characters." ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/cli.php
(4 hunks)app/controllers/api/databases.php
(94 hunks)app/init/resources.php
(7 hunks)app/worker.php
(5 hunks)tests/e2e/Services/Databases/DatabasesCustomServerTest.php
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/worker.php
- app/cli.php
- app/init/resources.php
- tests/e2e/Services/Databases/DatabasesCustomServerTest.php
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
# Conflicts: # .coderabbit.yaml
Feat: Lazy-load relationships
…nships Revert "Feat: Lazy-load relationships"
Add readable error for csv imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Appwrite/Utopia/Response/Model/Migration.php (2)
112-135
: Filter logic works, but side-effects on the passedDocument
object may be surprising
filter()
mutates the sameDocument
instance it receives and then returns it.
Given thatDocument
is passed by reference (standard PHP object behaviour), callers that still hold a reference to the original object will observe the filtered state even if they ignore the return value.
If this is intentional, consider documenting it in the PHPDoc for clarity; if not, clone the document before mutating to keep the method free of hidden side-effects.
119-129
: Consider explicit JSON-error handling
json_decode($error, true)
silently returnsnull
on malformed JSON.
Using theJSON_THROW_ON_ERROR
flag would catch corrupted payloads early and make debugging easier:- $decoded = json_decode($error, true); + try { + $decoded = json_decode($error, true, 512, JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + // keep original error string untouched + continue; + }This preserves your current behaviour for valid inputs but fails loudly (and skips the rewrite) when the payload is invalid.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/config/collections/projects.php
(1 hunks)src/Appwrite/Platform/Workers/Migrations.php
(5 hunks)src/Appwrite/Utopia/Response/Model/Migration.php
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/config/collections/projects.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Workers/Migrations.php
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build SDK
- GitHub Check: scan
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