Add support for parsing preformatted email addresses that include both name and address
dormitionskete opened this issue · 25 comments
I recently became acquainted with SimpleJavaMail. I've been testing it for a few days, and so far, I like it very much. Thank you very much for making it, and sharing it with us!
On your website, you asked for suggestions for how it could be improved. I have a few "wishlist" items for it. I'll put them in separate tickets so you can evaluate and track them individually.
You currently have the ability to add multiple recipients with:
email.addRecipients("jsmith@somewhere.net, jsmith@nowhere.com", RecipientType.TO);
It'd be very much appreciated if you would extend its capabilities to accept entries such as:
email.addRecipients("\"John Smith\" <jsmith@somewhere.net>, \"Jane Doe\" <jsmith@nowhere.com>", RecipientType.TO);
Release in 4.4.0. Please verify.
It still won't build for me, so I can't verify it for you.
Tomorrow is Transfiguration, so I won't be able to work on it until Monday.
Forgive me, please.
I do very much appreciate you working on this.
cd /Users/frpeter/NetBeansProjects/simple-java-mail; JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_102.jdk/Contents/Home "/Applications/NetBeans/NetBeans 8.2.app/Contents/Resources/NetBeans/java/maven/bin/mvn" clean install
Scanning for projects...
simplejavamail.util.ConfigLoaderTest
15:54:49 [main] INFO SMTPServer - SMTP server *:251 starting
15:54:49 [main] INFO SMTPServer - SMTP server *:251 starting
Tests run: 3, Failures: 0, Errors: 2, Skipped: 1, Time elapsed: 4.468 sec <<< FAILURE! - in org.simplejavamail.mailer.MailerLiveTest
createMailSession_StandardDummyMail(org.simplejavamail.mailer.MailerLiveTest) Time elapsed: 1.742 sec <<< ERROR!
java.lang.RuntimeException: java.net.BindException: Permission denied
Caused by: java.net.BindException: Permission denied
createMailSession_OutlookMessageTest(org.simplejavamail.mailer.MailerLiveTest) Time elapsed: 0.002 sec <<< ERROR!
java.lang.RuntimeException: java.net.BindException: Permission denied
Caused by: java.net.BindException: Permission denied
---
Hi @dormitionskete, no need to apologise! There's no hurry and you don't owe me anything. I'm just thankful for your helpful review of the API and suggestions to improve it.
Now for the error, it seems like a permission issue at your end. Travis is having no issues (runs fine for me as well of course), but when the MailerLiveTest runs it starts a dummy SMTP server, which fails for you on account of permissions.
Two things to note:
- You're free to build manually of course, but I already released this feature in Maven (4.4.0). You can also directly download the jar from there if you want.
- You can also temporarily set that test class to
@Ignore, so the build skips the part where it tries to start the dummy SMTP server.
Hello. I'm usually not such a nuisance, but this gives me a null pointer exception when I try to run it with the 4.4.0 jar downloaded from Maven, and with the 4.4.1-SNAPSHOT from GitHub, in projects run in both Netbeans and Eclipse.
I've changed the confidential info to dummy info, but otherwise, the following simple test code throws the exception at the "mailer.sendMail(email);" line.
Email email = new Email();
email.setFromAddress("Test Sender", "myemailaddress@mydomain.net");
email.addRecipient("Test Recipient", "myrecipientemailaddress@somedomain.net", RecipientType.TO);
email.setSubject("Test Message Subject");
email.setText("Hello.\n\nThis is a test.");
Mailer mailer = new Mailer("myserver.net", 25, "username", "password");
mailer.sendMail(email);
It appears to be a problem on or around Line 400 of Mailer.java:
scanForInjectionAttack(email.getReplyToRecipient().getName(), "email.replyToRecipient.name");
By inserting the @ignore directive in your MailerLiveTest class, I was able to get simplejavamail to build with code from GitHub. So if you can fix it, I should be able to test it further for you.
Thank you for all of your patience with me.
I'm glad you are a 'nuisance' 👍
Should be solved in 4.4.2, please check if you have some time (might take a few minutes to appear in Maven Central).
I updated the basic test case to include the new headers, rather than adding a new one. So the old test never encountered the NPE. D'oh!
With something like this, it works:
email.addRecipient("John Smith", "jsmith@somewhere.net", RecipientType.TO);
With something like this, it throws an NPE:
email.addRecipients("\"John Smith\" <jsmith@somewhere.net>, \"Jane Doe\" <jsmith@nowhere.com>", RecipientType.TO);
I have to get some sleep now. I'm eight hours behind you! 😕
It seems you are trying to use this method:
public void addRecipients(String recipientName, RecipientType type, String... recipientEmailAddressesToAdd) {}
So it seems you are using the name field to provide addresses.
Any of the following should work for you:
email.addRecipients(RecipientType.TO, "\"John Smith\" <jsmith@somewhere.net>, \"Jane Doe\" <jsmith@nowhere.com>");
email.addRecipients(RecipientType.TO, "\"John Smith\" <jsmith@somewhere.net>", "\"Jane Doe\" <jsmith@nowhere.com>");
email.addRecipients("Default Name", "\"John Smith\" <jsmith@somewhere.net>, \"Jane Doe\" <jsmith@nowhere.com>", RecipientType.TO);
// this one doesn't work yet, though (fixed with the next release):
// email.addRecipients("Default Name", RecipientType.TO, "\"John Smith\" <jsmith@somewhere.net>,\"Jane Doe\" <jsmith@nowhere.com>");
email.addRecipients("Default Name", RecipientType.TO, "\"John Smith\" <jsmith@somewhere.net>", "\"Jane Doe\" <jsmith@nowhere.com>");
But it seems some normalization is in order, with the varargs versions it seems some API has become duplicate.
Currently working on streamlining these methods. I'll have a 4.4.3 out tonight.
Ah, yes. This works for me:
email.addRecipients(RecipientType.TO, "\"John Smith\" <jsmith@somewhere.net>", "\"Jane Doe\" <jsmith@nowhere.com>");
And it provides the functionality that I need.
I find it a bit confusing, though, that your placement of the RecipientType.TO field seems to shift back and forth between the first and last field in the argument list; especially since this works:
emailNormal.addRecipients("twister@sweets.com,blue.tongue@sweets.com;honey@sweets.com", RecipientType.To);
but this does not:
email.addRecipients("\"John Smith\" <jsmith@somewhere.net>", "\"Jane Doe\" <jsmith@nowhere.com>", RecipientType.TO);
I would have built this additional functionality into that same (previous) constructor.
What makes it even more confusing is that it accepts this:
email.addRecipients("\"John Smith\" <jsmith@somewhere.net>", "\"Jane Doe\" <jsmith@nowhere.com>", RecipientType.TO);
But gives a runtime error. (NPE)
Regardless, though, I'm happy. You implemented the functionality that I wanted, and needed, so as not to have to add additional library-specific code to our software in order to accommodate your previously existing constructors.
Also, as a friendly suggestion, please consider whether you really want the Subject to be a required field.
I stumbled across this unexpectedly in my testing.
This leaves me having to decide, "Do I want to display an error message to our users, requiring them to enter text in the 'Subject' field of our GUI, or do I want to have our software simply place '(No Subject)' in the outgoing message." From a user perspective, I think the library should handle this, not me. But admittedly, I'm not a library developer, so I realize that might not be reasonable.
I also discovered in my testing that simply placing code such as this in my program resulted in runtime errors when msgCC and / or msgBCC were empty Strings:
email.addRecipients(RecipientType.CC, msgCC);
email.addRecipients(RecipientType.BCC, msgBCC);
Thus, I had to do error checking for these fields as well, such as the following, to ovoid runtime errors:
// CC:
if (msgCC.isEmpty() == false) {
email.addRecipients(RecipientType.CC, msgCC);
}
// BCC:
if (msgBCC.isEmpty() == false) {
email.addRecipients(RecipientType.BCC, msgBCC);
}
I have not had a chance to test your EmailBuilder additions yet. I will try to do further testing as I have time later today.
Thank you again for all of your hard work. It is very much appreciated.
I would have built this additional functionality into that same (previous) constructor.
That's not possible in Java, as this is solved using the 'varargs...' parameter. This can only be the last parameter of a method signature, or alternatively it should be an explicit array[].
What makes it even more confusing is that it accepts this:
email.addRecipients("\"John Smith\" <jsmith@somewhere.net>", "\"Jane Doe\" <jsmith@nowhere.com>", RecipientType.TO);
It accepts this call because the argument types are correct, but you're giving ""John Smith" jsmith@somewhere.net" as the name. As a name this should still be fine, so the NPE should not happen regardless. I've already done some streamlining in this regard though, so it might be cleared up completely with the next release. Please have a look when it's up.
Other than that, I think it is reasonable to expect of library users to read the Javadoc in case of doubt. Every method is documented and should be clear enough.
Also, as a friendly suggestion, please consider whether you really want the Subject to be a required field.
This is a completely valid point. All clients I know allow empty subjects. I'll change this requirement as it should definitely be optional. #98.
Thus, I had to do error checking for these fields as well, such as the following, to ovoid runtime errors:
I'm not sure why Simple Java Mail should accept invalid input as recipient values? An empty string is not valid input.
I've release 4.4.3. Take a look if you have some time!
Actually I'm going to switch to recipient-type dedicated methods, so the type parameter is not needed at all anymore. That just makes much more sense and the EmailBuilder already does this with .to(), .cc() and .bcc().
Well if you do that, please keep in mind cases like mine, where we don't know if there will be data in those fields, such as CC, BCC, etc.
This is currently what it gets broken down to.
// ========================================================================
// Detailed Constructor.
public String dsSendMail(
String smtpServer,
String smtpUserName,
String smtpPassword,
boolean bUseSmtpAuth,
int smtpPort,
String smtpProtocol,
String msgFrom,
String msgTo,
String msgCC,
String msgBCC,
String msgSubject,
int msgType, // 0 = Html, 1= Plain
String msgBodyPlain,
String msgBodyHTML,
int msgPriority, // 5 = High, 3 = Normal, 1 = Low
boolean bReturnReceiptRequested,
ArrayList<String> emailAttachmentsAL) {
String sErrorMessage = "";
try {
// System.out.println("DsMailManager.dsSendMail");
//
Email email = new Email();
email.setFromAddress(msgFrom, msgFrom);
// Add recipients in a String such as this:
//
// "John Doe" <JohnDoe@OneDomain.net>, "Jane Doe" <JaneDoe@DomainTwo.com>
//
// TO:
if (msgTo.isEmpty() == false) {
email.addRecipients(RecipientType.TO, msgTo);
} else {
return "Error: Recipient(s) TO Required.";
}
// CC:
if (msgCC.isEmpty() == false) {
email.addRecipients(RecipientType.CC, msgCC);
}
// BCC:
if (msgBCC.isEmpty() == false) {
email.addRecipients(RecipientType.BCC, msgBCC);
}
// Subject.
if (msgSubject.isEmpty() == false) {
email.setSubject(msgSubject);
} else {
return "Error: Subject is Required.";
}
// Message Body.
if (msgType == 0) {
// HTML
email.setTextHTML(msgBodyHTML);
} else {
// Plain Text.
// Put a few new lines at the end so if there are any attachments,
// they'll be a break between the text and them.
email.setText(msgBodyPlain + "\n\n");
}
// Set Message Priority.
// Values: 1 = High, 3 = Normal, 5 = Low
email.addHeader("X-Priority", msgPriority);
// Return Receipt Requested.
if (bReturnReceiptRequested) {
email.addHeader("Disposition-Notification-To", msgFrom);
}
// Attachments
// Process attachments, if any.
if (emailAttachmentsAL.size() > 0) {
for (int i = 0; i < emailAttachmentsAL.size(); i++) {
String sFileName = (String) emailAttachmentsAL.get(i);
// System.out.println("Attachment: " + sFileName);
FileDataSource fds = new FileDataSource(sFileName);
String sFileShortName = fds.getName();
email.addAttachment(sFileShortName, fds);
}
}
// Create the Mailer object.
Mailer mailer;
String sProtocol = smtpProtocol.toUpperCase().trim();
if (bUseSmtpAuth) {
// FALSE - Do not use SMTP Authentication.
// System.out.println("Building Mailer Without Authentication.");
mailer = new Mailer(smtpServer, smtpPort, "", "");
} else {
if (sProtocol.equals("SSL")) {
// System.out.println("Building Mailer SSL.");
mailer = new Mailer(smtpServer, smtpPort, smtpUserName, smtpPassword, TransportStrategy.SMTP_SSL);
} else if (sProtocol.equals("TLS")) {
// System.out.println("Building Mailer TLS.");
mailer = new Mailer(smtpServer, smtpPort, smtpUserName, smtpPassword, TransportStrategy.SMTP_TLS);
} else if (sProtocol.equals("PLAIN")) {
// System.out.println("Building Mailer PLAIN.");
mailer = new Mailer(smtpServer, smtpPort, smtpUserName, smtpPassword, TransportStrategy.SMTP_PLAIN);
} else {
// NO TRANSPORT STRATEGY.
// System.out.println("Building Mailer NO TRANSPORT STRATEGY.");
mailer = new Mailer(smtpServer, smtpPort, smtpUserName, smtpPassword);
}
}
// Send the email.
mailer.sendMail(email);
// System.out.println("Message sent OK.");
sErrorMessage = "Message sent OK.";
} catch (Exception e) {
sErrorMessage = e.getCause().toString();
}
return sErrorMessage;
}
And the more I look at our GUI code for our most important application we will use this in, the less I want to rework it to reuse the Mailer and Email objects. It's a multi-threaded application, and I don't see it being worth the effort.
This works, and it's light years better than what we've been using.
While I like the idea of reusing those objects, using a straight-forward constructor like this makes it so much more portable across applications.
But if I can come up with a way to reuse them, I might. I'm managing multiple major projects right now, so the multi-tasking is creating some challenges at this end. Unfortunately.
I hope I'm not trying your patience too much.
You lost me :)
I don't see how we go from "addRecipient methods" to "GUI" and "reusable objects". The code you provided can be updated easily enough though (if I knew what you meant).
I might have lost myself, too! Oops.
I'm sitting here watching our email program hang on the first record as I try to send out our weekly newsletter using simplejavamail code that tested fine this morning using test data... And since it's a multi-threaded application, it isn't outputting the error to me... It makes debugging much more troublesome.
Those Null Pointer problems I found did the same thing. The program just hung, since it occurred in the other thread, I think. I'm not real good at multi-threaded stuff. I don't do this for a living like you do.
I'm wondering if we are running into a problem with this person's email address not validating, or whether it is because her recipient name ends with a space. (I need a .trim() statement, apparently.) Oh, the joys of programming...
"Matushka Angela " hername@abc.bg
Anyway, do you remember how when we started talking last week, how I said ideally, I'd like to try to reuse the Mailer object, and if possible, the Email object, changing only the TO recipient information? That's what I was referring to.
I'm sure it's possible to reuse those objects. I'm just not sure it's going to be worth the trouble. But for now, I need to find out why in the world this isn't working.
Sounds to me you desperately need to configure SLF4J in you application together with either Log4j or Logback!! That way you get everything logged to a file. Then open it in something like Baretail or Logfusion to see it updated live.
That's a wonderful suggestion! I just need to learn how to do that, while I'm also doing 6,000 other things! 😕
In all seriousness, I do appreciate your last suggestion. You gave me valuable information, pointing me in a good direction to take. It's time to call it a night, though. I'll revisit it in the coming days.
After taking a short break, I decided to do some testing within the IDE. Indeed, that space at the end of the recipient name was enough to cause problems, because adding trim() statements fixed that.
Then a few more records down, we had a name that had a comma in it. And in English, this can even be valid. But it was enough to split that name between the recipient.name field and the recipient.address fields, thus causing an error on that record.
So I reverted back to code that I wrote last week to add them as:
email.addRecipient("mom", "jean.baker@hotmail.com", RecipientType.TO);
email.addRecipient("dad", "StevenOakly1963@hotmail.com", RecipientType.TO);
And it sent out all ninety-nine emails without a single error.
So I'm not sure what to say at this point, but this is what I found.
I've written code to parse data for something similar to this before. Let me see if I can come up with a reliable way to parse data such as this. And if I can, I'll send it to you. If that's ok with you?
Regarding the comma in names: It seems safer to just allow only semicolon as delimiter. Or perhaps have comma enabled by default and disable as option in case you really need names with comma's.
Also, I'm surprised a space at the end of a name causes problems. I'll run some tests with that, but I'm a little skeptical to be honest.
I don't think semicolons are safe, either. Not with a simple split() scheme of parsing. I haven't looked at your code, but I'm assuming that's what you're doing.
Why don't you leave this as closed for now, and let me sleep on it. There's no hurry. Ok?
And thanks again for your patience.
You're not the only user of this library, and these changes are live now. As such, allowing only semicolon is safer, until I have a better alternative. That's what I meant. #100.
You may want to try again with 4.4.4. I fixed the delimiter issue, names can now now safely contain comma's or semicolons.