Define custom mutate response #163
Conversation
WalkthroughThe pull request introduces a new public method Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant PM as PerformMutation Trait
C->>PM: Request mutation
PM->>PM: Execute applyMutation (perform mutation)
PM->>PM: Call formatModelResponse (format model output)
PM-->>C: Return formatted response
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
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: 2
🧹 Nitpick comments (1)
src/Query/Traits/PerformMutation.php (1)
123-129: Enhance method documentationThe method comments could be more comprehensive to explain the purpose and behavior of this new feature.
/** - * Modify the response according to $responseFields in the resource + * Format the model response based on configured fields and relations * + * This method customizes the mutation response format: + * - If no response fields are defined, returns just the model's primary key (legacy behavior) + * - If response fields are defined, returns the specified model attributes + * - If response relations are defined, includes related models with their specified fields * * @param Model $model * @return mixed|array */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Query/Traits/PerformMutation.php(5 hunks)
🔇 Additional comments (3)
src/Query/Traits/PerformMutation.php (3)
37-38: Improved response formatting looks goodThe update to utilize the new
formatModelResponsemethod instead of directly retrieving the model key is a clean way to implement the custom response feature.
47-47: Documentation improvements are helpfulThese documentation comment updates improve clarity by being more specific about parameter types and descriptions.
Also applies to: 80-82
132-136: Backward compatibility implementation looks goodThe check for empty response fields preserves backward compatibility by returning just the model key when no fields are specified, which aligns with the PR objectives.
|
Code Rabbit is very relevant :P I will correct his feedback :) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Query/Traits/PerformMutation.php (2)
150-154:⚠️ Potential issueFix condition order in relationship check
There's a logic issue in the relationship checking order that could lead to undefined index errors.
- $relationFields = $relation->resource()->responseFields ?? []; - - if (empty($relationsInModel[$relationName]) || !in_array($relationName, $defaultRelations) || empty($relationFields)) { + if (!isset($relationsInModel[$relationName]) || !in_array($relationName, $defaultRelations)) { continue; } + + $relationFields = $relation->resource()->responseFields ?? []; + + if (empty($relationFields)) { + continue; + }This change ensures we first check if the relationship exists in the model before attempting to access it, preventing potential undefined index errors.
156-159: 🛠️ Refactor suggestionAdd support for collection relationships
The current implementation doesn't handle one-to-many relationships correctly, as it treats all relations as single models.
- $relationAttributes = collect($relationsInModel[$relationName])->only($relationFields); - - $formattedRelations[$relationName] = $relationAttributes; + $relationModel = $relationsInModel[$relationName]; + + if ($relationModel instanceof \Illuminate\Database\Eloquent\Collection) { + $formattedRelations[$relationName] = $relationModel->map(function($model) use ($relationFields) { + return collect($model->getAttributes())->only($relationFields); + })->toArray(); + } else { + $formattedRelations[$relationName] = collect($relationModel->getAttributes())->only($relationFields); + }
🧹 Nitpick comments (3)
src/Query/Traits/PerformMutation.php (3)
138-145: Enhance variable naming for clarityThe variable names are generally good, but could be more descriptive to improve code readability.
- $attributes = collect($model->getAttributes())->only($fields); + $selectedAttributes = collect($model->getAttributes())->only($fields); - $relationsInResource = $this->resource->getRelations(app()->make(RestRequest::class)); + $resourceRelations = $this->resource->getRelations(app()->make(RestRequest::class)); - $relationsInModel = $model->getRelations(); + $loadedModelRelations = $model->getRelations(); - $formattedRelations = []; + $formattedRelationAttributes = [];
161-161: Consider separating attributes and relations in the response structureMerging attributes and relations directly can lead to property name conflicts. Consider a clearer structure.
- return $attributes->merge($formattedRelations)->toArray(); + $result = $attributes->toArray(); + if (!empty($formattedRelations)) { + // Option 1: Keep flat structure but with a clearer merge approach + foreach ($formattedRelations as $relation => $value) { + $result[$relation] = $value; + } + + // Option 2: Nest relations for clearer separation + // $result['relations'] = $formattedRelations; + } + return $result;A flat structure maintains compatibility with the examples in the PR objectives, but the nested option provides clearer separation if needed in the future.
123-129: Improve method documentationThe PHPDoc can be enhanced with more details about the method's behavior and parameters.
/** - * Modify the response according to $responseFields in the resource + * Format the model response based on configured response fields. + * + * If responseFields is defined in the resource, returns an array of model attributes + * and related models based on those fields. Otherwise, returns only the model key. * + * @param Model $model The model to format for response - * @return mixed|array + * @return mixed The model key or an array of attributes and related models */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Query/Traits/PerformMutation.php(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/Query/Traits/PerformMutation.php (7)
tests/Support/Rest/Resources/ModelWithHooksResource.php (1)
fields(37-45)tests/Support/Rest/Resources/BelongsToResource.php (1)
fields(24-30)tests/Support/Rest/Resources/NoAuthorizationResource.php (1)
fields(18-27)src/Concerns/Resource/ConfiguresRestParameters.php (1)
fields(26-29)src/Relations/Relation.php (1)
resource(104-114)src/Concerns/Resource/Relationable.php (2)
getRelations(65-74)relation(19-37)src/Http/Resource.php (1)
newModel(54-60)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests on PHP 8.2 with Laravel ^12.0 and sqlite
🔇 Additional comments (3)
src/Query/Traits/PerformMutation.php (3)
37-38: LGTM - Updated mutate method to use the new response formatterThe refactoring of this code segment is clean and aligns with the PR objective of enhancing response customization. The code now uses the new
formatModelResponsemethod to format the model response based on specified fields.
132-136: LGTM - Backward compatibility is properly maintainedThe code correctly checks for the presence of response fields and returns just the model key if none are defined, ensuring backward compatibility as specified in the PR objectives.
145-145:❓ Verification inconclusive
Verify that responseRelations exists in the resource
The code assumes that
$this->resource->responseRelationsis defined or accessible, but it's not clear from the provided context if this property exists in the Resource class.
🏁 Script executed:
#!/bin/bash # Check for the responseRelations property in resource classes echo "Searching for the 'responseRelations' property in resource classes..." rg -l "public\s+(array|static\s+array|static)\s+\\\$responseRelations" --type php echo "Searching for the 'responseFields' property in resource classes..." rg -l "public\s+(array|static\s+array|static)\s+\\\$responseFields" --type phpLength of output: 464
Action Required: Confirm the Definition of
responseRelationson the ResourceThe initial grep did not return any results showing an explicit declaration of
responseRelationswithin resource classes. Please verify manually whetherresponseRelationsis:
- Explicitly defined in any Resource class, or
- Provided dynamically via a magic method (e.g.,
__get).If the latter is true, consider adding documentation or comments to clarify its origin. This will help future maintainers understand the expected structure of the resource.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Query/Traits/PerformMutation.php (2)
130-174: The implementation looks good, but some refinements could improve it.The method correctly implements the custom response formatting functionality as described in the PR objectives. It properly handles the backward compatibility case and correctly processes both model attributes and relations.
Some suggestions for improvement:
There's an inconsistency in the return type handling between collections and single models:
- For collections (line 162-164), you map and convert to array
- For single models (line 166), you don't call
toArray()on the collectionConsider adding more robust error handling for edge cases.
The method documentation could be more detailed about the expected return format.
Here's a suggested fix for the inconsistency:
if ($relationModel instanceof \Illuminate\Database\Eloquent\Collection) { $formattedRelations[$relationName] = $relationModel->map(function($model) use ($relationFields) { return collect($model->getAttributes())->only($relationFields); })->toArray(); } else { - $formattedRelations[$relationName] = collect($relationModel->getAttributes())->only($relationFields); + $formattedRelations[$relationName] = collect($relationModel->getAttributes())->only($relationFields)->toArray(); }
145-149: Consider more efficient relationship matching.The current approach for finding the relation in resources works, but it relies on table name singularization which might not always match the actual relationship name defined in models.
For better maintainability, consider implementing a more direct way to get relations by name, such as adding a method to retrieve a relation by its name in the resource.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Query/Traits/PerformMutation.php(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/Query/Traits/PerformMutation.php (7)
tests/Support/Rest/Resources/ModelWithHooksResource.php (1)
fields(37-45)tests/Support/Rest/Resources/BelongsToResource.php (1)
fields(24-30)tests/Support/Rest/Resources/NoAuthorizationResource.php (1)
fields(18-27)src/Concerns/Resource/ConfiguresRestParameters.php (1)
fields(26-29)src/Relations/Relation.php (1)
resource(104-114)src/Concerns/Resource/Relationable.php (2)
getRelations(65-74)relation(19-37)src/Http/Resource.php (1)
newModel(54-60)
🔇 Additional comments (3)
src/Query/Traits/PerformMutation.php (3)
38-38: Good refactoring to support custom response formatting.The modification properly uses the new
formatModelResponsemethod instead of directly using the model key, enabling the custom response format feature.
47-47: Documentation improvements add clarity.The parameter documentation updates provide better clarity about the expected input parameters.
Also applies to: 81-83
133-135: Ensure response consistency in your API documentation.The backward compatibility approach is good - when no response fields are defined, it returns just the model key. Make sure this difference in response format is well-documented in your API documentation to avoid confusion for API consumers.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Query/Traits/PerformMutation.php (1)
137-169: Relation handling approach could be more robust.The implementation for handling relations works correctly but relies on table name singularization to match relation names, which might not always be reliable across all models.
Consider implementing a more direct mapping between relation names and table names, or adding a configurable mapping option to handle edge cases where the singularized table name doesn't match the relation name:
- $relation = collect($relationsInResource)->first(function ($rel) use ($relationName) { - return Str::singular($rel->resource()::newModel()->getTable()) === $relationName; - }); + $relation = collect($relationsInResource)->first(function ($rel) use ($relationName) { + // Try direct match first + if ($rel->relation === $relationName) { + return true; + } + // Fall back to table name singularization + return Str::singular($rel->resource()::newModel()->getTable()) === $relationName; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Query/Traits/PerformMutation.php(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/Query/Traits/PerformMutation.php (7)
tests/Support/Rest/Resources/ModelWithHooksResource.php (1)
fields(37-45)tests/Support/Rest/Resources/BelongsToResource.php (1)
fields(24-30)tests/Support/Rest/Resources/NoAuthorizationResource.php (1)
fields(18-27)src/Concerns/Resource/ConfiguresRestParameters.php (1)
fields(26-29)src/Relations/Relation.php (1)
resource(104-114)src/Concerns/Resource/Relationable.php (2)
getRelations(65-74)relation(19-37)src/Http/Resource.php (1)
newModel(54-60)
🔇 Additional comments (3)
src/Query/Traits/PerformMutation.php (3)
38-39: Improved response formatting is properly integrated.The change properly integrates the new
formatModelResponsemethod into the mutation flow, enhancing the response format while maintaining backward compatibility.
124-136: Good implementation of the response formatting with backward compatibility.The method correctly implements the feature described in the PR objectives, allowing for customized response fields while maintaining backward compatibility by falling back to returning just the model key when no response fields are defined.
170-173: The result merging approach is clear and maintainable.This approach of building the result array is clear and easy to understand, making it more maintainable than directly merging collections.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Query/Traits/PerformMutation.php (1)
131-181: Consider adding documentation and unit tests.While the implementation looks solid, it would benefit from thorough unit tests to ensure all edge cases are covered, especially for relationship handling.
For example:
- Test with empty responseFields
- Test with only model fields, no relations
- Test with relations that don't exist in the model
- Test with null relations
- Test with collection relations
I can help draft unit tests for this new functionality if you'd like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Query/Traits/PerformMutation.php(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/Query/Traits/PerformMutation.php (7)
tests/Support/Rest/Resources/ModelWithHooksResource.php (1)
fields(37-45)tests/Support/Rest/Resources/BelongsToResource.php (1)
fields(24-30)tests/Support/Rest/Resources/NoAuthorizationResource.php (1)
fields(18-27)src/Concerns/Resource/ConfiguresRestParameters.php (1)
fields(26-29)src/Relations/Relation.php (1)
resource(104-114)src/Concerns/Resource/Relationable.php (2)
getRelations(65-74)relation(19-37)src/Http/Resource.php (1)
newModel(54-60)
🔇 Additional comments (8)
src/Query/Traits/PerformMutation.php (8)
5-7: New imports added appropriately.The added imports (
CollectionandStr) support the new functionality for custom response formatting.
38-39: Good update to utilize the new response formatting.The modified
mutatemethod now correctly uses the newformatModelResponsemethod instead of directly retrieving the model key, enabling the customized response format while maintaining the same overall structure.
124-136: Good backward compatibility implementation.The new
formatModelResponsemethod correctly checks for the presence of$responseFieldsand falls back to the original behavior (returning just the model key) when not defined, maintaining backward compatibility as specified in the PR objectives.
138-142: Well-structured initialization of relations.The code appropriately retrieves relations from both the resource and model, and initializes the necessary variables for relation formatting.
143-150: Good error handling for edge cases.The implementation correctly checks for missing or null relations, preventing potential errors when dealing with incomplete relation data.
167-173: Good handling of both collection and single model relations.The implementation correctly differentiates between collections and single models, and formats them consistently by ensuring both return arrays.
175-180: Effective result composition.The code properly constructs the final response by combining model attributes with formatted relations. This approach maintains the expected response structure while adding the requested customization.
152-163:❓ Verification inconclusive
Relationship discovery may be brittle.
The current implementation for finding the matching relation relies on table name singularization, which might not always match the actual relation name defined in models.
Consider if there's a more direct way to match relations by name rather than relying on table name conversion. Run the following to check relation matching in the codebase:
🏁 Script executed:
#!/bin/bash # Find relation definitions that might use custom names that wouldn't match with table singularization rg -A 2 "function \w+\(\)" --type php | rg -B 1 "return \$this->belongsTo|return \$this->hasMany|return \$this->hasOne|return \$this->belongsToMany"Length of output: 146
Action Required: Verify Relation Matching Logic in PerformMutation Trait
The current implementation in
src/Query/Traits/PerformMutation.php(lines 152–163) usesStr::singularon the model's table name to identify the target relation. This approach can be brittle since it assumes that the singularized table name always matches the defined relation name—even though custom naming patterns might be used in models.
- The code snippet under review:
$relation = collect($relationsInResource)->first(function ($rel) use ($relationName) { return Str::singular($rel->resource()::newModel()->getTable()) === $relationName; }); if (!$relation) { continue; } $relationFields = $relation->resource()->responseFields ?? []; if (empty($relationFields)) { continue; }- Concern: Relying solely on table name singularization may fail when models define relations with custom names that do not conform to typical table naming conventions.
- Current Verification: An initial search for custom relation definitions (using an
rgcommand to look for relation methods returningbelongsTo,hasMany, etc.) produced no output. However, this result is inconclusive due to the low-quality inference of the search output.- Recommendation: Please manually verify whether any models in the codebase use custom relation naming conventions that could lead to mismatches. If such cases exist, consider adopting a more direct matching strategy (e.g., using the relation's actual name) to improve the robustness of relation discovery.
|
This needs to follow the "search" method, to allow includes / selects / etc |
closes #151
closes #139
Description
This PR adds the possibility of customizing the response format in the event of a Mutate action.
Main changes
It is now possible to define
$responseFieldsarray containing the fields to be returned in the response.$responseRelationscontaining the relations to be returned in the responseTechnical Details
The relations that will be returned, defined in
$responseRelations, will in turn be based on their$responseFields(in their own model).To avoid a breaking change, if the array
$responseFieldsis not defined or empty, the response remains unchanged:(For example)
{ "created": [ 137 ], "updated": [] }Examples
When
$responseFieldsis empty or undefined, the response will look like this:{ "created": [ 137 ], "updated": [] }If
$responseFieldsis set to :Then the answer will be something like this:
{ "created": [ { "name": "Gautier Deleglise", "id": 136, "user": { "id": 1, "name": "Guest User" } } ], "updated": [] }If
$responseRelationsincludes:Then, the response will look like :
{ "created": [ { "name": "Gautier Deleglise", "id": 139, "user": { "id": 1, "name": "Guest User" } } ], "updated": [] }Knowing that
$reponseFieldsin UserResource is defined as follows :Tests
If you are happy with the functional part, please let me know and I will proceed with the test coverage.
Docs
If you are satisfied with the functional part, let me know and I will update the documentation.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation