General: Fix an issue when trying to delete too many users at once
#9430
Conversation
github-actions
bot
added
tests
server
WalkthroughThe pull request modifies the user deletion functionality in the application. Specifically, it changes the Changes
Assessment against linked issues
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (2)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (2)
Line range hint
292-318: Consider refactoring for improved readability and maintainability.While the method's logic is sound, its structure could be improved for better readability and maintainability. Consider extracting the user deletion logic into a separate private method. This would make the main method more concise and easier to understand at a glance.
Here's a suggested refactoring:
@DeleteMapping("users") @EnforceAdmin public ResponseEntity<List<String>> deleteUsers(@RequestBody List<String> logins) { log.debug("REST request to delete {} users", logins.size()); List<String> deletedUsers = Collections.synchronizedList(new ArrayList<>()); var currentUser = userRepository.getUser(); logins.remove(currentUser.getLogin()); logins.parallelStream().forEach(login -> deleteUserIfNotCurrent(login, deletedUsers)); return ResponseEntity.ok() .headers(HeaderUtil.createAlert(applicationName, "artemisApp.userManagement.batch.deleted", String.valueOf(deletedUsers.size()))) .body(deletedUsers); } private void deleteUserIfNotCurrent(String login, List<String> deletedUsers) { try { if (!userRepository.isCurrentUser(login)) { userService.softDeleteUser(login); deletedUsers.add(login); } } catch (Exception exception) { log.error("REST request to delete user {} failed", login); log.error(exception.getMessage(), exception); } }This refactoring improves the method's structure by separating concerns and reducing the cognitive load when reading the main method.
Line range hint
292-318: Consider batch processing for very large lists of users.While the current implementation using parallel streams is efficient for moderate-sized lists, it may not be optimal for extremely large lists of users. For scenarios involving thousands of users, consider implementing a batch processing approach. This could involve processing users in smaller chunks and potentially using database batch operations for better performance and resource management.
Would you like assistance in implementing a batch processing approach for this method?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (1 hunks)
- src/main/webapp/app/core/user/admin-user.service.ts (2 hunks)
- src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/core/user/admin-user.service.ts (1)
src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (2)
src/main/webapp/app/core/user/admin-user.service.ts (1)
Line range hint
1-106: Summary: Effective implementation of bulk user deletion.The changes to the
deleteUsersmethod inAdminUserServicesuccessfully address the issue of deleting multiple users simultaneously. By moving the user logins from query parameters to the request body, the implementation avoids URL length limitations and adheres to best practices for bulk operations in RESTful APIs.The modification is localized and doesn't impact other methods in the service, maintaining the overall structure and functionality. This change aligns well with the PR objectives and coding guidelines.
To ensure that this change is properly integrated, please verify the corresponding server-side implementation:
This will help confirm that the server is prepared to handle the new request format with the user logins in the body.
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (1)
292-292: LGTM: Parameter change aligns with PR objective.The change from
@RequestParamto@RequestBodysuccessfully addresses the issue of deleting a large number of users at once by moving the user logins from URL parameters to the request body. This modification aligns well with the PR objective and resolves the HTTP failure responses encountered when dealing with an extensive list of users.
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java
Show resolved
Hide resolved
…ment/delete-inactive-users # Conflicts: # src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java
chrisknedl
dismissed stale reviews from coderabbitai[bot], ole-ve, JohannesStoehr, and dfuchss
via
7e1c19c
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (2)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (2)
Line range hint
296-301: Consider enhancing error logging for better traceability.While the current error logging is functional, it could be improved to provide more context about the specific error that occurred during user deletion.
Consider modifying the error logging as follows:
- log.error("REST request to delete user {} failed", login); - log.error(exception.getMessage(), exception); + log.error("Failed to delete user {}: {}", login, exception.getMessage()); + log.debug("Detailed stack trace for user {} deletion failure:", login, exception);This change provides a concise error message at the ERROR level while still allowing for detailed debugging when needed.
Line range hint
296-301: Consider using a more specific exception type for better error handling.The current implementation catches all exceptions, which might mask specific issues. Using a more targeted exception type could improve error handling and debugging.
Consider creating a custom exception type for user deletion failures:
public class UserDeletionException extends RuntimeException { public UserDeletionException(String login, Throwable cause) { super("Failed to delete user: " + login, cause); } }Then, modify the exception handling in the
deleteUsersmethod:try { if (!userRepository.isCurrentUser(login)) { userService.softDeleteUser(login); deletedUsers.add(login); } } -catch (Exception exception) { +catch (UserDeletionException exception) { // In order to handle all users even if some users produce exceptions, we catch them and ignore them and proceed with the remaining users - log.error("REST request to delete user {} failed", login); - log.error(exception.getMessage(), exception); + log.error("Failed to delete user {}: {}", login, exception.getMessage()); + log.debug("Detailed stack trace for user {} deletion failure:", login, exception); }This change would require updating the
userService.softDeleteUser()method to throwUserDeletionExceptionwhen appropriate. This approach provides more specific error handling while maintaining the current behavior of continuing with remaining users.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminUserResource.java (1)
283-283: LGTM: Successfully addressed the PR objective.The change from
@RequestParamto@RequestBodyfor theloginsparameter effectively moves the user logins from URL parameters to the request body. This modification aligns with the PR objective and resolves the issue of HTTP failure responses when deleting a large number of users.
krusche
changed the title
Development: Fix issue when trying to delete too many users at onceGeneral: Fix an issue when trying to delete too many users at once
Checklist
General
Server
Client
Motivation and Context
Closes #9364
Description
Instead of sending the logins of the users in the URL, we put them in the request body.
Steps for Testing
Can't be tested. The related server and client tests still work.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Summary by CodeRabbit
New Features
Bug Fixes
Documentation