From 81ee4c2073f8b7efecce2c70e05801556289f7d5 Mon Sep 17 00:00:00 2001 From: Filip Hrisafov Date: Fri, 11 Oct 2024 16:06:32 +0200 Subject: [PATCH 1/5] Flag and maybe delete messages after messages have been copied --- .../mail/AbstractMailReceiver.java | 22 +++++--- .../mail/ImapMailReceiverTests.java | 51 ++++++++++++++++++- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java b/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java index 8000586b71c..04fa6173cae 100755 --- a/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java +++ b/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java @@ -503,17 +503,27 @@ private Object byteArrayToContent(Map headers, ByteArrayOutputSt } private void postProcessFilteredMessages(Message[] filteredMessages) throws MessagingException { - setMessageFlags(filteredMessages); - - if (shouldDeleteMessages()) { - deleteMessages(filteredMessages); - } // Copy messages to cause an eager fetch if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) { + Message[] originalMessages = new Message[filteredMessages.length]; for (int i = 0; i < filteredMessages.length; i++) { - MimeMessage mimeMessage = new IntegrationMimeMessage((MimeMessage) filteredMessages[i]); + Message originalMessage = filteredMessages[i]; + originalMessages[i] = originalMessage; + MimeMessage mimeMessage = new IntegrationMimeMessage((MimeMessage) originalMessage); filteredMessages[i] = mimeMessage; } + setMessageFlagsAndMaybeDeleteMessages(originalMessages); + } + else { + setMessageFlagsAndMaybeDeleteMessages(filteredMessages); + } + } + + private void setMessageFlagsAndMaybeDeleteMessages(Message[] messages) throws MessagingException { + setMessageFlags(messages); + + if (shouldDeleteMessages()) { + deleteMessages(messages); } } diff --git a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java index 277bf0a38a9..3bb7e7f3cad 100644 --- a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java +++ b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java @@ -17,6 +17,7 @@ package org.springframework.integration.mail; import java.io.IOException; +import java.io.OutputStream; import java.lang.reflect.Field; import java.util.ArrayList; import java.util.Arrays; @@ -88,6 +89,7 @@ import org.springframework.util.MimeTypeUtils; 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.anyString; import static org.mockito.BDDMockito.given; @@ -299,6 +301,11 @@ public void receiveAndMarkAsReadDontDelete() throws Exception { private AbstractMailReceiver receiveAndMarkAsReadDontDeleteGuts(AbstractMailReceiver receiver, Message msg1, Message msg2) throws NoSuchFieldException, IllegalAccessException, MessagingException { + return receiveAndMarkAsReadDontDeleteGuts(receiver, msg1, msg2, true); + } + + private AbstractMailReceiver receiveAndMarkAsReadDontDeleteGuts(AbstractMailReceiver receiver, Message msg1, + Message msg2, boolean receive) throws NoSuchFieldException, IllegalAccessException, MessagingException { ((ImapMailReceiver) receiver).setShouldMarkMessagesAsRead(true); receiver = spy(receiver); @@ -326,7 +333,9 @@ private AbstractMailReceiver receiveAndMarkAsReadDontDeleteGuts(AbstractMailRece willAnswer(invocation -> messages).given(folder).search(any(SearchTerm.class)); willAnswer(invocation -> null).given(receiver).fetchMessages(messages); - receiver.receive(); + if (receive) { + receiver.receive(); + } return receiver; } @@ -980,6 +989,28 @@ private void setUpScheduler(ImapMailReceiver mailReceiver, ThreadPoolTaskSchedul mailReceiver.setBeanFactory(bf); } + @Test + public void receiveAndMarkAsReadDontDeleteWithThrowingWhenCopying() throws Exception { + AbstractMailReceiver receiver = new ImapMailReceiver(); + MimeMessage msg1 = GreenMailUtil.newMimeMessage("test1"); + MimeMessage greenMailMsg2 = GreenMailUtil.newMimeMessage("test2"); + TestThrowingMimeMessage msg2 = new TestThrowingMimeMessage(greenMailMsg2, 1); + receiver = receiveAndMarkAsReadDontDeleteGuts(receiver, msg1, msg2, false); + assertThatThrownBy(receiver::receive) + .isInstanceOf(MessagingException.class) + .hasMessage("IOException while copying message") + .cause() + .isInstanceOf(IOException.class) + .hasMessage("Simulated exception"); + assertThat(msg1.getFlags().contains(Flag.SEEN)).isFalse(); + assertThat(msg2.getFlags().contains(Flag.SEEN)).isFalse(); + + receiver.receive(); + assertThat(msg1.getFlags().contains(Flag.SEEN)).isTrue(); + assertThat(msg2.getFlags().contains(Flag.SEEN)).isTrue(); + verify(receiver, times(0)).deleteMessages(Mockito.any()); + } + private static class ImapSearchLoggingHandler extends Handler { private final List searches = new ArrayList<>(); @@ -1015,4 +1046,22 @@ public void close() throws SecurityException { } + private static class TestThrowingMimeMessage extends MimeMessage { + + protected final AtomicInteger exceptionsBeforeWrite; + + private TestThrowingMimeMessage(MimeMessage source, int exceptionsBeforeWrite) throws MessagingException { + super(source); + this.exceptionsBeforeWrite = new AtomicInteger(exceptionsBeforeWrite); + } + + @Override + public void writeTo(OutputStream os) throws IOException, MessagingException { + if (this.exceptionsBeforeWrite.decrementAndGet() >= 0) { + throw new IOException("Simulated exception"); + } + super.writeTo(os); + } + } + } From 45668a4e4c9e28fa8b2bb29bc70bcbefce1d9ab8 Mon Sep 17 00:00:00 2001 From: Filip Hrisafov Date: Fri, 11 Oct 2024 17:24:53 +0200 Subject: [PATCH 2/5] Apply review feedback --- .../integration/mail/AbstractMailReceiver.java | 7 +++++++ .../integration/mail/ImapMailReceiverTests.java | 1 + 2 files changed, 8 insertions(+) diff --git a/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java b/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java index 04fa6173cae..152ce74b19d 100755 --- a/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java +++ b/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java @@ -68,6 +68,7 @@ * @author Dominik Simmen * @author Yuxin Wang * @author Ngoc Nhan + * @author Filip Hrisafov */ public abstract class AbstractMailReceiver extends IntegrationObjectSupport implements MailReceiver, DisposableBean { @@ -503,6 +504,12 @@ private Object byteArrayToContent(Map headers, ByteArrayOutputSt } private void postProcessFilteredMessages(Message[] filteredMessages) throws MessagingException { + // It is more intuitive use a local variable Message[] messages = filteredMessages; + // and then call setMessageFlagsAndMaybeDeleteMessages(messages); after the if, i.e. remove the else. + // However, in setMessageFlagsAndMaybeDeleteMessages we are calling Message#setFlag and Message#setFlags + // which have different implementations in different implementations of Message. + // e.g. IMAPMessage has a different implementation of those two methods. + // Copy messages to cause an eager fetch if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) { Message[] originalMessages = new Message[filteredMessages.length]; diff --git a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java index 3bb7e7f3cad..65289841ca0 100644 --- a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java +++ b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java @@ -109,6 +109,7 @@ * @author Artem Bilan * @author Alexander Pinske * @author Dominik Simmen + * @author Filip Hrisafov */ @SpringJUnitConfig @ContextConfiguration( From b5d27105b65aab542fc871f59624288cd24ac150 Mon Sep 17 00:00:00 2001 From: Filip Hrisafov Date: Fri, 11 Oct 2024 18:52:23 +0200 Subject: [PATCH 3/5] Apply review feedback --- .../mail/AbstractMailReceiver.java | 17 +++++------------ .../mail/ImapMailReceiverTests.java | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java b/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java index 152ce74b19d..a352a0d422a 100755 --- a/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java +++ b/spring-integration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java @@ -504,26 +504,19 @@ private Object byteArrayToContent(Map headers, ByteArrayOutputSt } private void postProcessFilteredMessages(Message[] filteredMessages) throws MessagingException { - // It is more intuitive use a local variable Message[] messages = filteredMessages; - // and then call setMessageFlagsAndMaybeDeleteMessages(messages); after the if, i.e. remove the else. - // However, in setMessageFlagsAndMaybeDeleteMessages we are calling Message#setFlag and Message#setFlags - // which have different implementations in different implementations of Message. - // e.g. IMAPMessage has a different implementation of those two methods. - // Copy messages to cause an eager fetch + Message[] messages = filteredMessages; if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) { - Message[] originalMessages = new Message[filteredMessages.length]; + messages = new Message[filteredMessages.length]; for (int i = 0; i < filteredMessages.length; i++) { Message originalMessage = filteredMessages[i]; - originalMessages[i] = originalMessage; + messages[i] = originalMessage; MimeMessage mimeMessage = new IntegrationMimeMessage((MimeMessage) originalMessage); filteredMessages[i] = mimeMessage; } - setMessageFlagsAndMaybeDeleteMessages(originalMessages); - } - else { - setMessageFlagsAndMaybeDeleteMessages(filteredMessages); } + + setMessageFlagsAndMaybeDeleteMessages(messages); } private void setMessageFlagsAndMaybeDeleteMessages(Message[] messages) throws MessagingException { diff --git a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java index 65289841ca0..9df10cd228d 100644 --- a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java +++ b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java @@ -1005,10 +1005,12 @@ public void receiveAndMarkAsReadDontDeleteWithThrowingWhenCopying() throws Excep .hasMessage("Simulated exception"); assertThat(msg1.getFlags().contains(Flag.SEEN)).isFalse(); assertThat(msg2.getFlags().contains(Flag.SEEN)).isFalse(); + assertThat(msg2.getMyTestFlags().contains(Flag.SEEN)).isFalse(); receiver.receive(); assertThat(msg1.getFlags().contains(Flag.SEEN)).isTrue(); assertThat(msg2.getFlags().contains(Flag.SEEN)).isTrue(); + assertThat(msg2.getMyTestFlags().contains(Flag.SEEN)).isTrue(); verify(receiver, times(0)).deleteMessages(Mockito.any()); } @@ -1051,6 +1053,8 @@ private static class TestThrowingMimeMessage extends MimeMessage { protected final AtomicInteger exceptionsBeforeWrite; + protected final Flags myTestFlags = new Flags(); + private TestThrowingMimeMessage(MimeMessage source, int exceptionsBeforeWrite) throws MessagingException { super(source); this.exceptionsBeforeWrite = new AtomicInteger(exceptionsBeforeWrite); @@ -1063,6 +1067,21 @@ public void writeTo(OutputStream os) throws IOException, MessagingException { } super.writeTo(os); } + + @Override + public synchronized void setFlags(Flags flag, boolean set) throws MessagingException { + super.setFlags(flag, set); + if (set) { + myTestFlags.add(flag); + } + else { + myTestFlags.remove(flag); + } + } + + private Flags getMyTestFlags() { + return myTestFlags; + } } } From 62638e88d6d499e7a7bb5762bdcac7db21594ed0 Mon Sep 17 00:00:00 2001 From: Filip Hrisafov Date: Fri, 11 Oct 2024 18:55:32 +0200 Subject: [PATCH 4/5] Adjust test --- .../integration/mail/ImapMailReceiverTests.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java index 9df10cd228d..f48686cf86b 100644 --- a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java +++ b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java @@ -25,6 +25,7 @@ import java.util.Properties; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Handler; @@ -995,7 +996,7 @@ public void receiveAndMarkAsReadDontDeleteWithThrowingWhenCopying() throws Excep AbstractMailReceiver receiver = new ImapMailReceiver(); MimeMessage msg1 = GreenMailUtil.newMimeMessage("test1"); MimeMessage greenMailMsg2 = GreenMailUtil.newMimeMessage("test2"); - TestThrowingMimeMessage msg2 = new TestThrowingMimeMessage(greenMailMsg2, 1); + TestThrowingMimeMessage msg2 = new TestThrowingMimeMessage(greenMailMsg2); receiver = receiveAndMarkAsReadDontDeleteGuts(receiver, msg1, msg2, false); assertThatThrownBy(receiver::receive) .isInstanceOf(MessagingException.class) @@ -1051,18 +1052,17 @@ public void close() throws SecurityException { private static class TestThrowingMimeMessage extends MimeMessage { - protected final AtomicInteger exceptionsBeforeWrite; + protected final AtomicBoolean throwExceptionBeforeWrite = new AtomicBoolean(true); protected final Flags myTestFlags = new Flags(); - private TestThrowingMimeMessage(MimeMessage source, int exceptionsBeforeWrite) throws MessagingException { + private TestThrowingMimeMessage(MimeMessage source) throws MessagingException { super(source); - this.exceptionsBeforeWrite = new AtomicInteger(exceptionsBeforeWrite); } @Override public void writeTo(OutputStream os) throws IOException, MessagingException { - if (this.exceptionsBeforeWrite.decrementAndGet() >= 0) { + if (this.throwExceptionBeforeWrite.getAndSet(false)) { throw new IOException("Simulated exception"); } super.writeTo(os); From 7c7c48230e03bc90cbe1dd57e813ebbdab9aa1a9 Mon Sep 17 00:00:00 2001 From: Filip Hrisafov Date: Fri, 11 Oct 2024 19:33:02 +0200 Subject: [PATCH 5/5] Use spy instead of own property --- .../mail/ImapMailReceiverTests.java | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java index f48686cf86b..7f6bc7e0576 100644 --- a/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java +++ b/spring-integration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java @@ -994,7 +994,7 @@ private void setUpScheduler(ImapMailReceiver mailReceiver, ThreadPoolTaskSchedul @Test public void receiveAndMarkAsReadDontDeleteWithThrowingWhenCopying() throws Exception { AbstractMailReceiver receiver = new ImapMailReceiver(); - MimeMessage msg1 = GreenMailUtil.newMimeMessage("test1"); + MimeMessage msg1 = spy(GreenMailUtil.newMimeMessage("test1")); MimeMessage greenMailMsg2 = GreenMailUtil.newMimeMessage("test2"); TestThrowingMimeMessage msg2 = new TestThrowingMimeMessage(greenMailMsg2); receiver = receiveAndMarkAsReadDontDeleteGuts(receiver, msg1, msg2, false); @@ -1006,12 +1006,13 @@ public void receiveAndMarkAsReadDontDeleteWithThrowingWhenCopying() throws Excep .hasMessage("Simulated exception"); assertThat(msg1.getFlags().contains(Flag.SEEN)).isFalse(); assertThat(msg2.getFlags().contains(Flag.SEEN)).isFalse(); - assertThat(msg2.getMyTestFlags().contains(Flag.SEEN)).isFalse(); + verify(msg1, times(0)).setFlags(Mockito.any(), Mockito.anyBoolean()); receiver.receive(); assertThat(msg1.getFlags().contains(Flag.SEEN)).isTrue(); assertThat(msg2.getFlags().contains(Flag.SEEN)).isTrue(); - assertThat(msg2.getMyTestFlags().contains(Flag.SEEN)).isTrue(); + // msg2 is marked with the user and seen flags + verify(msg1, times(2)).setFlags(Mockito.any(), Mockito.anyBoolean()); verify(receiver, times(0)).deleteMessages(Mockito.any()); } @@ -1054,8 +1055,6 @@ private static class TestThrowingMimeMessage extends MimeMessage { protected final AtomicBoolean throwExceptionBeforeWrite = new AtomicBoolean(true); - protected final Flags myTestFlags = new Flags(); - private TestThrowingMimeMessage(MimeMessage source) throws MessagingException { super(source); } @@ -1067,21 +1066,6 @@ public void writeTo(OutputStream os) throws IOException, MessagingException { } super.writeTo(os); } - - @Override - public synchronized void setFlags(Flags flag, boolean set) throws MessagingException { - super.setFlags(flag, set); - if (set) { - myTestFlags.add(flag); - } - else { - myTestFlags.remove(flag); - } - } - - private Flags getMyTestFlags() { - return myTestFlags; - } } }