playframework / play-mailer

Play mailer plugin
Apache License 2.0
250 stars 74 forks source link

Play2.4 Field Injection always return null MailerClient #78

Open piacia opened 8 years ago

piacia commented 8 years ago

Was following the Java example on home page, use Field injection in Play 2.4. But in debug it seems always have null for MailerClient.

code looks like this:

class EnvelopJob implements Runnable {
        Mail.Envelop envelop;

        @Inject MailerClient mailerClient;

        public EnvelopJob(Mail.Envelop envelop) {
            this.envelop = envelop;
        }

        public void run() {
            Email email = new Email();

            final Configuration root = Configuration.root();
            final String mailFrom = root.getString("mail.from");
            email.setFrom(mailFrom);
            email.setSubject(envelop.subject);
            for (String toEmail : envelop.toEmails) {
                email.addTo(toEmail);
                Logger.debug("Mail.sendMail: Mail will be sent to " + toEmail);
            }

            final String mailSign = root.getString("mail.sign");
            email.setBodyText(envelop.message + "\n\n " + mailSign);
            mailerClient.send(email);
        }
}
ggrossetie commented 8 years ago

Hello @piacia, which version do you use ?

piacia commented 8 years ago

4.0.0-M1 and play 2.4.2

piacia commented 8 years ago

Then I was trying to do something like this:

private static SMTPConfiguration smtpConfiguration;
    static {
        final Configuration root = Configuration.root();
        smtpConfiguration = new SMTPConfiguration(root.getString("smtp.host"),
                root.getInt("smtp.port"),
                root.getBoolean("smtp.ssl"),
                root.getBoolean("smtp.tls"),
                Option.apply(root.getString("smtp.user")),
                Option.apply(root.getString("smtp.password")),
                root.getBoolean("smtp.debug"),
                Option.apply(root.getInt("smtp.timeout")),
                Option.apply(root.getInt("smtp.connectionTimeout")),
                root.getBoolean("smtp.mock"));
    }

new SMTPMailer(smtpConfiguration).send(email);

but could not get it working. Tried all 465, 587, 25 with ssl tls possibilities

piacia commented 8 years ago

looks like it is because I am using it in models, but not controllers. Is Guice the only way if I want to use mailer in models?

ggrossetie commented 8 years ago

You can use the static method send on MailerPlugin: https://github.com/playframework/play-mailer/blob/master/src/main/java/play/libs/mailer/MailerPlugin.java#L14

You can also instantiate a SMTPMailer with a SMTPConfiguration parameter like what you've done.

but could not get it working. Tried all 465, 587, 25 with ssl tls possibilities

This should work, do you get an exception ?

alaouy commented 8 years ago

I had the same problem, for some reason injecting the MailerClient as a dependancy into the model always returned null , the solution as @Mogztter suggested is to manually inject it like this

return Play.application().injector().instanceOf(MailerClient.class).send(email);
schmitch commented 8 years ago

This is a problem caused by the constructor:

 @Inject MailerClient mailerClient;

        public EnvelopJob(Mail.Envelop envelop) {
            this.envelop = envelop;
        }

You can't have a noninjected constructor with a injected field. that is not possible. Also I think you guys try to call the class with new EnvelopJob() which doesn't work, too. The class needs to be injected.

ggrossetie commented 8 years ago

@piacia @alaouy @schmitch Do you think we need to improve the documentation or can we close this issue/question ?

schmitch commented 8 years ago

@Mogztter I wouldn't improve the documentation since that is guice specific. The only thing maybe would be a link to the Guice documentation, somewhere.

luukjansen commented 8 years ago

Can I just make a comment on this. It look me a while to realise this. I am new to Play! 2,4 (having worked a with 1.x) and the @Inject is new. I created a class to log errors and mail them to an admin address, and this is not a controller, and I got the NullPointerException.

Using Play.application().injector().instanceOf(MailerClient.class).send(email); works fine, but that is not clear form the current documentation as it suggests you can inject in any class (I just copied the example class).

schmitch commented 8 years ago

@luukjansen you should avoid Play.application() and yes you can Inject in every class, as soon as you resolve the class via an injector, you should definitly take a look at the guice docs, how the dependency graph ist built, etc..

What that means is, if you have a class X like that:

class X(blabla: String) {

@Inject
var field: AnotherClass
}

