Skip to content

Conversation

@fogelito
Copy link
Contributor

@fogelito fogelito commented Jul 30, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of document relationships to ensure relationships are only populated when they exist, addressing potential issues with empty relationship arrays.
    • Refined nested selection processing and loop handling to enhance stability and correctness in relationship data population.
  • Tests

    • Added a comprehensive test covering complex relationships between collections, including various relationship types and attribute configurations. This enhances validation of relationship handling in the system.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 30, 2025

Walkthrough

This update refines relationship handling in the database logic by improving conditional checks, loop structures, and nested selection processing in Database.php. Additionally, it introduces a comprehensive end-to-end test, testZoo, which sets up multiple collections and relationships to validate complex relationship queries.

Changes

Cohort / File(s) Change Summary
Database Relationship Logic Refinement
src/Database/Database.php
Refactored relationship population to check for non-empty relationships before execution, renamed parameters for clarity, improved nested selection grouping, changed reference-based loops to indexed loops, and enhanced debug output for nested selections. Updated method signatures for internal consistency.
End-to-End Relationship Test
tests/e2e/Adapter/Scopes/RelationshipTests.php
Added a new test, testZoo, which creates and relates four collections with various attribute types and relationship types, inserts documents, and performs a multi-level relationship query with debug output and a placeholder assertion.

Sequence Diagram(s)

sequenceDiagram
    participant TestRunner
    participant Database
    participant CollectionZoo
    participant CollectionVeterinarians
    participant CollectionPresidents
    participant CollectionAnimals

    TestRunner->>Database: Create collections (zoo, veterinarians, presidents, animals)
    TestRunner->>Database: Define attributes and relationships
    TestRunner->>Database: Insert documents with relationships
    TestRunner->>Database: Query veterinarians with nested animals and zoos
    Database->>CollectionVeterinarians: Fetch veterinarians
    Database->>CollectionAnimals: Fetch related animals
    Database->>CollectionZoo: Fetch related zoos for animals
    Database-->>TestRunner: Return nested query result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Fix relationship select #632: Refactored and improved relationship query processing logic, including parameter renaming and nested selection grouping, closely related to the changes in Database.php.

Poem

In the codey warren, rabbits leap with glee,
New tests for the zoo, with relationships to see!
Loops now hop with care, no side effects in sight,
Nested queries burrow deep, everything feels right.
With paws on the keyboard, we debug and cheer—
Another hop forward, review time draws near! 🐇

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch deep-select-relationship

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@fogelito fogelito changed the title select nested relationships Select nested relationships Jul 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
tests/e2e/Adapter/Base.php (1)

6-12: Consider removing unused imports if traits are permanently disabled.

If the commented-out traits (lines 20-24) are permanently disabled, these import statements should also be removed to keep the code clean and avoid confusion.

If the trait commenting is permanent, apply this diff to remove unused imports:

-use Tests\E2E\Adapter\Scopes\AttributeTests;
-use Tests\E2E\Adapter\Scopes\CollectionTests;
-use Tests\E2E\Adapter\Scopes\DocumentTests;
-use Tests\E2E\Adapter\Scopes\IndexTests;
-use Tests\E2E\Adapter\Scopes\PermissionTests;
src/Database/Database.php (1)

3530-3530: Consider removing commented debug code.

The commented var_dump($queries) statement should be removed before merging to production to maintain code cleanliness, unless there's a specific reason to keep it for ongoing debugging purposes.

-                        //var_dump($queries);
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)

222-222: Remove debug code before production.

The var_dump statement appears to be debugging code that should be removed.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68545df and fd2120c.

