Joins #133
Joins #133
Conversation
Walkthroughcomposer.json: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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
src/Migration/Sources/Appwrite.php
Outdated
| /** @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.
…oins # Conflicts: # composer.json # composer.lock
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Migration/Sources/Appwrite/Reader.php`:
- Around line 82-85: The implementation of querySelect in API.php contradicts
the Reader::querySelect signature: either change the API.php method return type
to match the actual object being returned (return Query instance / QueryType) or
change the body to return a string representation; specifically, in API.php
update the method declaration for querySelect to return the correct type (e.g.,
Query or QueryType) and return new Query('select', $column) (removing the
unreachable second return), or if you must keep a string return type, remove the
new Query(...) return and instead return Query::select($column) (ensuring
Query::select returns a string); eliminate the unreachable code and ensure the
declared return type matches the actual returned value (refer to
Reader::querySelect, API::querySelect, Query and Query::select).
In `@src/Migration/Sources/Appwrite/Reader/Database.php`:
- Around line 362-369: The method querySelect currently calls Query::select with
a string; change it to pass an array containing the column name so the
utopia-php/database API is used correctly: update the return in querySelect to
call Query::select with [$column] instead of $column, keeping the method
signature and return type intact.
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite/Reader.php (1)
5-5: Unused import.The
Queryimport is not used in this interface file. Consider removing it.Suggested fix
-use Utopia\Database\Query;
📜 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 (5)
composer.jsonsrc/Migration/Sources/Appwrite.phpsrc/Migration/Sources/Appwrite/Reader.phpsrc/Migration/Sources/Appwrite/Reader/API.phpsrc/Migration/Sources/Appwrite/Reader/Database.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Migration/Sources/Appwrite.php
- src/Migration/Sources/Appwrite/Reader/API.php
🧰 Additional context used
🧠 Learnings (4)
📓 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.
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.
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.
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.
📚 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-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 (2)
src/Migration/Sources/Appwrite/Reader/Database.php (2)
src/Migration/Sources/Appwrite/Reader/API.php (1)
querySelect(218-226)src/Migration/Sources/Appwrite/Reader.php (1)
querySelect(85-85)
src/Migration/Sources/Appwrite/Reader.php (2)
src/Migration/Sources/Appwrite/Reader/Database.php (2)
Database(21-432)querySelect(366-369)src/Migration/Sources/Appwrite/Reader/API.php (1)
querySelect(218-226)
🔇 Additional comments (1)
composer.json (1)
29-29: Dev branch dependency should be replaced before merging to main.Using
dev-joins8 as 4.5.2pins to an unstable development branch. This is acceptable for development/testing, but should be updated to a stable release tag before merging to main to avoid:
- Breaking changes if the dev branch is rebased or deleted
- Dependency resolution issues for consumers of this library
Is this PR intended to be merged as-is, or should the dependency be updated to a stable release once
utopia-php/databasepublishes the joins feature?
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…oins # Conflicts: # composer.lock
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@composer.json`:
- Line 29: The composer.json currently pins "utopia-php/database" to the dev
branch ("dev-joins8 as 4.5.2"), which violates "minimum-stability": "stable";
update the dependency in composer.json to a stable constraint (e.g.,
"utopia-php/database": "4.*" or a specific released tag) and remove the
"dev-joins8 as 4.5.2" alias; also add a TODO comment in composer.json (or create
a tracking issue) referencing the joins8 branch and reminding to switch back to
the stable release once utopia-php/database publishes the merged release.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1155-1165: The bug is that inside the foreach over $manyToMany you
overwrite the $queries array instead of appending new Query objects; change the
assignment in the loop to append results of
$this->database->querySelect($relation . '.$id') to the existing $queries array
(so $queries[] = $this->database->querySelect(...)) so that the initial
$this->database->querySelect('$id') and all relation queries are preserved
before calling $this->database->getRow($table, $row['$id'], $queries) (check the
loop around the foreach ($manyToMany as $relation) and the subsequent getRow
call).
♻️ Duplicate comments (1)
src/Migration/Sources/Appwrite.php (1)
1148-1148: Stale PHPDoc annotation.The
@var Column|Relationship $attributeannotation is misplaced after the foreach loop that iterates over$attribute.
…oins # Conflicts: # composer.lock
…oins # Conflicts: # composer.json # composer.lock
1 participant
Summary by CodeRabbit
Dependencies
Improvements
Documentation