diff --git a/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaAlertManagerImpl.java b/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaAlertManagerImpl.java index 924d9c4f7d6..d2b6dd75074 100644 --- a/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaAlertManagerImpl.java +++ b/framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaAlertManagerImpl.java @@ -53,6 +53,7 @@ import java.util.Set; import org.apache.cloudstack.utils.mailing.MailAddress; import org.apache.cloudstack.utils.mailing.SMTPMailProperties; import org.apache.cloudstack.utils.mailing.SMTPMailSender; +import org.apache.commons.collections.CollectionUtils; import org.apache.commons.lang3.BooleanUtils; @Component @@ -222,7 +223,7 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana final String subject = templateEngine.replace(emailTemplate.getTemplateSubject()); final String body = templateEngine.replace(emailTemplate.getTemplateBody()); try { - sendQuotaAlert(emailRecipients, subject, body); + sendQuotaAlert(account.getUuid(), emailRecipients, subject, body); emailToBeSent.sentSuccessfully(_quotaAcc); } catch (Exception e) { s_logger.error(String.format("Unable to send quota alert email (subject=%s; body=%s) to account %s (%s) recipients (%s) due to error (%s)", subject, body, account.getAccountName(), @@ -325,7 +326,7 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana } }; - protected void sendQuotaAlert(List emails, String subject, String body) { + protected void sendQuotaAlert(String accountUuid, List emails, String subject, String body) { SMTPMailProperties mailProperties = new SMTPMailProperties(); mailProperties.setSender(new MailAddress(senderAddress)); @@ -333,6 +334,12 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana mailProperties.setContent(body); mailProperties.setContentType("text/html; charset=utf-8"); + if (CollectionUtils.isEmpty(emails)) { + s_logger.warn(String.format("Account [%s] does not have users with email registered, " + + "therefore we are unable to send quota alert email with subject [%s] and content [%s].", accountUuid, subject, body)); + return; + } + Set addresses = new HashSet<>(); for (String email : emails) { addresses.add(new MailAddress(email)); diff --git a/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaAlertManagerImplTest.java b/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaAlertManagerImplTest.java index 95033ccf234..63616083402 100644 --- a/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaAlertManagerImplTest.java +++ b/framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaAlertManagerImplTest.java @@ -157,11 +157,12 @@ public class QuotaAlertManagerImplTest extends TestCase { Mockito.when(userDao.listByAccount(Mockito.anyLong())).thenReturn(users); quotaAlertManager.mailSender = Mockito.mock(SMTPMailSender.class); - Mockito.when(quotaAlertManager.mailSender.sendMail(Mockito.anyObject())).thenReturn(Boolean.TRUE); + Mockito.doNothing().when(quotaAlertManager.mailSender).sendMail(Mockito.any()); quotaAlertManager.sendQuotaAlert(email); assertTrue(email.getSendDate() != null); - Mockito.verify(quotaAlertManager, Mockito.times(1)).sendQuotaAlert(Mockito.anyListOf(String.class), Mockito.anyString(), Mockito.anyString()); + + Mockito.verify(quotaAlertManager, Mockito.times(1)).sendQuotaAlert(Mockito.anyString(), Mockito.anyListOf(String.class), Mockito.anyString(), Mockito.anyString()); Mockito.verify(quotaAlertManager.mailSender, Mockito.times(1)).sendMail(Mockito.any(SMTPMailProperties.class)); } diff --git a/server/src/main/java/com/cloud/alert/AlertManagerImpl.java b/server/src/main/java/com/cloud/alert/AlertManagerImpl.java index 5ba8c26009c..51e5051bc9b 100644 --- a/server/src/main/java/com/cloud/alert/AlertManagerImpl.java +++ b/server/src/main/java/com/cloud/alert/AlertManagerImpl.java @@ -78,10 +78,11 @@ import java.util.Set; import org.apache.cloudstack.utils.mailing.MailAddress; import org.apache.cloudstack.utils.mailing.SMTPMailProperties; import org.apache.cloudstack.utils.mailing.SMTPMailSender; +import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.math.NumberUtils; public class AlertManagerImpl extends ManagerBase implements AlertManager, Configurable { - private static final Logger s_logger = Logger.getLogger(AlertManagerImpl.class.getName()); + protected Logger logger = Logger.getLogger(AlertManagerImpl.class.getName()); private static final long INITIAL_CAPACITY_CHECK_DELAY = 30L * 1000L; // Thirty seconds expressed in milliseconds. @@ -226,7 +227,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi try { clearAlert(alertType.getType(), dataCenterId, podId); } catch (Exception ex) { - s_logger.error("Problem clearing email alert", ex); + logger.error("Problem clearing email alert", ex); } } @@ -242,11 +243,11 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi if (mailSender != null) { sendAlert(alertType, dataCenterId, podId, null, subject, body); } else { - s_logger.warn("AlertType:: " + alertType + " | dataCenterId:: " + dataCenterId + " | podId:: " + podId + + logger.warn("AlertType:: " + alertType + " | dataCenterId:: " + dataCenterId + " | podId:: " + podId + " | message:: " + subject + " | body:: " + body); } } catch (Exception ex) { - s_logger.error("Problem sending email alert", ex); + logger.error("Problem sending email alert", ex); } } @@ -261,9 +262,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi try { - if (s_logger.isDebugEnabled()) { - s_logger.debug("recalculating system capacity"); - s_logger.debug("Executing cpu/ram capacity update"); + if (logger.isDebugEnabled()) { + logger.debug("recalculating system capacity"); + logger.debug("Executing cpu/ram capacity update"); } // Calculate CPU and RAM capacities @@ -280,9 +281,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi _capacityMgr.updateCapacityForHost(host, offeringsMap); } } - if (s_logger.isDebugEnabled()) { - s_logger.debug("Done executing cpu/ram capacity update"); - s_logger.debug("Executing storage capacity update"); + if (logger.isDebugEnabled()) { + logger.debug("Done executing cpu/ram capacity update"); + logger.debug("Executing storage capacity update"); } // Calculate storage pool capacity List storagePools = _storagePoolDao.listAll(); @@ -295,9 +296,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi } } - if (s_logger.isDebugEnabled()) { - s_logger.debug("Done executing storage capacity update"); - s_logger.debug("Executing capacity updates for public ip and Vlans"); + if (logger.isDebugEnabled()) { + logger.debug("Done executing storage capacity update"); + logger.debug("Executing capacity updates for public ip and Vlans"); } List datacenters = _dcDao.listAll(); @@ -324,9 +325,9 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi } } - if (s_logger.isDebugEnabled()) { - s_logger.debug("Done capacity updates for public ip and Vlans"); - s_logger.debug("Executing capacity updates for private ip"); + if (logger.isDebugEnabled()) { + logger.debug("Done capacity updates for public ip and Vlans"); + logger.debug("Executing capacity updates for private ip"); } // Calculate new Private IP capacity @@ -338,13 +339,13 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi createOrUpdateIpCapacity(dcId, podId, Capacity.CAPACITY_TYPE_PRIVATE_IP, _configMgr.findPodAllocationState(pod)); } - if (s_logger.isDebugEnabled()) { - s_logger.debug("Done executing capacity updates for private ip"); - s_logger.debug("Done recalculating system capacity"); + if (logger.isDebugEnabled()) { + logger.debug("Done executing capacity updates for private ip"); + logger.debug("Done recalculating system capacity"); } } catch (Throwable t) { - s_logger.error("Caught exception in recalculating capacity", t); + logger.error("Caught exception in recalculating capacity", t); } } @@ -419,11 +420,11 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi @Override protected void runInContext() { try { - s_logger.debug("Running Capacity Checker ... "); + logger.debug("Running Capacity Checker ... "); checkForAlerts(); - s_logger.debug("Done running Capacity Checker ... "); + logger.debug("Done running Capacity Checker ... "); } catch (Throwable t) { - s_logger.error("Exception in CapacityChecker", t); + logger.error("Exception in CapacityChecker", t); } } } @@ -628,13 +629,13 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi } try { - if (s_logger.isDebugEnabled()) { - s_logger.debug(msgSubject); - s_logger.debug(msgContent); + if (logger.isDebugEnabled()) { + logger.debug(msgSubject); + logger.debug(msgContent); } sendAlert(alertType, dc.getId(), podId, clusterId, msgSubject, msgContent); } catch (Exception ex) { - s_logger.error("Exception in CapacityChecker", ex); + logger.error("Exception in CapacityChecker", ex); } } @@ -682,7 +683,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi public void sendAlert(AlertType alertType, long dataCenterId, Long podId, Long clusterId, String subject, String content) throws MessagingException, UnsupportedEncodingException { - s_logger.warn(String.format("alertType=[%s] dataCenterId=[%s] podId=[%s] clusterId=[%s] message=[%s].", alertType, dataCenterId, podId, clusterId, subject)); + logger.warn(String.format("alertType=[%s] dataCenterId=[%s] podId=[%s] clusterId=[%s] message=[%s].", alertType, dataCenterId, podId, clusterId, subject)); AlertVO alert = null; 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) @@ -706,14 +707,13 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi newAlert.setName(alertType.getName()); _alertDao.persist(newAlert); } else { - if (s_logger.isDebugEnabled()) { - s_logger.debug("Have already sent: " + alert.getSentCount() + " emails for alert type '" + alertType + "' -- skipping send email"); - } + logger.debug("Have already sent: " + alert.getSentCount() + " emails for alert type '" + alertType + "' -- skipping send email"); return; } - if (recipients == null) { - s_logger.warn(String.format("No recipients set in 'alert.email.addresses', skipping sending alert with subject: %s and content: %s", subject, content)); + if (ArrayUtils.isEmpty(recipients)) { + logger.warn(String.format("No recipients set in global setting 'alert.email.addresses', " + + "skipping sending alert with subject [%s] and content [%s].", subject, content)); return; } @@ -734,7 +734,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi } - private void sendMessage(SMTPMailProperties mailProps) { + protected void sendMessage(SMTPMailProperties mailProps) { _executor.execute(new Runnable() { @Override public void run() { @@ -770,7 +770,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi sendAlert(alertType, dataCenterId, podId, msg, msg); return true; } catch (Exception ex) { - s_logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg); + logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg); return false; } } diff --git a/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java new file mode 100644 index 00000000000..b78eec9a6fc --- /dev/null +++ b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java @@ -0,0 +1,99 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.alert; + +import java.io.UnsupportedEncodingException; + +import javax.mail.MessagingException; + +import org.apache.cloudstack.utils.mailing.SMTPMailSender; +import org.apache.log4j.Logger; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.alert.dao.AlertDao; + +@RunWith(MockitoJUnitRunner.class) +public class AlertManagerImplTest { + + @Spy + @InjectMocks + AlertManagerImpl alertManagerImplMock; + + @Mock + AlertDao alertDaoMock; + + @Mock + AlertVO alertVOMock; + + @Mock + Logger loggerMock; + + @Mock + SMTPMailSender mailSenderMock; + + private void sendMessage (){ + try { + alertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, 1l, "", ""); + } catch (UnsupportedEncodingException | MessagingException e) { + Assert.fail(); + } + } + + @Test + public void sendAlertTestSendMail() { + Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), + Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any()); + alertManagerImplMock.recipients = new String [] {""}; + + sendMessage(); + + Mockito.verify(alertManagerImplMock).sendMessage(Mockito.any()); + } + + @Test + public void sendAlertTestDebugLogging() { + Mockito.doReturn(0).when(alertVOMock).getSentCount(); + Mockito.doReturn(alertVOMock).when(alertDaoMock).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()); + } + + @Test + public void sendAlertTestWarnLogging() { + Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), + Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(null).when(alertDaoMock).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()); + } +} \ No newline at end of file diff --git a/usage/src/main/java/com/cloud/usage/UsageAlertManagerImpl.java b/usage/src/main/java/com/cloud/usage/UsageAlertManagerImpl.java index 3ee64e60b21..aa20f52bf1f 100644 --- a/usage/src/main/java/com/cloud/usage/UsageAlertManagerImpl.java +++ b/usage/src/main/java/com/cloud/usage/UsageAlertManagerImpl.java @@ -35,17 +35,18 @@ import java.util.Set; import org.apache.cloudstack.utils.mailing.MailAddress; import org.apache.cloudstack.utils.mailing.SMTPMailProperties; import org.apache.cloudstack.utils.mailing.SMTPMailSender; +import org.apache.commons.lang3.ArrayUtils; @Component public class UsageAlertManagerImpl extends ManagerBase implements AlertManager { - private static final Logger s_logger = Logger.getLogger(UsageAlertManagerImpl.class.getName()); + protected Logger logger = Logger.getLogger(UsageAlertManagerImpl.class.getName()); private String senderAddress; protected SMTPMailSender mailSender; protected String[] recipients; @Inject - private AlertDao _alertDao; + protected AlertDao _alertDao; @Inject private ConfigurationDao _configDao; @@ -72,7 +73,7 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager { try { clearAlert(alertType.getType(), dataCenterId, podId); } catch (Exception ex) { - s_logger.error("Problem clearing email alert", ex); + logger.error("Problem clearing email alert", ex); } } @@ -96,7 +97,7 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager { newAlert.setName(alertType.getName()); _alertDao.persist(newAlert); } else { - s_logger.debug(String.format("Have already sent: [%s] emails for alert type [%s] -- skipping send email.", alert.getSentCount(), alertType)); + logger.debug(String.format("Have already sent [%s] emails for alert type [%s] -- skipping send email.", alert.getSentCount(), alertType)); return; } @@ -106,6 +107,12 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager { mailProps.setContent(content); mailProps.setContentType("text/plain"); + if (ArrayUtils.isEmpty(recipients)) { + logger.warn(String.format("No recipients set in global setting 'alert.email.addresses', " + + "skipping sending alert with subject [%s] and content [%s].", subject, content)); + return; + } + Set addresses = new HashSet<>(); for (String recipient : recipients) { addresses.add(new MailAddress(recipient)); @@ -138,7 +145,7 @@ public class UsageAlertManagerImpl extends ManagerBase implements AlertManager { sendAlert(alertType, dataCenterId, podId, msg, msg); return true; } catch (Exception ex) { - s_logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg, ex); + logger.warn("Failed to generate an alert of type=" + alertType + "; msg=" + msg, ex); return false; } } diff --git a/usage/src/test/java/com/cloud/usage/UsageAlertManagerImplTest.java b/usage/src/test/java/com/cloud/usage/UsageAlertManagerImplTest.java new file mode 100644 index 00000000000..58c5702f311 --- /dev/null +++ b/usage/src/test/java/com/cloud/usage/UsageAlertManagerImplTest.java @@ -0,0 +1,85 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.usage; + +import org.apache.cloudstack.utils.mailing.SMTPMailSender; +import org.apache.log4j.Logger; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.alert.AlertManager; +import com.cloud.alert.AlertVO; +import com.cloud.alert.dao.AlertDao; + +@RunWith(MockitoJUnitRunner.class) +public class UsageAlertManagerImplTest { + + @Spy + @InjectMocks + UsageAlertManagerImpl usageAlertManagerImplMock; + + @Mock + AlertDao alertDaoMock; + + @Mock + AlertVO alertVOMock; + + @Mock + Logger loggerMock; + + @Mock + SMTPMailSender mailSenderMock; + + @Test + public void sendAlertTestSendMail() { + Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any()); + usageAlertManagerImplMock.recipients = new String [] {""}; + + usageAlertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, "", ""); + + Mockito.verify(usageAlertManagerImplMock.mailSender).sendMail(Mockito.any()); + } + + @Test + public void sendAlertTestDebugLogging() { + Mockito.doReturn(0).when(alertVOMock).getSentCount(); + Mockito.doReturn(alertVOMock).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), Mockito.anyLong()); + + usageAlertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, "", ""); + + Mockito.verify(usageAlertManagerImplMock.logger).debug(Mockito.anyString()); + Mockito.verify(usageAlertManagerImplMock.mailSender, Mockito.never()).sendMail(Mockito.any()); + } + + @Test + public void sendAlertTestWarnLogging() { + Mockito.doReturn(null).when(alertDaoMock).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(null).when(alertDaoMock).persist(Mockito.any()); + usageAlertManagerImplMock.recipients = null; + + usageAlertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1l, "", ""); + + Mockito.verify(usageAlertManagerImplMock.logger).warn(Mockito.anyString()); + Mockito.verify(usageAlertManagerImplMock.mailSender, Mockito.never()).sendMail(Mockito.any()); + } +} diff --git a/utils/src/main/java/org/apache/cloudstack/utils/mailing/SMTPMailSender.java b/utils/src/main/java/org/apache/cloudstack/utils/mailing/SMTPMailSender.java index 42497eb2922..6268a5fb263 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/mailing/SMTPMailSender.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/mailing/SMTPMailSender.java @@ -31,6 +31,9 @@ import javax.mail.PasswordAuthentication; import javax.mail.Session; import javax.mail.URLName; import javax.mail.internet.InternetAddress; + +import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.BooleanUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.math.NumberUtils; @@ -39,7 +42,7 @@ import org.apache.log4j.Logger; public class SMTPMailSender { - private Logger logger = Logger.getLogger(SMTPMailSender.class); + protected Logger logger = Logger.getLogger(SMTPMailSender.class); protected Session session = null; protected SMTPSessionProperties sessionProps; @@ -151,28 +154,30 @@ public class SMTPMailSender { return sessionProps; } - public boolean sendMail(SMTPMailProperties mailProps) { + public void sendMail(SMTPMailProperties mailProps) { if (session == null) { logger.error("Unable to send mail due to null session."); - return false; + return; } try { SMTPMessage message = createMessage(mailProps); + if (ArrayUtils.isEmpty(message.getAllRecipients())) { + logger.error(String.format("Unable to send mail [%s] due to the recipient list of the message being empty.", + mailProps.getSubject())); + return; + } + SMTPTransport smtpTrans = createSmtpTransport(); smtpTrans.connect(); smtpTrans.sendMessage(message, message.getAllRecipients()); smtpTrans.close(); - return true; } catch (MessagingException | UnsupportedEncodingException ex) { logger.error(String.format("Unable to send mail [%s] to the recipcients [%s].", mailProps.getSubject(), mailProps.getRecipients().toString()), ex); } - - return false; - } protected SMTPMessage createMessage(SMTPMailProperties mailProps) throws MessagingException, UnsupportedEncodingException { @@ -207,7 +212,12 @@ public class SMTPMailSender { return new SMTPTransport(session, urlName); } - protected boolean setMailRecipients(SMTPMessage message, Set recipients, String subject) throws UnsupportedEncodingException, MessagingException { + protected void setMailRecipients(SMTPMessage message, Set recipients, String subject) throws UnsupportedEncodingException, MessagingException { + if (CollectionUtils.isEmpty(recipients)) { + logger.error("Unable to set recipients due to the recipient list being empty."); + return; + } + for (MailAddress recipient : recipients) { if (StringUtils.isNotBlank(recipient.getAddress())) { try { @@ -218,14 +228,6 @@ public class SMTPMailSender { } } } - - if (recipients.isEmpty() || message.getAllRecipients().length == 0) { - logger.error("Unable to send mail due to empty list of recipients."); - logger.debug(String.format("Unable to send message [%s].", subject)); - return false; - } - - return true; } } diff --git a/utils/src/test/java/org/apache/cloudstack/utils/mailing/SMTPMailSenderTest.java b/utils/src/test/java/org/apache/cloudstack/utils/mailing/SMTPMailSenderTest.java index bb2290b3793..e42c5d4f849 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/mailing/SMTPMailSenderTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/mailing/SMTPMailSenderTest.java @@ -28,20 +28,36 @@ import java.util.Set; import javax.mail.Address; import javax.mail.MessagingException; import javax.mail.Session; +import javax.mail.internet.InternetAddress; import javax.mail.internet.MimeMessage; import junit.framework.TestCase; import org.apache.commons.lang3.time.DateUtils; import org.apache.commons.mail.EmailConstants; +import org.apache.log4j.Logger; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) public class SMTPMailSenderTest extends TestCase { private SMTPMailSender smtpMailSender; + + @Spy + @InjectMocks + private SMTPMailSender smtpMailSenderMock; + + @Mock + Logger loggerMock; + + @Mock + SMTPMessage messageMock; + private Map configsMock = Mockito.mock(Map.class); private String namespace = "test"; private String enabledProtocols = "mail.smtp.ssl.protocols"; @@ -447,8 +463,10 @@ public class SMTPMailSenderTest extends TestCase { mailProps.setSender(new MailAddress("test@test.com")); mailProps.setContent("A simple test"); mailProps.setContentType("text/plain"); + mailProps.setRecipients(new HashSet<>()); + mailProps.setSubject("Test"); - Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any()); + Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString()); SMTPMessage message = smtpMailSender.createMessage(mailProps); @@ -465,8 +483,10 @@ public class SMTPMailSenderTest extends TestCase { mailProps.setFrom(new MailAddress("test2@test2.com", "TEST2")); mailProps.setContent("A simple test"); mailProps.setContentType("text/plain"); + mailProps.setRecipients(new HashSet<>()); + mailProps.setSubject("Test"); - Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any()); + Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString()); SMTPMessage message = smtpMailSender.createMessage(mailProps); @@ -490,8 +510,10 @@ public class SMTPMailSenderTest extends TestCase { cal.set(Calendar.DAY_OF_MONTH, 1); mailProps.setSentDate(cal.getTime()); + mailProps.setRecipients(new HashSet<>()); + mailProps.setSubject("Test"); - Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any()); + Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString()); SMTPMessage message = smtpMailSender.createMessage(mailProps); assertTrue(DateUtils.truncatedEquals(cal.getTime(), message.getSentDate(), Calendar.SECOND)); @@ -511,8 +533,9 @@ public class SMTPMailSenderTest extends TestCase { mailProps.setSubject(subject); mailProps.setContent(content); mailProps.setContentType(contentType); + mailProps.setRecipients(new HashSet<>()); - Mockito.doReturn(true).when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.any(), Mockito.any()); + Mockito.doNothing().when(smtpMailSender).setMailRecipients(Mockito.any(SMTPMessage.class), Mockito.anySet(), Mockito.anyString()); SMTPMessage message = smtpMailSender.createMessage(mailProps); assertEquals(subject, message.getSubject()); @@ -531,7 +554,7 @@ public class SMTPMailSenderTest extends TestCase { recipients.add(new MailAddress("smtp.acme.org")); recipients.add(new MailAddress("smtp.acme2.org", "Coyote")); - boolean returnOfSetEmail = smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test"); + smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test"); Address[] allRecipients = messageMock.getAllRecipients(); @@ -540,8 +563,6 @@ public class SMTPMailSenderTest extends TestCase { assertEquals("\"smtp.acme.org\" ", allRecipients[0].toString()); assertEquals("Coyote ", allRecipients[1].toString()); - - assertTrue(returnOfSetEmail); } @Test @@ -556,38 +577,45 @@ public class SMTPMailSenderTest extends TestCase { recipients.add(new MailAddress("")); recipients.add(new MailAddress(" ")); - boolean returnOfSetEmail = smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test"); + smtpMailSender.setMailRecipients(messageMock, recipients, "A simple test"); Address[] allRecipients = messageMock.getAllRecipients(); int expectedNumberOfValidRecipientsConfigured = 0; assertEquals(expectedNumberOfValidRecipientsConfigured, allRecipients.length); + } - assertFalse(returnOfSetEmail); + @Test + public void setMailRecipientsTestWarnLogging() throws UnsupportedEncodingException, MessagingException { + SMTPMessage messageMock = new SMTPMessage(Mockito.mock(MimeMessage.class)); + + smtpMailSenderMock.setMailRecipients(messageMock, null, ""); + + Mockito.verify(smtpMailSenderMock.logger).error(Mockito.anyString()); } @Test public void validateSMTPMailSenderSendMailWithNullSession() { SMTPMailProperties mailProps = new SMTPMailProperties(); - boolean returnOfSendMail = smtpMailSender.sendMail(mailProps); + smtpMailSenderMock.sendMail(mailProps); - assertFalse(returnOfSendMail); + Mockito.verify(smtpMailSenderMock.logger).error(Mockito.anyString()); } @Test public void validateSMTPMailSenderSendMailWithValidSession() throws MessagingException, UnsupportedEncodingException { - smtpMailSender = smtpMailSender = Mockito.spy(smtpMailSender); SMTPMailProperties mailProps = new SMTPMailProperties(); + smtpMailSenderMock.session = Session.getDefaultInstance(Mockito.mock(Properties.class)); + Address[] recipients = {new InternetAddress("email@acme.com")}; - smtpMailSender.session = Session.getDefaultInstance(Mockito.mock(Properties.class)); + Mockito.doReturn(recipients).when(messageMock).getAllRecipients(); + Mockito.doReturn(messageMock).when(smtpMailSenderMock).createMessage(Mockito.any()); + Mockito.doReturn(Mockito.mock(SMTPTransport.class)).when(smtpMailSenderMock).createSmtpTransport(); - Mockito.doReturn(Mockito.mock(SMTPMessage.class)).when(smtpMailSender).createMessage(Mockito.any(SMTPMailProperties.class)); - Mockito.doReturn(Mockito.mock(SMTPTransport.class)).when(smtpMailSender).createSmtpTransport(); + smtpMailSenderMock.sendMail(mailProps); - boolean returnOfSendMail = smtpMailSender.sendMail(mailProps); - - assertTrue(returnOfSendMail); + Mockito.verify(smtpMailSenderMock.logger, Mockito.never()).error(Mockito.anyString()); } @Test