RockteMQ-AI commented on code in PR #2007:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2007#discussion_r3766008088
##########
server/src/main/java/org/apache/rocketmq/studio/instance/acl/MybatisPlusAclRepository.java:
##########
@@ -111,6 +111,21 @@ public AclUserVO saveUser(AclUserVO user) {
return user;
}
+ @Override
+ public Optional<AclUserVO> replaceUser(AclUserVO user) {
+ RmqAclUser existing = userMapper.selectById(user.getId());
+ if (existing == null) {
Review Comment:
The select-then-update pattern here is not atomic, but the `updateById`
returning 0 correctly handles the race where another thread deletes between the
select and update. This is a pragmatic approach that avoids adding
database-level locking while still preventing the recreate-on-update bug.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/acl/AclService.java:
##########
@@ -134,7 +134,8 @@ public AclUserVO updateUser(UpdateAclUserDTO user) {
.clusters(user.getClusters() == null ? existing.getClusters()
: user.getClusters())
.createdAt(existing.getCreatedAt())
.build();
- AclUserVO saved = aclRepository.saveUser(merged);
+ AclUserVO saved = aclRepository.replaceUser(merged)
Review Comment:
Switching from `saveUser` (which could recreate a deleted identity) to
`replaceUser` with `orElseThrow` is the correct fix. The 404 error message
includes the user ID, which helps with debugging.
##########
server/src/test/java/org/apache/rocketmq/studio/instance/acl/AclServiceTest.java:
##########
@@ -466,7 +468,23 @@ void updateUserShouldThrowWhenUserDoesNotExist() {
.isInstanceOf(BusinessException.class)
.hasMessage("ACL user not found: missing")
.satisfies(ex -> assertThat(((BusinessException)
ex).getCode()).isEqualTo(404));
- verify(aclRepository, never()).saveUser(any(AclUserVO.class));
+ verify(aclRepository, never()).replaceUser(any(AclUserVO.class));
+ }
+
+ @Test
+ void updateUserShouldRejectConcurrentDeletion() {
Review Comment:
Good test coverage for the concurrent deletion scenario. The test verifies
both the exception and that `replaceUser` was called, ensuring the update-only
contract is enforced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