📒 Files selected for processing (3)
  • src/Database/Database.php (6 hunks)
  • tests/e2e/Adapter/Base.php (1 hunks)
  • tests/e2e/Adapter/Scopes/RelationshipTests.php (1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/RelationshipTests.php

107-107: Avoid unused local variables such as '$animal1'. (Unused Code Rules)

(UnusedLocalVariable)


127-127: Avoid unused local variables such as '$animal2'. (Unused Code Rules)

(UnusedLocalVariable)


147-147: Avoid unused local variables such as '$animal3'. (Unused Code Rules)

(UnusedLocalVariable)


186-186: Avoid unused local variables such as '$president1'. (Unused Code Rules)

(UnusedLocalVariable)


200-200: Avoid unused local variables such as '$president2'. (Unused Code Rules)

(UnusedLocalVariable)


211-211: Avoid unused local variables such as '$president3'. (Unused Code Rules)

(UnusedLocalVariable)


224-224: Avoid unused local variables such as '$docs'. (Unused Code Rules)

(UnusedLocalVariable)

🔇 Additional comments (4)
tests/e2e/Adapter/Base.php (1)

20-24: Clarify intent: temporary debugging or permanent test scope reduction?

The trait imports are commented out rather than removed entirely, which suggests this might be temporary for debugging or development focus. However, this significantly reduces test coverage by excluding CollectionTests, DocumentTests, AttributeTests, IndexTests, and PermissionTests.

If this is temporary for debugging relationship functionality, please:

  1. Document this intent with a comment explaining why and when to re-enable
  2. Consider using a separate test configuration instead of modifying the base class

If this is a permanent change to focus only on relationship testing:

  1. Remove the commented imports entirely to avoid confusion
  2. Ensure other test suites cover the excluded functionality elsewhere

Could you clarify the intent behind this change?

src/Database/Database.php (3)

3286-3286: LGTM! Good defensive programming practice.

The additional !empty($relationships) check prevents unnecessary calls to populateDocumentRelationships when no relationships are defined, improving performance and code clarity. This change is consistent with the same pattern used later in the find method.


6096-6096: LGTM! Consistent defensive programming.

This change mirrors the improvement made in the getDocument method, maintaining consistency across the codebase by adding the same !empty($relationships) check before attempting to populate document relationships.


6807-6809: Excellent refactoring for performance and readability.

This refactoring eliminates duplicate explode('.', $value) calls by extracting the result into a $nesting variable that's reused throughout the method. The changes provide:

  1. Performance improvement: Avoids multiple string parsing operations
  2. Code clarity: The $nesting variable name clearly indicates the dot-notation hierarchy
  3. Maintainability: Single point of string parsing logic
  4. Better formatting: Added blank line improves code structure

The functionality remains identical while the implementation is more efficient.

Also applies to: 6822-6823, 6852-6852

Comment on lines 30 to 237
public function testZoo(): void
{
/** @var Database $database */
$database = static::getDatabase();

if (!$database->getAdapter()->getSupportForRelationships()) {
$this->expectNotToPerformAssertions();
return;
}

$database->createCollection('zoo');
$database->createAttribute('zoo', 'name', Database::VAR_STRING, 256, true);

$database->createCollection('veterinarians');
$database->createAttribute('veterinarians', 'fullname', Database::VAR_STRING, 256, true);

$database->createCollection('presidents');
$database->createAttribute('presidents', 'first_name', Database::VAR_STRING, 256, true);
$database->createAttribute('presidents', 'last_name', Database::VAR_STRING, 256, true);
$database->createRelationship(
collection: 'presidents',
relatedCollection: 'veterinarians',
type: Database::RELATION_MANY_TO_MANY,
twoWay: true,
id: 'votes',
twoWayKey: 'presidents'
);

$database->createCollection('animals');
$database->createAttribute('animals', 'name', Database::VAR_STRING, 256, true);
$database->createAttribute('animals', 'age', Database::VAR_INTEGER, 0, false);
$database->createAttribute('animals', 'price', Database::VAR_FLOAT, 0, false);
$database->createAttribute('animals', 'date_of_birth', Database::VAR_DATETIME, 0, true, filters:['datetime']);
$database->createAttribute('animals', 'longtext', Database::VAR_STRING, 100000000, false);
$database->createAttribute('animals', 'is_active', Database::VAR_BOOLEAN, 0, false, default: true);
$database->createAttribute('animals', 'integers', Database::VAR_INTEGER, 0, false, array: true);
$database->createAttribute('animals', 'email', Database::VAR_STRING, 255, false);
$database->createAttribute('animals', 'ip', Database::VAR_STRING, 255, false);
$database->createAttribute('animals', 'url', Database::VAR_STRING, 255, false);
$database->createAttribute('animals', 'enum', Database::VAR_STRING, 255, false);

$database->createRelationship(
collection: 'presidents',
relatedCollection: 'animals',
type: Database::RELATION_ONE_TO_ONE,
twoWay: true,
id: 'animal',
twoWayKey: 'president'
);

$database->createRelationship(
collection: 'veterinarians',
relatedCollection: 'animals',
type: Database::RELATION_ONE_TO_MANY,
twoWay: true,
id: 'animals',
twoWayKey: 'veterinarian'
);

$database->createRelationship(
collection: 'animals',
relatedCollection: 'zoo',
type: Database::RELATION_MANY_TO_ONE,
twoWay: true,
id: 'zoo',
twoWayKey: 'animals'
);

$zoo1 = $database->createDocument('zoo', new Document([
'$id' => 'zoo1',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'name' => 'Bronx Zoo'
]));

$animal1 = $database->createDocument('animals', new Document([
'$id' => 'iguana',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'name' => 'Iguana',
'age' => 11,
'price' => 50.5,
'date_of_birth' => '1975-06-12',
'longtext' => 'I am a pretty long text',
'is_active' => true,
'integers' => [1, 2, 3],
'email' => 'iguana@appwrite.io',
'enum' => 'maybe',
'ip' => '127.0.0.1',
'url' => 'https://appwrite.io/',
'zoo' => $zoo1->getId(),
]));

$animal2 = $database->createDocument('animals', new Document([
'$id' => 'tiger',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'name' => 'Tiger',
'age' => 5,
'price' => 1000,
'date_of_birth' => '2020-06-12',
'longtext' => 'I am a hungry tiger',
'is_active' => false,
'integers' => [9, 2, 3],
'email' => 'tiger@appwrite.io',
'enum' => 'yes',
'ip' => '255.0.0.1',
'url' => 'https://appwrite.io/',
'zoo' => $zoo1->getId(),
]));

$animal3 = $database->createDocument('animals', new Document([
'$id' => 'lama',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'name' => 'Lama',
'age' => 15,
'price' => 1000,
'date_of_birth' => '1975-06-12',
'is_active' => true,
'integers' => null,
'email' => null,
'enum' => null,
'ip' => '255.0.0.1',
'url' => 'https://appwrite.io/',
'zoo' => null,
]));

$veterinarian1 = $database->createDocument('veterinarians', new Document([
'$id' => 'dr.pol',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'fullname' => 'The Incredible Dr. Pol',
'animals' => ['iguana'],
]));

$veterinarian2 = $database->createDocument('veterinarians', new Document([
'$id' => 'dr.seuss',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'fullname' => 'Dr. Seuss',
'animals' => ['tiger'],
]));

$president1 = $database->createDocument('presidents', new Document([
'$id' => 'trump',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'first_name' => 'Donald',
'last_name' => 'Trump',
'votes' => [
$veterinarian1->getId(),
$veterinarian2->getId(),
],
]));

$president2 = $database->createDocument('presidents', new Document([
'$id' => 'bush',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'first_name' => 'George',
'last_name' => 'Bush',
'animal' => 'iguana',
]));

$president3 = $database->createDocument('presidents', new Document([
'$id' => 'biden',
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'first_name' => 'Joe',
'last_name' => 'Biden',
'animal' => 'tiger',
]));

var_dump('=== start === === start === === start === === start === === start === === start === === start === === start === === start ===');

$docs = $database->find('veterinarians',
[
Query::select([
'*',
'animals.*',
'animals.zoo.*',
//'animals.president.*',
])
]
);

$this->assertEquals('shmuel', 'fogel');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Comprehensive test setup but needs completion and cleanup.

This test method creates a complex relationship scenario which is excellent for testing nested relationship queries. However, several issues need attention:

  1. Unused variables: Static analysis correctly identifies unused document variables
  2. Debug code: The var_dump statement should be removed
  3. Placeholder assertion: The final assertion doesn't validate the actual test results

Apply this diff to address the critical issues:

-        $animal1 = $database->createDocument('animals', new Document([
+        $database->createDocument('animals', new Document([
             '$id' => 'iguana',
             // ... rest of document
         ]));

-        $animal2 = $database->createDocument('animals', new Document([
+        $database->createDocument('animals', new Document([
             '$id' => 'tiger',
             // ... rest of document
         ]));

-        $animal3 = $database->createDocument('animals', new Document([
+        $database->createDocument('animals', new Document([
             '$id' => 'lama',
             // ... rest of document
         ]));

-        $veterinarian1 = $database->createDocument('veterinarians', new Document([
+        $database->createDocument('veterinarians', new Document([
             '$id' => 'dr.pol',
             // ... rest of document
         ]));

-        $veterinarian2 = $database->createDocument('veterinarians', new Document([
+        $database->createDocument('veterinarians', new Document([
             '$id' => 'dr.seuss',
             // ... rest of document
         ]));

-        $president1 = $database->createDocument('presidents', new Document([
+        $database->createDocument('presidents', new Document([
             '$id' => 'trump',
             // ... rest of document
         ]));

-        $president2 = $database->createDocument('presidents', new Document([
+        $database->createDocument('presidents', new Document([
             '$id' => 'bush',
             // ... rest of document
         ]));

-        $president3 = $database->createDocument('presidents', new Document([
+        $database->createDocument('presidents', new Document([
             '$id' => 'biden',
             // ... rest of document
         ]));

-        var_dump('=== start === === start === === start === === start === === start === === start === === start === === start === === start ===');

         $docs = $database->find('veterinarians',
             [
                 Query::select([
                     '*',
                     'animals.*',
                     'animals.zoo.*',
-                    //'animals.president.*',
                 ])
             ]
         );

-        $this->assertEquals('shmuel', 'fogel');
+        // Add meaningful assertions here to validate the nested relationship query results
+        $this->assertNotEmpty($docs);
+        // TODO: Add specific assertions to validate the structure and content of returned documents

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 PHPMD (2.15.0)

107-107: Avoid unused local variables such as '$animal1'. (Unused Code Rules)

(UnusedLocalVariable)


127-127: Avoid unused local variables such as '$animal2'. (Unused Code Rules)

(UnusedLocalVariable)


147-147: Avoid unused local variables such as '$animal3'. (Unused Code Rules)

(UnusedLocalVariable)


186-186: Avoid unused local variables such as '$president1'. (Unused Code Rules)

(UnusedLocalVariable)


200-200: Avoid unused local variables such as '$president2'. (Unused Code Rules)

(UnusedLocalVariable)


211-211: Avoid unused local variables such as '$president3'. (Unused Code Rules)

(UnusedLocalVariable)


224-224: Avoid unused local variables such as '$docs'. (Unused Code Rules)

(UnusedLocalVariable)

🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/RelationshipTests.php from lines 30 to 237, the
testZoo method has unused document variables, a var_dump debug statement, and a
placeholder assertion that does not validate test results. Remove the var_dump
statement, replace the placeholder assertion with meaningful assertions that
verify the expected outcomes of the relationship queries, and clean up any
unused variables to ensure the test is complete and clean.

]
);

$this->assertEquals('shmuel', 'fogel');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Replace placeholder assertion with meaningful tests.

The current assertion compares two unrelated strings and doesn't validate the test results. This test should verify that the nested relationship query returns the expected structure and data.

The test should validate:

  • That documents are returned with the correct structure
  • That nested relationships (animals.zoo.*) are properly populated
  • That the relationship data is accurate

Would you like me to help generate proper assertions for this complex relationship scenario?

🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/RelationshipTests.php at line 235, replace the
placeholder assertion comparing unrelated strings with meaningful assertions
that verify the nested relationship query results. Specifically, assert that the
returned documents have the correct structure, that nested relationships under
animals.zoo are properly populated, and that the relationship data matches
expected values. Use assertions to check the presence and correctness of nested
data rather than simple string equality.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 (1)
src/Database/Database.php (1)

6810-6836: Critical: Remove debug statements immediately

The refactoring to explode the string once is good, but the debug var_dump statements must be removed. These will output to the response in production, potentially exposing sensitive information and breaking API responses.

-                $nesting = \explode('.', $value);
-                $selectedKey = \array_shift($nesting); // Remove and return first item
+                $nesting = \explode('.', $value);
+                $selectedKey = \array_shift($nesting); // Remove and return first item

                 $relationship = \array_values(\array_filter(
                     $relationships,
                     fn (Document $relationship) => $relationship->getAttribute('key') === $selectedKey,
                 ))[0] ?? null;

                 if (!$relationship) {
                     continue;
                 }

                 // Shift the top level off the dot-path to pass the selection down the chain
                 // 'foo.bar.baz' becomes 'bar.baz'
                 $nestedSelections[] = Query::select([
                     \implode('.', $nesting)
                 ]);

                 $type = $relationship->getAttribute('options')['relationType'];
                 $side = $relationship->getAttribute('options')['side'];

-                var_dump('=========== in ========');
-                var_dump($selectedKey);
-                var_dump($nesting);
-                var_dump($type);
-                var_dump($side);
-
                 switch ($type) {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91d2a6f and eb28398.

📒 Files selected for processing (1)
  • src/Database/Database.php (8 hunks)
⏰ 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). (10)
  • GitHub Check: Adapter Tests (MySQL)
  • GitHub Check: Adapter Tests (SharedTables/SQLite)
  • GitHub Check: Adapter Tests (SharedTables/MySQL)
  • GitHub Check: Adapter Tests (Pool)
  • GitHub Check: Adapter Tests (Mirror)
  • GitHub Check: Adapter Tests (SharedTables/MariaDB)
  • GitHub Check: Adapter Tests (SharedTables/Postgres)
  • GitHub Check: Adapter Tests (SQLite)
  • GitHub Check: Adapter Tests (Postgres)
  • GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (3)
src/Database/Database.php (3)

3286-3288: Good performance optimization!

Adding the check for non-empty relationships before attempting to populate them prevents unnecessary processing when no relationships are defined.


5095-5108: Safe iteration pattern adopted

Good change from reference-based to indexed iteration. This approach is safer and avoids potential issues with PHP references while maintaining the same functionality.


6352-6359: Consistent iteration pattern

Good consistency with the indexed iteration pattern used elsewhere in the codebase.

Comment on lines 3530 to 3531
//var_dump($queries);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove commented debug statement

Debug statements should not be committed to production code. Please remove this commented var_dump statement.

                 if (!empty($relatedIds)) {
-                    //var_dump($queries);
-
                     $foundRelated = $this->find($relatedCollection->getId(), [
📝 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.

Suggested change
//var_dump($queries);
if (!empty($relatedIds)) {
$foundRelated = $this->find($relatedCollection->getId(), [
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 3530 to 3531, there is a commented out
debug statement using var_dump. Remove this commented var_dump line entirely to
clean up the production code and avoid leaving debug artifacts.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (3)
tests/e2e/Adapter/Scopes/RelationshipTests.php (3)

238-239: Add meaningful test assertions.

The placeholder assertion is commented out and doesn't validate the nested relationship query results. This test should verify that the complex relationship structure is correctly populated.

Replace the commented placeholder with assertions that validate:

  • Documents are returned with correct structure
  • Nested relationships (animals.zoo.*) are properly populated
  • Relationship data matches expected values
-        //$this->assertEquals('shmuel', 'fogel');
+        // Validate that veterinarian documents are returned
+        $this->assertNotEmpty($docs);
+        $this->assertCount(2, $docs);
+        
+        // Validate nested animal relationships are populated
+        foreach ($docs as $veterinarian) {
+            $this->assertArrayHasKey('animals', $veterinarian);
+            $animals = $veterinarian->getAttribute('animals');
+            
+            if (!empty($animals)) {
+                foreach ($animals as $animal) {
+                    // Validate zoo relationship is populated
+                    $this->assertArrayHasKey('zoo', $animal);
+                    if (!empty($animal->getAttribute('zoo'))) {
+                        $zoo = $animal->getAttribute('zoo');
+                        $this->assertEquals('Bronx Zoo', $zoo->getAttribute('name'));
+                    }
+                }
+            }
+        }

222-222: Remove debug statement before merging.

The var_dump statement should be removed as it's debug code that doesn't belong in the final test.

-        var_dump('=== start === === start === === start === === start === === start === === start === === start === === start === === start ===');

224-234: Validate the relationship query structure.

The nested selection query ['*', 'animals.*', 'animals.zoo.*'] is testing complex relationship functionality. Consider adding assertions to validate that this query structure works correctly and returns the expected nested data.

Add validation for the query structure:

         $docs = $database->find(
             'veterinarians',
             [
                 Query::select([
                     '*',
                     'animals.*',
                     'animals.zoo.*',
-                    //'animals.president.*',
                 ])
             ]
         );
+        
+        // Validate the query returns expected structure
+        foreach ($docs as $doc) {
+            // Verify veterinarian attributes are selected
+            $this->assertArrayHasKey('fullname', $doc);
+            
+            // Verify animals relationship is properly nested
+            if (!empty($doc->getAttribute('animals'))) {
+                foreach ($doc->getAttribute('animals') as $animal) {
+                    $this->assertArrayHasKey('name', $animal);
+                    $this->assertArrayHasKey('zoo', $animal);
+                }
+            }
+        }
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)

107-107: Consider removing unused variables or add assertions.

Static analysis correctly identifies that the document creation return values are unused. While not critical for tests, you could either remove the variable assignments or use them in assertions to validate document creation.

For cleaner code, consider either:

Option 1 - Remove variable assignments:

-        $animal1 = $database->createDocument('__animals', new Document([
+        $database->createDocument('__animals', new Document([

Option 2 - Add basic validation assertions:

         $animal1 = $database->createDocument('__animals', new Document([
             // ... document data
         ]));
+        $this->assertEquals('iguana', $animal1->getId());

Also applies to: 127-127, 147-147, 166-166, 176-176, 186-186, 200-200, 211-211

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 47ad716 and 153fe2c.

📒 Files selected for processing (2)
  • src/Database/Database.php (10 hunks)
  • tests/e2e/Adapter/Scopes/RelationshipTests.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Database/Database.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/RelationshipTests.php

107-107: Avoid unused local variables such as '$animal1'. (Unused Code Rules)

(UnusedLocalVariable)


127-127: Avoid unused local variables such as '$animal2'. (Unused Code Rules)

(UnusedLocalVariable)


147-147: Avoid unused local variables such as '$animal3'. (Unused Code Rules)

(UnusedLocalVariable)


186-186: Avoid unused local variables such as '$president1'. (Unused Code Rules)

(UnusedLocalVariable)


200-200: Avoid unused local variables such as '$president2'. (Unused Code Rules)

(UnusedLocalVariable)


211-211: Avoid unused local variables such as '$president3'. (Unused Code Rules)

(UnusedLocalVariable)

⏰ 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). (10)
  • GitHub Check: Adapter Tests (MySQL)
  • GitHub Check: Adapter Tests (Mirror)
  • GitHub Check: Adapter Tests (SharedTables/Postgres)
  • GitHub Check: Adapter Tests (SharedTables/MySQL)
  • GitHub Check: Adapter Tests (Postgres)
  • GitHub Check: Adapter Tests (SharedTables/SQLite)
  • GitHub Check: Adapter Tests (Pool)
  • GitHub Check: Adapter Tests (MariaDB)
  • GitHub Check: Adapter Tests (SQLite)
  • GitHub Check: Adapter Tests (SharedTables/MariaDB)

]
);

var_dump($docs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove debug output.

The var_dump($docs) statement should be removed as it's debug code.

-        var_dump($docs);
📝 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.

Suggested change
var_dump($docs);
// Add meaningful assertions here to validate the nested relationship query results
$this->assertNotEmpty($docs);
// TODO: Add specific assertions to validate the structure and content of returned documents
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/RelationshipTests.php at line 236, remove the
var_dump($docs) statement as it is debug output and should not be present in the
final test code.

@abnegate abnegate merged commit 0e9b85b into main Jul 31, 2025
15 checks passed
@abnegate abnegate deleted the deep-select-relationship branch July 31, 2025 13:40
@coderabbitai coderabbitai bot mentioned this pull request Sep 7, 2025
@coderabbitai coderabbitai bot mentioned this pull request Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants