[patch] infinite loop in Mail.send override

[Dave Brosius]

This overridden Mail.send is a self-recursive infinite loop, because of the varargs attachments parameter. Code was intended to call the next send method with a null replyTo

    public static Future<Boolean> send(String from, String[] recipients, String subject, String body, Object... attachments) {
        return send(from, recipients, subject, body, null, "text/plain", attachments);
    }

patch fixes this

[Lawrence McAlpin (lmcalpin)]

There are a few other methods that were (as of play-1.0-r781) still going into an infinite loop due to a bad override.

[Nicolas Leroux (nicolas-lunatech)]

I don't think this is the case anymore and therefore the patch does not seem to apply. I will write a test case with all the send methods to be sure.

[Lawrence McAlpin (lmcalpin)]

I may be mistaken about the infinite loop. That may have occurred before I upgraded to play-1.0-r781. But there's definitely a problem in the latest cut of the code we can download or acquire via git, which my patch corrected.

The call
Mail.send(emailAddress, recipient, "test", "test");

results in:
play.exceptions.JavaExecutionException
        at play.mvc.ActionInvoker.invoke(ActionInvoker.java:229)
        at Invocation.HTTP Request(Play!)
Caused by: java.lang.NullPointerException
        at play.libs.Mail.handleAttachments(Mail.java:396)
        at play.libs.Mail.buildMessage(Mail.java:252)
        at play.libs.Mail.send(Mail.java:178)
        at play.libs.Mail.send(Mail.java:129)
        at play.libs.Mail.send(Mail.java:114)
        at play.libs.Mail.send(Mail.java:54)
        at controllers.Forms.form(Forms.java:116)
        at play.utils.Java.invokeStatic(Java.java:129)
        at play.mvc.ActionInvoker.invoke(ActionInvoker.java:138)
        ... 1 more

because it thinks there are attachments, even though I have not provided any. A unit test would be good. Should I file this as a new bug since the symptom is different (though the cause, varargs/override confusion is the same)?

[Nicolas Leroux (nicolas-lunatech)]

I am currently working on the tests. I don't think you need to fill a new bug as I cannot reproduce it at the moment. Thanks for the comments as it forces me to write tests ;)

[Nicolas Leroux (nicolas-lunatech)]

So I don't have a npe but there is definitively a problem with the send methods. Thanks again.

[Lawrence McAlpin (lmcalpin)]

The behavior seems to be different if you use the mock mailer. Here is an (incomplete) test case I wrote. It tests green with my patch applied, but fails against r781.

When it fails (if running against play-r781), it thinks we have three attachments. The test is clearly not supplying any attachments.

Thanks for working on this amazing framework!

[Nicolas Leroux (nicolas-lunatech)]

I applied your patch and fixed a few other things and added a test cases for all the send method. However I only used the Mock mailer. Could you confirm that this is working for you now (play-r782) using real email?

You can then comment on:
https://bugs.launchpad.net/play/+bug/503601

Thanks

[Stephane Godbillon (sgo)]

Hi,

Is there any reason to change the Mail.buildMessage method's signature (replyTo argument type) ?

r781 : public static MimeMessage buildMessage(InternetAddress from, Object replyTo, Object[] recipients, String subject, String body, String alternate, String contentType, String charset, Map<String, String> headers, Object... attachments)

r782: public static MimeMessage buildMessage(InternetAddress from, InternetAddress replyTo, Object[] recipients, String subject, String body, String alternate, String contentType, String charset, Map<String, String> headers, Object... attachments)

Object type had been chosen to allow to call buildMessage() with a replyTo as a String or as an InternetAdress object.

It breaks some applications that are already in production...

[Nicolas Leroux (nicolas-lunatech)]

Hi,

I thought that there were a buildMessage with a replyTo as a String. I will add it if it not there.

In that case using Object to support both InternetAddress and String is possible. However you are only allowed to use String or InternetAddress so why not having 2 methods reflecting this requirement?

However, in general I think that is a bad idea to use Object as parameters though. Java is a strongly typed language and that is for a good reason. Of course, the goal is to be 100% compatible with the 1.0.

I am fixing that ASAP.

[Nicolas Leroux (nicolas-lunatech)]

I created a new bug report so we have a trace between the 1.0.1 rc1 and the 1.0.1:

https://bugs.launchpad.net/play/+bug/504276

[Nicolas Leroux (nicolas-lunatech)]

Hi,

https://bugs.launchpad.net/play/+bug/504276 has been fixed. Can you confirm with a comment on https://bugs.launchpad.net/play/+bug/504276 that now the Mail library is 100% compatible with play 1.0 ?

Thanks

Nicolas

[Stephane Godbillon (sgo)]

I agree, we can make 2 methods, however in this particular case, I think using Object is a good choice to avoid a huge number of signatures. The previous implementation checks if the parameter is an InternetAddress or calls toString() on it (assuming it is not null) : after all, for this use case, this is the expected behavior, and the API is still clear.

[Nicolas Leroux (nicolas-lunatech)]

Where I understand why it was done and why it was practical I still disagree. What if it is not a String or an InternetAddress? It will throw an exception at runtime instead of compile time. If there is a huge number of signature it probably means that something is wrong and that instead it is probably getting too much procedural instead of OO. Why not passing only Object parameters otherwise to avoid huge number of signatures? Anyway, let's not fight about it, I guess we both have our point of view ;) The API is still backward compatible that was the main concern.