Fix relationship select #632
Conversation
WalkthroughThe changes refactor the handling of nested relationship selections in database queries by introducing a new private method to process relationship queries, replacing duplicated logic in two methods. Method visibility is adjusted for type checking, and related end-to-end tests are expanded with new scenarios and assertions for relationship attribute selection. Changes
Estimated code review effort2 (~18 minutes) Poem
📜 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
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 (1)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
1152-1212: Comprehensive bidirectional relationship testing - excellent coverage!This is a thorough test suite that validates relationship attribute selection in both directions. The test scenarios cover various combinations effectively.
Consider these improvements for robustness:
Avoid assuming result ordering: Tests assume
model[0]is always 'Fiesta', but database results may not be consistently ordered.System attributes behavior: Some tests assert system attributes (
$id,$permissions, etc.) are present without explicitly selecting them, which seems to be expected behavior but could be documented.To improve test reliability, consider sorting results or using more specific queries:
// Instead of assuming model[0] is 'Fiesta' $model = $database->findOne('model', [ Query::select(['name', 'make.name']), + Query::equal('name', 'Fiesta') ]);Also, consider adding a comment explaining why system attributes are automatically included even when not explicitly selected.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(4 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(3 hunks)
🧬 Code Graph Analysis (1)
src/Database/Database.php (3)
src/Database/Query.php (6)
getMethod(112-115)Query(8-730)getValues(128-131)select(463-466)getAttribute(120-123)setValues(174-179)src/Database/Exception/Query.php (1)
Query(7-9)src/Database/Document.php (1)
getAttribute(212-219)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Database/Database.php (3)
src/Database/Query.php (6)
getMethod(112-115)Query(8-730)getValues(128-131)select(463-466)getAttribute(120-123)setValues(174-179)src/Database/Exception/Query.php (1)
Query(7-9)src/Database/Document.php (1)
getAttribute(212-219)
🔇 Additional comments (7)
src/Database/Database.php (4)
3224-3224: LGTM: Good refactoring to extract duplicated logic.The extraction of relationship query processing into a dedicated method improves code maintainability and reduces duplication between
getDocumentandfindmethods.
6028-6028: Consistent refactoring application.Good to see the same refactoring pattern applied consistently in the
findmethod, maintaining code uniformity.
6710-6710: Appropriate method visibility and type declaration improvements.The change from public to private visibility with explicit
voidreturn type is good for encapsulation and type safety, assuming this method is only used internally within the Database class.
6723-6786: Well-structured relationship query processing method.The new
processRelationshipQueriesmethod effectively extracts and consolidates the relationship selection logic. The implementation correctly:
- Handles dot-notated selections by shifting path segments
- Processes different relationship types (many-to-many, one-to-many, many-to-one, one-to-one) with appropriate logic
- Maintains proper array indexing with
array_values()after unsetting elements- Uses clear variable names and follows existing code patterns
The string manipulation for path processing (
implode('.', array_slice(explode('.', $value), 1))) correctly converts "foo.bar.baz" to "bar.baz" for nested selection handling.tests/e2e/Adapter/Scopes/RelationshipTests.php (3)
934-934: LGTM!The addition of the
originattribute follows the established pattern and supports the expanded test scenarios for relationship attribute selection.
942-944: LGTM!Converting to a two-way relationship with explicit keys is necessary for the bidirectional relationship testing being added. The configuration correctly establishes the parent-child relationship structure.
953-953: LGTM!Adding the
originvalue provides the necessary test data for the new attribute and supports the expanded test scenarios.
src/Database/Database.php
Outdated
| if ($query->getMethod() == Query::TYPE_SELECT) { | ||
| $values = $query->getValues(); | ||
| foreach ($values as $valueIndex => $value) { | ||
| if (\str_contains($value, '.')) { |
There was a problem hiding this comment.
This can be a legit attribute name with DOT.
Can you move this check inside foreach ($relationships as $relationship) ?
So we know it must be a relationship attribute?
Summary by CodeRabbit
Tests
Refactor