if you now try to create the class via new X("Hase") that can't work since guice doesn't know anything about the class creation. Guice needs to create the class. (https://github.com/google/guice/wiki/Motivation)

luukjansen commented 8 years ago

This might not be the right place to ask, but could you provide an example in Java on how to instantiate a class with the MailerClient correctly? This is all new to me, and I am not getting my head around it yet.

I took the example on GitHub, so am trying to figure out how to insatiate this properly without using Play.application().

I tried:

Injector injector = Guice.createInjector(new MailerClient());
MyComponent myComponent = injector.getInstance(MyComponent);

But that doesn't work as MailerClient is abstract.

ggrossetie commented 8 years ago

Even if this is more Guice related, I think we should add documentation. The idea is not to copy-paste Guice documentation but to explain how to setup the mailer plugin in the context of Runtime DI (with Guice).

@luukjansen To answer your question, you can have a look here https://github.com/playframework/play-mailer/tree/master/samples/runtimeDI Le 9 févr. 2016 12:36 PM, "luukjansen" notifications@github.com a écrit :

This might not be the right place to ask, but could you provide an example in Java on how to instantiate a class with the MailerClient correctly? This is all new to me, and I am not getting my head around it yet.

I took the example on GitHub, so am trying to figure out how to insatiate this properly without using Play.application().

I tried:

Injector injector = Guice.createInjector(new MailerClient()); MyComponent myComponent = injector.getInstance(MyComponent);

But that doesn't work as MailerClient is abstract.

— Reply to this email directly or view it on GitHub https://github.com/playframework/play-mailer/issues/78#issuecomment-181827252 .

schmitch commented 8 years ago

@Mogztter and how would you do it, I mean setting up a mailer is just:

class MyService @Inject()(mailerClient: MailerClient) {
}

or on java:

class MyService {

private final MailerClient mailerClient;

@Inject
MyService(mailerClient: MailerClient) {
this.mailerClient = mailerClient;
}
}

And the Java one is a copy of the guice docs, however thanks to reference.conf we could skip the module creation. We also have something in the Readme: https://github.com/playframework/play-mailer#java (which works on the field level)

The thing is, why it's not working for them is cause they are trying that:

Component myComponent = new MyComponent();

which of course would fail since guice doesn't create the instance.

luukjansen commented 8 years ago

I think that is exactly the step I am looking for. How do I create an instance using Guice. Injection works in a controller, I have used that before with WS.

But how do I instantiate the class and sent an email (e.g like above and in the example) without a controller. I am just not getting it for some reason, I tried following the guise documentation, but am missing something on how to properly instantiate.

So e.g. like below (but then to instantiate with guice)

class MyService {

private final MailerClient mailerClient;

public class MyComponent {
  @Inject MailerClient mailerClient;

  public void sendEmail() {
    String cid = "1234";
    Email email = new Email()
      .setSubject("Simple email")
      .setFrom("Mister FROM <from@email.com>")
      .addTo("Miss TO <to@email.com>")
      .addAttachment("attachment.pdf", new File("/some/path/attachment.pdf"))
      .addAttachment("data.txt", "data".getBytes(), "text/plain", "Simple data", EmailAttachment.INLINE)
      .addAttachment("image.jpg", new File("/some/path/image.jpg"), cid)
      .setBodyText("A text message")
      .setBodyHtml("<html><body><p>An <b>html</b> message with cid <img src=\"cid:" + cid + "\"></p></body></html>");
    mailerClient.send(email);
  }
}

public ClassX {
    public sent email(){
         MyComponent component = new MyComponent();
         component.sentEmail();
    }  
}
schmitch commented 8 years ago

@luukjansen you should never create a class which injects something yourself. Which means, you have something like that:


public class MyController {
@Inject
private MyService service;

public Result sendMail() {
service.doMail();
return ok("Mail sent!");
}

}

public class MyService {

@Inject
private MailerClient mailerClient;

public void doMail() {
    String cid = "1234";
    Email email = new Email()
      .setSubject("Simple email")
      .setFrom("Mister FROM <from@email.com>")
      .addTo("Miss TO <to@email.com>")
      .addAttachment("attachment.pdf", new File("/some/path/attachment.pdf"))
      .addAttachment("data.txt", "data".getBytes(), "text/plain", "Simple data", EmailAttachment.INLINE)
      .addAttachment("image.jpg", new File("/some/path/image.jpg"), cid)
      .setBodyText("A text message")
      .setBodyHtml("<html><body><p>An <b>html</b> message with cid <img src=\"cid:" + cid + "\"></p></body></html>");
    mailerClient.send(email);
}

}

Now MyService gets injected inside your Controller and MyService gets an instance of mailerClient injected. If you now need another class, lets call it like you did, ClassX then you would need to inject that class, too. i.e:

public ClassX {
   @Inject
   private MyService myService;

   public void sentMail() {
      myService.sendEmail();
   }

}

// Somewhere else:
public ServiceXYZ {
   @Inject
   private ClassX classX;

   public void doSomething() {
     classX.sentMail();
   }

}

as a rule of thumb, when using DI you should only create data classes yourself, and data classes should never contain a service or something that got injected in any way.

luukjansen commented 8 years ago

@schmitch thanks a million, I think it becomes clearer. I implemented the above, and sending message leading back to a controller works fine now.

One final question then, and hopefully my difficulty might show difficulties other 'amateurs' might have. I user a Logger, much like the Play.Logger, and call stoically e.g. Logger.warn("Test", e) ;

I want top sent wanring email to the administrator, so I took the suggestion above to do this in the following way. But you mentioned top avoid to relying on Play.application(). Would that be necessary in such a case though:

Play.application().injector().instanceOf(Mails.class).sendLoggerEmail(message, t);

This because I fail to create a specific guice injector because the classes are not modules (e.g. implement AbstractModule) yo instantiate the Mails.class through guice.

schmitch commented 8 years ago

Actually you could try to create a LoggerService.

Something like that:


public class LoggerService {
    @Inject
    private MailerClient mailerClient;

    private Logger logger = new Logger();

    public void warn(String msg) {
       logger.warn(msg);
    }

}

// Class with LoggerService
public class MySuperService {

@Inject
private LoggerService logger;

public void doSomething() {
// do something
logger.warn("there was an error!");
}

}

Than you could inject the LoggerService to your services. Is a little bit boilerplate. There are other ways like here: https://github.com/google/guice/wiki/CustomInjections However you would've need a good understanding of dependency injection to do it 'correct'.

luukjansen commented 8 years ago

@schmitch thanks for the help. Everything seems to work fine now using @Inject with Controllers etc. In the rare occasion of the Logger I used the Play.application().injector() for the moment to generate the class guice dependent. And it seems all fine! Thanks a million!