Skip to content

Commit

Permalink
MAILBOX-363 MessageMoveEvent should not reference full messages
Browse files Browse the repository at this point in the history 
It makes more sens to reference only their MessageId
  • Loading branch information
@chibenwa
chibenwa committed Dec 18, 2018
1 parent d1fdf8b commit 9cf3186
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 86 deletions.
14 changes: 7 additions & 7 deletions ...pamassassin/src/main/java/org/apache/james/mailbox/spamassassin/SpamAssassinListener.java
View file
Expand Up @@ -83,7 +83,7 @@ public void event(Event event) {
try {
MailboxSession session = mailboxManager.createSystemSession(getClass().getCanonicalName());
if (event instanceof MessageMoveEvent) {
handleMessageMove(event, (MessageMoveEvent) event);
handleMessageMove(event, session, (MessageMoveEvent) event);
}
if (event instanceof Added) {
handleAdded(event, session, (Added) event);
Expand All @@ -107,14 +107,14 @@ private void handleAdded(Event event, MailboxSession session, Added addedEvent)
}
}

private void handleMessageMove(Event event, MessageMoveEvent messageMoveEvent) {
private void handleMessageMove(Event event, MailboxSession session, MessageMoveEvent messageMoveEvent) throws MailboxException {
if (isMessageMovedToSpamMailbox(messageMoveEvent)) {
LOGGER.debug("Spam event detected");
ImmutableList<InputStream> messages = retrieveMessages(messageMoveEvent);
ImmutableList<InputStream> messages = retrieveMessages(messageMoveEvent, session);
spamAssassin.learnSpam(messages, event.getUser());
}
if (isMessageMovedOutOfSpamMailbox(messageMoveEvent)) {
ImmutableList<InputStream> messages = retrieveMessages(messageMoveEvent);
ImmutableList<InputStream> messages = retrieveMessages(messageMoveEvent, session);
spamAssassin.learnHam(messages, event.getUser());
}
}
Expand All @@ -138,9 +138,9 @@ private boolean isAppendedToInbox(Added addedEvent) {
}
}

private ImmutableList<InputStream> retrieveMessages(MessageMoveEvent messageMoveEvent) {
return messageMoveEvent.getMessages()
.values()
private ImmutableList<InputStream> retrieveMessages(MessageMoveEvent messageMoveEvent, MailboxSession session) throws MailboxException {
return mapperFactory.getMessageIdMapper(session)
.find(messageMoveEvent.getMessageIds(), MessageMapper.FetchType.Full)
.stream()
.map(Throwing.function(Message::getFullContent))
.collect(Guavate.toImmutableList());
Expand Down
49 changes: 20 additions & 29 deletions ...ssassin/src/test/java/org/apache/james/mailbox/spamassassin/SpamAssassinListenerTest.java
View file
Expand Up @@ -59,7 +59,6 @@
import org.junit.Before;
import org.junit.Test;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;

public class SpamAssassinListenerTest {
Expand All @@ -68,6 +67,7 @@ public class SpamAssassinListenerTest {
private static final MailboxSession MAILBOX_SESSION = MailboxSessionUtil.create(USER);
private static final int UID_VALIDITY = 43;
private static final MessageUid UID = MessageUid.of(45);
private static final TestMessageId MESSAGE_ID = TestMessageId.of(45);

private SpamAssassin spamAssassin;
private SpamAssassinListener listener;
Expand Down Expand Up @@ -105,60 +105,56 @@ public void setup() throws Exception {
}

@Test
public void isEventOnSpamMailboxShouldReturnFalseWhenMessageIsMovedToANonSpamMailbox() throws Exception {
public void isEventOnSpamMailboxShouldReturnFalseWhenMessageIsMovedToANonSpamMailbox() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(mailboxId1)
.targetMailboxIds(mailboxId2)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox2)))
.messageId(MESSAGE_ID)
.build();

assertThat(listener.isMessageMovedToSpamMailbox(messageMoveEvent)).isFalse();
}

@Test
public void isEventOnSpamMailboxShouldReturnTrueWhenMailboxIsSpam() throws Exception {
public void isEventOnSpamMailboxShouldReturnTrueWhenMailboxIsSpam() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(mailboxId1)
.targetMailboxIds(spamMailboxId)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox1)))
.messageId(MESSAGE_ID)
.build();

