Multiple keys support #168
Conversation
# Conflicts: # src/Relations/MorphedByMany.php
|
Warning Rate limit exceeded@Samoht70 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis update introduces a new trait, Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Resource
participant PerformsRelationOperations
participant QueryBuilder
participant Model
participant Relation
Controller->>Resource: Receives mutation request
Resource->>PerformsRelationOperations: Calls operation method (e.g., attach, update)
PerformsRelationOperations->>QueryBuilder: Retrieves/creates related models (batch or single)
loop For each related model
PerformsRelationOperations->>Resource: Authorize attach/detach
end
PerformsRelationOperations->>Relation: Perform operation (attach/detach/sync/toggle)
Relation->>Model: Update relation
PerformsRelationOperations-->>Resource: Return result
Resource-->>Controller: Respond to request
Possibly related PRs
Poem
🪧 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 (11)
src/Query/Traits/PerformMutation.php (1)
77-95: Good addition for batch mutation support.
The newmutationsmethod provides clean handling of multiple keys in a single mutation request. This approach simplifies bulk operations. As an optional improvement, consider a safeguard for the case wherekeysmight be empty to prevent no-op loops.src/Relations/MorphToMany.php (1)
50-57: Consider handling unknown operations gracefully.
Thematchexpression currently omits a default branch. If unexpected operations appear, it will throw an unhandled exception. Adding a default case or validation might offer more robust error handling.match ($mutationRelation['operation']) { 'create' => $this->create($model, $relation, $mutationRelation), 'update' => $this->update($model, $relation, $mutationRelation), 'attach' => $this->attach($model, $relation, $mutationRelation), 'detach' => $this->detach($model, $relation, $mutationRelation), 'toggle' => $this->toggle($model, $relation, $mutationRelation), 'sync' => $this->sync($model, $relation, $mutationRelation, withoutDetaching: !isset($mutationRelation['without_detaching']) || !$mutationRelation['without_detaching']), + default => throw new \InvalidArgumentException("Unknown operation: {$mutationRelation['operation']}"), };src/Rules/OperationDependentRequiredKey.php (1)
43-63: Validate method checks required fields based on operation.
This logic accurately enforces conditional presence of keys for specific operations. If'create'or other operations require a similar check later, consider adding them here.src/Concerns/Relations/PerformsRelationOperations.php (3)
11-25: Ensure consistent type-hinting and docblocks in thecreatemethod.Although the logic for creating and attaching related models is clean, it may be beneficial to add docblocks or type hints for
$mutationand$attributesto improve readability and help static analysis tools detect type-related issues.+ /** + * Create a new related model in the specified relation. + * + * @param \Illuminate\Database\Eloquent\Model $model + * @param \Lomkit\Rest\Relations\Relation $relation + * @param array $mutation + * @param array $attributes + * @return void + */ public function create(Model $model, Relation $relation, $mutation = [], $attributes = []) { ... }
63-73: Properly handle missing or invalid keys in thedetachmethod.When the
mutations()method returns no valid models, or if any are invalid, consider logging or throwing an exception to inform the user or system of an unsuccessful detach operation.
97-119: Refactor repeatedsynclogic.The
sync,attach, andupdatemethods all contain similar blocks of code mapping$toPerformActionModelsto an array keyed by model primary keys plus pivot data. Consider consolidating these repeated patterns into a small private helper method to reduce duplication.tests/Feature/Controllers/MutateUpdateOperationsTest.php (2)
856-921: Update multiple has-many relations with consistent attribute changes.This test ensures all targeted records share the same updated attributes. It might be beneficial to also test partial updates or different attribute sets for separate records in the future.
2045-2102: Update multiple belongs-to-many relations’ pivot fields.The coverage for pivot field updates across multiple keys is thorough. Consider adding a test for distinct pivot data per item if your business logic eventually requires it.
tests/Feature/Controllers/MutateCreateOperationsTest.php (2)
732-795: Update multiple has-many relations during creation.Updating multiple existing has-many models during resource creation is verified. Consider testing scenarios where some keys are invalid or missing to confirm robust error handling.
1572-1634: Update multiple belongs-to-many during creation with shared attributes.The test is valid for applying a uniform attribute set to multiple related items. Consider testing each item with distinct pivot data if the domain calls for it.
tests/Feature/Controllers/MutateUpdateMorphOperationsTest.php (1)
1-3041: Consider refactoring repeated testing logic.
There is a noticeable pattern across these tests for handling “attach”, “detach”, “sync”, and “toggle” with single vs. multiple keys. You might reduce duplication by using parameterized tests (e.g., with PHPUnit data providers) or helper methods that cover the repeated steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Concerns/Relations/PerformsRelationOperations.php(1 hunks)src/Query/Traits/PerformMutation.php(2 hunks)src/Relations/BelongsToMany.php(3 hunks)src/Relations/HasMany.php(1 hunks)src/Relations/MorphMany.php(1 hunks)src/Relations/MorphToMany.php(3 hunks)src/Relations/MorphedByMany.php(3 hunks)src/Rules/MutateRules.php(2 hunks)src/Rules/OperationDependentRequiredKey.php(1 hunks)tests/Feature/Controllers/MutateCreateMorphOperationsTest.php(6 hunks)tests/Feature/Controllers/MutateCreateOperationsTest.php(6 hunks)tests/Feature/Controllers/MutateUpdateMorphOperationsTest.php(32 hunks)tests/Feature/Controllers/MutateUpdateOperationsTest.php(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/Query/Traits/PerformMutation.php (1)
src/Http/Resource.php (1)
newModel(61-67)
src/Relations/BelongsToMany.php (1)
src/Concerns/Relations/PerformsRelationOperations.php (6)
create(11-25)update(27-43)attach(45-61)detach(63-73)toggle(75-95)sync(97-118)
tests/Feature/Controllers/MutateCreateOperationsTest.php (8)
tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-29)tests/Support/Database/Factories/HasManyRelationFactory.php (1)
HasManyRelationFactory(8-28)tests/Support/Models/Model.php (3)
Model(10-112)hasManyRelation(45-48)belongsToManyRelation(60-65)tests/Support/Policies/GreenPolicy.php (1)
GreenPolicy(8-100)tests/Support/Models/HasManyRelation.php (1)
HasManyRelation(7-13)tests/Support/Traits/InteractsWithResource.php (1)
assertMutatedResponse(64-76)tests/Support/Database/Factories/BelongsToManyRelationFactory.php (1)
BelongsToManyRelationFactory(8-29)tests/Support/Models/BelongsToManyRelation.php (1)
BelongsToManyRelation(7-18)
tests/Feature/Controllers/MutateUpdateOperationsTest.php (8)
tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-29)tests/Support/Database/Factories/HasManyRelationFactory.php (1)
HasManyRelationFactory(8-28)tests/Support/Models/Model.php (3)
Model(10-112)hasManyRelation(45-48)belongsToManyRelation(60-65)tests/Support/Policies/GreenPolicy.php (1)
GreenPolicy(8-100)tests/Support/Models/HasManyRelation.php (1)
HasManyRelation(7-13)tests/Support/Traits/InteractsWithResource.php (1)
assertMutatedResponse(64-76)tests/Support/Database/Factories/BelongsToManyRelationFactory.php (1)
BelongsToManyRelationFactory(8-29)tests/Support/Models/BelongsToManyRelation.php (1)
BelongsToManyRelation(7-18)
🪛 GitHub Actions: tests
tests/Feature/Controllers/MutateCreateOperationsTest.php
[error] 1805-1805: PHPUnit test failure: Failed asserting that 20 matches expected 30 in test_creating_a_resource_with_updating_multiple_belongs_to_many_relation_with_pivot_fields.
🔇 Additional comments (62)
src/Relations/HasMany.php (2)
28-34: Confirm partial failure handling for create operations.By singling out the
'create'operation, if a failure occurs shortly after creation (e.g., in subsequent operations), the newly created record may remain attached. Consider whether this behavior is acceptable or if a rollback or more robust error handling is needed.
36-44: Verify attach logic for non-detach operations.All non-detach operations (e.g.,
'update','attach','sync','toggle') callauthorizeToAttach. Confirm whether these operations genuinely require an attach permission check, or whether a different check or an additional step is needed for'update','toggle', or'sync'.src/Relations/MorphMany.php (2)
29-35: Implementation matches HasMany pattern effectively.Separating the
'create'operation for single-model handling and authorization is consistent and aligns well with the approach inHasMany. The code is easy to follow.
37-45: Confirm attach/detach logic for all operations.Similar to
HasMany, all non-detach operations callauthorizeToAttach. Verify that'update','sync','toggle', and'attach'behaviors align with attach permissions. If a more granular check is needed for'update'or'toggle', consider extending the condition.src/Rules/MutateRules.php (2)
98-103: Enforce.keyconstraints thoroughly.Prohibiting array values in
.keyand disallowing the presence of.keysat the same time is a robust approach. Please confirm that there is sufficient test coverage for scenarios combining invalid usage of.keyand.keys.
104-112: Approve batch operation rules for.keys.Requiring
.keysto be an array, prohibiting it with.keyor with'create', and validating each item’s existence ensure proper batch operations. This logic appears well-structured and consistent with your multi-key mutation design.src/Query/Traits/PerformMutation.php (2)
52-52: Consider verifying the$keyusage.
Now that$keyis a newly introduced optional parameter, ensure it is validated or handled gracefully when absent or undefined. This helps avoid ambiguity in mutation flows.
59-59: Check desired exception handling for missing models.
Relying onfindOrFailforces an exception if a model is not found. Verify this is the intended flow for all mutation scenarios, or consider a more controlled error mechanism if needed.src/Relations/MorphedByMany.php (3)
7-7: Centralizing logic withPerformsRelationOperations.
Including this trait is a solid move for reusing standardized relation operations and improving maintainability.
17-17: Ensuring class-level usage of the trait.
Declaring the trait within the class guarantees access to its batch operations and Eloquent relation handling methods.
50-57: Refactoring to a match expression.
Replacing conditional branches with a match expression tidies the logic and neatly delegates operations to the trait's methods. This is a clean, concise solution.src/Relations/BelongsToMany.php (3)
7-7: Introducing thePerformsRelationOperationstrait.
This approach promotes consistency and centralizes relation mutation logic across different relation types.
18-18: Reusing trait’s multi-key functionalities.
Explicitly using the trait within the class ensures streamlined handling of batch operations and pivot data manipulation.
51-63: Improved readability with match-based delegation.
Switching to a match expression clarifies the mutation branching and delegates each operation (create, update, attach, detach, toggle, sync) to specialized trait methods.src/Relations/MorphToMany.php (2)
7-7: Good use of the trait to centralize operations.
The inclusion ofPerformsRelationOperationshere follows the pattern of using specialized traits to keep code maintainable and consistent across relation classes.
17-17: Trait synergy looks consistent.
UsingHasPivotFields,HasMultipleResults, andPerformsRelationOperationstogether appears well-structured, ensuring pivot data handling and multiple result logic remain cohesive.src/Rules/OperationDependentRequiredKey.php (3)
11-19: Rule is well-declared as implicit.
Marking this rule as implicit ensures it runs even if the field is missing, which is useful for enforcing operation-based requirements.
20-37: Data binding approach is correct.
Allowing the rule to store the entire request data viasetDatais consistent withDataAwareRule, enabling more flexible validation.
39-41: Constructor neatly sets the related field.
The constructor injection for$otherFieldis straightforward, simplifying usage with wildcards in attribute paths.tests/Feature/Controllers/MutateCreateMorphOperationsTest.php (5)
301-352: Test coverage for multiple “attach” on morphMany is solid.
This validates attaching multiple existing morphMany relations using the newkeys, confirming the batch processing logic works for creation scenarios.
850-901: Batch attach test on morphToMany is comprehensive.
Ensures that multiple morph-to-many relations can be attached in a single request. Good job verifying the final relation count.
958-1020: Verifies bulk “update” on morphToMany.
This test ensures multiple existing relations can be updated with a single operation. Coverage looks solid, especially checking the updated field.
1259-1310: Multiple “attach” on morphedByMany tested thoroughly.
Confirms that attaching multiple morphed-by-many relations in one request updates the database as intended.
1366-1429: Bulk update on morphedByMany well-validated.
Demonstrates that multiple relations can be updated in one go, confirming new attribute values and linkage.src/Concerns/Relations/PerformsRelationOperations.php (2)
1-4: Centralize relation operations in a dedicated trait.The introduction of this trait is a good step towards DRY and maintainable code, as it centralizes the create/attach/detach/toggle/sync logic for Eloquent relations and ensures consistent authorization checks.
45-61: Confirm pivot data usage in theattachmethod.The pivot data is mapped to every model in the
attacharray. If pivot data must vary per model, users may need a separate structure. Verify this approach fits your typical pivot usage patterns or consider an alternative that allows distinct pivot data per item.tests/Feature/Controllers/MutateUpdateOperationsTest.php (4)
597-645: Check pivot usage for multiple has-many attachments.The test verifies that multiple models are attached, but has-many relations typically don’t have pivot data in Eloquent. Confirm that no pivot fields are required here and that uniqueness constraints (if any) are handled.
748-797: Detach flow for multiple has-many relations looks correct.Removing several related models in a single request is tested thoroughly. Ensure that any custom constraints (e.g., restricting detachment under certain conditions) are enforced in the production code.
1412-1472: Toggle multiple belongs-to-many relations, ensuring correct final state.This test covers the toggling of multiple records. The final assertion of having exactly 1 related model ensures that detached/toggled-off items are excluded. The coverage is solid.
1624-1696: Sync multiple belongs-to-many relations, verifying pivot usage.The approach for syncing multiple resources at once is correct and ensures old attachments are removed. The pivot data is verified, which is good. Consider edge cases for partially invalid keys.
tests/Feature/Controllers/MutateCreateOperationsTest.php (4)
624-675: Attach multiple has-many relations upon creation.This test demonstrates multi-attach usage. Everything appears consistent, given that pivot fields do not typically apply to has-many relationships.
1313-1365: Attach multiple belongs-to-many relations during creation.The test ensures that multiple IDs can be passed to attach concurrently. The logic and final assertion appear correct.
1437-1515: Attach multiple belongs-to-many with pivot fields.Pivot data is validated for multiple attachments. The pipeline error at line 1805 from your logs might be unrelated, but ensure that pivot changes match your domain requirements.
1719-1813: Update multiple belongs-to-many pivot fields with consolidated logic.This final test checks pivot updates for multiple keys. The pipeline failure you encountered (expected 30 but found 20) suggests verifying the test logic vs. actual pivot assignment. Double-check the test cases or source code to confirm the pivot data is assigned as intended.
🧰 Tools
🪛 GitHub Actions: tests
[error] 1805-1805: PHPUnit test failure: Failed asserting that 20 matches expected 30 in test_creating_a_resource_with_updating_multiple_belongs_to_many_relation_with_pivot_fields.
tests/Feature/Controllers/MutateUpdateMorphOperationsTest.php (28)
378-431: Good coverage for multiple attach in morphManyRelation.
This test effectively verifies attaching multiple related records viakeys.
481-530: Effective check for multiple detach in morphManyRelation.
The logic and assertions look solid.
589-653: Correct approach for updating multiple morphManyRelation entries.
The test properly confirms the updated attributes on each entry.
1093-1146: Nice addition for verifying multiple attach in morphToManyRelation.
Test flow aligns well with the feature’s requirement.
1148-1195: Clean detach test for morphToManyRelation.
It properly ensures that the relation is cleared when requested.
1196-1245: Accurate test for detaching multiple morphToManyRelation records at once.
All assertions align with expectations.
1247-1302: Good coverage for updating a single morphToManyRelation.
Successfully validates the updated attributes on the related record.
1304-1369: Thorough test for updating multiple morphToManyRelation records.
Ensures each record receives the expected attributes.
1370-1413: Ensures creating a new morphedByManyRelation works correctly in an update mutation.
Covers the essential creation scenario well.
1415-1452: Appropriate pivot field authorization test for morphedByManyRelation.
Confirming correct validation handling of unauthorized fields.
1454-1515: Pivot field creation test in morphedByManyRelation is well-structured.
Checks both the relation link and pivot fields thoroughly.
1616-1669: Multiple attach logic in morphedByManyRelation is verified effectively.
Assertions confirm that multiple records are now linked.
1719-1768: Detaching multiple morphedByManyRelation records test looks correct.
Confirms the count after detachment is accurate.
1770-1825: Single record update in morphedByManyRelation is validated thoroughly.
Ensures the updated attributes are applied as intended.
1827-1891: Correct procedure for updating multiple morphedByManyRelation records.
Ensures each record receives the changes.
1893-1960: Syncing a single morphedByManyRelation record is tested well.
Covers pivot attribute assignment and ensures the old record is removed.
1962-2033: Syncing multiple morphedByManyRelation records test is sound.
Properly updates pivot fields and replaces prior relations.
2035-2106: Syncing without detaching in morphedByManyRelation (single record) is well-verified.
Checks that the existing record remains and only the new record is added.
2108-2181: Sync multiple morphedByManyRelation without detaching is accurately tested.
Ensures both old and new relations coexist.
2183-2252: Sync with detaching in morphedByManyRelation is validated.
Confirms that previous attachments are replaced as expected.
2254-2311: Toggle operation (single record) in morphedByManyRelation is tested well.
Checks that the attached record is detached, and the unattached record is attached.
2314-2367: Toggle operation (multiple keys) in morphedByManyRelation is logically consistent.
Verifies correct toggling of attached/unattached items.
2370-2450: Toggle with attribute/pivot updates in morphedByManyRelation is rigorous.
Precisely covers the combination of toggling plus attribute changes.
2452-2536: Toggling multiple keys in morphedByManyRelation with attribute/pivot updates is well handled.
Confirms final state and pivot data.
2538-2605: Sync operation for morphToManyRelation is tested effectively.
Checks replaced attachments and pivot data thoroughly.
2607-2678: Multi-key sync test for morphToManyRelation is complete.
Includes verifying pivot attribute assignments.
2680-2764: Syncing morphToManyRelation without detaching is verified.
Ensures existing attachments remain while adding new ones.
2766-2852: Testing multiple sync operations morphToManyRelation without detaching is robust.
Checks that old attachments remain and pivot info is updated.
| public function toggle(Model $model, Relation $relation, $mutation = [], $attributes = []) | ||
| { | ||
| $toPerformActionModels = app()->make(QueryBuilder::class, ['resource' => $relation->resource()]) | ||
| ->mutations($mutation, $attributes); | ||
|
|
||
| $results = $model->{$relation->relation}() | ||
| ->toggle( | ||
| collect($toPerformActionModels) | ||
| ->mapWithKeys( | ||
| fn(Model $toPerformActionModel) => [$toPerformActionModel->getKey() => $mutation['pivot'] ?? []] | ||
| )->toArray(), | ||
| ); | ||
|
|
||
| foreach ($results['attached'] as $attached) { | ||
| $this->resource()->authorizeToAttach($model, $relation->resource()::$model::find($attached)); | ||
| } | ||
|
|
||
| foreach ($results['detached'] as $detached) { | ||
| $this->resource()->authorizeToDetach($model, $relation->resource()::$model::find($detached)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check null return from find calls during the toggle operation.
After toggling, the code calls $relation->resource()::$model::find($attached) and $relation->resource()::$model::find($detached) to re-check authorization. If the record is missing in the database, $find will return null. Consider handling this case explicitly, possibly skipping or throwing a domain exception.
| public function update(Model $model, Relation $relation, $mutation = [], $attributes = []) | ||
| { | ||
| $toPerformActionModels = app()->make(QueryBuilder::class, ['resource' => $relation->resource()]) | ||
| ->mutations($mutation, $attributes); | ||
|
|
||
| foreach ($toPerformActionModels as $toPerformActionModel) { | ||
| $this->resource()->authorizeToAttach($model, $toPerformActionModel); | ||
| } | ||
|
|
||
| $model->{$relation->relation}() | ||
| ->syncWithoutDetaching( | ||
| collect($toPerformActionModels) | ||
| ->mapWithKeys( | ||
| fn(Model $toPerformActionModel) => [$toPerformActionModel->getKey() => $mutation['pivot'] ?? []] | ||
| )->toArray() | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Handle absent or invalid related models in the update method.
When iterating through multiple models, consider handling cases where querying might return null or an unauthorized operation. For instance, if one of the IDs is not valid, you could skip or fail gracefully.
🏁 Script executed:
#!/bin/bash
# Check if there's any usage or mention of graceful error handling for nonexistent models in the update method.
rg --context 5 "authorizeToAttach" Length of output: 13398
Action Required: Add Validation for Absent or Invalid Models
In the update method of src/Concerns/Relations/PerformsRelationOperations.php (lines 27–43), you're iterating over models returned by the QueryBuilder and immediately calling authorizeToAttach on each. However, there’s no check to confirm that each $toPerformActionModel is valid (i.e. non-null and properly instantiated) before invoking authorization. This may lead to runtime exceptions if a queried model is absent or invalid.
- Suggestion: Add a conditional check (for example,
if ($toPerformActionModel) { … }) before callingauthorizeToAttachto either skip over null/invalid models or handle them gracefully (e.g., log a warning or return a controlled error). - Note: This pattern should be consistently applied wherever
authorizeToAttachis called, to secure the authorization process against unexpected invalid model instances.
This extension request enables you to use the “keys” attribute to create, update, attach, detach and sync, toggle several resources at the same time.
Issue
Summary by CodeRabbit
Summary by CodeRabbit