Fixup alerting and logging error in BGPServiceImpl (#10252)

* Fixup alerting and logging error in BGPServiceImpl

* Update unit tests

* Apply suggestions from code review

Co-authored-by: dahn <daan.hoogland@gmail.com>

---------

Co-authored-by: dahn <daan.hoogland@gmail.com>

server/src/main/java/com/cloud/alert/AlertManagerImpl.java

@@ -778,13 +778,14 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
         AlertVO alert = null;
         Long clusterId = cluster == null ? null : cluster.getId();
         Long podId = pod == null ? null : pod.getId();
+        long dcId = dataCenter == null ? 0L : dataCenter.getId();
         if ((alertType != AlertManager.AlertType.ALERT_TYPE_HOST) && (alertType != AlertManager.AlertType.ALERT_TYPE_USERVM)
                 && (alertType != AlertManager.AlertType.ALERT_TYPE_DOMAIN_ROUTER) && (alertType != AlertManager.AlertType.ALERT_TYPE_CONSOLE_PROXY)
                 && (alertType != AlertManager.AlertType.ALERT_TYPE_SSVM) && (alertType != AlertManager.AlertType.ALERT_TYPE_STORAGE_MISC)
                 && (alertType != AlertManager.AlertType.ALERT_TYPE_MANAGEMENT_NODE) && (alertType != AlertManager.AlertType.ALERT_TYPE_RESOURCE_LIMIT_EXCEEDED)
                 && (alertType != AlertManager.AlertType.ALERT_TYPE_UPLOAD_FAILED) && (alertType != AlertManager.AlertType.ALERT_TYPE_OOBM_AUTH_ERROR)
                 && (alertType != AlertManager.AlertType.ALERT_TYPE_HA_ACTION) && (alertType != AlertManager.AlertType.ALERT_TYPE_CA_CERT)) {
-            alert = _alertDao.getLastAlert(alertType.getType(), dataCenter.getId(), podId, clusterId);
+            alert = _alertDao.getLastAlert(alertType.getType(), dcId, podId, clusterId);
         }
 
         if (alert == null) {
@@ -794,7 +795,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi
             newAlert.setContent(content);
             newAlert.setClusterId(clusterId);
             newAlert.setPodId(podId);
-            newAlert.setDataCenterId(dataCenter.getId());
+            newAlert.setDataCenterId(dcId);
             newAlert.setSentCount(1);
             newAlert.setLastSent(new Date());
             newAlert.setName(alertType.getName());

server/src/main/java/com/cloud/bgp/BGPServiceImpl.java

@@ -255,9 +255,9 @@ public class BGPServiceImpl implements BGPService {
             netName = network.getName();
         }
 
-        LOGGER.debug("Allocating the AS Number {} to {} on zone {}", asNumber::toString,
+        String networkName = Objects.nonNull(vpcId) ? ("VPC " + vpc) : ("network " + network);
-                (Objects.nonNull(vpcId) ? "VPC " + vpc : "network " + network)::toString,
+        LOGGER.debug("Allocating the AS Number {} to {} on zone {}", asNumberVO::toString,
-                () -> dataCenterDao.findById(zoneId));
+                networkName::toString, () -> dataCenterDao.findById(zoneId));
         asNumberVO.setAllocated(true);
         asNumberVO.setAllocatedTime(new Date());
         if (Objects.nonNull(vpcId)) {

server/src/test/java/com/cloud/alert/AlertManagerImplTest.java

@@ -26,8 +26,10 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
 import org.apache.cloudstack.utils.mailing.SMTPMailSender;
 import org.apache.logging.log4j.Logger;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Mockito;
@@ -48,6 +50,12 @@ import com.cloud.host.HostVO;
 import com.cloud.host.dao.HostDao;
 import com.cloud.storage.StorageManager;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.verify;
+
 @RunWith(MockitoJUnitRunner.class)
 public class AlertManagerImplTest {
 
@@ -56,7 +64,7 @@ public class AlertManagerImplTest {
     AlertManagerImpl alertManagerImplMock;
 
     @Mock
-    AlertDao alertDaoMock;
+    AlertDao _alertDao;
 
     @Mock
     private DataCenterDao _dcDao;
@@ -88,6 +96,15 @@ public class AlertManagerImplTest {
     @Mock
     SMTPMailSender mailSenderMock;
 
+    private final String[] recipients = new String[]{"test@test.com"};
+    private final String senderAddress = "sender@test.com";
+
+    @Before
+    public void setUp() {
+        alertManagerImplMock.recipients = recipients;
+        alertManagerImplMock.senderAddress = senderAddress;
+    }
+
     private void sendMessage() {
         try {
             DataCenterVO zone = Mockito.mock(DataCenterVO.class);
@@ -100,7 +117,7 @@ public class AlertManagerImplTest {
             Mockito.when(cluster.getId()).thenReturn(1L);
             Mockito.when(_clusterDao.findById(1L)).thenReturn(cluster);
 
-            alertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, 1l, "", "");
+            alertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1L, 1L, "", "");
         } catch (UnsupportedEncodingException | MessagingException e) {
             Assert.fail();
         }
@@ -108,39 +125,69 @@ public class AlertManagerImplTest {
 
     @Test
     public void sendAlertTestSendMail() {
-        Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
+        Mockito.doReturn(null).when(_alertDao).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
                 Mockito.anyLong(), Mockito.anyLong());
-        Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any());
+        Mockito.doReturn(null).when(_alertDao).persist(any());
         alertManagerImplMock.recipients = new String[]{""};
 
         sendMessage();
 
-        Mockito.verify(alertManagerImplMock).sendMessage(Mockito.any());
+        Mockito.verify(alertManagerImplMock).sendMessage(any());
     }
 
     @Test
     public void sendAlertTestDebugLogging() {
         Mockito.doReturn(0).when(alertVOMock).getSentCount();
-        Mockito.doReturn(alertVOMock).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
+        Mockito.doReturn(alertVOMock).when(_alertDao).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
                 Mockito.anyLong(), Mockito.anyLong());
 
         sendMessage();
 
         Mockito.verify(alertManagerImplMock.logger).debug(Mockito.anyString());
-        Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(Mockito.any());
+        Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(any());
     }
 
     @Test
     public void sendAlertTestWarnLogging() {
-        Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
+        Mockito.doReturn(null).when(_alertDao).getLastAlert(Mockito.anyShort(), Mockito.anyLong(),
                 Mockito.anyLong(), Mockito.anyLong());
-        Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any());
+        Mockito.doReturn(null).when(_alertDao).persist(Mockito.any());
         alertManagerImplMock.recipients = null;
 
         sendMessage();
 
         Mockito.verify(alertManagerImplMock.logger, Mockito.times(2)).warn(Mockito.anyString());
-        Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(Mockito.any());
+        Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(any());
+    }
+
+    @Test
+    public void testSendAlertWithNullParameters() throws MessagingException, UnsupportedEncodingException {
+        // Given
+        String subject = "Test Subject";
+        String content = "Test Content";
+        AlertManager.AlertType alertType = AlertManager.AlertType.ALERT_TYPE_MEMORY;
+
+        // When
+        alertManagerImplMock.sendAlert(alertType, null, null, null, subject, content);
+
+        // Then
+        ArgumentCaptor<AlertVO> alertCaptor = ArgumentCaptor.forClass(AlertVO.class);
+        verify(_alertDao).persist(alertCaptor.capture());
+
+        AlertVO capturedAlert = alertCaptor.getValue();
+        assertNotNull("Captured alert should not be null", capturedAlert);
+        assertEquals(0L, capturedAlert.getDataCenterId());
+        assertNull(capturedAlert.getPodId());
+        assertNull(capturedAlert.getClusterId());
+        assertEquals(subject, capturedAlert.getSubject());
+        assertEquals(content, capturedAlert.getContent());
+        assertEquals(alertType.getType(), capturedAlert.getType());
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void testSendAlertWithNullAlertType() throws MessagingException, UnsupportedEncodingException {
+        // When
+        alertManagerImplMock.sendAlert(null, 0, 1L, 1L, "subject", "content");
     }
 
     @Test