Skip to content

Flag and maybe delete messages after messages have been copied #9546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 11, 2024
Merged

Flag and maybe delete messages after messages have been copied #9546

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
81ee4c2
Flag and maybe delete messages after messages have been copied
filiphr Oct 11, 2024
45668a4
Apply review feedback
filiphr Oct 11, 2024
b5d2710
Apply review feedback
filiphr Oct 11, 2024
62638e8
Adjust test
filiphr Oct 11, 2024
7c7c482
Use spy instead of own property
filiphr Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions ...gration-mail/src/main/java/org/springframework/integration/mail/AbstractMailReceiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -503,17 +504,33 @@ private Object byteArrayToContent(Map<String, Object> headers, ByteArrayOutputSt
}

private void postProcessFilteredMessages(Message[] filteredMessages) throws MessagingException {
setMessageFlags(filteredMessages);
// 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.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May we consider a local method variable for Message[] instead of else branch with the same method call?

I mean something like:

Message[] messages = filteredMessages;
if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) {
    messages = new Message[filteredMessages.length];
    ...
}
setMessageFlagsAndMaybeDeleteMessages(messages);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wanted to do that initially as well. However, in the if we are creating a new Message of the IntegrationMimeMessage. This means that when we call setMessageFlagsAndMaybeDeleteMessages and we invoke Message#setFlags the actual implementation differs. If we look into the IMAPMessage#setFlags the implementation looks like

synchronized (getMessageCacheLock()) {
    try {
        IMAPProtocol p = getProtocol();
        checkExpunged(); // Insure that this message is not expunged
        p.storeFlags(getSequenceNumber(), flag, set);
    } catch (ConnectionException cex) {
        throw new FolderClosedException(folder, cex.getMessage());
    } catch (ProtocolException pex) {
        throw new MessagingException(pex.getMessage(), pex);
    }
}

whereas the MimeMessage#setFlags looks like

if (set)
    flags.add(flag);
else
    flags.remove(flag);

I am going to add a comment there to explain why it is done in the way I did it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure why does that matter since you call setMessageFlagsAndMaybeDeleteMessages in both cases? So, one local variable for both cases feels right.
Ok, I’ll look into that locally on merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artembilan it matters a lot because what will be invoked in the end is not the same. The behaviour of the setFlags will change due to the fact that there will be different implementations.

Prior to this PR the setFlags was invoked on the original messages received from the folder. The goal of the PR is to still invoke the setFlags on the original messages. However, instead of doing it before copying it, to do it after it has been copied.

I have an idea for a test case. I'll add it to illustrate the problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still failing to understand why this code is not OK with you:

	private void postProcessFilteredMessages(Message[] filteredMessages) throws MessagingException {
		Message[] messagesToProcess = filteredMessages;

		// Copy messages to cause an eager fetch
		if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) {
			messagesToProcess = new Message[filteredMessages.length];
			for (int i = 0; i < filteredMessages.length; i++) {
				Message originalMessage = filteredMessages[i];
				messagesToProcess[i] = originalMessage;
				MimeMessage mimeMessage = new IntegrationMimeMessage((MimeMessage) originalMessage);
				filteredMessages[i] = mimeMessage;
			}
		}

		setMessageFlagsAndMaybeDeleteMessages(messagesToProcess);
	}

You see, we still fulfill that messagesToProcess with original messages.
And then call setMessageFlagsAndMaybeDeleteMessages() only against a single source of truth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @artembilan, I misunderstood you. I know what you mean now. The messages will be the original messages populated in the loop.

I adjusted the PR. Please correct me if I am wrong.

}
}

private void setMessageFlagsAndMaybeDeleteMessages(Message[] messages) throws MessagingException {
setMessageFlags(messages);

if (shouldDeleteMessages()) {
deleteMessages(messages);
}
}

Expand Down
52 changes: 51 additions & 1 deletion ...ration-mail/src/test/java/org/springframework/integration/mail/ImapMailReceiverTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -107,6 +109,7 @@
* @author Artem Bilan
* @author Alexander Pinske
* @author Dominik Simmen
* @author Filip Hrisafov
*/
@SpringJUnitConfig
@ContextConfiguration(
Expand Down Expand Up @@ -299,6 +302,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);
Expand Down Expand Up @@ -326,7 +334,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;
}

Expand Down Expand Up @@ -980,6 +990,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<String> searches = new ArrayList<>();
Expand Down Expand Up @@ -1015,4 +1047,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like just AtomicBoolean will be enough without any extra ctor arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I've adjusted it

throw new IOException("Simulated exception");
}
super.writeTo(os);
}
}

}