From 35304a1491bb6a615282c8ebc0328d9920228db3 Mon Sep 17 00:00:00 2001 From: Ludy Date: Wed, 21 May 2025 16:42:08 +0200 Subject: [PATCH] Enhance email error handling and expand test coverage (#3561) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description of Changes Please provide a summary of the changes, including: - **What was changed** - **EmailController**: Added a `catch (MailSendException)` block to handle invalid-address errors, log the exception, and return a 500 response with the raw error message. - **EmailServiceTest**: Added unit tests for attachment-related error cases (missing filename, null filename, missing file, null file) and invalid “to” address (null or empty), expecting `MessagingException` or `MailSendException`. - **MailConfigTest**: New test class verifying `MailConfig.java` correctly initializes `JavaMailSenderImpl` with host, port, username, password, default encoding, and SMTP properties. - **EmailControllerTest**: Refactored into a parameterized test (`shouldHandleEmailRequests`) covering four scenarios: success, generic messaging error, missing `to` parameter, and invalid address formatting. - **Why the change was made** - To ensure invalid email addresses and missing attachments are handled gracefully at the controller layer, providing clearer feedback to API clients. - To improve overall test coverage and guard against regressions in email functionality. - To enforce correct mail configuration via automated tests. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/HowToAddNewLanguage.md) (if applicable) - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md#6-testing) for more details. --- .../config/security/mail/EmailService.java | 15 ++- .../SPDF/controller/api/EmailController.java | 6 + .../security/mail/EmailServiceTest.java | 109 +++++++++++++++++ .../config/security/mail/MailConfigTest.java | 54 +++++++++ .../controller/api/EmailControllerTest.java | 111 ++++++++++-------- 5 files changed, 243 insertions(+), 52 deletions(-) create mode 100644 src/test/java/stirling/software/SPDF/config/security/mail/MailConfigTest.java diff --git a/src/main/java/stirling/software/SPDF/config/security/mail/EmailService.java b/src/main/java/stirling/software/SPDF/config/security/mail/EmailService.java index 8939fbab6..507e51599 100644 --- a/src/main/java/stirling/software/SPDF/config/security/mail/EmailService.java +++ b/src/main/java/stirling/software/SPDF/config/security/mail/EmailService.java @@ -37,8 +37,21 @@ public class EmailService { */ @Async public void sendEmailWithAttachment(Email email) throws MessagingException { - ApplicationProperties.Mail mailProperties = applicationProperties.getMail(); MultipartFile file = email.getFileInput(); + // 1) Validate recipient email address + if (email.getTo() == null || email.getTo().trim().isEmpty()) { + throw new MessagingException("Invalid Addresses"); + } + + // 2) Validate attachment + if (file == null + || file.isEmpty() + || file.getOriginalFilename() == null + || file.getOriginalFilename().isEmpty()) { + throw new MessagingException("An attachment is required to send the email."); + } + + ApplicationProperties.Mail mailProperties = applicationProperties.getMail(); // Creates a MimeMessage to represent the email MimeMessage message = mailSender.createMimeMessage(); diff --git a/src/main/java/stirling/software/SPDF/controller/api/EmailController.java b/src/main/java/stirling/software/SPDF/controller/api/EmailController.java index 6f7dd3867..dc1c9dff4 100644 --- a/src/main/java/stirling/software/SPDF/controller/api/EmailController.java +++ b/src/main/java/stirling/software/SPDF/controller/api/EmailController.java @@ -3,6 +3,7 @@ package stirling.software.SPDF.controller.api; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.mail.MailSendException; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestMapping; @@ -53,6 +54,11 @@ public class EmailController { // Calls the service to send the email with attachment emailService.sendEmailWithAttachment(email); return ResponseEntity.ok("Email sent successfully"); + } catch (MailSendException ex) { + // handles your "Invalid Addresses" case + String errorMsg = ex.getMessage(); + log.error("MailSendException: {}", errorMsg, ex); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(errorMsg); } catch (MessagingException e) { // Catches any messaging exception (e.g., invalid email address, SMTP server issues) String errorMsg = "Failed to send email: " + e.getMessage(); diff --git a/src/test/java/stirling/software/SPDF/config/security/mail/EmailServiceTest.java b/src/test/java/stirling/software/SPDF/config/security/mail/EmailServiceTest.java index 777b2b658..1781eb1bb 100644 --- a/src/test/java/stirling/software/SPDF/config/security/mail/EmailServiceTest.java +++ b/src/test/java/stirling/software/SPDF/config/security/mail/EmailServiceTest.java @@ -1,5 +1,7 @@ package stirling.software.SPDF.config.security.mail; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.*; import org.junit.jupiter.api.Test; @@ -57,4 +59,111 @@ public class EmailServiceTest { // Verify that the email was sent using mailSender verify(mailSender).send(mimeMessage); } + + @Test + void testSendEmailWithAttachmentThrowsExceptionForMissingFilename() throws MessagingException { + Email email = new Email(); + email.setTo("test@example.com"); + email.setSubject("Test Email"); + email.setBody("This is a test email."); + email.setFileInput(fileInput); + + when(fileInput.isEmpty()).thenReturn(false); + when(fileInput.getOriginalFilename()).thenReturn(""); + + try { + emailService.sendEmailWithAttachment(email); + fail("Expected MessagingException to be thrown"); + } catch (MessagingException e) { + assertEquals("An attachment is required to send the email.", e.getMessage()); + } + } + + @Test + void testSendEmailWithAttachmentThrowsExceptionForMissingFilenameNull() + throws MessagingException { + Email email = new Email(); + email.setTo("test@example.com"); + email.setSubject("Test Email"); + email.setBody("This is a test email."); + email.setFileInput(fileInput); + + when(fileInput.isEmpty()).thenReturn(false); + when(fileInput.getOriginalFilename()).thenReturn(null); + + try { + emailService.sendEmailWithAttachment(email); + fail("Expected MessagingException to be thrown"); + } catch (MessagingException e) { + assertEquals("An attachment is required to send the email.", e.getMessage()); + } + } + + @Test + void testSendEmailWithAttachmentThrowsExceptionForMissingFile() throws MessagingException { + Email email = new Email(); + email.setTo("test@example.com"); + email.setSubject("Test Email"); + email.setBody("This is a test email."); + email.setFileInput(fileInput); + + when(fileInput.isEmpty()).thenReturn(true); + + try { + emailService.sendEmailWithAttachment(email); + fail("Expected MessagingException to be thrown"); + } catch (MessagingException e) { + assertEquals("An attachment is required to send the email.", e.getMessage()); + } + } + + @Test + void testSendEmailWithAttachmentThrowsExceptionForMissingFileNull() throws MessagingException { + Email email = new Email(); + email.setTo("test@example.com"); + email.setSubject("Test Email"); + email.setBody("This is a test email."); + email.setFileInput(null); // Missing file + + try { + emailService.sendEmailWithAttachment(email); + fail("Expected MessagingException to be thrown"); + } catch (MessagingException e) { + assertEquals("An attachment is required to send the email.", e.getMessage()); + } + } + + @Test + void testSendEmailWithAttachmentThrowsExceptionForInvalidAddressNull() + throws MessagingException { + Email email = new Email(); + email.setTo(null); // Invalid address + email.setSubject("Test Email"); + email.setBody("This is a test email."); + email.setFileInput(fileInput); + + try { + emailService.sendEmailWithAttachment(email); + fail("Expected MailSendException to be thrown"); + } catch (MessagingException e) { + assertEquals("Invalid Addresses", e.getMessage()); + } + } + + @Test + void testSendEmailWithAttachmentThrowsExceptionForInvalidAddressEmpty() + throws MessagingException { + Email email = new Email(); + email.setTo(""); // Invalid address + email.setSubject("Test Email"); + email.setBody("This is a test email."); + email.setFileInput(fileInput); + + try { + emailService.sendEmailWithAttachment(email); + fail("Expected MailSendException to be thrown"); + } catch (MessagingException e) { + assertEquals("Invalid Addresses", e.getMessage()); + } + } } diff --git a/src/test/java/stirling/software/SPDF/config/security/mail/MailConfigTest.java b/src/test/java/stirling/software/SPDF/config/security/mail/MailConfigTest.java new file mode 100644 index 000000000..2e47f14e3 --- /dev/null +++ b/src/test/java/stirling/software/SPDF/config/security/mail/MailConfigTest.java @@ -0,0 +1,54 @@ +package stirling.software.SPDF.config.security.mail; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Properties; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.mail.javamail.JavaMailSender; +import org.springframework.mail.javamail.JavaMailSenderImpl; + +import stirling.software.SPDF.model.ApplicationProperties; + +class MailConfigTest { + + private ApplicationProperties.Mail mailProps; + + @BeforeEach + void initMailProperties() { + mailProps = mock(ApplicationProperties.Mail.class); + when(mailProps.getHost()).thenReturn("smtp.example.com"); + when(mailProps.getPort()).thenReturn(587); + when(mailProps.getUsername()).thenReturn("user@example.com"); + when(mailProps.getPassword()).thenReturn("password"); + } + + @Test + void shouldConfigureJavaMailSenderWithCorrectProperties() { + ApplicationProperties appProps = mock(ApplicationProperties.class); + when(appProps.getMail()).thenReturn(mailProps); + + MailConfig config = new MailConfig(appProps); + JavaMailSender sender = config.javaMailSender(); + + assertInstanceOf(JavaMailSenderImpl.class, sender); + JavaMailSenderImpl impl = (JavaMailSenderImpl) sender; + + Properties props = impl.getJavaMailProperties(); + + assertAll( + "SMTP configuration", + () -> assertEquals("smtp.example.com", impl.getHost()), + () -> assertEquals(587, impl.getPort()), + () -> assertEquals("user@example.com", impl.getUsername()), + () -> assertEquals("password", impl.getPassword()), + () -> assertEquals("UTF-8", impl.getDefaultEncoding()), + () -> assertEquals("true", props.getProperty("mail.smtp.auth")), + () -> assertEquals("true", props.getProperty("mail.smtp.starttls.enable"))); + } +} diff --git a/src/test/java/stirling/software/SPDF/controller/api/EmailControllerTest.java b/src/test/java/stirling/software/SPDF/controller/api/EmailControllerTest.java index d33f3c947..dfd68e069 100644 --- a/src/test/java/stirling/software/SPDF/controller/api/EmailControllerTest.java +++ b/src/test/java/stirling/software/SPDF/controller/api/EmailControllerTest.java @@ -1,18 +1,25 @@ package stirling.software.SPDF.controller.api; -import static org.mockito.Mockito.*; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.multipart; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.mail.MailSendException; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.setup.MockMvcBuilders; -import org.springframework.web.multipart.MultipartFile; import jakarta.mail.MessagingException; @@ -20,7 +27,7 @@ import stirling.software.SPDF.config.security.mail.EmailService; import stirling.software.SPDF.model.api.Email; @ExtendWith(MockitoExtension.class) -public class EmailControllerTest { +class EmailControllerTest { private MockMvc mockMvc; @@ -28,59 +35,61 @@ public class EmailControllerTest { @InjectMocks private EmailController emailController; - @Mock private MultipartFile fileInput; - @BeforeEach void setUp() { - // Set up the MockMvc instance for testing mockMvc = MockMvcBuilders.standaloneSetup(emailController).build(); } - @Test - void testSendEmailWithAttachmentSuccess() throws Exception { - // Create a mock Email object - Email email = new Email(); - email.setTo("test@example.com"); - email.setSubject("Test Email"); - email.setBody("This is a test email."); - email.setFileInput(fileInput); + @ParameterizedTest(name = "Case {index}: exception={0}, includeTo={1}") + @MethodSource("emailParams") + void shouldHandleEmailRequests( + Exception serviceException, + boolean includeTo, + int expectedStatus, + String expectedContent) + throws Exception { + if (serviceException == null) { + doNothing().when(emailService).sendEmailWithAttachment(any(Email.class)); + } else { + doThrow(serviceException).when(emailService).sendEmailWithAttachment(any(Email.class)); + } - // Mock the service to not throw any exception - doNothing().when(emailService).sendEmailWithAttachment(any(Email.class)); + var request = + multipart("/api/v1/general/send-email") + .file("fileInput", "dummy-content".getBytes()) + .param("subject", "Test Email") + .param("body", "This is a test email."); - // Perform the request and verify the response - mockMvc.perform( - multipart("/api/v1/general/send-email") - .file("fileInput", "dummy-content".getBytes()) - .param("to", email.getTo()) - .param("subject", email.getSubject()) - .param("body", email.getBody())) - .andExpect(status().isOk()) - .andExpect(content().string("Email sent successfully")); + if (includeTo) { + request = request.param("to", "test@example.com"); + } + + mockMvc.perform(request) + .andExpect(status().is(expectedStatus)) + .andExpect(content().string(expectedContent)); } - @Test - void testSendEmailWithAttachmentFailure() throws Exception { - // Create a mock Email object - Email email = new Email(); - email.setTo("test@example.com"); - email.setSubject("Test Email"); - email.setBody("This is a test email."); - email.setFileInput(fileInput); - - // Mock the service to throw a MessagingException - doThrow(new MessagingException("Failed to send email")) - .when(emailService) - .sendEmailWithAttachment(any(Email.class)); - - // Perform the request and verify the response - mockMvc.perform( - multipart("/api/v1/general/send-email") - .file("fileInput", "dummy-content".getBytes()) - .param("to", email.getTo()) - .param("subject", email.getSubject()) - .param("body", email.getBody())) - .andExpect(status().isInternalServerError()) - .andExpect(content().string("Failed to send email: Failed to send email")); + static Stream emailParams() { + return Stream.of( + // success case + Arguments.of(null, true, 200, "Email sent successfully"), + // generic messaging error + Arguments.of( + new MessagingException("Failed to send email"), + true, + 500, + "Failed to send email: Failed to send email"), + // missing 'to' results in MailSendException + Arguments.of( + new MailSendException("Invalid Addresses"), + false, + 500, + "Invalid Addresses"), + // invalid email address formatting + Arguments.of( + new MessagingException("Invalid Addresses"), + true, + 500, + "Failed to send email: Invalid Addresses")); } }