Safely read message subject for debug message

[domi-87]

If Spring Boot Integration Mail is connected to a Domino mail server via IMAP, it can happen from time to time that a message is expunged. This leads to a MessageRemovedException when calling IMAPMessage#getSubject. And although Debug is set to false this again leads to a MessageException and the whole integration flow stops.

[pivotal-cla]

@domi-87 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@domi-87 Thank you for signing the Contributor License Agreement!

[artembilan]

@domi-87 ,

thank you for contribution, but I'm not sure how this try..catch around getSubject() for the filtered out messages makes us safe?
I looks like if message has been expunged, it is also not available afterwards when we done with filtering and go ahead for processing them.
I mean what difference does this change do if we still going to fail with the same exception lately?

Do I miss anything?

[domi-87]

@artembilan

thanks for your reply. We have implemented a filter (selectorExpression) which filters expunged messages. At this code point these very messages are logged again as debug (messages that have been filtered out beforehand and are not to be processed further). For this log message (debug) getSubject() is called, which in turn throws a MessageRemovedException at IMAPMessages. From my point of view and "only" for a debug message completely unnecessary. Therefore I suggest this try..catch.

[artembilan]

Hm. How about to have that getSubject() called before the selectorExpression? Or this still does not make sense since the message is already expunged?

[epictecch]

The problem arises together with IMAP messages only as for expunged IMAP messages getSubject() throws a MessagingException. See https://github.com/javaee/javamail/blob/8106b9dd15e917da63e96c33f8c6aea1cad40045/mail/src/main/java/com/sun/mail/imap/IMAPMessage.java#L433 and https://github.com/javaee/javamail/blob/8106b9dd15e917da63e96c33f8c6aea1cad40045/mail/src/main/java/com/sun/mail/imap/IMAPMessage.java#L280

The commit of @domi-87 enables the users to filter such expunged IMAP messages by manually filtering them via the selectorExpression.
Without the proposed change, there is no possibility to filter out expunged IMAP messages. Currently, filtering them out leads to an immediate exception (which aborts the whole message processing) because we call getSubject() for the debug message.

@artembilan Calling getSubject() before selectorExpression will lead to the situation that the processing of the messages abords even earlier if one of the messages is an expunged IMAP message.
Either we do not call getSubject() at all (as it may throw) or we handle the MessagingException of the call to omit aborting the message processsing.

[artembilan]

Yeah... I see your point. So, isn't it will be more natural to call message.isExpunged() in this part of code instead of catching an exception?

[epictecch]

You mean something like

for (message : Messages) {
  if (message.isExpunged()) {
    this.logger.debug("Expunged message received and will not be further processed.");
  } else if (this.selectorExpression != null) {
    // Current logic
  } else {
    filteredMessages.add(message);
  }
}

I think this would be a clean solution for IMAP messages. I'm not sure whether this works for other message types. E.g., can POP3 messages also be expunged and if so, do they contain any useful information the user might be interested in? If yes, such a change would prevent the user to do useful stuff with the information that an expunged message was retrieved.
If expunged messages never contain useful information, the use of message.isExpunged() is helpful and we should add this if-clause independent of this pull request.

Nevertheless, other reasons for exceptions when calling getSubject() would still lead to the situation that the processing of the messages is aborted. One other reason might be:

1. MimeMessage.getSubject() calls getHeader()
2. For the POP3Message class, getHeader() calls loadHeaders()
3. POP3Message.loadHeaders() can throw exceptions for several reasons.

If we want to be sure that the message processing is not aborted, we need to really catch the exceptions which might be thrown by the call to getSubject(). Alternatively, we either do not log anymore or we do not log the message's subject. But in the latter case, this makes the message nearly useless. Logging another identifier such as the message-id instead of the subject does not change anything because MimeMessage.getMessageID() might also throw a MessagingException.

[artembilan]

I think we still go the current selectorExpression logic, but don't call getSubject() for debug if message is expunged.
Otherwise we just print a simple "Expunged message received and will not be further processed." as you suggest.
This way we won't need to have a try..catch around that debug logging.
The rest of logic downstream where we fetch the message remains the same.
I think we have over there some error handling already for any other reason when we already done with filtering.

[artembilan]

LGTM.
Although may we have a simple unit (mock?) test to cover this functionality after messages selection? See ImapMailReceiverTests as a place where this can be added as a new test.

[artembilan]

LGTM.
Merging after adding your name to the @author list

[artembilan]

Merged as c189a12 and cherry-picked to 5.4.x.

@domi-87 ,
thank you very much for contribution; looking forward for more!