🚑️ rules exploit #172
🚑️ rules exploit #172
Conversation
WalkthroughThis update refactors and modernizes the validation system for resources, relations, and search/mutation operations. It introduces a new abstract validation rule base ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant RequestValidator
participant RestRule (new)
participant Resource/Relation
Client->>Controller: Submit request (mutate/search/operate)
Controller->>RequestValidator: Validate request
RequestValidator->>RestRule: Use modular rule (e.g., Mutate, Search)
RestRule->>Resource/Relation: Fetch resource metadata (fields, relations, etc.)
RestRule->>RequestValidator: Return validation rules
RequestValidator->>Controller: Pass/fail validation
Controller->>Client: Respond with result or validation errors
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ 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
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: 21
♻️ Duplicate comments (1)
tests/Feature/Controllers/ActionsOperationsTest.php (1)
146-146: Same stricter assertion pattern as other test files.Both test methods now use
assertExactJsonStructureinstead ofassertJsonStructure, following the same pattern seen inSearchPaginateOperationsTest.php. This enforces exact JSON structure matching for field validation error responses.The same verification concern applies here as mentioned in the previous test file - ensure that Laravel's validation error responses don't include additional fields that could cause these stricter assertions to fail.
Also applies to: 166-166
🧹 Nitpick comments (18)
src/Documentation/Schemas/License.php (2)
79-83: Update DocBlock to match the modernized type hint.The method parameter type was updated to use the modern
?stringsyntax, but the DocBlock comment still uses the oldstring|nullformat for consistency./** * Set the URL to the license used for the API. * - * @param string|null $url + * @param ?string $url * * @return License */
90-95: Update DocBlock to match the modernized return type.The method return type was updated to use the modern
?stringsyntax, but the DocBlock comment still uses the oldstring|nullformat./** * Get the URL to the license used for the API. * - * @return string|null + * @return ?string */src/Documentation/Schemas/Info.php (2)
131-135: Update DocBlock to match the modernized type hint.The method parameter type was updated to use the modern
?stringsyntax, but the DocBlock comment still uses the oldstring|nullformat./** * Set the URL to the Terms of Service for the API. * - * @param string|null $termsOfService + * @param ?string $termsOfService * * @return Info */
143-147: Update DocBlock to match the modernized return type.The method return type was updated to use the modern
?stringsyntax, but the DocBlock comment still uses the oldstring|nullformat./** * Get the URL to the Terms of Service for the API. * - * @return string|null + * @return ?string */src/Rules/Search/SearchScope.php (1)
13-13: Consider dependency injection over service locator.Using
app(RestRequest::class)is a service locator pattern. Consider injecting theRestRequestdependency through the constructor or method parameter for better testability and explicit dependencies.- $request = app(RestRequest::class); + public function buildValidationRules(string $attribute, mixed $value): array + { + $request = request();Or even better, if the parent
RestRuleclass could provide access to the request:- $request = app(RestRequest::class); + $request = $this->getRequest();src/Rules/Search/SearchSelect.php (2)
13-13: Consider dependency injection instead of service container resolution.Using
app(RestRequest::class)directly in the validation rule may not always resolve the correct request instance, especially in complex scenarios or testing environments.Consider injecting the request through the
RestRulebase class or accessing it via the validator context:- $request = app(RestRequest::class); + // Access request through validator if available + $request = $this->validator->getData()['request'] ?? app(RestRequest::class);Alternatively, the
RestRulebase class could be enhanced to provide request access.
16-20: Reorder validation rules for better error messaging.The current rule order places
Rule::in()beforerequiredandstring, which may produce confusing error messages if the field is missing or not a string.Apply this diff to improve error message clarity:
$attribute.'.field' => [ - Rule::in($this->resource->getFields($request)), 'required', 'string', + Rule::in($this->resource->getFields($request)), ],This ensures users get "field is required" instead of "field must be one of: ..." when the field is missing.
src/Rules/Resource/ResourceMutateRelationOperation.php (1)
23-29: Potential rule conflicts need handling.The current logic could result in the same relation path having conflicting rules (both 'required' and 'prohibited') if a relation matches both conditions. While this may be caught by Laravel's validation system, it could lead to confusing error messages.
Consider adding logic to prevent conflicting rules:
if ($value['operation'] === 'update') { if ($relation->isProhibitedOnUpdate($request)) { $rules[$attribute.'.relations.'.$relation->relation] = 'prohibited'; + } elseif ($relation->isRequiredOnUpdate($request)) { + $rules[$attribute.'.relations.'.$relation->relation] = 'required'; } - - if ($relation->isRequiredOnUpdate($request)) { - $rules[$attribute.'.relations.'.$relation->relation] = 'required'; - } }Also applies to: 33-39
src/Rules/Search/SearchInstruction.php (1)
13-13: Consider dependency injection instead of service location.Using
app(RestRequest::class)creates tight coupling and makes testing difficult. Consider injecting the request through the constructor or method parameters.- $request = app(RestRequest::class); + $request = $this->getRequest(); // or inject via constructorsrc/Rules/Resource/ResourceCustomRules.php (1)
12-12: Consider dependency injection instead of service location.Same issue as in SearchInstruction - using
app(RestRequest::class)creates tight coupling and hampers testability.src/Rules/Search/SearchInclude.php (1)
13-13: Consider dependency injection instead of service location.Consistent with other files, this service location pattern should be refactored for better testability.
src/Rules/Search/Search.php (1)
20-20: Consider using constants for gate values.The hardcoded array of gate values should ideally be defined as constants or an enum for better maintainability and to avoid typos.
Consider defining these values in a constant or enum:
+ private const ALLOWED_GATES = ['viewAny', 'view', 'create', 'update', 'delete', 'restore', 'forceDelete']; + public function buildValidationRules(string $attribute, mixed $value): array { // ... - $attribute.'gates' => ['sometimes', 'array', Rule::in(['viewAny', 'view', 'create', 'update', 'delete', 'restore', 'forceDelete'])], + $attribute.'.gates' => ['sometimes', 'array', Rule::in(self::ALLOWED_GATES)],src/Rules/Mutate/Mutate.php (5)
21-21: Consider dependency injection instead of service location.Using
app(RestRequest::class)creates a direct dependency on the service container. Consider injecting the request through the constructor or a setter method for better testability and explicit dependencies.- $request = app(RestRequest::class); + $request = $this->request ?? app(RestRequest::class);Add a property and setter:
protected ?RestRequest $request = null; public function setRequest(RestRequest $request): static { $this->request = $request; return $this; }
23-23: Simplify the operation rules construction.The current spread operator usage with conditional array is complex and hard to read. Consider extracting this logic to a dedicated method.
- $operationRules = Rule::in('create', 'update', ...($this->depth === 0 ? [] : ['attach', 'detach', 'toggle', 'sync'])); + $operationRules = Rule::in($this->getAllowedOperations());Add a helper method:
private function getAllowedOperations(): array { $baseOperations = ['create', 'update']; if ($this->depth > 0) { $baseOperations = array_merge($baseOperations, ['attach', 'detach', 'toggle', 'sync']); } return $baseOperations; }
43-51: Extract complex validation rules for better readability.The
.keyfield validation rules are quite verbose and could be extracted to improve readability and maintainability.- $attributeConsideringRelationType.'.key' => [ - 'required_if:'.$attributeConsideringRelationType.'.operation,update', - 'required_if:'.$attributeConsideringRelationType.'.operation,attach', - 'required_if:'.$attributeConsideringRelationType.'.operation,detach', - 'required_if:'.$attributeConsideringRelationType.'.operation,toggle', - 'required_if:'.$attributeConsideringRelationType.'.operation,sync', - 'prohibited_if:'.$attributeConsideringRelationType.'.operation,create', - 'exists:'.$this->resource::newModel()->getTable().','.$this->resource::newModel()->getKeyName(), - ], + $attributeConsideringRelationType.'.key' => $this->buildKeyValidationRules($attributeConsideringRelationType),Add helper method:
private function buildKeyValidationRules(string $attribute): array { $model = $this->resource::newModel(); $operationsRequiringKey = ['update', 'attach', 'detach', 'toggle', 'sync']; $rules = []; foreach ($operationsRequiringKey as $operation) { $rules[] = "required_if:{$attribute}.operation,{$operation}"; } $rules[] = "prohibited_if:{$attribute}.operation,create"; $rules[] = "exists:{$model->getTable()},{$model->getKeyName()}"; return $rules; }
50-50: Optimize model instantiation for performance.The validation rule creates two new model instances unnecessarily, which could impact performance and potentially cause side effects.
-'exists:'.$this->resource::newModel()->getTable().','.$this->resource::newModel()->getKeyName(), +sprintf('exists:%s,%s', + $this->resource::newModel()->getTable(), + $this->resource::newModel()->getKeyName() +),Or better yet, cache the model instance:
+$model = $this->resource::newModel(); +'exists:'.$model->getTable().','.$model->getKeyName(),
19-71: Consider breaking down the complex validation rules method.The
buildValidationRulesmethod is quite complex with multiple concerns. Consider extracting smaller methods for different validation rule groups to improve readability and maintainability:public function buildValidationRules(string $attribute, mixed $value): array { $request = app(RestRequest::class); $operationRules = Rule::in('create', 'update', ...($this->depth === 0 ? [] : ['attach', 'detach', 'toggle', 'sync'])); $attributeConsideringRelationType = $attribute.($this->relation?->hasMultipleEntries() ? '.*' : ''); return array_merge( - [ - // ... complex validation rules array - ], + $this->buildBaseValidationRules($attributeConsideringRelationType), + $this->buildOperationValidationRules($attributeConsideringRelationType, $operationRules), + $this->buildAttributeValidationRules($attributeConsideringRelationType, $request), + $this->buildKeyValidationRules($attributeConsideringRelationType), + $this->buildRelationValidationRules($attributeConsideringRelationType, $request), $this->relation?->rules($this->resource, $attribute) ?? [] ); }src/Rules/RestRule.php (1)
14-22: Add null checks for required properties.The validator and resource properties are used without null checks in the validate method. Consider adding validation or making these properties required in the constructor to prevent runtime errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
composer.json(1 hunks)src/Concerns/Resource/ConfiguresRestParameters.php(1 hunks)src/Concerns/Resource/Relationable.php(1 hunks)src/Contracts/RelationResource.php(1 hunks)src/Documentation/Schemas/Info.php(3 hunks)src/Documentation/Schemas/License.php(3 hunks)src/Http/Requests/MutateRequest.php(2 hunks)src/Http/Requests/OperateRequest.php(3 hunks)src/Http/Requests/SearchRequest.php(2 hunks)src/Http/Response.php(1 hunks)src/Query/Builder.php(1 hunks)src/Relations/BelongsToMany.php(2 hunks)src/Relations/MorphToMany.php(2 hunks)src/Relations/MorphedByMany.php(2 hunks)src/Relations/Relation.php(3 hunks)src/Rules/ActionField.php(0 hunks)src/Rules/AggregateField.php(0 hunks)src/Rules/AggregateFilterable.php(0 hunks)src/Rules/ArrayWithKey.php(1 hunks)src/Rules/CustomRulable.php(0 hunks)src/Rules/Includable.php(0 hunks)src/Rules/Instruction.php(0 hunks)src/Rules/InstructionField.php(0 hunks)src/Rules/IsNestedField.php(0 hunks)src/Rules/Mutate/Mutate.php(1 hunks)src/Rules/Mutate/MutateRelation.php(1 hunks)src/Rules/MutateItemRelations.php(0 hunks)src/Rules/MutateRules.php(0 hunks)src/Rules/NestedRelation.php(0 hunks)src/Rules/Operate/OperateField.php(1 hunks)src/Rules/ProhibitedRelationOnCreation.php(0 hunks)src/Rules/ProhibitedRelationOnUpdate.php(0 hunks)src/Rules/RequiredRelationOnCreation.php(0 hunks)src/Rules/RequiredRelationOnUpdate.php(0 hunks)src/Rules/Resource/ResourceCustomRules.php(1 hunks)src/Rules/Resource/ResourceFieldOrNested.php(1 hunks)src/Rules/Resource/ResourceMutateRelationOperation.php(1 hunks)src/Rules/Resource/ResourceRelationOrNested.php(1 hunks)src/Rules/RestRule.php(1 hunks)src/Rules/Search/Search.php(1 hunks)src/Rules/Search/SearchAggregate.php(1 hunks)src/Rules/Search/SearchFilter.php(1 hunks)src/Rules/Search/SearchInclude.php(1 hunks)src/Rules/Search/SearchInstruction.php(1 hunks)src/Rules/Search/SearchInstructionField.php(1 hunks)src/Rules/Search/SearchScope.php(1 hunks)src/Rules/Search/SearchSelect.php(1 hunks)src/Rules/Search/SearchSort.php(1 hunks)src/Rules/Search/SearchText.php(1 hunks)src/Rules/SearchRules.php(0 hunks)tests/Feature/Controllers/ActionsOperationsTest.php(2 hunks)tests/Feature/Controllers/MutateCreateMorphOperationsTest.php(2 hunks)tests/Feature/Controllers/MutateCreateOperationsTest.php(9 hunks)tests/Feature/Controllers/MutateCreateThroughOperationsTest.php(7 hunks)tests/Feature/Controllers/MutateUpdateMorphOperationsTest.php(3 hunks)tests/Feature/Controllers/MutateUpdateOperationsTest.php(6 hunks)tests/Feature/Controllers/SearchAggregatesOperationsTest.php(5 hunks)tests/Feature/Controllers/SearchFilteringOperationsTest.php(2 hunks)tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php(2 hunks)tests/Feature/Controllers/SearchInstructionsOperationsTest.php(3 hunks)tests/Feature/Controllers/SearchLimitingOperationsTest.php(1 hunks)tests/Feature/Controllers/SearchPaginateOperationsTest.php(1 hunks)tests/Feature/Controllers/SearchScopingOperationsTest.php(1 hunks)tests/Feature/Controllers/SearchScoutOperationsTest.php(8 hunks)tests/Feature/Controllers/SearchSelectingOperationsTest.php(1 hunks)tests/Feature/Controllers/SearchSortingOperationsTest.php(1 hunks)
💤 Files with no reviewable changes (16)
- src/Rules/IsNestedField.php
- src/Rules/NestedRelation.php
- src/Rules/MutateItemRelations.php
- src/Rules/CustomRulable.php
- src/Rules/InstructionField.php
- src/Rules/RequiredRelationOnCreation.php
- src/Rules/AggregateFilterable.php
- src/Rules/Includable.php
- src/Rules/RequiredRelationOnUpdate.php
- src/Rules/ProhibitedRelationOnCreation.php
- src/Rules/ProhibitedRelationOnUpdate.php
- src/Rules/Instruction.php
- src/Rules/ActionField.php
- src/Rules/MutateRules.php
- src/Rules/SearchRules.php
- src/Rules/AggregateField.php
🧰 Additional context used
🧬 Code Graph Analysis (7)
tests/Feature/Controllers/SearchInstructionsOperationsTest.php (1)
tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-29)
src/Http/Requests/SearchRequest.php (3)
src/Rules/Search/Search.php (1)
Search(9-37)src/Http/Controllers/Controller.php (1)
newResource(28-38)src/Rules/RestRule.php (1)
setResource(54-59)
src/Documentation/Schemas/License.php (2)
src/Documentation/Schemas/Contact.php (2)
url(66-69)withUrl(54-59)src/Documentation/Schemas/Server.php (2)
url(55-58)withUrl(41-46)
src/Rules/Search/SearchSelect.php (4)
src/Rules/Search/Search.php (2)
Search(9-37)buildValidationRules(11-36)src/Http/Requests/RestRequest.php (1)
RestRequest(9-23)src/Rules/RestRule.php (2)
RestRule(12-62)buildValidationRules(61-61)src/Concerns/Resource/ConfiguresRestParameters.php (1)
getFields(38-41)
src/Relations/Relation.php (3)
src/Contracts/RelationResource.php (1)
filter(10-10)src/Query/Traits/PerformSearch.php (1)
filter(93-118)src/Concerns/Resource/Relationable.php (1)
relation(17-35)
tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (3)
tests/Support/Models/Model.php (1)
Model(10-112)tests/Support/Policies/GreenPolicy.php (1)
GreenPolicy(8-100)tests/Support/Models/HasManyRelation.php (1)
HasManyRelation(7-13)
tests/Feature/Controllers/SearchLimitingOperationsTest.php (4)
tests/Support/Database/Factories/ModelFactory.php (1)
ModelFactory(8-29)tests/Support/Models/HasOneRelation.php (1)
HasOneRelation(7-13)tests/Support/Models/Model.php (1)
Model(10-112)tests/Support/Policies/GreenPolicy.php (1)
GreenPolicy(8-100)
🔇 Additional comments (70)
src/Documentation/Schemas/License.php (1)
26-26: LGTM! Modern nullable type syntax is more concise.The change from
string|nullto?stringis a good modernization that improves code readability.src/Documentation/Schemas/Info.php (1)
33-33: LGTM! Consistent nullable type modernization.The change to
?stringaligns well with the similar updates inLicense.phpand represents good code modernization.src/Rules/ArrayWithKey.php (1)
8-8:✅ Verification successful
LGTM! More descriptive class name improves code clarity.
The rename from
ArrayWithtoArrayWithKeybetter describes the validation rule's purpose of validating array keys. According to the AI summary, the usage has been consistently updated in relation classes.Let me verify that all references to the old class name have been updated:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old ArrayWith class name # Expected: No occurrences of the old class name should remain echo "Searching for old class name 'ArrayWith'..." rg -i "ArrayWith" --type php -A 2 -B 2 echo "Searching for imports of the old class..." rg "use.*ArrayWith" --type php echo "Searching for instantiations of the old class..." rg "new ArrayWith" --type phpLength of output: 4763
Verified: Complete rename from
ArrayWithtoArrayWithKeyI searched for the old class name, its imports, and instantiations—only the new
ArrayWithKeyappears in:
- src/Rules/ArrayWithKey.php
- src/Rules/Mutate/Mutate.php
- src/Relations/BelongsToMany.php
- src/Relations/MorphToMany.php
- src/Relations/MorphedByMany.php
No remaining references to
ArrayWith. LGTM!tests/Feature/Controllers/SearchSortingOperationsTest.php (1)
33-33: Excellent improvement to test precision!The change from
assertJsonStructuretoassertExactJsonStructureenforces stricter validation by ensuring the JSON response contains exactly the specified keys with no additional fields. This helps catch unexpected changes in the API error response structure and aligns with the validation rule refactoring efforts.src/Concerns/Resource/Relationable.php (1)
17-17: Good type safety improvement!Adding the explicit
stringtype hint to the$nameparameter improves type safety and makes the method signature more explicit. This change aligns with modern PHP best practices and provides better IDE support and static analysis capabilities.tests/Feature/Controllers/SearchScopingOperationsTest.php (1)
33-33: Consistent improvement in test validation!The change from
assertJsonStructuretoassertExactJsonStructureprovides stricter validation of the error response structure, ensuring no unexpected additional fields are present. This change is consistent with the overall testing improvements across the codebase.tests/Feature/Controllers/SearchSelectingOperationsTest.php (1)
33-33: Consistent test validation enhancement!This change maintains consistency with the stricter test validation approach being applied across the test suite. Using
assertExactJsonStructureensures precise validation of error response formats and helps maintain API contract integrity.src/Relations/MorphToMany.php (1)
11-11:✅ Verification successful
Verify ArrayWithKey compatibility with existing pivot validation.
The import and usage have been updated from
ArrayWithtoArrayWithKeyfor pivot field validation. This appears to be part of the broader validation rule refactoring mentioned in the PR objectives.Please verify that
ArrayWithKeyprovides equivalent or improved validation behavior compared to the previousArrayWithrule to ensure existing pivot field validation logic is preserved:Also applies to: 33-33
🏁 Script executed:
#!/bin/bash # Description: Verify ArrayWithKey class exists and check its usage pattern # Expected: Class should exist and be used consistently across relation files # Check if ArrayWithKey class exists fd "ArrayWithKey.php" --type f # Verify consistent usage across relation files rg "ArrayWithKey" --type php -A 2 -B 2Length of output: 3734
ArrayWithKey validation rule verified across relation classes
The
ArrayWithKeyrule is implemented insrc/Rules/ArrayWithKey.phpand has been consistently applied in all pivot-related validations, including:
src/Relations/MorphToMany.phpsrc/Relations/MorphedByMany.phpsrc/Relations/BelongsToMany.phpsrc/Rules/Mutate/Mutate.phpNo discrepancies were found between the intended refactoring and its usage. Approving the change.
src/Relations/BelongsToMany.php (1)
11-11: LGTM! Consistent validation rule update.The changes mirror those in
MorphToMany.php, replacingArrayWithwithArrayWithKeyfor pivot field validation. This consistency across relation classes indicates a well-coordinated refactoring effort.Also applies to: 33-33
tests/Feature/Controllers/SearchPaginateOperationsTest.php (1)
31-31:✅ Verification successful
Verify stricter JSON assertion doesn't break with additional response fields.
The change from
assertJsonStructuretoassertExactJsonStructuremakes the test more restrictive by requiring an exact structure match rather than just checking for the presence of specified keys.This could cause test failures if Laravel's validation error responses include additional metadata fields. Please verify the actual JSON response structure:
🏁 Script executed:
#!/bin/bash # Description: Check for similar assertion patterns and potential response structures # Expected: Consistent usage of assertExactJsonStructure and understanding of actual response format # Find all usages of assertExactJsonStructure to see the pattern rg "assertExactJsonStructure" --type php -A 1 -B 1 # Look for any documentation or examples of validation error response structures rg "validation.*error.*response" --type php -A 3 -B 3Length of output: 18392
Approve stricter JSON assertion
The switch toassertExactJsonStructurealigns with the rest of the test suite—dozens of other feature tests already use this assertion—and Laravel’s default validation error payload only includes the top-levelmessageanderrorskeys. No extra fields are present by default, so this change won’t introduce breaks.tests/Feature/Controllers/SearchInstructionsOperationsTest.php (4)
33-33: LGTM: Stricter JSON structure validationThe change from
assertJsonStructuretoassertExactJsonStructureenforces stricter validation, ensuring the response structure exactly matches the expected format rather than just containing the specified keys.
87-87: LGTM: Consistent stricter validationGood consistency in applying the stricter JSON structure assertion across validation error tests.
92-92: LGTM: Code cleanupRemoving the unused variable assignment while keeping the model creation and refresh is a good cleanup.
115-115: LGTM: Consistent validation approachThe stricter assertion aligns with the overall pattern of enforcing exact JSON response structure matching.
src/Relations/MorphedByMany.php (2)
11-11: LGTM: Updated validation rule importThe import change from
ArrayWithtoArrayWithKeyaligns with the validation rule refactoring described in the PR summary.
33-33: LGTM: Consistent validation rule usageThe validation rule instantiation correctly uses the new
ArrayWithKeyclass with the samegetPivotFields()parameter, maintaining the intended validation behavior.tests/Feature/Controllers/MutateUpdateMorphOperationsTest.php (3)
45-45: LGTM: Enhanced validation precisionThe stricter JSON structure assertion ensures the field validation error response format is exactly as expected.
756-756: LGTM: Precise pivot validation testingThe exact structure assertion for unauthorized pivot field errors provides better test coverage for morph-to-many relation validation.
1107-1107: LGTM: Consistent morphed-by-many validationThe stricter assertion maintains consistency across all morph relation pivot validation tests.
tests/Feature/Controllers/MutateCreateThroughOperationsTest.php (7)
50-50: LGTM: Precise through relation validationThe exact JSON structure assertion properly validates that creating through relations returns the expected error format.
92-92: LGTM: Consistent attach operation validationStricter validation for attach operations on through relations ensures proper error response formatting.
135-135: LGTM: Update operation validationThe exact structure assertion confirms that update operations on through relations are properly rejected with precise error formatting.
170-170: LGTM: Many-through relation validationConsistent stricter assertion for has-many-through relation create operations.
209-209: LGTM: Multiple creation validationThe exact structure assertion for multiple through relation creation attempts ensures proper error handling.
253-253: LGTM: Through relation attach validationStricter validation for attach operations maintains consistency across all through relation operation tests.
298-298: LGTM: Complete through relation coverageThe final stricter assertion completes the systematic improvement of through relation validation testing.
tests/Feature/Controllers/MutateCreateMorphOperationsTest.php (2)
531-531: LGTM: Stricter validation assertion aligns with validation rule refactoring.The change from
assertJsonStructuretoassertExactJsonStructureprovides more precise validation of the error response structure, ensuring the JSON exactly matches the expected format rather than just containing the required keys. This aligns well with the broader validation rule refactoring mentioned in the PR summary.
866-866: LGTM: Consistent stricter validation assertion.This change matches the pattern established in the previous test method, providing consistent and precise validation of error response structures throughout the test suite.
src/Http/Response.php (1)
56-56: LGTM: Explicit nullable type hint improves code clarity.The change from
Relation $relation = nullto?Relation $relation = nulluses modern PHP nullable syntax, making the nullable intention explicit and improving type safety. This aligns with similar type hint improvements mentioned across the codebase.tests/Feature/Controllers/SearchFilteringOperationsTest.php (2)
36-36: LGTM: Stricter validation assertion improves test precision.The change to
assertExactJsonStructureensures the error response format exactly matches expectations when filtering by unauthorized fields, providing more precise validation than the previousassertJsonStructure.
58-58: LGTM: Consistent validation improvement.This change maintains consistency with the previous assertion update and the broader pattern of stricter validation testing across the codebase.
src/Concerns/Resource/ConfiguresRestParameters.php (1)
50-50: LGTM: Explicit nullable type hint follows best practices.The change to
?Relation $relation = nullmakes the nullable parameter intention explicit and clear, following modern PHP type declaration best practices. This is consistent with similar improvements made throughout the codebase.tests/Feature/Controllers/MutateUpdateOperationsTest.php (7)
44-44: LGTM: Stricter JSON validation enforcement.The switch from
assertJsonStructuretoassertExactJsonStructureimproves test precision by requiring exact structure matches rather than partial key presence validation.
57-57: LGTM: Test data aligned with authorization rules.The change from
not_authorized_fieldtonameproperly uses an authorized attribute key, which aligns with the new validation architecture.
65-65: LGTM: Consistent stricter validation pattern.Consistent application of exact JSON structure validation improves test reliability.
89-89: LGTM: Stricter validation consistency maintained.Continues the pattern of enforcing exact JSON structure validation for better test precision.
119-122: LGTM: Enhanced validation with multiple error keys.The stricter assertion combined with expanded error keys reflects improved validation that now catches multiple validation issues simultaneously, providing better user feedback.
149-149: LGTM: Continued stricter validation implementation.Maintains consistency in applying exact JSON structure validation across test methods.
231-231: LGTM: Complete transition to exact structure validation.Completes the consistent refactoring to use exact JSON structure validation throughout the test file.
src/Query/Builder.php (1)
27-27: LGTM: Improved type safety with explicit nullable parameter.Adding
|nullto the type declaration makes the nullable nature of the$queryparameter explicit, which aligns perfectly with the constructor logic that already handles null values with the null coalescing operator.tests/Feature/Controllers/SearchScoutOperationsTest.php (2)
41-41: LGTM: Comprehensive transition to exact JSON structure validation.The consistent replacement of
assertJsonStructurewithassertExactJsonStructureacross all test methods enforces stricter validation requirements, ensuring that JSON responses match expected structures exactly rather than just checking for key presence.Also applies to: 66-66, 91-91, 116-116, 141-141, 255-255, 280-280, 305-305
58-58: LGTM: Test data aligned with field authorization.Changing from
'id'to'allowed_scout_field'ensures the test uses a field that's properly authorized for scout operations, aligning with the new validation rule architecture.src/Http/Requests/SearchRequest.php (3)
5-5: LGTM: Updated import for new validation architecture.The import change from
SearchRulestoSearchaligns with the new modular validation rule architecture that extendsRestRule.
16-16: LGTM: Proper resource retrieval using controller method.Using
$this->route()->controller::newResource()correctly retrieves the resource instance through the controller's singleton pattern, ensuring proper resource initialization.
19-19: LGTM: Improved validation rule instantiation pattern.The new approach using
(new Search())->setResource($resource)follows a cleaner builder pattern compared to constructor injection, providing better flexibility and separation of concerns in the validation architecture.src/Contracts/RelationResource.php (1)
10-10: Good improvement to type safety!Making the
$callbackparameter explicitly nullable with?Closureimproves type clarity and aligns with modern PHP type hinting best practices. This change is backward compatible and makes the interface more explicit about accepting null values.src/Http/Requests/MutateRequest.php (3)
5-5: Good refactoring to new modular validation system!The change from
MutateRulesto the newMutateclass represents a positive move toward a more modular validation architecture based on theRestRulepattern.
22-24: Clean validation rules implementation.The simplified rules array with the fluent
setResource()method pattern is much cleaner than the previous approach. The separation of concerns between requiring the 'mutate' key and validating its contents is well-structured.
19-19:✅ Verification successful
Verify controller access safety.
The direct access to the controller via
$this->route()->controller::newResource()should be verified to ensure it's safe and the controller always has this method available.
🏁 Script executed:
#!/bin/bash # Description: Verify that controllers using MutateRequest have the newResource method # Expected: Find controller classes that should implement newResource method # Search for classes that might be used with MutateRequest ast-grep --pattern $'class $_ { $$$ public function newResource() { $$$ } $$$ }' # Also search for any controller base classes that might define this method rg -A 5 "newResource.*function"Length of output: 127
🏁 Script executed:
#!/bin/bash # 1. Locate the MutateRequest file and show the context around the newResource call echo "== MutateRequest usage ==" rg -n "route()->controller" -g "src/Http/Requests/MutateRequest.php" echo # Print first 50 lines for context sed -n '1,50p' "$(rg -l 'class MutateRequest' -g '*.php')" # 2. List all controller files under src/Http/Controllers echo "== Controller files ==" fd --type f -e php src/Http/Controllers # 3. Check each controller for a public newResource method echo "== Controllers with public function newResource ==" fd --type f -e php src/Http/Controllers | xargs rg -n "public function newResource" || echo "No controllers define public function newResource" # 4. Search entire src directory for any newResource definitions (including traits or base classes) echo "== All newResource definitions in src ==" rg -n "function newResource" src || echo "No newResource definitions found in src"Length of output: 2285
Controller’s static newResource method is defined and available
Verified thatsrc/Http/Controllers/Controller.phpdeclares:public static function newResource(): ResourceBecause all of the package’s controllers inherit from this base
Controller, they all have thenewResourcemethod via inheritance. The call$this->route()->controller::newResource()is therefore safe and does not require changes.
src/Rules/Search/SearchScope.php (2)
15-17: Good early return for Scout mode.The early return when in Scout mode is a clean way to handle different validation contexts. This prevents unnecessary validation rule generation when Scout search is being used.
19-30: Well-structured validation rules.The validation rules are comprehensive and well-organized:
- Scope name validation using
Rule::in()with resource-defined scopes- Required string validation for the name
- Optional array validation for parameters
The attribute path building with
$attribute.'.name'and$attribute.'.parameters'correctly handles nested validation.tests/Feature/Controllers/SearchAggregatesOperationsTest.php (1)
40-40: Excellent improvement to test strictness!Changing from
assertJsonStructuretoassertExactJsonStructureprovides much stricter validation of API error responses. This ensures that:
- The JSON structure matches exactly (no extra fields)
- Validation error keys are precisely as expected
- Any changes in error response format will be caught immediately
This aligns well with the new validation rule architecture and helps maintain API consistency.
Also applies to: 67-67, 94-94, 120-120, 147-147
src/Rules/Search/SearchText.php (1)
7-22: LGTM! Well-implemented search text validation rule.The validation logic is sound:
- Properly checks if the resource supports model search functionality
- Returns
prohibitedrule when search is not supported- Validates expected structure (
arraywithstringvalue) when search is enabled- Follows the established pattern of other search validation rules
The implementation correctly extends
RestRuleand integrates with the modular validation architecture.tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php (2)
53-53: Good improvement to use stricter JSON structure assertions.Changing from
assertJsonStructuretoassertExactJsonStructureprovides more precise validation of error responses, ensuring the JSON structure matches exactly what's expected.Also applies to: 79-79
82-109: Comprehensive test for multiple includes validation scenarios.The new test method effectively covers:
- Multiple includes with different authorization scenarios
- Unauthorized filters on valid relations
- Nested relation authorization (hasManyRelation.model)
- Proper error structure validation for multiple validation failures
The test correctly expects validation errors for both the unauthorized filter field (
search.includes.1.filters.0.field) and the unauthorized nested relation (search.includes.0.relation).src/Http/Requests/OperateRequest.php (3)
8-9: Imports updated for new modular validation architecture.The imports correctly reference the new validation rule classes that replace the deprecated ones as part of the broader refactoring effort.
54-54: Proper integration with new Search validation rule.The code correctly instantiates the new
Searchrule and configures it with the resource context using thesetResource()method. This follows the established pattern for the new modular validation architecture.
65-65: Correct usage of new OperateField validation rule.The implementation properly instantiates
OperateFieldand configures it with the action context viasetAction(). This maintains the same validation functionality while using the new modular rule architecture.tests/Feature/Controllers/SearchLimitingOperationsTest.php (1)
34-35: LGTM - Proper validation testing.The test correctly validates that unauthorized limit values return a 422 status with the expected error structure. The use of
assertExactJsonStructureensures stricter validation compliance.src/Rules/Search/SearchSort.php (1)
11-30: LGTM - Well-structured validation rule.The implementation correctly:
- Handles Scout mode vs normal mode field validation
- Requires field to be a string and from allowed fields
- Makes direction optional with valid enum values
- Uses proper Laravel validation syntax
This fits well with the modular validation architecture refactoring.
tests/Feature/Controllers/MutateCreateOperationsTest.php (2)
41-41: LGTM - Improved test assertion strictness.The migration from
assertJsonStructuretoassertExactJsonStructureprovides more precise validation testing, ensuring the JSON response exactly matches the expected structure rather than just containing the specified keys. This aligns well with the stricter validation rules introduced in the refactoring.Also applies to: 55-55, 117-117, 189-189, 224-224, 299-299, 1038-1038
75-75: LGTM - Enhanced validation for attach/detach operations.The changes correctly:
- Add the required 'key' field to the detach operation test (line 88)
- Update error expectations to include both 'operation' and 'key' validation errors (lines 75, 96)
This reflects more comprehensive validation of attach/detach operations in the new validation system.
Also applies to: 88-88, 96-96
src/Relations/Relation.php (3)
26-26: Good type safety improvement.Adding the
stringtype declaration to the$nameproperty enhances type safety and makes the code more self-documenting.
58-58: Correct nullable type syntax improvement.The change from
Closure $callback = nullto?Closure $callback = nullproperly declares the parameter as nullable, aligning with the interface signature insrc/Contracts/RelationResource.php.
43-43:✅ Verification successful
Verify semantic change from falsy to null coalescing operator.
The change from
?:(falsy coalescing) to??(null coalescing) alters the behavior when$this->nameis an empty string. Previously, an empty string would fallback to the class name, but now it would return the empty string.
🏁 Script executed:
#!/bin/bash # Description: Check if any existing relation instances rely on empty string names falling back to class names # Expected: Find usage patterns that might be affected by this semantic change echo "Searching for relation name assignments with empty strings..." rg -A 3 -B 3 '\$name\s*=\s*["\'"'"']?\s*["\'"'"']' --type php echo -e "\nSearching for relation instantiation patterns..." rg -A 5 'new.*Relation.*\(' --type php echo -e "\nSearching for name property usage in tests..." rg -A 3 -B 3 '->name.*=.*["\'"'"']["\'"'"']' --type phpLength of output: 10300
🏁 Script executed:
#!/bin/bash # Locate the Relation class fd Relation.php # Show the beginning of Relation.php to inspect declaration and default values sed -n '1,200p' src/Relations/Relation.php # Find where the $name property is declared (and its default) rg -n '^\s*(public|protected|private).*\$name' src/Relations/Relation.php # Show all uses of $this->name inside Relation.php rg -n '\$this->name' src/Relations/Relation.php # Look for any setter or fluent method named setName or name(...) in the repo rg -n 'setName' src rg -n 'function\s\+name\(' src # Search for any direct assignments of an empty-string name on relation instances rg -n "\->name\s*=\s*''" srcLength of output: 5854
🏁 Script executed:
#!/bin/bash # Search for assignments to $this->name across the codebase rg -n '\$this->name\s*=' -n src # Inspect the MorphRelation class for any name assignments sed -n '1,200p' src/Relations/MorphRelation.php # Inspect a test support model to see how name is set (e.g., BelongsToRelation) sed -n '1,200p' tests/Support/Models/BelongsToRelation.phpLength of output: 804
No impact from changing
?:to??in Relation::name()I’ve searched the entire codebase and found no assignments of an empty string to the relation’s
$nameproperty. All Relation instances either never set$name(so fall back to the class short name via Reflection) or set it to a non-empty string. Therefore, switching from the falsy (?:) to the null (??) coalescing operator does not alter any existing behavior.src/Rules/Search/SearchAggregate.php (1)
12-39: Excellent implementation of conditional validation logic.The
buildValidationRules()method is well-structured with proper conditional validation based on the relation resource existence. The use ofarray_merge()to conditionally add rules, along withrequired_ifandprohibited_ifvalidators based on aggregate type, demonstrates good understanding of Laravel validation patterns.The logic correctly:
- Validates relation using the new
ResourceRelationOrNestedrule- Restricts aggregate types to valid operations
- Conditionally requires/prohibits the field based on aggregate type
- Applies nested filter validation when relation resource exists
src/Rules/RestRule.php (4)
44-59: LGTM! Clean fluent interface implementation.The setter methods follow a clean fluent interface pattern with proper return types and clear documentation. The use of
staticreturn type is appropriate for method chaining.
61-61: Well-defined abstract contract.The abstract method signature is well-defined with proper type hints and clear naming. Subclasses will need to implement the specific validation logic while the base class handles the common validation orchestration.
24-39:✅ Verification successful
Consider the performance implications of validator cloning.
The current implementation clones the entire validator instance for each validation rule, which could be expensive in scenarios with complex validation or large datasets. Consider whether the cloning is necessary or if a more lightweight approach could be used.
Additionally, verify that the error message aggregation logic correctly preserves the original error context and doesn't introduce duplicate messages.
🏁 Script executed:
#!/bin/bash # Search for other instances where validator cloning is used to understand the pattern rg -A 3 -B 3 "clone.*validator|validator.*clone"Length of output: 445
Validator cloning and error aggregation verified
The only use of
clone $this->validatoris inRestRule.phpto isolate rule‐specific validation state; the overhead is negligible compared to the validation work itself. The error‐message aggregation correctly preserves keys and messages without introducing duplicates. No changes are needed.
44-59: Well-implemented fluent interface.The setter methods correctly implement the fluent interface pattern with proper return types and chaining support.
| * @var Relation $relation | ||
| */ | ||
| foreach ($resourceRelations as $relation) { | ||
| if ($value['operation'] === 'update') { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required array structure.
The code accesses $value['operation'] without validating that the 'operation' key exists, which could cause runtime errors if the input structure is unexpected.
Consider adding validation before array access:
foreach ($resourceRelations as $relation) {
+ if (!isset($value['operation'])) {
+ continue;
+ }
+
if ($value['operation'] === 'update') {Also applies to: 32-32
🤖 Prompt for AI Agents
In src/Rules/Resource/ResourceMutateRelationOperation.php at lines 22 and 32,
the code accesses $value['operation'] without checking if the 'operation' key
exists, which can cause runtime errors. Add a validation condition using isset()
or array_key_exists() to ensure the 'operation' key is present in $value before
accessing it. This will prevent errors when the input array structure is
unexpected.
| public function buildValidationRules(string $attribute, mixed $value): array | ||
| { | ||
| $relation = $this->resource->relation(Str::afterLast($attribute, '.')); | ||
|
|
||
| if (is_null($relation)) { | ||
| return []; | ||
| } | ||
|
|
||
| return (new Mutate(depth: ++$this->depth, relation: $relation))->setResource($relation->resource())->buildValidationRules($attribute, $value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add resource validation and recursion protection.
The method assumes $this->resource is set but doesn't validate this. Additionally, while depth is tracked, there's no maximum depth limit to prevent potential infinite recursion in circular relations.
public function buildValidationRules(string $attribute, mixed $value): array
{
+ if (!isset($this->resource)) {
+ throw new \RuntimeException('Resource must be set before building validation rules');
+ }
+
+ if ($this->depth > 10) { // Configurable max depth
+ return [];
+ }
+
$relation = $this->resource->relation(Str::afterLast($attribute, '.'));
if (is_null($relation)) {
return [];
}
return (new Mutate(depth: ++$this->depth, relation: $relation))->setResource($relation->resource())->buildValidationRules($attribute, $value);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function buildValidationRules(string $attribute, mixed $value): array | |
| { | |
| $relation = $this->resource->relation(Str::afterLast($attribute, '.')); | |
| if (is_null($relation)) { | |
| return []; | |
| } | |
| return (new Mutate(depth: ++$this->depth, relation: $relation))->setResource($relation->resource())->buildValidationRules($attribute, $value); | |
| } | |
| public function buildValidationRules(string $attribute, mixed $value): array | |
| { | |
| if (!isset($this->resource)) { | |
| throw new \RuntimeException('Resource must be set before building validation rules'); | |
| } | |
| if ($this->depth > 10) { // Configurable max depth | |
| return []; | |
| } | |
| $relation = $this->resource->relation(Str::afterLast($attribute, '.')); | |
| if (is_null($relation)) { | |
| return []; | |
| } | |
| return (new Mutate(depth: ++$this->depth, relation: $relation)) | |
| ->setResource($relation->resource()) | |
| ->buildValidationRules($attribute, $value); | |
| } |
🤖 Prompt for AI Agents
In src/Rules/Mutate/MutateRelation.php around lines 14 to 23, add a check to
ensure $this->resource is set before using it to get the relation, returning an
empty array or handling the error if not set. Also, implement a maximum depth
limit check before recursing with Mutate to prevent infinite recursion in
circular relations, returning an empty array or stopping recursion when the
limit is reached.
| public function validate(string $attribute, mixed $value, Closure $fail): void | ||
| { | ||
| if (!$this->resource->isNestedField($value)) { | ||
| $fail('The \''.Str::afterLast($attribute, '.').'\' field is not valid.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add resource validation and improve error message.
The validate method assumes $this->resource is set but doesn't verify this, which could lead to a fatal error. The error message could also be more descriptive.
public function validate(string $attribute, mixed $value, Closure $fail): void
{
+ if (!isset($this->resource)) {
+ $fail('Resource not configured for validation rule');
+ return;
+ }
+
if (!$this->resource->isNestedField($value)) {
- $fail('The \''.Str::afterLast($attribute, '.').'\' field is not valid.');
+ $fail('The field \''.Str::afterLast($attribute, '.').'\' with value \''.$value.'\' is not a valid nested field for this resource.');
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function validate(string $attribute, mixed $value, Closure $fail): void | |
| { | |
| if (!$this->resource->isNestedField($value)) { | |
| $fail('The \''.Str::afterLast($attribute, '.').'\' field is not valid.'); | |
| } | |
| } | |
| public function validate(string $attribute, mixed $value, Closure $fail): void | |
| { | |
| if (!isset($this->resource)) { | |
| $fail('Resource not configured for validation rule'); | |
| return; | |
| } | |
| if (!$this->resource->isNestedField($value)) { | |
| $fail( | |
| 'The field \''. | |
| Str::afterLast($attribute, '.'). | |
| '\' with value \''. | |
| $value. | |
| '\' is not a valid nested field for this resource.' | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Rules/Resource/ResourceFieldOrNested.php around lines 17 to 22, the
validate method uses $this->resource without checking if it is set, risking a
fatal error. Add a check to confirm $this->resource is initialized before
calling isNestedField. Also, improve the error message to be more descriptive by
including the resource context or clarifying why the field is invalid.
| public function buildValidationRules(string $attribute, mixed $value): array | ||
| { | ||
| $request = app(RestRequest::class); | ||
|
|
||
| $field = $this->instruction->field($request, $value['name'] ?? '') ?? []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety for the value parameter.
Similar to OperateField, this method assumes $value is an array when accessing $value['name'], but the parameter is typed as mixed.
Consider adding type validation:
public function buildValidationRules(string $attribute, mixed $value): array
{
+ if (!is_array($value)) {
+ return [];
+ }
+
$request = app(RestRequest::class);🤖 Prompt for AI Agents
In src/Rules/Search/SearchInstructionField.php around lines 13 to 17, the method
buildValidationRules assumes $value is an array when accessing $value['name'],
but the parameter is typed as mixed, which can cause runtime errors. Add a type
check to verify $value is an array before accessing $value['name'], and handle
the case when it is not an array, for example by setting a default or returning
early. This ensures type safety and prevents potential errors.
| ->toArray() | ||
| ), | ||
| ], | ||
| $attributeConsideringRelationType.'.relations.*' => (new MutateRelation(depth: $this->depth))->setResource($this->resource), |
There was a problem hiding this comment.
Add depth limit to prevent infinite recursion.
The recursive validation rule instantiation doesn't check for maximum depth, which could lead to infinite recursion in circular relationships or deeply nested structures.
- $attributeConsideringRelationType.'.relations.*' => (new MutateRelation(depth: $this->depth))->setResource($this->resource),
+ $attributeConsideringRelationType.'.relations.*' => $this->depth < 10
+ ? (new MutateRelation(depth: $this->depth + 1))->setResource($this->resource)
+ : 'array',Also consider adding a configurable maximum depth constant:
private const MAX_RECURSION_DEPTH = 10;🤖 Prompt for AI Agents
In src/Rules/Mutate/Mutate.php at line 67, the recursive instantiation of
MutateRelation lacks a depth limit, risking infinite recursion. Introduce a
private constant MAX_RECURSION_DEPTH set to 10, then modify the code to only
instantiate MutateRelation with increased depth if the current depth is less
than MAX_RECURSION_DEPTH; otherwise, avoid further recursion to prevent infinite
loops.
💡 Verification agent
🧩 Analysis chain
Verify recursion depth control in MutateRelation.
The recursive validation using MutateRelation could potentially cause infinite recursion if not properly controlled. Ensure that the depth parameter is being incremented and checked in the MutateRelation class.
🏁 Script executed:
#!/bin/bash
# Description: Check if MutateRelation class properly handles depth control to prevent infinite recursion
# Search for MutateRelation class and its depth handling
ast-grep --pattern 'class MutateRelation {
$$$
}'
# Look for depth incrementation or limits
rg -A 10 -B 5 "depth.*\+\+|depth.*increment|depth.*limit|depth.*max" --type phpLength of output: 651
🏁 Script executed:
#!/bin/bash
# Find Mutate class declaration and inspect depth handling
rg -n "class Mutate" --type php
rg -A 20 "depth" -n src/Rules/Mutate/Mutate.phpLength of output: 2890
🏁 Script executed:
#!/bin/bash
# Display the MutateRelation class to inspect depth handling and any guard conditions
sed -n '1,200p' src/Rules/Mutate/MutateRelation.phpLength of output: 662
Add a maximum-depth check to prevent unbounded recursion in MutateRelation
It looks like MutateRelation::buildValidationRules correctly increments $depth, but never stops once relations keep nesting. To avoid potential infinite loops (for example if there’s a circular relation), you should bail out when a sensible maximum nesting level is reached—either via a constant or config setting.
• File: src/Rules/Mutate/MutateRelation.php
• Location: start of buildValidationRules()
Suggested diff (using a class constant; adjust to use your own limit or config):
class MutateRelation extends RestRule
{
+ protected const MAX_DEPTH = 5; // or pull from config()
public function __construct(protected int $depth = 0)
{
}
public function buildValidationRules(string $attribute, mixed $value): array
{
+ // stop recursing once we hit MAX_DEPTH
+ if ($this->depth >= self::MAX_DEPTH) {
+ return [];
+ }
$relation = $this->resource->relation(
Str::afterLast($attribute, '.')
);With this guard in place, deeper or circular relations won’t drive the recursion to runaway.
🤖 Prompt for AI Agents
In src/Rules/Mutate/MutateRelation.php around the start of the
buildValidationRules() method, add a maximum-depth check to prevent infinite
recursion caused by deeply nested or circular relations. Define a class constant
for the maximum allowed depth and return early from buildValidationRules() if
the current depth exceeds this limit. This will ensure the recursion stops at a
sensible level and avoids runaway loops.
| public function validate(string $attribute, mixed $value, Closure $fail): void | ||
| { | ||
| $ruleValidator = clone $this->validator; | ||
|
|
||
| $ruleValidator->setRules( | ||
| $this->buildValidationRules($attribute, $value) | ||
| ); | ||
|
|
||
| if ($ruleValidator->fails()) { | ||
| foreach ($ruleValidator->messages()->toArray() as $key => $value) { | ||
| foreach (Arr::wrap($value) as $message) { | ||
| $this->validator->errors()->add($key, $message); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential error key conflicts in validation error aggregation.
The current implementation adds validation errors to the original validator using the same keys from the cloned validator. This could potentially overwrite existing errors or create conflicts if the same attribute is validated multiple times.
Consider prefixing the error keys or using a different strategy to avoid conflicts:
if ($ruleValidator->fails()) {
foreach ($ruleValidator->messages()->toArray() as $key => $value) {
foreach (Arr::wrap($value) as $message) {
- $this->validator->errors()->add($key, $message);
+ $this->validator->errors()->add($attribute . '.' . $key, $message);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function validate(string $attribute, mixed $value, Closure $fail): void | |
| { | |
| $ruleValidator = clone $this->validator; | |
| $ruleValidator->setRules( | |
| $this->buildValidationRules($attribute, $value) | |
| ); | |
| if ($ruleValidator->fails()) { | |
| foreach ($ruleValidator->messages()->toArray() as $key => $value) { | |
| foreach (Arr::wrap($value) as $message) { | |
| $this->validator->errors()->add($key, $message); | |
| } | |
| } | |
| } | |
| } | |
| public function validate(string $attribute, mixed $value, Closure $fail): void | |
| { | |
| $ruleValidator = clone $this->validator; | |
| $ruleValidator->setRules( | |
| $this->buildValidationRules($attribute, $value) | |
| ); | |
| if ($ruleValidator->fails()) { | |
| foreach ($ruleValidator->messages()->toArray() as $key => $value) { | |
| foreach (Arr::wrap($value) as $message) { | |
| $this->validator->errors()->add($attribute . '.' . $key, $message); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Rules/RestRule.php around lines 24 to 39, the validation error
aggregation uses the same keys from the cloned validator when adding errors to
the original validator, which may cause key conflicts or overwrite existing
errors. To fix this, modify the error keys by prefixing them or otherwise
altering them before adding to the original validator's errors to ensure
uniqueness and avoid conflicts.
|
|
||
| public function buildValidationRules(string $attribute, mixed $value): array | ||
| { | ||
| $request = app(RestRequest::class); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct service container dependency.
Using app(RestRequest::class) creates a hard dependency on the service container, making the class harder to test and less flexible.
Consider injecting the request through a setter method or constructor parameter:
+protected RestRequest $request;
+
+public function setRequest(RestRequest $request): static
+{
+ $this->request = $request;
+ return $this;
+}
public function buildValidationRules(string $attribute, mixed $value): array
{
- $request = app(RestRequest::class);
+ $request = $this->request;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $request = app(RestRequest::class); | |
| // src/Rules/Mutate/Mutate.php | |
| class Mutate extends RestRule | |
| { | |
| + protected RestRequest $request; | |
| + | |
| + public function setRequest(RestRequest $request): static | |
| + { | |
| + $this->request = $request; | |
| + return $this; | |
| + } | |
| public function buildValidationRules(string $attribute, mixed $value): array | |
| { | |
| - $request = app(RestRequest::class); | |
| + $request = $this->request; | |
| // ... rest of method unchanged | |
| } | |
| // ... rest of class unchanged | |
| } |
🤖 Prompt for AI Agents
In src/Rules/Mutate/Mutate.php at line 21, avoid using app(RestRequest::class)
to get the request directly from the service container. Instead, refactor the
class to accept the RestRequest instance via constructor injection or a setter
method. This will remove the hard dependency on the service container, making
the class more testable and flexible. Update the class constructor or add a
setter to receive the RestRequest and use that instance instead of calling
app().
# Conflicts: # src/Rules/MutateRules.php # src/Rules/SearchRules.php
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests