ERMailDelivery.sendMail() does not allow for null returned from Message.getAllRecipients()

[paulhoadley]

The issue here is that ERMailDelivery.sendMail() checks for zero recipients by looking at the length of an Address[] array:

if (mimeMessage().getAllRecipients().length == 0) {
    return;
}

That's fine, except the ultimate origin of that array is a call to javax.mail.Message.getAllRecipients(), where the Javadocs explicitly state that "This method returns null if none of the recipient headers are present in this message." If that array is null, this check obviously throws a NullPointerException.

Presumably you could trigger this on purpose by setting no recipients and calling sendMail(), but you can run into it simply by using the features of ERJavaMail.framework. Specifically, set the black list to exclude addresses in example.com:

er.javamail.BlackListEmailAddressPatterns=("*@example.com")

And then try to send a message to an address in that domain. The address is correctly excluded from recipients, then you have zero recipients, then you get the NullPointerException. My argument is that this blacklist feature should be usable just like this, where I'm just trying to suppress outgoing mail to a domain even for single-recipient messages.

Aside from my use case here, getAllRecipients() is documented as potentially returning null, so this should be fixed regardless.

[lbane]

I don't like this solution. Having no recipients is often an error, and silently dropping the mail makes it worse. I would prefer some kind of explicit (maybe even checked) NoReceiverException thrown, if a NullPointerException is not good enough. The javax.mail.Transport class would throw a SendFailedException("No recipient addresses") in this case.

[paulhoadley]

I don't like this solution. Having no recipients is often an error, and silently dropping the mail makes it worse.

Yeah, your argument is probably reasonable, but silently dropping the mail was already the (intended) behaviour. The bug here was that the original code wasn't allowing for a null being returned by javax.mail.Message.getAllRecipients(). To my eye, the intention of the original code was to bail out and return on zero recipients:

if (mimeMessage().getAllRecipients().length == 0) {
    return;
}

Can we read that in any other way? So it's not that a NullPointerException wasn't good enough, it's that it wasn't even anticipated. All I've done is to fix the intended behaviour.

[lbane]

I see what you mean. My thinking is, that having no receiver for a mail is an exceptional case, where the caller should be notified, and this case cannot be handled by the library. So, I see the intended behaviour as wrong in the first place, but the implementation was accidentally (somewhat) right.
With the phrase "good enough" I meant, that in some cases the natural encountered exception is sometimes okay to use, in this case the unintended NullPointerException. I know you get the exception because you are filtering all receiving addresses. But as long as the library cannot distinguish between filtering all addresses and not getting any in the first place, I see the real danger of mails being dropped without any trace or hint where the problem might be. As you said yourself, it's possible to get to this point without using the blacklist at all.