RockteMQ-AI commented on code in PR #2005:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2005#discussion_r3766011126
##########
server/src/main/java/org/apache/rocketmq/studio/ops/alert/MybatisPlusAlertRepository.java:
##########
@@ -87,14 +87,8 @@ public List<SystemAlertVO> findAlerts(String level) {
}
@Override
- public SystemAlertVO saveAlert(SystemAlertVO alert) {
- RmqSystemAlert entity = toAlertEntity(alert);
- if (entity.getId() != null && alertMapper.selectById(entity.getId())
!= null) {
- alertMapper.updateById(entity);
- } else {
- alertMapper.insert(entity);
- }
- return alert;
+ public boolean acknowledgeAlert(SystemAlertVO alert) {
Review Comment:
Replacing the check-then-insert `saveAlert` with a simple `updateById` is
the correct fix. The old pattern could silently recreate a concurrently deleted
alert; the new pattern correctly returns false when no row is updated.
##########
server/src/main/java/org/apache/rocketmq/studio/ops/alert/AlertService.java:
##########
@@ -155,10 +155,12 @@ public SystemAlertVO acknowledgeAlert(String id) {
.findFirst()
.orElseThrow(() -> new
org.apache.rocketmq.studio.common.exception.BusinessException(404, "System
alert not found: " + id));
alert.setAcknowledged(true);
- SystemAlertVO saved = alertRepository.saveAlert(alert);
- recordAudit("ACKNOWLEDGE_SYSTEM_ALERT", "SYSTEM_ALERT", saved.getId(),
null,
+ if (!alertRepository.acknowledgeAlert(alert)) {
Review Comment:
The audit record is now only written when `acknowledgeAlert` returns true,
which correctly prevents a false audit trail when the alert was concurrently
removed. The 404 error includes the alert ID for debugging.
##########
server/src/test/java/org/apache/rocketmq/studio/ops/alert/AlertServiceTest.java:
##########
@@ -584,6 +584,21 @@ void acknowledgeAlertShouldThrowWhenAlertNotFound() {
.hasMessageContaining("System alert not found: non-existent");
}
+ @Test
+ void acknowledgeAlertShouldRejectConcurrentRemoval() {
+ SystemAlertVO existing =
SystemAlertVO.builder().id("a1").level(AlertLevel.error)
+ .title("Broker Down").acknowledged(false).build();
Review Comment:
Good test — verifies both the exception and that no audit record is written
when the alert was concurrently removed. This ensures the fix prevents both the
data integrity issue and the false audit trail.
--
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]