assertThat(listener.isMessageMovedToSpamMailbox(messageMoveEvent)).isTrue();
}

@Test
public void isEventOnSpamMailboxShouldReturnFalseWhenMailboxIsSpamOtherCase() throws Exception {
public void isEventOnSpamMailboxShouldReturnFalseWhenMailboxIsSpamOtherCase() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(mailboxId1)
.targetMailboxIds(spamCapitalMailboxId)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox1)))
.messageId(MESSAGE_ID)
.build();

assertThat(listener.isMessageMovedToSpamMailbox(messageMoveEvent)).isFalse();
}

@Test
public void eventShouldCallSpamAssassinSpamLearningWhenTheEventMatches() throws Exception {
public void eventShouldCallSpamAssassinSpamLearningWhenTheEventMatches() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(mailboxId1)
.targetMailboxIds(spamMailboxId)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox1)))
.messageId(MESSAGE_ID)
.build();

listener.event(messageMoveEvent);
Expand All @@ -167,75 +163,70 @@ public void eventShouldCallSpamAssassinSpamLearningWhenTheEventMatches() throws
}

@Test
public void isMessageMovedOutOfSpamMailboxShouldReturnFalseWhenMessageMovedBetweenNonSpamMailboxes() throws Exception {
public void isMessageMovedOutOfSpamMailboxShouldReturnFalseWhenMessageMovedBetweenNonSpamMailboxes() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(mailboxId1)
.targetMailboxIds(mailboxId2)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox2)))
.messageId(MESSAGE_ID)
.build();

assertThat(listener.isMessageMovedOutOfSpamMailbox(messageMoveEvent)).isFalse();
}

@Test
public void isMessageMovedOutOfSpamMailboxShouldReturnFalseWhenMessageMovedOutOfCapitalSpamMailbox() throws Exception {
public void isMessageMovedOutOfSpamMailboxShouldReturnFalseWhenMessageMovedOutOfCapitalSpamMailbox() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(spamCapitalMailboxId)
.targetMailboxIds(mailboxId2)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox2)))
.messageId(MESSAGE_ID)
.build();

assertThat(listener.isMessageMovedOutOfSpamMailbox(messageMoveEvent)).isFalse();
}

@Test
public void isMessageMovedOutOfSpamMailboxShouldReturnTrueWhenMessageMovedOutOfSpamMailbox() throws Exception {
public void isMessageMovedOutOfSpamMailboxShouldReturnTrueWhenMessageMovedOutOfSpamMailbox() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(spamMailboxId)
.targetMailboxIds(mailboxId2)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox2)))
.messageId(MESSAGE_ID)
.build();

assertThat(listener.isMessageMovedOutOfSpamMailbox(messageMoveEvent)).isTrue();
}

@Test
public void isMessageMovedOutOfSpamMailboxShouldReturnFalseWhenMessageMovedToTrash() throws Exception {
public void isMessageMovedOutOfSpamMailboxShouldReturnFalseWhenMessageMovedToTrash() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(spamMailboxId)
.targetMailboxIds(trashMailboxId)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox2)))
.messageId(MESSAGE_ID)
.build();

assertThat(listener.isMessageMovedOutOfSpamMailbox(messageMoveEvent)).isFalse();
}

