Polish introduction of CredentialsProvider

CredentialsProvider interface is reduced to the bare
minimum of methods (getting username and password).

Test in connection recovery test suite is fixed, as NIO
mode can retrieve username several times for logging.

References #350

Author: acogoluegnes

src/main/java/com/rabbitmq/client/ConnectionFactory.java

@@ -53,7 +53,6 @@
 /**
  * Convenience "factory" class to facilitate opening a {@link Connection} to an AMQP broker.
  */
-
 public class ConnectionFactory implements Cloneable {
 
     /** Default user name */
@@ -100,34 +99,30 @@ public class ConnectionFactory implements Cloneable {
 
     private static final String FALLBACK_TLS_PROTOCOL = "TLSv1";
 
-    private String virtualHost                    = DEFAULT_VHOST;
-    private String host                           = DEFAULT_HOST;
-    private int port                              = USE_DEFAULT_PORT;
-    private int requestedChannelMax               = DEFAULT_CHANNEL_MAX;
-    private int requestedFrameMax                 = DEFAULT_FRAME_MAX;
-    private int requestedHeartbeat                = DEFAULT_HEARTBEAT;
-    private int connectionTimeout                 = DEFAULT_CONNECTION_TIMEOUT;
-    private int handshakeTimeout                  = DEFAULT_HANDSHAKE_TIMEOUT;
-    private int shutdownTimeout                   = DEFAULT_SHUTDOWN_TIMEOUT;
-    private Map<String, Object> _clientProperties = AMQConnection.defaultClientProperties();
-    private SocketFactory factory                 = SocketFactory.getDefault();
-    private SaslConfig saslConfig                 = DefaultSaslConfig.PLAIN;
+    private String virtualHost                      = DEFAULT_VHOST;
+    private String host                             = DEFAULT_HOST;
+    private int port                                = USE_DEFAULT_PORT;
+    private int requestedChannelMax                 = DEFAULT_CHANNEL_MAX;
+    private int requestedFrameMax                   = DEFAULT_FRAME_MAX;
+    private int requestedHeartbeat                  = DEFAULT_HEARTBEAT;
+    private int connectionTimeout                   = DEFAULT_CONNECTION_TIMEOUT;
+    private int handshakeTimeout                    = DEFAULT_HANDSHAKE_TIMEOUT;
+    private int shutdownTimeout                     = DEFAULT_SHUTDOWN_TIMEOUT;
+    private Map<String, Object> _clientProperties   = AMQConnection.defaultClientProperties();
+    private SocketFactory factory                   = SocketFactory.getDefault();
+    private SaslConfig saslConfig                   = DefaultSaslConfig.PLAIN;
     private ExecutorService sharedExecutor;
-    private ThreadFactory threadFactory           = Executors.defaultThreadFactory();
+    private ThreadFactory threadFactory             = Executors.defaultThreadFactory();
     // minimises the number of threads rapid closure of many
     // connections uses, see rabbitmq/rabbitmq-java-client#86
     private ExecutorService shutdownExecutor;
     private ScheduledExecutorService heartbeatExecutor;
-    private SocketConfigurator socketConf         = new DefaultSocketConfigurator();
-    private ExceptionHandler exceptionHandler     = new DefaultExceptionHandler();
-    private CredentialsProvider credentialsProv   = new DefaultCredentialsProvider();
-    {
-        credentialsProv.setUsername(DEFAULT_USER);
-        credentialsProv.setPassword(DEFAULT_PASS);
-    }
+    private SocketConfigurator socketConf           = new DefaultSocketConfigurator();
+    private ExceptionHandler exceptionHandler       = new DefaultExceptionHandler();
+    private CredentialsProvider credentialsProvider = new DefaultCredentialsProvider(DEFAULT_USER, DEFAULT_PASS);
 
-    private boolean automaticRecovery             = true;
-    private boolean topologyRecovery              = true;
+    private boolean automaticRecovery               = true;
+    private boolean topologyRecovery                = true;
 
     // long is used to make sure the users can use both ints
     // and longs safely. It is unlikely that anybody'd need
@@ -190,41 +185,48 @@ public void setPort(int port) {
      * @return the AMQP user name to use when connecting to the broker
      */
     public String getUsername() {
-        return credentialsProv.getUsername();
+        return credentialsProvider.getUsername();
     }
 
     /**
      * Set the user name.
      * @param username the AMQP user name to use when connecting to the broker
      */
     public void setUsername(String username) {
-        credentialsProv.setUsername(username);
+        this.credentialsProvider = new DefaultCredentialsProvider(
+            username,
+            this.credentialsProvider.getPassword()
+        );
     }
 
     /**
      * Retrieve the password.
      * @return the password to use when connecting to the broker
      */
     public String getPassword() {
-        return credentialsProv.getPassword();
+        return credentialsProvider.getPassword();
     }
 
     /**
      * Set the password.
      * @param password the password to use when connecting to the broker
      */
     public void setPassword(String password) {
-        credentialsProv.setPassword(password);
+        this.credentialsProvider = new DefaultCredentialsProvider(
+            this.credentialsProvider.getUsername(),
+            password
+        );
     }
 
     /**
      * Set a custom credentials provider.
+     * Default implementation uses static username and password.
      * @param credentialsProvider The custom implementation of CredentialsProvider to use when connecting to the broker.
      * @see com.rabbitmq.client.impl.DefaultCredentialsProvider
-     * @see com.rabbitmq.client.impl.AbstractCredentialsProvider
+     * @since 4.5.0
      */
     public void setCredentialsProvider(CredentialsProvider credentialsProvider) {
-        this.credentialsProv = credentialsProvider;
+        this.credentialsProvider = credentialsProvider;
     }
 
     /**
@@ -982,7 +984,7 @@ public Connection newConnection(ExecutorService executor, AddressResolver addres
     public ConnectionParams params(ExecutorService consumerWorkServiceExecutor) {
         ConnectionParams result = new ConnectionParams();
 
-        result.setCredentialsProvider(credentialsProv);
+        result.setCredentialsProvider(credentialsProvider);
         result.setConsumerWorkServiceExecutor(consumerWorkServiceExecutor);
         result.setVirtualHost(virtualHost);
         result.setClientProperties(getClientProperties());
@@ -1081,7 +1083,6 @@ protected AddressResolver createAddressResolver(List<Address> addresses) {
     @Override public ConnectionFactory clone(){
         try {
             ConnectionFactory clone = (ConnectionFactory)super.clone();
-            clone.credentialsProv = (CredentialsProvider)clone.credentialsProv.clone();
             return clone;
         } catch (CloneNotSupportedException e) {
             throw new Error(e);

src/main/java/com/rabbitmq/client/impl/AMQConnection.java

@@ -1021,7 +1021,7 @@ public AMQCommand transformReply(AMQCommand command) {
 
     @Override public String toString() {
         final String virtualHost = "/".equals(_virtualHost) ? _virtualHost : "/" + _virtualHost;
-        return "amqp://" + credentialsProvider.getUsername() + "@" + getHostAddress() + ":" + getPort() + virtualHost;
+        return "amqp://" + this.credentialsProvider.getUsername() + "@" + getHostAddress() + ":" + getPort() + virtualHost;
     }
 
     private String getHostAddress() {

src/main/java/com/rabbitmq/client/impl/AbstractCredentialsProvider.java

@@ -4,19 +4,13 @@
  * Provider interface for establishing credentials for connecting to the broker. Especially useful
  * for situations where credentials might change before a recovery takes place or where it is 
  * convenient to plug in an outside custom implementation.
- * 
- * @see com.rabbitmq.client.impl.AbstractCredentialsProvider
+ *
+ * @since 4.5.0
  */
-public interface CredentialsProvider extends Cloneable {
+public interface CredentialsProvider {
 
     String getUsername();
 
     String getPassword();
 
-    void setUsername(String username);
-
-    void setPassword(String password);
-
-    Object clone() throws CloneNotSupportedException;
-    
 }

src/main/java/com/rabbitmq/client/impl/CredentialsProvider.java

@@ -3,12 +3,19 @@
 /**
  * Default implementation of a CredentialsProvider which simply holds a static
  * username and password.
+ *
+ * @since 4.5.0
  */
-public class DefaultCredentialsProvider extends AbstractCredentialsProvider {
+public class DefaultCredentialsProvider implements CredentialsProvider {
+
+    private final String username;
+    private final String password;
+
+    public DefaultCredentialsProvider(String username, String password) {
+        this.username = username;
+        this.password = password;
+    }
 
-    protected String username;
-    protected String password;
-    
     @Override
     public String getUsername() {
         return username;
@@ -19,14 +26,4 @@ public String getPassword() {
         return password;
     }
 
-    @Override
-    public void setUsername(String username) {
-        this.username = username;
-    }
-    
-    @Override
-    public void setPassword(String password) {
-        this.password = password;
-    }
-    
 }

src/main/java/com/rabbitmq/client/impl/DefaultCredentialsProvider.java

@@ -16,7 +16,7 @@
 package com.rabbitmq.client.test.functional;
 
 import com.rabbitmq.client.*;
-import com.rabbitmq.client.impl.AbstractCredentialsProvider;
+import com.rabbitmq.client.impl.CredentialsProvider;
 import com.rabbitmq.client.impl.NetworkConnection;
 import com.rabbitmq.client.impl.recovery.*;
 import com.rabbitmq.client.test.BrokerTestCase;
@@ -37,6 +37,8 @@
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
 
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
+import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.*;
 
 @SuppressWarnings("ThrowFromFinallyBlock")
@@ -123,14 +125,15 @@ public class ConnectionRecovery extends BrokerTestCase {
         }
     }
 
-    // See https://github.com/rabbitmq/rabbitmq-java-client/pull/350 . We want to request fresh creds when recovering.
+    // See https://github.com/rabbitmq/rabbitmq-java-client/pull/350 .
+    // We want to request fresh creds when recovering.
     @Test public void connectionRecoveryRequestsCredentialsAgain() throws Exception {
         ConnectionFactory cf = buildConnectionFactoryWithRecoveryEnabled(false);
         final String username = cf.getUsername();
         final String password = cf.getPassword();
-        final AtomicLong usernameRequested = new AtomicLong(0);
-        final AtomicLong passwordRequested = new AtomicLong(0);
-        cf.setCredentialsProvider(new AbstractCredentialsProvider() {
+        final AtomicInteger usernameRequested = new AtomicInteger(0);
+        final AtomicInteger passwordRequested = new AtomicInteger(0);
+        cf.setCredentialsProvider(new CredentialsProvider() {
 
             @Override
             public String getUsername() {
@@ -147,13 +150,14 @@ public String getPassword() {
         RecoverableConnection c = (RecoverableConnection) cf.newConnection();
         try {
             assertTrue(c.isOpen());
-            assertEquals(1, usernameRequested.get());
-            assertEquals(1, passwordRequested.get());
-            
+            assertThat(usernameRequested.get(), is(1));
+            assertThat(passwordRequested.get(), is(1));
+
             closeAndWaitForRecovery(c);
             assertTrue(c.isOpen());
-            assertEquals(2, usernameRequested.get());
-            assertEquals(2, passwordRequested.get());
+            // username is requested in AMQConnection#toString, so it can be accessed at any time
+            assertThat(usernameRequested.get(), greaterThanOrEqualTo(2));
+            assertThat(passwordRequested.get(), is(2));
         } finally {
             c.abort();
         }

src/test/java/com/rabbitmq/client/test/functional/ConnectionRecovery.java

@@ -122,7 +122,7 @@ public void connectionCloseAuthFailure(String username, String password) throws
     // to be reported by the broker by closing the connection
     private Connection connectionWithoutCapabilities(String username, String password)
             throws IOException, TimeoutException {
-        ConnectionFactory customFactory = connectionFactory.clone();
+        ConnectionFactory customFactory = TestUtils.connectionFactory();
         customFactory.setUsername(username);
         customFactory.setPassword(password);
         Map<String, Object> customProperties = AMQConnection.defaultClientProperties();