Throw a UserAdminException when deleting a referenced group#98
Conversation
Replace the generic DataIntegrityViolationException raised when trying to delete a group that still has users with a dedicated business exception (USER_ADMIN_GROUP_STILL_REFERENCED) so the error is mapped to HTTP 422 through the global exception handler and exposes a stable business error code to clients. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR refactors group deletion error handling by replacing a low-level database exception ( ChangesError handling for referenced groups
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/useradmin/server/service/UserGroupService.java (1)
103-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd defensive exception handling for concurrent user references in delete flow.
Lines 107–112 perform a pre-check for group users, but this check is not atomic with the actual delete at line 114. Under concurrent updates, users can be assigned to a group between the check and the delete, causing
deleteAllByNameIn()to fail withDataIntegrityViolationException. UnlikeUserProfileController(lines 87–89),UserGroupControllerhas no handler for this exception, so it will surface as a 500 error instead of the expected 422.Wrap the delete call in a try-catch to convert late-arriving
DataIntegrityViolationExceptionback toUserAdminException.groupStillReferenced():Suggested fix
+import org.springframework.dao.DataIntegrityViolationException; @@ `@Transactional` public long deleteGroups(List<String> names) { adminRightService.assertIsAdmin(); @@ - return userGroupRepository.deleteAllByNameIn(names); + try { + return userGroupRepository.deleteAllByNameIn(names); + } catch (DataIntegrityViolationException e) { + // defensive fallback for concurrent references created after pre-check + String referencedName = names.stream() + .map(userGroupRepository::findByName) + .flatMap(Optional::stream) + .filter(group -> !CollectionUtils.isEmpty(group.getUsers())) + .map(GroupInfosEntity::getName) + .findFirst() + .orElse("unknown"); + throw UserAdminException.groupStillReferenced(referencedName); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/useradmin/server/service/UserGroupService.java` around lines 103 - 115, In UserGroupService.deleteGroups, make the delete atomic-safe by wrapping the call to userGroupRepository.deleteAllByNameIn(names) in a try-catch that catches DataIntegrityViolationException and rethrows UserAdminException.groupStillReferenced(...) so late-arriving user assignments convert to the expected 422; keep the existing pre-checks that use userGroupRepository.findByName(...) and adminRightService.assertIsAdmin(), and ensure DataIntegrityViolationException is imported so the catch maps DB integrity failures to UserAdminException.groupStillReferenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/java/org/gridsuite/useradmin/server/service/UserGroupService.java`:
- Around line 103-115: In UserGroupService.deleteGroups, make the delete
atomic-safe by wrapping the call to userGroupRepository.deleteAllByNameIn(names)
in a try-catch that catches DataIntegrityViolationException and rethrows
UserAdminException.groupStillReferenced(...) so late-arriving user assignments
convert to the expected 422; keep the existing pre-checks that use
userGroupRepository.findByName(...) and adminRightService.assertIsAdmin(), and
ensure DataIntegrityViolationException is imported so the catch maps DB
integrity failures to UserAdminException.groupStillReferenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 909e37c1-8569-4af4-a144-19506438ad0c
📒 Files selected for processing (5)
src/main/java/org/gridsuite/useradmin/server/controller/UserGroupController.javasrc/main/java/org/gridsuite/useradmin/server/error/UserAdminBusinessErrorCode.javasrc/main/java/org/gridsuite/useradmin/server/error/UserAdminException.javasrc/main/java/org/gridsuite/useradmin/server/error/UserAdminExceptionHandler.javasrc/main/java/org/gridsuite/useradmin/server/service/UserGroupService.java
Replace the generic DataIntegrityViolationException raised when trying to delete a profile that is still referenced by users with a dedicated business exception (USER_ADMIN_PROFILE_STILL_REFERENCED) so the error is mapped to HTTP 422 through the global exception handler and exposes a stable business error code to clients. The check is now explicit through a UserInfosRepository query rather than relying on the database foreign-key constraint failure. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Drop the explicit JPQL @query: with no top-level `profileName` field on UserInfosEntity, Spring Data resolves `ProfileName` as the nested `profile.name` traversal automatically, and the method name no longer needs the underscore form rejected by checkstyle. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Drop the explicit existence check in UserProfileService.deleteProfiles together with the USER_ADMIN_PROFILE_STILL_REFERENCED business error code, the matching factory and handler mapping, and the UserInfosRepository.existsByProfileName query. The original DataIntegrityViolationException catch in UserProfileController was unreachable, so the controller try/catch (and its now-misleading 422 ApiResponse) are kept removed. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|



Summary
Group deletion (main change)
Replace the generic
DataIntegrityViolationException(previously swallowed inUserGroupController) with a typedUserAdminExceptionmapped toHTTP 422through the globalUserAdminExceptionHandler, so the API exposes a stable business error code instead of a Spring DAO exception.USER_ADMIN_GROUP_STILL_REFERENCEDbusiness error code andUserAdminException.groupStillReferenced(name)factory.UserGroupService.deleteGroupsnow throws the typed exception (message:`Group `<name>` is still referenced by users`) instead ofDataIntegrityViolationException.try/catchremoved fromUserGroupController.deleteGroups— the global advice produces the 422.Profile deletion (dead-code cleanup)
The
try { ... } catch (DataIntegrityViolationException e)block inUserProfileController.deleteProfileswas unreachable: nothing inUserProfileService.deleteProfilesraises that exception (the previous foreign-key behaviour wasn't actually being thrown there). Removed:try/catchand the now-unusedDataIntegrityViolationExceptionimport.@ApiResponse(responseCode = "422", ...)annotation that documented a status the endpoint can no longer produce.No behavioural change for profile deletion.