refactor(delete): change delete method return type and behavior #404
Conversation
- Update IRepository::deleteById to return int instead of bool - Update IService::deleteById to return int and remove force parameter - Modify User model to detach roles after deletion - Update related files to reflect these changes
dosubot
bot
added
the
size:S
WalkthroughThe changes introduce several updates across multiple files, primarily focusing on modifying testing workflows and enhancing functionality within the user and repository models. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeletedEvent
User->>DeletedEvent: Trigger deletion
DeletedEvent->>User: Call deleted method
User->>User: Detach roles
User->>Repository: Call deleteById
Repository->>Database: Delete user record
Database-->>Repository: Return number of deleted records
Repository-->>User: Return number of deleted records
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
- Update swoole version from 'v5.1.3' to 'v5.1.5' in the test matrix - Keep other configurations unchanged
- Remove false parameter from deleteById method calls in MenuController, RoleController, and UserController - This change simplifies the code and removes an unnecessary parameter that was always set to false
![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
🧹 Outside diff range and nitpick comments (4)
app/Service/IService.php (2)
66-66: Update method documentation to reflect changes.The method signature has changed significantly (return type and removed parameter) but lacks PHPDoc to explain:
- The new return type meaning (number of records deleted)
- The removal of force deletion capability
Add this documentation:
+ /** + * Delete a record by its primary key + * @param mixed $id The primary key + * @return int The number of records deleted + */ public function deleteById(mixed $id): int
66-69: Consider documenting the architectural decision.The removal of force deletion from the service layer represents a significant architectural change in how deletion strategies are managed. Consider:
- Documenting this architectural decision (ADR)
- Ensuring the repository layer properly documents its deletion strategy
- Updating developer guides to reflect the new deletion pattern
app/Model/Permission/User.php (1)
87-91: Implementation looks good, consider adding error handlingThe event handler correctly detaches roles upon user deletion, preventing orphaned role associations. However, consider these improvements:
- Add error handling for the detach operation
- Consider logging the number of detached roles for monitoring purposes, especially for users with many roles
Consider enhancing the implementation with error handling and logging:
public function deleted(Deleted $event) { - $this->roles()->detach(); + try { + $count = $this->roles()->count(); + $this->roles()->detach(); + logger()->info("Successfully detached {$count} roles from user {$this->id}"); + } catch (\Exception $e) { + logger()->error("Failed to detach roles from user {$this->id}: " . $e->getMessage()); + // Consider if you want to re-throw the exception or handle it silently + } }app/Repository/IRepository.php (1)
109-112: Add PHPDoc to document the new return type behavior.The method's return type has changed from
booltoint, but there's no documentation explaining what the returned integer represents (number of deleted records).Add PHPDoc explaining the return value:
+ /** + * Delete model record(s) by ID. + * @param mixed $id Single ID or array of IDs + * @return int Number of records deleted + */ public function deleteById(mixed $id): int
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)app/Model/Permission/User.php(2 hunks)app/Repository/IRepository.php(1 hunks)app/Service/IService.php(1 hunks)
🔇 Additional comments (4)
app/Model/Permission/User.php (1)
20-20: LGTM: Import statement is correctly placed
The import for the Deleted event is properly added and aligns with the new event handler implementation.
app/Repository/IRepository.php (2)
109-112: Verify impact of return type change.
The change from bool to int return type is a breaking change that might affect existing code.
Let's verify the usage of this method in the codebase:
#!/bin/bash
# Search for deleteById method calls to assess impact
rg "->deleteById\(" -A 2
# Search for potential boolean checks on the result
rg "if.*deleteById\(" -A 2Consider the following architectural improvements:
- Add a migration period where both return types are supported
- Add a method like
deleteByIdAndConfirm(): boolfor code requiring boolean results - Update the service layer to handle the new return type appropriately
112-112: Consider removing the PHPStan ignore comment.
The PHPStan ignore comment suggests potential type-safety issues. Instead of suppressing the warning, consider explicitly handling the type safety.
Let's verify if this is a common pattern in the codebase:
Consider replacing with proper type handling:
- // @phpstan-ignore-next-line
- return $this->model::destroy($id);
+ return is_array($id)
+ ? $this->model::destroy($id)
+ : $this->model::destroy([$id]);.github/workflows/test.yml (1)
48-48: LGTM on Swoole version update.
The update from v5.1.3 to v5.1.5 is a minor version bump that maintains backward compatibility.
Given that this PR changes the return type of delete methods from bool to int, let's verify the test coverage:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/Http/Admin/Controller/Permission/MenuController.php (1)
115-117: LGTM! Consider adding error handling.The removal of the force parameter aligns with the PR objectives. However, consider these improvements:
- Add error handling to handle potential deletion failures
- Validate the input data before deletion
Consider updating the implementation:
public function delete(): Result { - $this->service->deleteById($this->getRequestData()); + $id = $this->getRequestData(); + if (empty($id)) { + return $this->error('Invalid menu ID'); + } + + $count = $this->service->deleteById($id); + if ($count === 0) { + return $this->error('Menu not found or already deleted'); + } return $this->success(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/Http/Admin/Controller/Permission/MenuController.php(1 hunks)app/Http/Admin/Controller/Permission/RoleController.php(1 hunks)app/Http/Admin/Controller/Permission/UserController.php(1 hunks)
🔇 Additional comments (3)
app/Http/Admin/Controller/Permission/MenuController.php (1)
115-115: Verify consistent implementation across controllers.
Let's verify that this change pattern is consistently applied across other controllers in the codebase.
✅ Verification successful
Implementation is consistent across controllers
The deleteById pattern is consistently implemented across all controllers in the Admin namespace:
- All controllers follow the same pattern of calling
deleteByIdon their respective services - All implementations return a success response after deletion
- The pattern is uniform across Permission controllers (User, Menu, Role) and Logstash controllers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other controller delete methods to verify consistent implementation
ast-grep --pattern 'function delete(): Result {
$$$
deleteById($$$)
$$$
}'
Length of output: 82
Script:
#!/bin/bash
# Let's try a broader search for deleteById usage in controllers
rg -l "deleteById" app/Http/Admin/Controller
# Then check the specific implementation patterns
rg -B 2 -A 2 "deleteById" app/Http/Admin/Controller
# Also search for delete method implementations
ast-grep --pattern 'public function delete() {
$$$
}'
Length of output: 3183
app/Http/Admin/Controller/Permission/RoleController.php (1)
123-123: LGTM! The change aligns with PR objectives.
The removal of the second argument from deleteById() correctly implements the interface changes described in the PR objectives.
app/Http/Admin/Controller/Permission/UserController.php (1)
130-130: Verify the service layer implementation.
Let's verify that the UserService properly handles role detachment during deletion as mentioned in the PR objectives.
✅ Verification successful
Let me try a different approach to find the service implementation and role detachment logic.
Let me check the User model's role relationship and any deletion events.
Let me check the complete content of the User model to see the full implementation of the deleted event handler we found partially.
Role detachment during user deletion is properly implemented
The verification confirms that role detachment is correctly handled:
- The User model implements a
deletedevent handler that automatically detaches all roles when a user is deleted:
public function deleted(Deleted $event)
{
$this->roles()->detach();
}- The deletion is handled through the standard repository pattern:
- UserController calls
UserService->deleteById() - UserService inherits from IService which uses the repository's
deleteById() - The model's
deletedevent is triggered during this process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify UserService implementation of deleteById and role detachment
# Test 1: Check UserService deleteById implementation
echo "Checking UserService deleteById implementation..."
ast-grep --pattern 'class UserService {
$$$
deleteById($$$) {
$$$
}
$$$
}'
# Test 2: Check for role detachment in User model
echo "Checking User model deleted event handler..."
ast-grep --pattern 'class User {
$$$
deleted($$$) {
$$$
}
$$$
}'
Length of output: 375
Script:
#!/bin/bash
# Search for UserService class implementation
echo "Searching for UserService implementation..."
rg -l "class UserService" --type php
# Once we find the file, let's look at its content
echo "\nSearching for deleteById method and role-related code..."
rg "deleteById|detach.*role|role.*detach" --type php -A 5
# Look for any role relationship or detachment in User model
echo "\nSearching for role relationships in User model..."
rg "class User.*extends.*Model" --type php -A 20
Length of output: 9791
Script:
#!/bin/bash
# Check User model's role relationship and deletion events
echo "Checking User model implementation..."
rg -A 10 "public function roles|protected static function boot" app/Model/Permission/User.php
# Check if there are any model events or observers
echo "\nChecking for model events/observers..."
rg -l "deleting|deleted.*User|UserObserver" --type php
# Look at the content of any found observer
echo "\nChecking observer implementation..."
rg "class.*Observer" --type php -A 20
Length of output: 737
Script:
#!/bin/bash
# Get the complete User model implementation
echo "Getting User model content..."
cat app/Model/Permission/User.php
# Also check the UserService implementation
echo "\nGetting UserService content..."
cat app/Service/Permission/UserService.php
Length of output: 5684
- Add `created_by` field when creating a new user - Add `updated_by` field when updating an existing user
- 修改 API 基础 URL 为 Docker 内网地址 - 优化权限批量授予请求的验证规则 - 重构当前用户类，增加菜单过滤和权限检查方法 - 更新菜单和角色关联关系，移除与 Casbin Rule 的直接关联 - 修改权限控制器和中间件以适应新的逻辑 - 更新相关测试用例
- Change VITE_APP_API_BASEURL from http://172.18.0.3:9501 to http://127.0.0.1:9501 - This update ensures that the development environment uses the local API server
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
app/Http/Admin/Request/Permission/BatchGrantPermissionsForRoleRequest.php (1)
Line range hint
17-24: Update Swagger annotations to match the code.The Swagger documentation shows
permission_idsbut the actual field name ispermissions. Additionally, it should indicate that this field is optional.Update the Property annotation to:
#[Schema( title: '批量授权角色权限', properties: [ - new Property('permission_ids', description: '权限ID', type: 'array', example: '[1,2,3]'), + new Property('permissions', description: '权限名称列表（可选）', type: 'array', example: '["menu:add","menu:edit"]'), ] )]app/Service/Permission/RoleService.php (1)
39-42: Consider adding a docblock comment.The new behavior for empty permissions array is significant and should be documented.
+ /** + * Grant permissions to a role or revoke all permissions if none specified. + * + * @param int $id The role ID + * @param array $permissionsCode Array of permission codes to grant + * @return void + */ public function batchGrantPermissionsForRole(int $id, array $permissionsCode): void { if (\count($permissionsCode) === 0) {app/Http/Admin/Middleware/PermissionMiddleware.php (1)
Line range hint
71-77: Consider caching the user() result for better performance.The current implementation calls
user()for each permission code in the loop. This could be optimized by storing the user instance before the loop.Consider this optimization:
private function handlePermission(Permission $permission): void { if ($this->currentUser->isSuperAdmin()) { return; } $operation = $permission->getOperation(); $codes = $permission->getCode(); + $user = $this->currentUser->user(); foreach ($codes as $code) { - $isMenu = $this->currentUser->user()->hasPermission($code); + $isMenu = $user->hasPermission($code); if ($operation === Permission::OPERATION_AND && ! $isMenu) { throw new BusinessException(code: ResultCode::FORBIDDEN); } if ($operation === Permission::OPERATION_OR && $isMenu) { return; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
app/Http/Admin/Controller/Permission/UserController.php(3 hunks)app/Http/Admin/Controller/PermissionController.php(2 hunks)app/Http/Admin/Middleware/PermissionMiddleware.php(1 hunks)app/Http/Admin/Request/Permission/BatchGrantPermissionsForRoleRequest.php(1 hunks)app/Http/CurrentUser.php(2 hunks)app/Model/Permission/Menu.php(2 hunks)app/Model/Permission/Role.php(0 hunks)app/Model/Permission/User.php(3 hunks)app/Service/Permission/RoleService.php(1 hunks)tests/Feature/Admin/Permission/RoleControllerTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- app/Model/Permission/Role.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Http/Admin/Controller/Permission/UserController.php
🔇 Additional comments (10)
app/Http/Admin/Request/Permission/BatchGrantPermissionsForRoleRequest.php (1)
35-35: Verify the implications of optional permissions.
The change from 'required' to 'sometimes' makes the permissions field optional. Based on the AI summary, this aligns with RoleService::batchGrantPermissionsForRole to support permission removal via empty array. However, this is a significant change in behavior that could lead to unintended permission removals if not properly handled by clients.
✅ Verification successful
Let me gather more information about the service layer implementation since the previous results didn't show the batchGrantPermissionsForRole method.
Based on the results, I can now generate the final response since I found the service implementation showing how empty permissions are handled.
The optional permissions field is safely implemented
The change from 'required' to 'sometimes' is intentionally designed and safely implemented in the service layer. The RoleService::batchGrantPermissionsForRole method explicitly handles empty permission arrays by detaching all menus, and non-empty arrays using a sync operation, which is an atomic operation in Laravel/Hyperf that safely updates permissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify how the permissions field is handled in the service layer
# and if there are any safety checks before removing permissions
# Check the implementation of batchGrantPermissionsForRole
ast-grep --pattern 'function batchGrantPermissionsForRole($$$) {
$$$
}'
# Look for any permission removal safeguards
rg -A 5 "detach|removeRole|removePermission"
Length of output: 3235
Script:
#!/bin/bash
# Search for the service method implementation and related permission granting logic
rg -A 10 "batchGrantPermissionsForRole|grantPermission" app/Service/
# Check for any permission sync methods that might be used
rg -A 5 "sync.*Permission" app/Service/
# Look for permission-related methods in the Role model
ast-grep --pattern 'class Role {
$$$
}'
Length of output: 978
app/Service/Permission/RoleService.php (1)
39-42: LGTM, but note this is a breaking change.
The implementation correctly handles empty permissions by detaching all menus and using an early return. However, this represents a change in behavior where empty permissions now explicitly revokes all access instead of potentially maintaining existing permissions.
Let's verify the impact:
app/Model/Permission/Menu.php (1)
38-38: LGTM: Property documentation is accurate and helpful.
The @property documentation for $children correctly reflects the relationship type and improves IDE support.
app/Http/Admin/Middleware/PermissionMiddleware.php (2)
Line range hint 71-77: Verify the behavior with multiple permission codes.
The permission check is part of an AND/OR operation system. We need to ensure the new hasPermission method works correctly in both scenarios.
Let's verify the permission check behavior:
#!/bin/bash
# Description: Verify the usage of Permission attributes with multiple codes
# Expected: Find examples of Permission attributes with multiple codes to validate AND/OR operations
# Search for Permission attribute usage with multiple codes
echo "=== Searching for Permission attributes with multiple codes ==="
rg "Permission\(.*code.*=.*\[" -A 2
# Search for test cases covering multiple permission scenarios
echo "=== Searching for permission-related tests ==="
fd -e php -e feature -e spec --exec rg "test.*permission" -l {}71-71: Verify the security implications of the new permission check.
The change from hasMenu to user()->hasPermission represents a significant modification in how permissions are verified. While this aligns with the PR objectives, we should ensure this change maintains or enhances security.
Let's verify the implementation of the new permission check:
✅ Verification successful
Based on the gathered information, I can now provide a final response about the security implications of the permission check change.
The permission check change is secure and properly implemented
The change from hasMenu to user()->hasPermission is a well-implemented security enhancement because:
- The
hasPermissionmethod inapp/Model/Permission/User.phpproperly checks permissions through role-menu relationships usingwhereRelation('menus', 'name', $permission)->exists() - The permission system is built on a robust role-based access control (RBAC) model where:
- Users have roles (through
roles()relationship) - Roles are associated with menus
- Permissions are verified through menu names
- Users have roles (through
- The additional
user()call ensures permissions are always checked against the authenticated user model
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of hasPermission method and its usage
# Expected: Find the hasPermission method implementation and all its usages
# Find the hasPermission method implementation
echo "=== Searching for hasPermission implementation ==="
ast-grep --pattern 'function hasPermission($_) {
$$$
}'
# Find all usages of hasPermission
echo "=== Searching for hasPermission usages ==="
rg "hasPermission\(" -A 2
Length of output: 1005
Script:
#!/bin/bash
# Let's gather more context about the permission system
# Find the User class implementation to understand the permission model
echo "=== User class implementation ==="
rg -l "class User" --type php
# Check the roles relationship and menu permissions
echo "=== Roles relationship in User model ==="
rg "roles\(\)" -A 3 --type php
# Look for any hasMenu method that was replaced
echo "=== Previous hasMenu implementation ==="
rg "hasMenu\(" -A 2 --type php
# Check the Permission class constants
echo "=== Permission class constants ==="
rg "class Permission" -A 10 --type php
Length of output: 11431
app/Model/Permission/User.php (2)
20-20: LGTM!
The Deleted event import is correctly added and necessary for the new deletion handling.
128-134: LGTM! Efficient implementation using eager loading.
The method effectively prevents N+1 queries by using eager loading and provides proper type hints.
tests/Feature/Admin/Permission/RoleControllerTest.php (1)
199-201: LGTM! Improved accuracy of HTTP status codes.
The changes correctly update the expected response codes to align with standard HTTP status codes:
- 401 (UNAUTHORIZED) when authentication is missing
- 403 (FORBIDDEN) when authorization fails
This is more accurate than the previous 422 (UNPROCESSABLE_ENTITY) as these scenarios are authentication/authorization failures rather than validation errors.
app/Http/Admin/Controller/PermissionController.php (2)
87-87: Approve selective field retrieval with verification needed.
The explicit field selection in getRoles(['name', 'code', 'remark']) is a good optimization that follows the principle of least privilege. However, please verify:
- These fields are sufficient for all frontend requirements
- No dependent code relies on other role fields
Let's verify the role field usage:
#!/bin/bash
# Search for role field usage in frontend components and other controllers
rg -t js -t php -t vue "role\.(name|code|remark)" 61-67: Verify the behavioral change in menu filtering.
The switch from permissions() to filterCurrentUser() represents a significant change in how non-admin user permissions are handled. Please ensure that:
- The
filterCurrentUser()method returns a compatible data structure with the menu list. - The permission filtering logic maintains proper access control.
Let's verify the implementation of filterCurrentUser():
✅ Verification successful
The previous search might have failed due to syntax. Let's try a broader search to locate the method and understand its implementation. We're also searching for User-related files as the method might be defined in a trait or the User model itself.
The menu filtering change is properly implemented and maintains access control
The implementation of filterCurrentUser() in app/Http/CurrentUser.php shows that:
- The method maintains proper access control by filtering menus based on user permissions (
$this->user()->getPermissions()). - It returns a compatible array structure as it:
- Takes an optional menu tree list as input
- Recursively processes children menus
- Returns filtered array data that matches the repository's list structure
The change from permissions() to filterCurrentUser() is a safe refactoring that properly handles menu access control for non-admin users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for filterCurrentUser implementation and its return type
ast-grep --pattern 'function filterCurrentUser()
{
$$$
}'
Length of output: 79
Script:
#!/bin/bash
# Let's try a broader search for filterCurrentUser method
rg "filterCurrentUser" -A 5
# Also search for the User model or related classes that might contain this method
fd -e php User
Length of output: 2611
- Remove unused import - Optimize array filtering logic - Use null coalescing operator for concise code - Add phpstan ignore comment for false positive - Improve code readability with consistent spacing and indentation
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/Http/CurrentUser.php(2 hunks)app/Service/Permission/RoleService.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Service/Permission/RoleService.php
🔇 Additional comments (5)
app/Http/CurrentUser.php (5)
19-20: LGTM: Required imports added correctly.
The new imports are necessary for the collection and array manipulation functionality used in the new methods.
48-50: Previous null check concern is still valid.
The null check issue identified in the previous review remains unaddressed.
55-56: Previous null check concern is still valid.
The null check issue for user permissions identified in the previous review remains unaddressed.
69-70: Previous null check concern is still valid.
The null check issue identified in the previous review remains unaddressed.
75-80: LGTM: Well-structured recursive implementation.
The private helper method correctly handles the recursive filtering of nested menu structures.
Summary by CodeRabbit
New Features
Changes
These updates enhance functionality and streamline user interactions with the application.