Joins #133
Joins #133
Conversation
WalkthroughThis pull request updates the utopia-php/database dependency from version 3 to a development branch aliased as version 4.3.0. Correspondingly, the querySelect method signature across the Reader interface and its implementations is refactored to accept a single string column parameter instead of an array of columns. The Appwrite class is updated to iterate over the selects array and call querySelect individually for each column. A PHPDoc entry in the Database class is also expanded to document an additional field. The Reader/API.php file contains unreachable code with a TODO comment regarding a Query::select method fix. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Migration/Sources/Appwrite.php (1)
1160-1164: Critical:querySelectcalled with array instead of string.The
$stackvariable is an array, butquerySelectnow expects a singlestring $columnparameter per the updated interface. This will cause a type mismatch at runtime.This code path needs to iterate over
$stackelements, similar to lines 1146-1148:🔎 Proposed fix
+ $stackQueries = []; + foreach ($stack as $stackColumn) { + $stackQueries[] = $this->database->querySelect($stackColumn); + } + $rowItem = $this->database->getRow( $table, $row['$id'], - [$this->database->querySelect($stack)] + $stackQueries );
🤖 Fix all issues with AI Agents
In @src/Migration/Sources/Appwrite.php:
- Around line 1144-1148: The stale PHPDoc "@var Column|Relationship $attribute"
above the foreach is incorrect because the loop iterates over $selects as
$select; remove the erroneous PHPDoc or relocate it to the actual context where
$attribute is used (e.g., the block that handles $attribute variables), and
ensure the foreach uses no misleading docblock so PHPStan no longer flags
$attribute in the scope of the foreach over $selects/$select.
In @src/Migration/Sources/Appwrite/Reader/API.php:
- Around line 207-219: The querySelect method currently returns a Query object
via new Query('select', $column) while its native signature declares : string
and the PHPDoc says @return Query; replace the implementation to mirror other
methods by returning the static string form Query::select($column), update the
native return type to string (or adjust PHPDoc if you intentionally want a Query
object), and remove the unreachable/new Query(...) code so querySelect
consistently returns a string like the other query* methods.
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite/Reader.php (1)
5-5: Unused import.The
Utopia\Database\Queryimport is not used in this interface file. The interface only declares method signatures without referencing theQueryclass directly.🔎 Proposed fix
namespace Utopia\Migration\Sources\Appwrite; -use Utopia\Database\Query; use Utopia\Migration\Resource; use Utopia\Migration\Resources\Database\Database; use Utopia\Migration\Resources\Database\Table;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
composer.jsonsrc/Migration/Resources/Database/Database.phpsrc/Migration/Sources/Appwrite.phpsrc/Migration/Sources/Appwrite/Reader.phpsrc/Migration/Sources/Appwrite/Reader/API.phpsrc/Migration/Sources/Appwrite/Reader/Database.php
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Applied to files:
src/Migration/Sources/Appwrite/Reader/Database.phpsrc/Migration/Sources/Appwrite/Reader.php
📚 Learning: 2025-11-20T13:12:43.482Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/migration PR: 114
File: src/Migration/Resources/Database/Collection.php:14-37
Timestamp: 2025-11-20T13:12:43.482Z
Learning: In the utopia-php/migration codebase, when Collection::fromArray is called, the input array is guaranteed to contain both the 'database' key and the nested 'type' key ($array['database']['type']), so defensive guards for these keys are not necessary.
Applied to files:
src/Migration/Resources/Database/Database.php
📚 Learning: 2025-06-28T09:45:36.026Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Resources/Database/Row.php:60-60
Timestamp: 2025-06-28T09:45:36.026Z
Learning: In the utopia-php/migration codebase, the `fromArray` method is not used on Row objects, so mismatches between `jsonSerialize()` output keys and `fromArray()` input expectations for Row class are not problematic.
Applied to files:
src/Migration/Resources/Database/Database.php
📚 Learning: 2025-07-30T12:06:02.331Z
Learnt from: abnegate
Repo: utopia-php/migration PR: 0
File: :0-0
Timestamp: 2025-07-30T12:06:02.331Z
Learning: In the utopia-php/migration codebase, the Utopia Database package does not have a Memory adapter. When testing classes that require a Database instance (like CSV), use PHPUnit's createMock() method to create proper mocks instead of trying to instantiate real database adapters.
Applied to files:
src/Migration/Sources/Appwrite/Reader.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
Applied to files:
src/Migration/Sources/Appwrite/Reader.php
🧬 Code graph analysis (4)
src/Migration/Sources/Appwrite.php (3)
src/Migration/Sources/Appwrite/Reader/API.php (1)
querySelect(211-219)src/Migration/Sources/Appwrite/Reader.php (1)
querySelect(84-84)src/Migration/Sources/Appwrite/Reader/Database.php (1)
querySelect(359-362)
src/Migration/Sources/Appwrite/Reader/Database.php (2)
src/Migration/Sources/Appwrite/Reader/API.php (1)
querySelect(211-219)src/Migration/Sources/Appwrite/Reader.php (1)
querySelect(84-84)
src/Migration/Sources/Appwrite/Reader.php (2)
src/Migration/Sources/Appwrite/Reader/Database.php (2)
Database(21-426)querySelect(359-362)src/Migration/Sources/Appwrite/Reader/API.php (1)
querySelect(211-219)
src/Migration/Sources/Appwrite/Reader/API.php (2)
src/Migration/Sources/Appwrite/Reader.php (1)
querySelect(84-84)src/Migration/Sources/Appwrite/Reader/Database.php (1)
querySelect(359-362)
🪛 GitHub Actions: CodeQL
src/Migration/Sources/Appwrite.php
[error] 1146-1146: PHPStan: Variable $attribute in PHPDoc tag @var does not match any variable in the foreach loop: $selects, $select
src/Migration/Sources/Appwrite/Reader/API.php
[error] 211-211: PHPStan: PHPDoc tag @return with type Appwrite\Query is incompatible with native type string.
🔇 Additional comments (4)
src/Migration/Resources/Database/Database.php (1)
41-41: LGTM!The PHPDoc update correctly documents the new
typefield, which is properly handled throughout the class (constructor,fromArray,jsonSerialize, andgetType()).composer.json (1)
29-29: Using a development branch dependency in a library is risky.The dependency
dev-joins8 as 4.3.0points to an unstable development branch. This is appropriate for local development and testing, but should be updated to a tagged stable release before merging to main to avoid:
- Unexpected breaking changes if the branch is updated
- Build reproducibility issues
- Potential instability for downstream consumers
Please confirm whether this is intentional for testing purposes and will be updated to a stable version before release.
src/Migration/Sources/Appwrite/Reader.php (1)
81-84: LGTM!The interface signature change from
array $columnstostring $columnis consistent with the PR's refactoring objective.src/Migration/Sources/Appwrite/Reader/Database.php (1)
355-362: Cannot verifyQuery::select()accepts a single string without access to the dev-joins8 branch implementation.The implementation assumes
Query::select($column)accepts a single string parameter, which cannot be confirmed from the available codebase. The vendor code is not accessible in this environment, and external documentation for thedev-joins8branch changes was not found. To validate this assumption, you must either:
- Check the actual
Query::select()method signature in theutopia-php/databasedev-joins8 branch- Run the migration code against the target database to verify it functions correctly
| /** @var Column|Relationship $attribute */ | ||
|
|
||
| $queries[] = $this->database->querySelect($selects); | ||
| foreach ($selects as $select) { | ||
| $queries[] = $this->database->querySelect($select); | ||
| } |
There was a problem hiding this comment.
Stale PHPDoc annotation causes pipeline failure.
The @var Column|Relationship $attribute annotation at line 1144 references $attribute, but the subsequent loop iterates over $selects/$select. This is flagged by PHPStan.
Remove or relocate the stale PHPDoc comment.
🔎 Proposed fix
- /** @var Column|Relationship $attribute */
-
foreach ($selects as $select) {
$queries[] = $this->database->querySelect($select);
}📝 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 Column|Relationship $attribute */ | |
| $queries[] = $this->database->querySelect($selects); | |
| foreach ($selects as $select) { | |
| $queries[] = $this->database->querySelect($select); | |
| } | |
| foreach ($selects as $select) { | |
| $queries[] = $this->database->querySelect($select); | |
| } |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 1146-1146: PHPStan: Variable $attribute in PHPDoc tag @var does not match any variable in the foreach loop: $selects, $select
🤖 Prompt for AI Agents
In @src/Migration/Sources/Appwrite.php around lines 1144 - 1148, The stale
PHPDoc "@var Column|Relationship $attribute" above the foreach is incorrect
because the loop iterates over $selects as $select; remove the erroneous PHPDoc
or relocate it to the actual context where $attribute is used (e.g., the block
that handles $attribute variables), and ensure the foreach uses no misleading
docblock so PHPStan no longer flags $attribute in the scope of the foreach over
$selects/$select.
| /** | ||
| * @param array $columns | ||
| * @return string | ||
| * @param string $column | ||
| * @return Query | ||
| */ | ||
| public function querySelect(array $columns): string | ||
| public function querySelect(string $column): string | ||
| { | ||
| return Query::select($columns); | ||
| return new Query('select', $column); | ||
|
|
||
| /** | ||
| * todo fix Query::select to get attribute not array | ||
| */ | ||
| return Query::select($column); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the actual code
find . -name "API.php" -path "*/Migration/Sources/Appwrite/Reader/*" | head -5Repository: utopia-php/migration
Length of output: 111
🏁 Script executed:
# Check the exact code at those lines
if [ -f "src/Migration/Sources/Appwrite/Reader/API.php" ]; then
sed -n '200,225p' src/Migration/Sources/Appwrite/Reader/API.php
fiRepository: utopia-php/migration
Length of output: 601
🏁 Script executed:
# Look at similar methods in the same file to understand the pattern
if [ -f "src/Migration/Sources/Appwrite/Reader/API.php" ]; then
rg -n "public function query" src/Migration/Sources/Appwrite/Reader/API.php -A 8
fiRepository: utopia-php/migration
Length of output: 973
🏁 Script executed:
# Search for Appwrite Query imports or usage
rg -n "Query" src/Migration/Sources/Appwrite/Reader/API.php | head -20Repository: utopia-php/migration
Length of output: 609
🏁 Script executed:
# Check what version of Appwrite is being used (composer.json)
if [ -f "composer.json" ]; then
cat composer.json | jq '.require | select(. != null) | to_entries[] | select(.key | contains("appwrite"))'
fiRepository: utopia-php/migration
Length of output: 115
🌐 Web query:
Appwrite PHP SDK 15.x Query class select method documentation
💡 Result:
The Query.select operator is documented on the Appwrite Queries page — it lets you specify which columns/relationship fields to return (you can pass a single name or an array). Use it with SDK list endpoints via the queries parameter. Example (PHP, Appwrite 15.x):
listDocuments( databaseId: '', collectionId: '', queries: [ Query::select(['name','email']), // select specific fields ] ); Docs: - Queries / Select operator (Appwrite docs). [1] - Databases PHP reference (showing use of queries parameter). [2] Sources: [1] https://appwrite.io/docs/products/databases/queries [2] https://appwrite.io/docs/references/cloud/server-php/databases --- 🌐 Web query: `Appwrite PHP SDK Query class static methods documentation` 💡 Result: Here are the main static query helper methods provided by Appwrite SDKs (available via Appwrite\Query in PHP). Use them to build the queries[] array passed to list/read endpoints: - Selection: select(...) - Comparison: equal(...), notEqual(...), less(...), lessEqual(...), greater(...), greaterEqual(...), between(...), notBetween(...) - Null checks: isNull(...), isNotNull(...) - String/array ops: startsWith(...), notStartsWith(...), endsWith(...), notEndsWith(...), contains(...), notContains(...), search(...), notSearch(...) - Logical: AND (by passing an array of subqueries), OR (multiple values in a method) - Ordering: orderAsc(...), orderDesc(...), orderRandom() - Pagination/cursors: limit(...), offset(...), cursorAfter(...), cursorBefore(...) - Time helpers: createdBefore(...), createdAfter(...), updatedBefore(...), updatedAfter(...) - Geo / spatial: distanceEqual(...), distanceNotEqual(...), distanceGreater(...), distanceLess(...), intersects(...), notIntersects(...), overlaps(...), notOverlaps(...), touches(...) - Others / patterns: (supports selecting relationship fields, wildcards, etc.) For full details, examples and exact signatures see the Appwrite Queries docs and the PHP quick-start examples. [1][2] Sources: [1] Appwrite — Queries documentation. [2] Appwrite — PHP quick start / Query examples. ---
Summary by CodeRabbit
Dependencies
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.