Merge pull request #13262 from dskarzh/fix/alarm-delete-event-order

Emit `ALARM_DELETE` event only after the delete operation completes

application/src/main/java/org/thingsboard/server/controller/AlarmController.java

@@ -157,7 +157,7 @@ public class AlarmController extends BaseController {
     @PreAuthorize("hasAnyAuthority('TENANT_ADMIN', 'CUSTOMER_USER')")
     @RequestMapping(value = "/alarm/{alarmId}", method = RequestMethod.DELETE)
     @ResponseBody
-    public Boolean deleteAlarm(@Parameter(description = ALARM_ID_PARAM_DESCRIPTION) @PathVariable(ALARM_ID) String strAlarmId) throws ThingsboardException {
+    public boolean deleteAlarm(@Parameter(description = ALARM_ID_PARAM_DESCRIPTION) @PathVariable(ALARM_ID) String strAlarmId) throws ThingsboardException {
         checkParameter(ALARM_ID, strAlarmId);
         AlarmId alarmId = new AlarmId(toUUID(strAlarmId));
         Alarm alarm = checkAlarmId(alarmId, Operation.DELETE);

application/src/main/java/org/thingsboard/server/service/entitiy/AbstractTbEntityService.java

@@ -17,10 +17,8 @@ package org.thingsboard.server.service.entitiy;
 
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
-import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.beans.factory.annotation.Value;
 import org.springframework.context.annotation.Lazy;
 import org.springframework.core.env.Environment;
 import org.thingsboard.server.cluster.TbClusterService;
@@ -31,16 +29,10 @@ import org.thingsboard.server.common.data.exception.ThingsboardException;
 import org.thingsboard.server.common.data.id.EntityId;
 import org.thingsboard.server.common.data.id.EntityIdFactory;
 import org.thingsboard.server.dao.alarm.AlarmService;
-import org.thingsboard.server.dao.asset.AssetProfileService;
-import org.thingsboard.server.dao.asset.AssetService;
 import org.thingsboard.server.dao.customer.CustomerService;
-import org.thingsboard.server.dao.device.DeviceProfileService;
-import org.thingsboard.server.dao.device.DeviceService;
 import org.thingsboard.server.dao.edge.EdgeService;
 import org.thingsboard.server.dao.entity.EntityService;
 import org.thingsboard.server.dao.model.ModelConstants;
-import org.thingsboard.server.dao.tenant.TenantService;
-import org.thingsboard.server.service.executors.DbCallbackExecutorService;
 import org.thingsboard.server.service.sync.vc.EntitiesVersionControlService;
 import org.thingsboard.server.service.telemetry.AlarmSubscriptionService;
 
@@ -55,12 +47,6 @@ public abstract class AbstractTbEntityService {
     @Autowired
     private Environment env;
 
-    @Value("${server.log_controller_error_stack_trace}")
-    @Getter
-    private boolean logControllerErrorStackTrace;
-
-    @Autowired
-    protected DbCallbackExecutorService dbExecutor;
     @Autowired(required = false)
     protected TbLogEntityActionService logEntityActionService;
     @Autowired(required = false)

application/src/main/java/org/thingsboard/server/service/entitiy/alarm/DefaultTbAlarmService.java

@@ -190,11 +190,24 @@ public class DefaultTbAlarmService extends AbstractTbEntityService implements Tb
     }
 
     @Override
-    public Boolean delete(Alarm alarm, User user) {
-        TenantId tenantId = alarm.getTenantId();
-        logEntityActionService.logEntityAction(tenantId, alarm.getOriginator(), alarm, alarm.getCustomerId(),
-                ActionType.ALARM_DELETE, user, alarm.getId());
-        return alarmSubscriptionService.deleteAlarm(tenantId, alarm.getId());
+    public boolean delete(Alarm alarm, User user) {
+        var tenantId = alarm.getTenantId();
+        var alarmId = alarm.getId();
+        var alarmOriginator = alarm.getOriginator();
+
+        boolean deleted;
+        try {
+            deleted = alarmSubscriptionService.deleteAlarm(tenantId, alarmId);
+        } catch (Exception e) {
+            logEntityActionService.logEntityAction(tenantId, emptyId(alarmOriginator.getEntityType()), ActionType.ALARM_DELETE, user, e, alarmId);
+            throw e;
+        }
+
+        if (deleted) {
+            logEntityActionService.logEntityAction(tenantId, alarmOriginator, alarm, alarm.getCustomerId(), ActionType.ALARM_DELETE, user, alarmId);
+        }
+
+        return deleted;
     }
 
     private static long getOrDefault(long ts) {

application/src/main/java/org/thingsboard/server/service/entitiy/alarm/TbAlarmService.java

@@ -43,5 +43,6 @@ public interface TbAlarmService {
 
     void unassignDeletedUserAlarms(TenantId tenantId, UserId userId, String userTitle, List<UUID> alarms, long unassignTs);
 
-    Boolean delete(Alarm alarm, User user);
+    boolean delete(Alarm alarm, User user);
+
 }

application/src/main/java/org/thingsboard/server/service/telemetry/DefaultAlarmSubscriptionService.java

@@ -115,7 +115,7 @@ public class DefaultAlarmSubscriptionService extends AbstractSubscriptionService
     }
 
     @Override
-    public Boolean deleteAlarm(TenantId tenantId, AlarmId alarmId) {
+    public boolean deleteAlarm(TenantId tenantId, AlarmId alarmId) {
         AlarmApiCallResult result = alarmService.delAlarm(tenantId, alarmId);
         onAlarmDeleted(result);
         return result.isSuccessful();

application/src/test/java/org/thingsboard/server/controller/AlarmControllerTest.java

@@ -15,6 +15,7 @@
  */
 package org.thingsboard.server.controller;
 
+import com.datastax.oss.driver.api.core.uuid.Uuids;
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.JsonNode;
 import lombok.extern.slf4j.Slf4j;
@@ -57,6 +58,8 @@ import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.mockito.Mockito.verifyNoInteractions;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 
 @Slf4j
@@ -308,6 +311,21 @@ public class AlarmControllerTest extends AbstractControllerTest {
         testNotifyEntityNever(alarm.getId(), alarm);
     }
 
+    @Test
+    public void testDeleteNonExistentAlarm() throws Exception {
+        loginTenantAdmin();
+
+        var nonExistentAlarmId = Uuids.timeBased();
+
+        Mockito.reset(tbClusterService, auditLogService);
+
+        doDelete("/api/alarm/" + nonExistentAlarmId)
+                .andExpect(status().isNotFound())
+                .andExpect(statusReason(is("Alarm with id [" + nonExistentAlarmId + "] is not found")));
+
+        verifyNoInteractions(tbClusterService, auditLogService);
+    }
+
     @Test
     public void testClearAlarmViaCustomer() throws Exception {
         loginCustomerUser();
@@ -634,12 +652,12 @@ public class AlarmControllerTest extends AbstractControllerTest {
 
         doDelete("/api/user/" + savedUser.getId().getId()).andExpect(status().isOk());
 
-         Awaitility.await().atMost(TIMEOUT, TimeUnit.SECONDS).untilAsserted(() -> {
-             AlarmInfo alarmInfo = doGet("/api/alarm/info/" + alarmId.getId(), AlarmInfo.class);
-             Assert.assertNotNull(alarmInfo);
-             Assert.assertNull(alarmInfo.getAssigneeId());
-             Assert.assertTrue(alarmInfo.getAssignTs() >= afterAssignmentTs);
-         });
+        Awaitility.await().atMost(TIMEOUT, TimeUnit.SECONDS).untilAsserted(() -> {
+            AlarmInfo alarmInfo = doGet("/api/alarm/info/" + alarmId.getId(), AlarmInfo.class);
+            Assert.assertNotNull(alarmInfo);
+            Assert.assertNull(alarmInfo.getAssigneeId());
+            Assert.assertTrue(alarmInfo.getAssignTs() >= afterAssignmentTs);
+        });
     }
 
     @Test

application/src/test/java/org/thingsboard/server/service/entitiy/alarm/DefaultTbAlarmServiceTest.java

@@ -15,15 +15,12 @@
  */
 package org.thingsboard.server.service.entitiy.alarm;
 
+import com.datastax.oss.driver.api.core.uuid.Uuids;
 import com.fasterxml.jackson.databind.node.ObjectNode;
-import lombok.extern.slf4j.Slf4j;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.mock.mockito.MockBean;
-import org.springframework.boot.test.mock.mockito.SpyBean;
-import org.springframework.test.context.ContextConfiguration;
-import org.springframework.test.context.TestPropertySource;
-import org.springframework.test.context.junit4.SpringRunner;
+import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
 import org.thingsboard.common.util.JacksonUtil;
 import org.thingsboard.server.cluster.TbClusterService;
 import org.thingsboard.server.common.data.User;
@@ -35,6 +32,9 @@ import org.thingsboard.server.common.data.alarm.AlarmInfo;
 import org.thingsboard.server.common.data.audit.ActionType;
 import org.thingsboard.server.common.data.exception.ThingsboardException;
 import org.thingsboard.server.common.data.id.AlarmId;
+import org.thingsboard.server.common.data.id.CustomerId;
+import org.thingsboard.server.common.data.id.DeviceId;
+import org.thingsboard.server.common.data.id.EntityId;
 import org.thingsboard.server.common.data.id.TenantId;
 import org.thingsboard.server.common.data.id.UserId;
 import org.thingsboard.server.dao.alarm.AlarmService;
@@ -47,7 +47,6 @@ import org.thingsboard.server.dao.edge.EdgeService;
 import org.thingsboard.server.dao.entity.EntityService;
 import org.thingsboard.server.dao.tenant.TenantService;
 import org.thingsboard.server.service.entitiy.TbLogEntityActionService;
-import org.thingsboard.server.service.executors.DbCallbackExecutorService;
 import org.thingsboard.server.service.security.permission.AccessControlService;
 import org.thingsboard.server.service.sync.vc.EntitiesVersionControlService;
 import org.thingsboard.server.service.telemetry.AlarmSubscriptionService;
@@ -55,58 +54,57 @@ import org.thingsboard.server.service.telemetry.AlarmSubscriptionService;
 import java.util.List;
 import java.util.UUID;
 
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 
-@Slf4j
-@RunWith(SpringRunner.class)
-@ContextConfiguration(classes = DefaultTbAlarmService.class)
-@TestPropertySource(properties = {
-        "server.log_controller_error_stack_trace=false"
-})
-public class DefaultTbAlarmServiceTest {
+@SpringJUnitConfig(DefaultTbAlarmService.class)
+class DefaultTbAlarmServiceTest {
 
     @MockBean
-    protected DbCallbackExecutorService dbExecutor;
+    TbLogEntityActionService logEntityActionService;
     @MockBean
-    protected TbLogEntityActionService logEntityActionService;
+    EdgeService edgeService;
     @MockBean
-    protected EdgeService edgeService;
+    AlarmService alarmService;
     @MockBean
-    protected AlarmService alarmService;
+    TbAlarmCommentService alarmCommentService;
     @MockBean
-    protected TbAlarmCommentService alarmCommentService;
+    AlarmSubscriptionService alarmSubscriptionService;
     @MockBean
-    protected AlarmSubscriptionService alarmSubscriptionService;
+    CustomerService customerService;
     @MockBean
-    protected CustomerService customerService;
+    TbClusterService tbClusterService;
     @MockBean
-    protected TbClusterService tbClusterService;
+    EntitiesVersionControlService vcService;
     @MockBean
-    private EntitiesVersionControlService vcService;
+    AccessControlService accessControlService;
     @MockBean
-    private AccessControlService accessControlService;
+    TenantService tenantService;
     @MockBean
-    private TenantService tenantService;
+    AssetService assetService;
     @MockBean
-    private AssetService assetService;
+    DeviceService deviceService;
     @MockBean
-    private DeviceService deviceService;
+    AssetProfileService assetProfileService;
     @MockBean
-    private AssetProfileService assetProfileService;
+    DeviceProfileService deviceProfileService;
     @MockBean
-    private DeviceProfileService deviceProfileService;
-    @MockBean
-    private EntityService entityService;
-    @SpyBean
+    EntityService entityService;
+
+    @Autowired
     DefaultTbAlarmService service;
 
+    TenantId tenantId = TenantId.fromUUID(Uuids.timeBased());
+    CustomerId customerId = new CustomerId(Uuids.timeBased());
+
     @Test
-    public void testSave() throws ThingsboardException {
+    void testSave() throws ThingsboardException {
         var alarm = new AlarmInfo();
         when(alarmSubscriptionService.createAlarm(any())).thenReturn(AlarmApiCallResult.builder()
                 .successful(true)
@@ -115,45 +113,99 @@ public class DefaultTbAlarmServiceTest {
                 .build());
         service.save(alarm, new User());
 
-        verify(logEntityActionService, times(1)).logEntityAction(any(), any(), any(), any(), eq(ActionType.ADDED), any());
-        verify(alarmSubscriptionService, times(1)).createAlarm(any());
+        verify(logEntityActionService).logEntityAction(any(), any(), any(), any(), eq(ActionType.ADDED), any());
+        verify(alarmSubscriptionService).createAlarm(any());
     }
 
     @Test
-    public void testAck() throws ThingsboardException {
+    void testAck() throws ThingsboardException {
         var alarm = new Alarm();
         when(alarmSubscriptionService.acknowledgeAlarm(any(), any(), anyLong()))
                 .thenReturn(AlarmApiCallResult.builder().successful(true).modified(true).alarm(new AlarmInfo()).build());
         service.ack(alarm, new User(new UserId(UUID.randomUUID())));
 
-        verify(alarmCommentService, times(1)).saveAlarmComment(any(), any(), any());
-        verify(logEntityActionService, times(1)).logEntityAction(any(), any(), any(), any(), eq(ActionType.ALARM_ACK), any());
-        verify(alarmSubscriptionService, times(1)).acknowledgeAlarm(any(), any(), anyLong());
+        verify(alarmCommentService).saveAlarmComment(any(), any(), any());
+        verify(logEntityActionService).logEntityAction(any(), any(), any(), any(), eq(ActionType.ALARM_ACK), any());
+        verify(alarmSubscriptionService).acknowledgeAlarm(any(), any(), anyLong());
     }
 
     @Test
-    public void testClear() throws ThingsboardException {
+    void testClear() throws ThingsboardException {
         var alarm = new Alarm();
         alarm.setAcknowledged(true);
         when(alarmSubscriptionService.clearAlarm(any(), any(), anyLong(), any()))
                 .thenReturn(AlarmApiCallResult.builder().successful(true).cleared(true).alarm(new AlarmInfo()).build());
         service.clear(alarm, new User(new UserId(UUID.randomUUID())));
 
-        verify(alarmCommentService, times(1)).saveAlarmComment(any(), any(), any());
-        verify(logEntityActionService, times(1)).logEntityAction(any(), any(), any(), any(), eq(ActionType.ALARM_CLEAR), any());
-        verify(alarmSubscriptionService, times(1)).clearAlarm(any(), any(), anyLong(), any());
+        verify(alarmCommentService).saveAlarmComment(any(), any(), any());
+        verify(logEntityActionService).logEntityAction(any(), any(), any(), any(), eq(ActionType.ALARM_CLEAR), any());
+        verify(alarmSubscriptionService).clearAlarm(any(), any(), anyLong(), any());
     }
 
     @Test
-    public void testDelete() {
-        service.delete(new Alarm(), new User());
+    void testDelete_deleteApiReturnsTrue_shouldLogActionAndReturnTrue() {
+        // GIVEN
+        var alarmOriginator = new DeviceId(Uuids.timeBased());
+
+        var alarm = new Alarm(new AlarmId(Uuids.timeBased()));
+        alarm.setTenantId(tenantId);
+        alarm.setCustomerId(customerId);
+        alarm.setOriginator(alarmOriginator);
+
+        var user = new User();
+
+        when(alarmSubscriptionService.deleteAlarm(tenantId, alarm.getId())).thenReturn(true);
 
-        verify(logEntityActionService, times(1)).logEntityAction(any(), any(), any(), any(), eq(ActionType.ALARM_DELETE), any(), any());
-        verify(alarmSubscriptionService, times(1)).deleteAlarm(any(), any());
+        // WHEN
+        boolean actual = service.delete(alarm, user);
+
+        assertThat(actual).isTrue();
+        verify(logEntityActionService).logEntityAction(tenantId, alarmOriginator, alarm, alarm.getCustomerId(), ActionType.ALARM_DELETE, user, alarm.getId());
+        verify(alarmSubscriptionService).deleteAlarm(tenantId, alarm.getId());
     }
 
     @Test
-    public void testUnassignAlarm() throws ThingsboardException {
+    void testDelete_deleteApiReturnsFalse_shouldNotLogActionAndReturnFalse() {
+        // GIVEN
+        var alarm = new Alarm(new AlarmId(Uuids.timeBased()));
+        alarm.setTenantId(tenantId);
+
+        var user = new User();
+
+        // WHEN
+        boolean actual = service.delete(alarm, user);
+
+        assertThat(actual).isFalse();
+        verifyNoInteractions(logEntityActionService);
+        verify(alarmSubscriptionService).deleteAlarm(tenantId, alarm.getId());
+    }
+
+    @Test
+    void testDelete_deleteApiThrowsException_shouldLogFailedActionAndRethrow() {
+        // GIVEN
+        var alarmOriginator = new DeviceId(Uuids.timeBased());
+
+        var alarm = new Alarm(new AlarmId(Uuids.timeBased()));
+        alarm.setTenantId(tenantId);
+        alarm.setOriginator(alarmOriginator);
+
+        var user = new User();
+
+        var exception = new RuntimeException("failed to delete alarm");
+
+        when(alarmSubscriptionService.deleteAlarm(tenantId, alarm.getId())).thenThrow(exception);
+
+        // WHEN-THEN
+        assertThatThrownBy(() -> service.delete(alarm, user))
+                .isInstanceOf(RuntimeException.class)
+                .hasMessage("failed to delete alarm");
+
+        verify(logEntityActionService).logEntityAction(tenantId, new DeviceId(EntityId.NULL_UUID), ActionType.ALARM_DELETE, user, exception, alarm.getId());
+        verify(alarmSubscriptionService).deleteAlarm(tenantId, alarm.getId());
+    }
+
+    @Test
+    void testUnassignAlarm() throws ThingsboardException {
         AlarmInfo alarm = new AlarmInfo();
         alarm.setId(new AlarmId(UUID.randomUUID()));
         when(alarmSubscriptionService.unassignAlarm(any(), any(), anyLong()))
@@ -174,12 +226,11 @@ public class DefaultTbAlarmServiceTest {
                 .comment(commentNode)
                 .build();
 
-        verify(alarmCommentService, times(1))
-                .saveAlarmComment(eq(alarm), eq(expectedAlarmComment), eq(user));
+        verify(alarmCommentService).saveAlarmComment(eq(alarm), eq(expectedAlarmComment), eq(user));
     }
 
     @Test
-    public void testUnassignDeletedUserAlarms() throws ThingsboardException {
+    void testUnassignDeletedUserAlarms() throws ThingsboardException {
         AlarmInfo alarm = new AlarmInfo();
         alarm.setId(new AlarmId(UUID.randomUUID()));
 
@@ -189,7 +240,7 @@ public class DefaultTbAlarmServiceTest {
         User user = new User();
         user.setEmail("testEmail@gmail.com");
         user.setId(new UserId(UUID.randomUUID()));
-        service.unassignDeletedUserAlarms(new TenantId(UUID.randomUUID()), user.getId(), user.getTitle(), List.of(alarm.getUuidId()), System.currentTimeMillis());
+        service.unassignDeletedUserAlarms(tenantId, user.getId(), user.getTitle(), List.of(alarm.getUuidId()), System.currentTimeMillis());
 
         ObjectNode commentNode = JacksonUtil.newObjectNode();
         commentNode.put("subtype", "ASSIGN");
@@ -200,9 +251,7 @@ public class DefaultTbAlarmServiceTest {
                 .comment(commentNode)
                 .build();
 
-        verify(alarmCommentService, times(1))
-                .saveAlarmComment(eq(alarm), eq(expectedAlarmComment), eq(null));
+        verify(alarmCommentService).saveAlarmComment(eq(alarm), eq(expectedAlarmComment), eq(null));
     }
 
-
 }

rule-engine/rule-engine-api/src/main/java/org/thingsboard/rule/engine/api/RuleEngineAlarmService.java

@@ -70,7 +70,7 @@ public interface RuleEngineAlarmService {
     AlarmApiCallResult unassignAlarm(TenantId tenantId, AlarmId alarmId, long assignTs);
 
     // Other API
-    Boolean deleteAlarm(TenantId tenantId, AlarmId alarmId);
+    boolean deleteAlarm(TenantId tenantId, AlarmId alarmId);
 
     ListenableFuture<Alarm> findAlarmByIdAsync(TenantId tenantId, AlarmId alarmId);
 
@@ -99,4 +99,5 @@ public interface RuleEngineAlarmService {
     PageData<AlarmData> findAlarmDataByQueryForEntities(TenantId tenantId, AlarmDataQuery query, Collection<EntityId> orderedEntityIds);
 
     PageData<EntitySubtype> findAlarmTypesByTenantId(TenantId tenantId, PageLink pageLink);
+
 }