🐛 include in include #190
🐛 include in include #190
Conversation
WalkthroughRemoves the explicit prohibition of the includes sub-field in SearchInclude validation, enabling nested includes. Adds feature tests verifying nested includes for a belongs-to relation including its own has-many relation in search results. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Controller
participant Validator as SearchInclude Validator
participant Rel as Related Resource Validator
participant Repo as Data Layer
Client->>API: POST /api/models/search { search: { includes: [{ relation: A, includes: [{ relation: B }] }] } }
API->>Validator: Build & run validation rules for includes
Note over Validator: ".includes" is not prohibited anymore
Validator->>Rel: Build nested rules for relation A (including its includes)
Rel-->>Validator: Nested rules for relation B
Validator-->>API: Validation passes
API->>Repo: Execute search with nested eager-loads (A, A.B)
Repo-->>API: Paginated results with nested includes
API-->>Client: 200 OK with data
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (3)
334-339: Stabilize ordering of nested models to avoid flaky assertionsEven though this fixture currently returns a single nested model, future changes (or added factories) could introduce multiple models and non-deterministic ordering. Make the order explicit to keep the test robust.
Apply this minimal diff to ensure a stable order:
- 'models' => $matchingModelBelongsToRelation->models - ->map(function ($model) { + 'models' => $matchingModelBelongsToRelation->models() + ->orderBy('id') + ->get() + ->map(function ($model) { return $model->only((new ModelResource())->getFields(app()->make(RestRequest::class))); - }) + }) ->toArray(),
305-320: Add a negative test to validate nested include errors and error-path keysNow that nested includes are accepted, we should also assert that unauthorized nested relations fail with the correct error key path (e.g., search.includes.0.includes.0.relation). This complements your existing unauthorized tests for top-level includes.
Here’s a small test you can add alongside this one:
public function test_including_unauthorized_nested_relation_returns_validation_error(): void { Gate::policy(Model::class, GreenPolicy::class); Gate::policy(BelongsToRelation::class, GreenPolicy::class); $response = $this->post( '/api/models/search', [ 'search' => [ 'includes' => [ [ 'relation' => 'belongsToRelation', 'includes' => [ ['relation' => 'unauthorized'], ], ], ], ], ], ['Accept' => 'application/json'] ); $response->assertStatus(422); $response->assertExactJsonStructure(['message', 'errors' => ['search.includes.0.includes.0.relation']]); }
324-346: Reduce duplication by consolidating with the dot-notation testThis test’s setup and assertions are nearly identical to test_getting_a_list_of_resources_including_belongs_to_has_many_relation. Consider a small refactor (e.g., data provider for the two include syntaxes or a helper that builds the expected structure) to keep the suite DRY and easier to maintain.
If helpful, I can sketch a data provider that feeds both payload shapes into a single test method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Rules/Search/SearchInclude.php(0 hunks)tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- src/Rules/Search/SearchInclude.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (9)
tests/Support/Database/Factories/BelongsToRelationFactory.php (1)
BelongsToRelationFactory(8-28)tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-29)tests/Support/Models/Model.php (2)
Model(10-112)belongsToRelation(23-26)tests/Support/Models/BelongsToRelation.php (2)
BelongsToRelation(7-13)models(9-12)tests/Support/Traits/InteractsWithResource.php (1)
assertResourcePaginated(13-47)tests/Support/Rest/Resources/ModelResource.php (1)
ModelResource(29-160)tests/Support/Rest/Resources/BelongsToResource.php (1)
BelongsToResource(11-31)src/Concerns/Resource/ConfiguresRestParameters.php (1)
getFields(38-41)src/Http/Requests/RestRequest.php (1)
RestRequest(9-23)
🔇 Additional comments (1)
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (1)
293-347: Good addition: covers nested include (array syntax) and guards against regressionsThis test mirrors the existing dot-notation variant and validates the nested include pathway via the "includes" array. It directly addresses issues #189 and #175. Looks correct and consistent with the surrounding tests and resources.
closes #189
closes #175
Summary by CodeRabbit