nithril / smtp-connection-pool

SMTP Connection Pool
Apache License 2.0
47 stars 19 forks source link

Supply preconfigured session to factory builder #2

Closed ghost closed 9 years ago

ghost commented 9 years ago

It looks like the username and password is null unless explicity supplied in the Session builder. No?

@Override public PooledObject makeObject() throws Exception { LOG.debug("makeObject");

    Transport transport = getTransport(getSession());

   // host port username and password all null unless provided to builder
  // seperate from session
    transport.connect(getHost(), getPort(), username, password);

    DefaultClosableSmtpConnection closableSmtpTransport = new DefaultClosableSmtpConnection(transport);
    initDefaultListeners(closableSmtpTransport);

    return new DefaultPooledObject(closableSmtpTransport);
}
ghost commented 9 years ago

See ... protected SmtpConnectionFactory(SmtpConnectionFactoryBuilder builder) { this.session = builder.session; this.protocol = builder.protocol; this.host = builder.host; this.port = builder.port; this.username = builder.username != null && builder.username.length() > 0 ? builder.username : null; this.password = builder.password != null && builder.password.length() > 0 ? builder.password : null; this.defaultTransportListeners = new LinkedBlockingDeque<>(builder.defaultTransportListeners); }

nithril commented 9 years ago

You right. Is there an issue about that?

ghost commented 9 years ago

I need to be able to pass just a session. The session already had an authenticator. Then, or just needs .connect() instead of connect with port, host, user, password.

nithril commented 9 years ago

I could add to the builder the method SmtpConnectionFactoryBuilder#session(Session session).

The connect() calls connect(host, port, user, password); with host, user and password set to null and port equals to -1.

I can add a new method to the builder which will initialize an empty builder with protocol, host and port set to the above one

ghost commented 9 years ago

Didn't know that connect could take nulls. Yes, that would solve it. Also for your consideration, to enable the pool to take other versions of the factory, how about a common interface, something like ...

public interface ConnectionFactory {

default Transport getTransport(Session session) throws NoSuchProviderException {
    return getSession().getTransport(getProtocol());
}

default String getProtocol() {
    return getSession().getProperty("mail.transport.protocol");
}

Session getSession();

}

ghost commented 9 years ago

BTW - this is a great work you posted, just figuring how to integrate it, a little customization and it should fit perfectly. :-)

nithril commented 9 years ago

Thanks!

I have made some refactoring to allow a better integration with JavaMail.

For your use case you should be able to do:

SmtpConnectionFactories.newSmtpFactory(yourSession);

The factory will be instanciated with:

You may have a look at the code.

ghost commented 9 years ago

Hey, that's great! I will integrate it later, this should do the trick :-)

nithril commented 9 years ago

I will release the version 1.1.0 once you do not have any more requirements

ghost commented 9 years ago

That's all on my wishlist. :-) Once I get using it for a while I will chime back in with any suggestions.