Skip to content
Snippets Groups Projects
Commit f4f8418f authored by Denis Gervalle's avatar Denis Gervalle
Browse files

XWIKI-12128: Use standard Message ID to track mails in place of custom X-MailID header

Code style and bulletproofing:
- prevent issue when Message-ID change during MimeMessage#writeTo() at serialization time
- prevent null Message-ID to used in statuses by forcing MimeMessage#saveChanges() when needed
- helper for MailListener implementation
- fix a raise condition in integration tests
parent a2852941
No related branches found
No related tags found
No related merge requests found
Showing
with 259 additions and 125 deletions
......@@ -103,7 +103,7 @@ public MailStatus()
public MailStatus(String batchId, MimeMessage message, MailState state)
{
try {
setMessageId(message.getMessageID());
setMessageId(getMessageId(message));
setBatchId(batchId);
setType(message.getHeader("X-MailType", null));
setRecipients(InternetAddress.toString(message.getAllRecipients()));
......@@ -337,4 +337,15 @@ public String toString()
}
return builder.toString();
}
private String getMessageId(MimeMessage message) throws MessagingException
{
String id = message.getMessageID();
if (id == null) {
message.saveChanges();
id = message.getMessageID();
}
return id;
}
}
/*
* See the NOTICE file distributed with this work for additional
* information regarding copyright ownership.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
package org.xwiki.mail.internal;
import java.util.Map;
import javax.inject.Inject;
import javax.mail.MessagingException;
import javax.mail.internet.MimeMessage;
import org.slf4j.Logger;
import org.xwiki.mail.MailListener;
/**
* Helper for implementation of {@Link MailListener}.
*
* @version $Id$
*/
public abstract class AbstractMailListener implements MailListener
{
@Inject
protected Logger logger;
private String batchId;
protected String getBatchId()
{
return batchId;
}
protected String getMessageId(MimeMessage message)
{
try {
String messageId = message.getMessageID();
// Ensure that a messageId is generated if the caller have omitted calling saveChanges()
if (messageId == null) {
message.saveChanges();
messageId = message.getMessageID();
}
return messageId;
} catch (MessagingException e) {
// This cannot happen in practice since the implementation never throws any exception!
logger.error("Failed to retrieve messageID from the message.", e);
return null;
}
}
@Override
public void onPrepareBegin(String batchId, Map<String, Object> parameters)
{
if (this.batchId != null) {
throw new RuntimeException("A mail listener cannot be reused. This listener has been used for batch ["
+ this.batchId + "] and is now called for batch [" + batchId + "].");
}
logger.debug("Mail preparation begins for batch [{}].", batchId);
this.batchId = batchId;
}
@Override
public void onPrepareMessageSuccess(MimeMessage message, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail preparation succeed for message [{}] of batch [{}].", getMessageId(message),
batchId);
}
}
@Override
public void onPrepareMessageError(MimeMessage message, Exception exception, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail preparation failed for message [{}] of batch [{}].", getMessageId(message), batchId,
exception);
}
}
@Override
public void onPrepareFatalError(Exception exception, Map<String, Object> parameters)
{
logger.debug("Failure during preparation phase of thread [" + batchId + "]");
}
@Override
public void onPrepareEnd(Map<String, Object> parameters)
{
logger.debug("Mail preparation ended for batch [{}].", batchId);
}
@Override
public void onSendMessageSuccess(MimeMessage message, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail sent successfully for message [{}] of batch [{}].", getMessageId(message), batchId);
}
}
@Override
public void onSendMessageError(MimeMessage message, Exception exception, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail sending failed for message [{}] of batch [{}].", getMessageId(message), batchId,
exception);
}
}
@Override
public void onSendMessageFatalError(String messageId, Exception exception, Map<String, Object> parameters)
{
logger.debug("Mail loading failed for message [{}] of batch [{}].", messageId, batchId, exception);
}
}
......@@ -78,6 +78,19 @@ public void save(String batchId, MimeMessage message) throws MailStoreException
messageFile = getMessageFile(batchId, messageId);
OutputStream os = new FileOutputStream(messageFile);
message.writeTo(os);
// Since message#writeTo() may call message#updateMessageID() before serializing in some cases,
// we ensure that the messageId is unchanged after the serialization process.
if (!messageId.equals(message.getMessageID())) {
// If the messageId has changed, we move the serialized file to the new identifier
File oldMessageFile = messageFile;
messageId = message.getMessageID();
messageFile = getMessageFile(batchId, messageId);
if (!oldMessageFile.renameTo(messageFile)) {
throw new MailStoreException(String.format(
"Failed to rename saved message (id [%s], batch id [%s]) from file [%s] into file [%s]",
messageId, batchId, oldMessageFile, messageFile));
}
}
} catch (Exception e) {
throw new MailStoreException(String.format(
"Failed to save message (id [%s], batch id [%s]) into file [%s]",
......
......@@ -21,16 +21,12 @@
import java.util.Map;
import javax.inject.Inject;
import javax.inject.Named;
import javax.mail.MessagingException;
import javax.mail.internet.MimeMessage;
import org.slf4j.Logger;
import org.xwiki.component.annotation.Component;
import org.xwiki.component.annotation.InstantiationStrategy;
import org.xwiki.component.descriptor.ComponentInstantiationStrategy;
import org.xwiki.mail.MailListener;
import org.xwiki.mail.MailState;
import org.xwiki.mail.MailStatus;
import org.xwiki.mail.MailStatusResult;
......@@ -44,48 +40,25 @@
@Component
@Named("memory")
@InstantiationStrategy(ComponentInstantiationStrategy.PER_LOOKUP)
public class MemoryMailListener implements MailListener
public class MemoryMailListener extends AbstractMailListener
{
@Inject
private Logger logger;
private MemoryMailStatusResult mailStatusResult = new MemoryMailStatusResult();
private String batchId;
@Override
public void onPrepareBegin(String batchId, Map<String, Object> parameters)
{
if (this.batchId != null) {
throw new RuntimeException("A mail listener cannot be reused. This listener has been used for batch ["
+ this.batchId + "] and is now called for batch [" + batchId + "].");
}
logger.debug("Mail preparation begins for batch [{}].", batchId);
this.batchId = batchId;
}
@Override
public void onPrepareMessageSuccess(MimeMessage message, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail preparation succeed for message [{}] of batch [{}].", getMessageId(message), batchId);
}
super.onPrepareMessageSuccess(message, parameters);
MailStatus status = new MailStatus(batchId, message, MailState.PREPARE_SUCCESS);
MailStatus status = new MailStatus(getBatchId(), message, MailState.PREPARE_SUCCESS);
this.mailStatusResult.setStatus(status);
}
@Override
public void onPrepareMessageError(MimeMessage message, Exception exception, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail preparation failed for message [{}] of batch [{}].", getMessageId(message), batchId,
exception);
}
super.onPrepareMessageError(message, exception, parameters);
MailStatus status = new MailStatus(batchId, message, MailState.PREPARE_ERROR);
MailStatus status = new MailStatus(getBatchId(), message, MailState.PREPARE_ERROR);
status.setError(exception);
this.mailStatusResult.setStatus(status);
......@@ -94,26 +67,20 @@ public void onPrepareMessageError(MimeMessage message, Exception exception, Map<
}
@Override
public void onPrepareFatalError(Exception e, Map<String, Object> parameters)
public void onPrepareFatalError(Exception exception, Map<String, Object> parameters)
{
//TODO: Store failure exception
logger.error("Failure during preparation phase of thread [" + batchId + "]");
}
super.onPrepareFatalError(exception, parameters);
@Override
public void onPrepareEnd(Map<String, Object> parameters)
{
logger.debug("Mail preparation ended for batch [{}].", batchId);
//TODO: Store failure exception
logger.error("Failure during preparation phase of thread [" + getBatchId() + "]");
}
@Override
public void onSendMessageSuccess(MimeMessage message, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail sent successfully for message [{}] of batch [{}].", getMessageId(message), batchId);
}
super.onPrepareMessageSuccess(message, parameters);
MailStatus status = new MailStatus(batchId, message, MailState.SEND_SUCCESS);
MailStatus status = new MailStatus(getBatchId(), message, MailState.SEND_SUCCESS);
this.mailStatusResult.setStatus(status);
this.mailStatusResult.incrementCurrentSize();
}
......@@ -121,7 +88,7 @@ public void onSendMessageSuccess(MimeMessage message, Map<String, Object> parame
@Override
public void onSendMessageFatalError(String messageId, Exception exception, Map<String, Object> parameters)
{
logger.debug("Mail loading failed for message [{}] of batch [{}].", messageId, batchId, exception);
super.onSendMessageFatalError(messageId, exception, parameters);
MailStatus status = this.mailStatusResult.getStatus(messageId);
if (status != null) {
......@@ -130,7 +97,8 @@ public void onSendMessageFatalError(String messageId, Exception exception, Map<S
this.mailStatusResult.setStatus(status);
} else {
this.logger.error("Failed to find a previous mail status for message id [{}] of batch [{}]. "
+ "Unable to report the fatal error encountered during mail sending.", messageId, batchId, exception);
+ "Unable to report the fatal error encountered during mail sending.", messageId, getBatchId(),
exception);
}
this.mailStatusResult.incrementCurrentSize();
......@@ -139,12 +107,9 @@ public void onSendMessageFatalError(String messageId, Exception exception, Map<S
@Override
public void onSendMessageError(MimeMessage message, Exception exception, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail sending failed for message [{}] of batch [{}].", getMessageId(message), batchId,
exception);
}
super.onSendMessageError(message, exception, parameters);
MailStatus status = new MailStatus(batchId, message, MailState.SEND_ERROR);
MailStatus status = new MailStatus(getBatchId(), message, MailState.SEND_ERROR);
status.setError(exception);
this.mailStatusResult.setStatus(status);
......@@ -157,15 +122,4 @@ public MailStatusResult getMailStatusResult()
{
return this.mailStatusResult;
}
private String getMessageId(MimeMessage message)
{
try {
return message.getMessageID();
} catch (MessagingException e) {
// This cannot happen in practice since the implementation never throws any exception!
logger.error("Failed to retrieve messageID from the message.", e);
return null;
}
}
}
......@@ -163,6 +163,7 @@ private void prepareSingleMail(MimeMessage mimeMessage, PrepareMailQueueItem ite
// Step 1: Complete message with From and Bcc from configuration if needed
message = initializeMessage(mimeMessage);
// Step 2: Persist the MimeMessage
// Note: Message identifier is stabilized at this step by the serialization process
this.mailContentStore.save(item.getBatchId(), message);
// Step 3: Put the MimeMessage id on the Mail Send Queue for sending
this.sendMailQueueManager.addToQueue(new SendMailQueueItem(message.getMessageID(),
......
......@@ -189,10 +189,16 @@ public void sendTextMail() throws Exception
// We also test using some default BCC addresses from configuration in this test
this.configuration.setBCCAddresses(Arrays.asList("bcc1@doe.com", "bcc2@doe.com"));
// Ensure we do not reuse the same message identifier for multiple similar messages in this test
MimeMessage message2 = new MimeMessage(message);
message2.saveChanges();
MimeMessage message3 = new MimeMessage(message);
message3.saveChanges();
// Step 4: Send the mail and wait for it to be sent
// Send 3 mails (3 times the same mail) to verify we can send several emails at once.
MailListener memoryMailListener = this.componentManager.getInstance(MailListener.class, "memory");
this.sender.sendAsynchronously(Arrays.asList(message, message, message), session, memoryMailListener);
this.sender.sendAsynchronously(Arrays.asList(message, message2, message3), session, memoryMailListener);
// Note: we don't test status reporting from the listener since this is already tested in the
// ScriptingIntegrationTest test class.
......
......@@ -29,6 +29,7 @@
import java.util.Properties;
import java.util.UUID;
import javax.mail.MessagingException;
import javax.mail.Session;
import javax.mail.internet.MimeMessage;
......@@ -89,11 +90,41 @@ public void registerMockComponents() throws Exception
public void saveMessage() throws Exception
{
String batchId = UUID.randomUUID().toString();
String messageId = "<1128820400.0.1419205781342.JavaMail.contact@xwiki.org>";
Session session = Session.getInstance(new Properties());
MimeMessage message = new MimeMessage(session);
message.saveChanges();
message.setText("Lorem ipsum dolor sit amet, consectetur adipiscing elit");
this.mocker.getComponentUnderTest().save(batchId, message);
File tempDir = new File(TEMPORARY_DIRECTORY);
File batchDirectory =
new File(new File(tempDir, this.mocker.getComponentUnderTest().ROOT_DIRECTORY),
URLEncoder.encode(batchId, "UTF-8"));
File messageFile = new File(batchDirectory, URLEncoder.encode(message.getMessageID(), "UTF-8"));
InputStream in = new FileInputStream(messageFile);
String messageContent = IOUtils.toString(in);
assertTrue(messageContent.contains("Message-ID: " + message.getMessageID()));
assertTrue(messageContent.contains("Lorem ipsum dolor sit amet, consectetur adipiscing elit"));
}
@Test
public void saveMessageWithCustomMessageId() throws Exception
{
String batchId = UUID.randomUUID().toString();
String messageId = "<1128820400.0.1419205781342.JavaMail.contact@xwiki.org>";
Session session = Session.getInstance(new Properties());
MimeMessage message = new MimeMessage(session) {
@Override
protected void updateMessageID() throws MessagingException
{
if (getMessageID() == null) {
super.updateMessageID();
}
}
};
message.setHeader("Message-ID", messageId);
message.setText("Lorem ipsum dolor sit amet, consectetur adipiscing elit");
......@@ -111,6 +142,31 @@ public void saveMessage() throws Exception
assertTrue(messageContent.contains("Lorem ipsum dolor sit amet, consectetur adipiscing elit"));
}
@Test
public void saveMessageWhenInstableCustomMessageID() throws Exception
{
String batchId = UUID.randomUUID().toString();
String messageId = "<1128820400.0.1419205781342.JavaMail.contact@xwiki.org>";
Session session = Session.getInstance(new Properties());
MimeMessage message = new MimeMessage(session);
message.setHeader("Message-ID", messageId);
message.setText("Lorem ipsum dolor sit amet, consectetur adipiscing elit");
this.mocker.getComponentUnderTest().save(batchId, message);
File tempDir = new File(TEMPORARY_DIRECTORY);
File batchDirectory =
new File(new File(tempDir, this.mocker.getComponentUnderTest().ROOT_DIRECTORY),
URLEncoder.encode(batchId, "UTF-8"));
File messageFile = new File(batchDirectory, URLEncoder.encode(message.getMessageID(), "UTF-8"));
InputStream in = new FileInputStream(messageFile);
String messageContent = IOUtils.toString(in);
assertTrue(messageContent.contains("Message-ID: " + message.getMessageID()));
assertTrue(messageContent.contains("Lorem ipsum dolor sit amet, consectetur adipiscing elit"));
}
@Test
public void saveMessageThrowsMailStoreExceptionWhenError() throws Exception
{
......
......@@ -23,11 +23,9 @@
import javax.inject.Inject;
import javax.inject.Named;
import javax.mail.MessagingException;
import javax.mail.internet.MimeMessage;
import org.apache.commons.lang3.exception.ExceptionUtils;
import org.slf4j.Logger;
import org.xwiki.component.annotation.Component;
import org.xwiki.component.annotation.InstantiationStrategy;
import org.xwiki.component.descriptor.ComponentInstantiationStrategy;
......@@ -35,7 +33,6 @@
import org.xwiki.component.phase.InitializationException;
import org.xwiki.context.Execution;
import org.xwiki.mail.MailContentStore;
import org.xwiki.mail.MailListener;
import org.xwiki.mail.MailState;
import org.xwiki.mail.MailStatus;
import org.xwiki.mail.MailStatusResult;
......@@ -54,11 +51,8 @@
@Component
@Named("database")
@InstantiationStrategy(ComponentInstantiationStrategy.PER_LOOKUP)
public class DatabaseMailListener implements MailListener, Initializable
public class DatabaseMailListener extends AbstractMailListener implements Initializable
{
@Inject
private Logger logger;
@Inject
private Execution execution;
......@@ -75,8 +69,6 @@ public class DatabaseMailListener implements MailListener, Initializable
private DatabaseMailStatusResult mailStatusResult;
private String batchId;
@Override
public void initialize() throws InitializationException
{
......@@ -86,25 +78,16 @@ public void initialize() throws InitializationException
@Override
public void onPrepareBegin(String batchId, Map<String, Object> parameters)
{
if (this.batchId != null) {
throw new RuntimeException("A mail listener cannot be reused. This listener has been used for batch ["
+ this.batchId + "] and is now called for batch [" + batchId + "].");
}
logger.debug("Mail preparation begins for batch [{}].", batchId);
this.batchId = batchId;
super.onPrepareBegin(batchId, parameters);
mailStatusResult.setBatchId(batchId);
}
@Override
public void onPrepareMessageSuccess(MimeMessage message, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail preparation succeed for message [{}] of batch [{}].", getMessageId(message), batchId);
}
super.onPrepareMessageSuccess(message, parameters);
MailStatus status = new MailStatus(batchId, message, MailState.PREPARE_SUCCESS);
MailStatus status = new MailStatus(getBatchId(), message, MailState.PREPARE_SUCCESS);
status.setWiki(
((XWikiContext) execution.getContext().getProperty(XWikiContext.EXECUTIONCONTEXT_KEY)).getWikiId());
saveStatus(status, parameters);
......@@ -113,12 +96,9 @@ public void onPrepareMessageSuccess(MimeMessage message, Map<String, Object> par
@Override
public void onPrepareMessageError(MimeMessage message, Exception exception, Map<String, Object> parameters)
{
if (logger.isDebugEnabled()) {
logger.debug("Mail preparation failed for message [{}] of batch [{}].", getMessageId(message), batchId,
exception);
}
super.onPrepareMessageError(message, exception, parameters);
MailStatus status = new MailStatus(batchId, message, MailState.PREPARE_ERROR);
MailStatus status = new MailStatus(getBatchId(), message, MailState.PREPARE_ERROR);
status.setWiki(
((XWikiContext) execution.getContext().getProperty(XWikiContext.EXECUTIONCONTEXT_KEY)).getWikiId());
status.setError(exception);
......@@ -129,33 +109,28 @@ public void onPrepareMessageError(MimeMessage message, Exception exception, Map<
}
@Override
public void onPrepareFatalError(Exception e, Map<String, Object> parameters)
public void onPrepareFatalError(Exception exception, Map<String, Object> parameters)
{
//TODO: Store failure exception
logger.error("Failure during preparation phase of thread [" + batchId + "]");
}
super.onPrepareFatalError(exception, parameters);
@Override
public void onPrepareEnd(Map<String, Object> parameters)
{
logger.debug("Mail preparation ended for batch [{}].", batchId);
//TODO: Store failure exception
logger.error("Failure during preparation phase of thread [" + getBatchId() + "]");
}
@Override
public void onSendMessageSuccess(MimeMessage message, Map<String, Object> parameters)
{
String messageId = getMessageId(message);
logger.debug("Mail sent successfully for message [{}] of batch [{}].", messageId, batchId);
super.onSendMessageSuccess(message, parameters);
String messageId = getMessageId(message);
MailStatus status = retrieveExistingMailStatus(messageId, MailState.SEND_SUCCESS);
if (status != null) {
status.setState(MailState.SEND_SUCCESS);
} else {
this.logger.warn("Forcing a new mail status for message [{}] of batch [{}] to send_success state.",
messageId, batchId);
status = new MailStatus(batchId, message, MailState.SEND_SUCCESS);
messageId, getBatchId());
status = new MailStatus(getBatchId(), message, MailState.SEND_SUCCESS);
}
// Since the mail was sent successfully we don't need to keep its serialized content
......@@ -174,7 +149,7 @@ public void onSendMessageSuccess(MimeMessage message, Map<String, Object> parame
@Override
public void onSendMessageFatalError(String messageId, Exception exception, Map<String, Object> parameters)
{
logger.debug("Mail loading failed for message [{}] of batch [{}].", messageId, batchId, exception);
super.onSendMessageFatalError(messageId, exception, parameters);
MailStatus status = retrieveExistingMailStatus(messageId, MailState.SEND_FATAL_ERROR);
......@@ -184,7 +159,7 @@ public void onSendMessageFatalError(String messageId, Exception exception, Map<S
saveStatus(status, parameters);
} else {
this.logger.error("Unable to report the fatal error encountered during mail sending for message [{}] "
+ "of batch [{}].", messageId, batchId, exception);
+ "of batch [{}].", messageId, getBatchId(), exception);
}
this.mailStatusResult.incrementCurrentSize();
......@@ -193,18 +168,17 @@ public void onSendMessageFatalError(String messageId, Exception exception, Map<S
@Override
public void onSendMessageError(MimeMessage message, Exception exception, Map<String, Object> parameters)
{
String messageId = getMessageId(message);
logger.debug("Mail sending failed for message [{}] of batch [{}].", messageId, batchId, exception);
super.onSendMessageError(message, exception, parameters);
String messageId = getMessageId(message);
MailStatus status = retrieveExistingMailStatus(messageId, MailState.SEND_ERROR);
if (status != null) {
status.setState(MailState.SEND_ERROR);
} else {
this.logger.warn("Forcing a new mail status for message [{}] of batch [{}] to send_error state.",
messageId, batchId);
status = new MailStatus(batchId, message, MailState.SEND_ERROR);
messageId, getBatchId());
status = new MailStatus(getBatchId(), message, MailState.SEND_ERROR);
}
status.setError(exception);
saveStatus(status, parameters);
......@@ -221,11 +195,11 @@ private MailStatus retrieveExistingMailStatus(String messageId, MailState state)
// It's not normal to have no status in the mail status store since onPrepare should have been called
// before.
this.logger.error("Failed to find a previous mail status for message [{}] of batch [{}].",
messageId, batchId, state);
messageId, getBatchId(), state);
}
} catch (MailStoreException e) {
this.logger.error("Error when looking for a previous mail status for message [{}] of batch [{}].",
messageId, batchId, state, e);
messageId, getBatchId(), state, e);
status = null;
}
return status;
......@@ -237,17 +211,6 @@ public MailStatusResult getMailStatusResult()
return mailStatusResult;
}
private String getMessageId(MimeMessage message)
{
try {
return message.getMessageID();
} catch (MessagingException e) {
// This cannot happen in practice since the implementation never throws any exception!
logger.error("Failed to retrieve messageID from the message.", e);
return null;
}
}
private void saveStatus(MailStatus status, Map<String, Object> parameters)
{
try {
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment