1
\$\begingroup\$

I have a Spring @Service to asynchronously send emails. The part that seems to be a bit off to me is some code that is specific to our QA environment. We want to send and receive all emails through a test email account so that it does not impact live users if we work with real data imported to QA. I check for the environment and some settings in .properties files and adjust the outgoing email accordingly.

Is there a better way to handle this?

@Async
public void send(String email, String subject, String body) {

    if (email == null) {
        log.error("Not sending; null input: email");
        return;
    }
    try {
        MimeMessage message = this.mailSender.createMimeMessage();
        MimeMessageHelper helper = new MimeMessageHelper(message);
        helper.setFrom(mailSender.getUsername(), getFrom());
        helper.setReplyTo(mailSender.getUsername());
        helper.setSubject(subject);

        //HERE IS THE PART THAT'S IFFY:

        if ("QA".equalsIgnoreCase(EmailManager.getEnvironmentName())) {
            if (isTestMode()) {
                if (!email.equals(getTestEmailAddress())) {
                    helper.setTo(getTestEmailAddress());
                    body += "<br/>This email was originally intended for " + email +
                            " but was redirected to " + getTestEmailAddress() + " for testing.";
                    helper.setText(body, true); //HTML on
                }
            }
        }
        else {
            helper.setTo(email);
            helper.setText(body, true); //HTML on
        }

        if (message == null) {
            log.error("Email message to send is null.  Email={}, Subject={}", email, subject);
            log.error("Body:");
            log.error(body);
            return;
        }
        this.mailSender.send(message);
    }
    catch (MailException | UnsupportedEncodingException | MessagingException e) {
        log.error(e.toString());
        e.printStackTrace();
    }
}

private static String getFrom() {
    Locale defaultLocale = Locale.getDefault();
    ResourceBundle bundle = ResourceBundle.getBundle("messages", defaultLocale);
    return bundle.getString("label.project.name");
}

protected static String getTestEmailAddress() {
    return ResourceBundle.getBundle(AbstractEnvironment.ENVIRONMENT)
            .getString("testEmailAddress");
}

protected static Boolean isTestMode() {
    return Boolean.valueOf(ResourceBundle.getBundle(AbstractEnvironment.ENVIRONMENT)
                                   .getString("redirectAllEmailsToTestEmailAddress"));
}

@Autowired
private JavaMailSenderImpl mailSender;

//setter needed for Junit
public void setMailSender(JavaMailSenderImpl mailSender) {
    this.mailSender = mailSender;
}
\$\endgroup\$
1
\$\begingroup\$

Use Spring's dependency injection more aggressively

You are right to be suspicious about hardcoded environment-specific logic. But you're in luck. This is one of the typical problems that Spring can help you solve.

One way to solve this is to create an interface for controlling the destination email address, and inject an implementation appropriate for the environment. For example:

interface OutgoingEmailController {
    void adjustOutgoingEmail(MimeMessageHelper helper, String email, String body);
}

Implementation to inject in QA:

class QAOutgoingEmailController implements OutgoingEmailController {
    @Override
    public void adjustOutgoingEmail(MimeMessageHelper helper, String email, String body) {
        if (isTestMode()) {
            if (!email.equals(getTestEmailAddress())) {
                helper.setTo(getTestEmailAddress());
                body += "<br/>This email was originally intended for " + email +
                        " but was redirected to " + getTestEmailAddress() + " for testing.";
                helper.setText(body, true); //HTML on
            }
        }
    }
}

Implementation to inject in other environments:

class CommonOutgoingEmailController implements OutgoingEmailController {
    @Override
    public void adjustOutgoingEmail(MimeMessageHelper helper, String email, String body) {
        helper.setTo(email);
        helper.setText(body, true);
    }
}

In your original code you will have an @Autowired OutgoingEmailController outgoingEmailController;, and the IFFY code replaced with outgoingEmailController.adjustOutgoingEmail(helper, email, body);.

Or something like that.

Other remarks

It's odd that you check for null email at the beginning but check for null body at the end. It would be better to move the check on the body further up to the beginning.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That looks much better! Is there a best practice to tell Spring to load the correct environment? (Spring profiles?) You're correct about the null check too. \$\endgroup\$ – vphilipnyc May 17 '15 at 14:44
  • \$\begingroup\$ I don't know. I think it all depends on the XML file you put on your classpath. \$\endgroup\$ – janos May 17 '15 at 14:59
  • \$\begingroup\$ Hmm - not sure I'd want to go with this change until I know the best way for Spring to load the correct environment. Right now, it's ugly, but it works. \$\endgroup\$ – vphilipnyc May 18 '15 at 1:07
  • \$\begingroup\$ @bphilipnyc is correct, profiles are the way to go. Java configuration should be favored over XML configuration also. An alternative to profiles is @Conditional. We faced the exact same problem and used the presence of an environment variable to determine which bean is used to send e-mail (i.e. one sends e-mails to users and one sends them to testers instead). This post was invaluable in getting that set up: javacodegeeks.com/2013/10/spring-4-conditional.html \$\endgroup\$ – Paul Dec 16 '16 at 13:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.