@Test
public void eventShouldCallSpamAssassinHamLearningWhenTheEventMatches() throws Exception {
public void eventShouldCallSpamAssassinHamLearningWhenTheEventMatches() {
MessageMoveEvent messageMoveEvent = MessageMoveEvent.builder()
.session(MAILBOX_SESSION)
.messageMoves(MessageMoves.builder()
.previousMailboxIds(spamMailboxId)
.targetMailboxIds(mailboxId1)
.build())
.messages(ImmutableMap.of(UID,
createMessage(mailbox1)))
.messageId(MESSAGE_ID)
.build();

listener.event(messageMoveEvent);
Expand Down Expand Up @@ -274,7 +265,7 @@ private SimpleMailboxMessage createMessage(Mailbox mailbox) throws MailboxExcept
int size = 45;
int bodyStartOctet = 25;
byte[] content = "Subject: test\r\n\r\nBody\r\n".getBytes(StandardCharsets.UTF_8);
SimpleMailboxMessage message = new SimpleMailboxMessage(TestMessageId.of(58), new Date(),
SimpleMailboxMessage message = new SimpleMailboxMessage(MESSAGE_ID, new Date(),
size, bodyStartOctet, new SharedByteArrayInputStream(content), new Flags(), new PropertyBuilder(),
mailbox.getMailboxId());
MessageMetaData messageMetaData = mapperFactory.createMessageMapper(null).add(mailbox, message);
Expand Down
3 changes: 1 addition & 2 deletions mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageIdManager.java
View file
Expand Up @@ -70,7 +70,6 @@
import com.github.fge.lambdas.functions.ThrowingFunction;
import com.github.steveash.guavate.Guavate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;

Expand Down Expand Up @@ -281,7 +280,7 @@ private void applyMessageMoveNoMailboxChecks(MailboxSession mailboxSession, List

addMessageToMailboxes(mailboxMessage, messageMoves.addedMailboxIds(), mailboxSession);
removeMessageFromMailboxes(mailboxMessage, messageMoves.removedMailboxIds(), mailboxSession);
dispatcher.moved(mailboxSession, messageMoves, ImmutableMap.of(mailboxMessage.getUid(), mailboxMessage));
dispatcher.moved(mailboxSession, messageMoves, ImmutableList.of(mailboxMessage.getMessageId()));
}

private void removeMessageFromMailboxes(MailboxMessage message, Set<MailboxId> mailboxesToRemove, MailboxSession mailboxSession) throws MailboxException {
Expand Down
13 changes: 6 additions & 7 deletions mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java
View file
Expand Up @@ -90,7 +90,6 @@
import org.slf4j.LoggerFactory;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;

/**
* Base class for {@link org.apache.james.mailbox.MessageManager}
Expand Down Expand Up @@ -735,17 +734,17 @@ private SortedMap<MessageUid, MessageMetaData> copy(MessageRange set, StoreMessa

SortedMap<MessageUid, MessageMetaData> copiedUids = collectMetadata(to.copy(originalRows, session));

ImmutableMap.Builder<MessageUid, MailboxMessage> messagesMap = ImmutableMap.builder();
ImmutableList.Builder<MessageId> messageIds = ImmutableList.builder();
for (MailboxMessage message : originalRows.getEntriesSeen()) {
messagesMap.put(message.getUid(), immutableMailboxMessageFactory.from(to.getMailboxEntity().getMailboxId(), message));
messageIds.add(message.getMessageId());
}
dispatcher.added(session, copiedUids, to.getMailboxEntity());
dispatcher.moved(session,
MessageMoves.builder()
.previousMailboxIds(getMailboxEntity().getMailboxId())
.targetMailboxIds(to.getMailboxEntity().getMailboxId(), getMailboxEntity().getMailboxId())
.build(),
messagesMap.build());
messageIds.build());
return copiedUids;
}

Expand All @@ -755,9 +754,9 @@ private SortedMap<MessageUid, MessageMetaData> move(MessageRange set, StoreMessa
MoveResult moveResult = to.move(originalRows, session);
SortedMap<MessageUid, MessageMetaData> moveUids = collectMetadata(moveResult.getMovedMessages());

ImmutableMap.Builder<MessageUid, MailboxMessage> messagesMap = ImmutableMap.builder();
ImmutableList.Builder<MessageId> messageIds = ImmutableList.builder();
for (MailboxMessage message : originalRows.getEntriesSeen()) {
messagesMap.put(message.getUid(), immutableMailboxMessageFactory.from(to.getMailboxEntity().getMailboxId(), message));
messageIds.add(message.getMessageId());
}
dispatcher.added(session, moveUids, to.getMailboxEntity());
dispatcher.expunged(session, collectMetadata(moveResult.getOriginalMessages()), getMailboxEntity());
Expand All @@ -766,7 +765,7 @@ private SortedMap<MessageUid, MessageMetaData> move(MessageRange set, StoreMessa
.previousMailboxIds(getMailboxEntity().getMailboxId())
.targetMailboxIds(to.getMailboxEntity().getMailboxId())
.build(),
messagesMap.build());
messageIds.build());
return moveUids;
}

Expand Down
7 changes: 4 additions & 3 deletions mailbox/store/src/main/java/org/apache/james/mailbox/store/event/EventFactory.java
View file
Expand Up @@ -19,6 +19,7 @@

package org.apache.james.mailbox.store.event;

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.SortedMap;
Expand All @@ -32,12 +33,12 @@
import org.apache.james.mailbox.acl.ACLDiff;
import org.apache.james.mailbox.model.MailboxId;
import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.mailbox.model.MessageId;
import org.apache.james.mailbox.model.MessageMetaData;
import org.apache.james.mailbox.model.MessageMoves;
import org.apache.james.mailbox.model.QuotaRoot;
import org.apache.james.mailbox.model.UpdatedFlags;
import org.apache.james.mailbox.store.mail.model.Mailbox;
import org.apache.james.mailbox.store.mail.model.MailboxMessage;

public class EventFactory {

Expand Down Expand Up @@ -99,11 +100,11 @@ public MailboxListener.MailboxACLUpdated aclUpdated(MailboxSession.SessionId ses
return new MailboxListener.MailboxACLUpdated(sessionId, user, mailboxPath, aclDiff, mailboxId);
}

public MessageMoveEvent moved(MailboxSession session, MessageMoves messageMoves, Map<MessageUid, MailboxMessage> messages) {
public MessageMoveEvent moved(MailboxSession session, MessageMoves messageMoves, Collection<MessageId> messageIds) {
return MessageMoveEvent.builder()
.user(session.getUser())
.messageMoves(messageMoves)
.messages(messages)
.messageId(messageIds)
.build();
}
}
7 changes: 5 additions & 2 deletions mailbox/store/src/main/java/org/apache/james/mailbox/store/event/MailboxEventDispatcher.java
View file
Expand Up @@ -20,6 +20,7 @@
package org.apache.james.mailbox.store.event;

import java.time.Instant;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.SortedMap;
Expand All @@ -36,6 +37,7 @@
import org.apache.james.mailbox.acl.ACLDiff;
import org.apache.james.mailbox.model.MailboxId;
import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.mailbox.model.MessageId;
import org.apache.james.mailbox.model.MessageMetaData;
import org.apache.james.mailbox.model.MessageMoves;
import org.apache.james.mailbox.model.Quota;
Expand Down Expand Up @@ -163,8 +165,9 @@ public void aclUpdated(MailboxSession session, MailboxPath mailboxPath, ACLDiff
listener.event(eventFactory.aclUpdated(session, mailboxPath, aclDiff, mailboxId));
}

public void moved(MailboxSession session, MessageMoves messageMoves, Map<MessageUid, MailboxMessage> messages) {
MessageMoveEvent moveEvent = eventFactory.moved(session, messageMoves, messages);
public void moved(MailboxSession session, MessageMoves messageMoves, Collection<MessageId> messageIds) {
MessageMoveEvent moveEvent = eventFactory.moved(session, messageMoves, messageIds);

if (!moveEvent.isNoop()) {
listener.event(moveEvent);
}
Expand Down

0 comments on commit 9cf3186

Please sign in to comment.