Fix many to many select across collections #637
Conversation
WalkthroughA new environment variable was added to the Docker Compose test service. The database relationship population logic was optimized for many-to-many queries by batching related document retrieval. Minor code cleanups and clearer error messages were applied. Two new end-to-end tests were introduced to validate selective field retrieval in many-to-many and nested many-to-many relationships. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docker-compose.yml (1)
21-21: Quote the value for consistency & to avoid edge-case YAML parsing quirksWhile unquoted scalars containing
=are generally fine, quoting the value makes the intention explicit and aligns with the map-style environment blocks elsewhere in the file.- PHP_IDE_CONFIG: serverName=tests + PHP_IDE_CONFIG: "serverName=tests"tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
1549-1549: Remove unused variable.The
$doc1variable is assigned but never used in the test method.- $doc1 = $database->createDocument('select_m2m_collection1', new Document([ + $database->createDocument('select_m2m_collection1', new Document([
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose.yml(1 hunks)src/Database/Database.php(4 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
1549-1549: Avoid unused local variables such as '$doc1'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (7)
docker-compose.yml (1)
20-21: Environment variable addition is correct and useful for IDE ⇄ Xdebug mappingThe
PHP_IDE_CONFIGvariable is syntactically valid and should help the debugger resolve path mappings inside the container. Nothing else to flag.src/Database/Database.php (4)
4342-4342: LGTM: Improved error message capitalization.Minor improvement to capitalize "Collection" in the exception message for better consistency and readability.
4523-4535: Excellent optimization for many-to-many relationship fetching.This change significantly improves performance by replacing multiple individual
getDocumentcalls with a single batchfindquery. The optimization:
- ✅ Collects all related IDs from junction documents
- ✅ Performs a single batch query using
Query::equal('$id', $relatedIds)- ✅ Maintains the same filtering and selection logic
- ✅ Includes proper empty check to avoid unnecessary queries
This should reduce database round-trips from N+1 to 2 queries (1 for junction + 1 for related documents) for many-to-many relationships.
4645-4645: LGTM: Improved error message clarity.Minor improvement to the exception message for better readability and grammar consistency.
4492-4494: Relationship update logic verifiedThe change to include
RELATION_MANY_TO_ONEwithRELATION_SIDE_PARENTalongsideRELATION_ONE_TO_ONEcorrectly normalizes unchangedDocumentinstances to their IDs and skips redundant processing. Our codebase’s extensive end-to-end tests cover all relationship types and deletion/create flows, and no regressions have been observed.tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (2)
1522-1598: Excellent test coverage for many-to-many field selection.The test comprehensively validates that selective field retrieval works correctly in many-to-many relationships, ensuring only requested fields are returned while excluding unselected ones. The setup is clean and the assertions are thorough.
1600-1719: Outstanding test for nested many-to-many field selection.This test excellently validates the core PR objective by testing complex nested selection across multiple collections with many-to-many relationships. The three-tier structure (artists->albums->tracks) with dotted notation selection (
albums.name,albums.tracks.title) comprehensively verifies that the field selection optimization works correctly at all relationship levels.
Summary by CodeRabbit
Bug Fixes
Tests
Chores