Make RabbitProperties’ parsed address behaviour the same for all properties

Prior to this commit parsing addresses had an inconsistent effect on the
various properties that can be contained in an address (host, port,
username, password, and virtual host).

Three different approaches were used:

1. Return the property if no addresses were set. If there was one
   address set, return the property from the address. Otherwise return
   null. (host)
2. Return the property if no address were set, otherwise return the
   property from the first address (port)
3. Return the property if no addresses were set, otherwise return the
   property from the last address that had such property (username,
   password, virtual host).

This commit aims to make the behaviour consistent. If no addresses
were set the property is returned. Otherwise the value extracted from
the first address is returned. If the first address has no such value
the property is returned.

Closes gh-6424

spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfiguration.java

@@ -90,18 +90,19 @@ public class RabbitAutoConfiguration {
 		public CachingConnectionFactory rabbitConnectionFactory(RabbitProperties config)
 				throws Exception {
 			RabbitConnectionFactoryBean factory = new RabbitConnectionFactoryBean();
-			if (config.getHost() != null) {
-				factory.setHost(config.getHost());
-				factory.setPort(config.getPort());
+			if (config.determineHost() != null) {
+				factory.setHost(config.determineHost());
 			}
-			if (config.getUsername() != null) {
-				factory.setUsername(config.getUsername());
+			factory.setPort(config.determinePort());
+			factory.setHost(config.determineHost());
+			if (config.determineUsername() != null) {
+				factory.setUsername(config.determineUsername());
 			}
-			if (config.getPassword() != null) {
-				factory.setPassword(config.getPassword());
+			if (config.determinePassword() != null) {
+				factory.setPassword(config.determinePassword());
 			}
-			if (config.getVirtualHost() != null) {
-				factory.setVirtualHost(config.getVirtualHost());
+			if (config.determineVirtualHost() != null) {
+				factory.setVirtualHost(config.determineVirtualHost());
 			}
 			if (config.getRequestedHeartbeat() != null) {
 				factory.setRequestedHeartbeat(config.getRequestedHeartbeat());
@@ -123,7 +124,7 @@ public class RabbitAutoConfiguration {
 			factory.afterPropertiesSet();
 			CachingConnectionFactory connectionFactory = new CachingConnectionFactory(
 					factory.getObject());
-			connectionFactory.setAddresses(config.getAddresses());
+			connectionFactory.setAddresses(config.determineAddresses());
 			connectionFactory.setPublisherConfirms(config.isPublisherConfirms());
 			connectionFactory.setPublisherReturns(config.isPublisherReturns());
 			if (config.getCache().getChannel().getSize() != null) {

spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java

@@ -16,13 +16,14 @@
 
 package org.springframework.boot.autoconfigure.amqp;
 
-import java.util.LinkedHashSet;
-import java.util.Set;
+import java.util.ArrayList;
+import java.util.List;
 
 import org.springframework.amqp.core.AcknowledgeMode;
 import org.springframework.amqp.rabbit.connection.CachingConnectionFactory.CacheMode;
 import org.springframework.boot.context.properties.ConfigurationProperties;
 import org.springframework.boot.context.properties.NestedConfigurationProperty;
+import org.springframework.util.CollectionUtils;
 import org.springframework.util.StringUtils;
 
 /**
@@ -105,15 +106,24 @@ public class RabbitProperties {
 
 	private final Template template = new Template();
 
+	private List<Address> parsedAddresses;
+
 	public String getHost() {
-		if (this.addresses == null) {
-			return this.host;
-		}
-		String[] hosts = StringUtils.delimitedListToStringArray(this.addresses, ":");
-		if (hosts.length == 2) {
-			return hosts[0];
+		return this.host;
+	}
+
+	/**
+	 * Returns the host from the first address, or the configured host if no addresses
+	 * have been set.
+	 * @return the host
+	 * @see #setAddresses(String)
+	 * @see #getHost()
+	 */
+	public String determineHost() {
+		if (CollectionUtils.isEmpty(this.parsedAddresses)) {
+			return getHost();
 		}
-		return null;
+		return this.parsedAddresses.get(0).host;
 	}
 
 	public void setHost(String host) {
@@ -121,64 +131,80 @@ public class RabbitProperties {
 	}
 
 	public int getPort() {
-		if (this.addresses == null) {
-			return this.port;
-		}
-		String[] hosts = StringUtils.delimitedListToStringArray(this.addresses, ":");
-		if (hosts.length >= 2) {
-			return Integer
-					.valueOf(StringUtils.commaDelimitedListToStringArray(hosts[1])[0]);
-		}
 		return this.port;
 	}
 
-	public void setAddresses(String addresses) {
-		this.addresses = parseAddresses(addresses);
+	/**
+	 * Returns the port from the first address, or the configured port if no addresses
+	 * have been set.
+	 * @return the port
+	 * @see #setAddresses(String)
+	 * @see #getPort()
+	 */
+	public int determinePort() {
+		if (CollectionUtils.isEmpty(this.parsedAddresses)) {
+			return getPort();
+		}
+		Address address = this.parsedAddresses.get(0);
+		return address.port;
+	}
+
+	public void setPort(int port) {
+		this.port = port;
 	}
 
 	public String getAddresses() {
-		return (this.addresses == null ? this.host + ":" + this.port : this.addresses);
+		return this.addresses;
 	}
 
-	private String parseAddresses(String addresses) {
-		Set<String> result = new LinkedHashSet<String>();
-		for (String address : StringUtils.commaDelimitedListToStringArray(addresses)) {
-			address = address.trim();
-			if (address.startsWith("amqp://")) {
-				address = address.substring("amqp://".length());
-			}
-			if (address.contains("@")) {
-				String[] split = StringUtils.split(address, "@");
-				String creds = split[0];
-				address = split[1];
-				split = StringUtils.split(creds, ":");
-				this.username = split[0];
-				if (split.length > 0) {
-					this.password = split[1];
-				}
-			}
-			int index = address.indexOf("/");
-			if (index >= 0 && index < address.length()) {
-				setVirtualHost(address.substring(index + 1));
-				address = address.substring(0, index);
-			}
-			if (!address.contains(":")) {
-				address = address + ":" + this.port;
-			}
-			result.add(address);
+	/**
+	 * Returns the comma-separated addresses or a single address ({@code host:port})
+	 * created from the configured host and port if no addresses have been set.
+	 * @return the addresses
+	 */
+	public String determineAddresses() {
+		if (CollectionUtils.isEmpty(this.parsedAddresses)) {
+			return this.host + ":" + this.port;
+		}
+		List<String> addressStrings = new ArrayList<String>();
+		for (Address parsedAddress : this.parsedAddresses) {
+			addressStrings.add(parsedAddress.host + ":" + parsedAddress.port);
 		}
-		return (result.isEmpty() ? null
-				: StringUtils.collectionToCommaDelimitedString(result));
+		return StringUtils.collectionToCommaDelimitedString(addressStrings);
 	}
 
-	public void setPort(int port) {
-		this.port = port;
+	public void setAddresses(String addresses) {
+		this.addresses = addresses;
+		this.parsedAddresses = parseAddresses(addresses);
+	}
+
+	private List<Address> parseAddresses(String addresses) {
+		List<Address> parsedAddresses = new ArrayList<Address>();
+		for (String address : StringUtils.commaDelimitedListToStringArray(addresses)) {
+			parsedAddresses.add(new Address(address));
+		}
+		return parsedAddresses;
 	}
 
 	public String getUsername() {
 		return this.username;
 	}
 
+	/**
+	 * If addresses have been set and the first address has a username it is returned.
+	 * Otherwise returns the result of calling {@code getUsername()}.
+	 * @return the username
+	 * @see #setAddresses(String)
+	 * @see #getUsername()
+	 */
+	public String determineUsername() {
+		if (CollectionUtils.isEmpty(this.parsedAddresses)) {
+			return this.username;
+		}
+		Address address = this.parsedAddresses.get(0);
+		return address.username == null ? this.username : address.username;
+	}
+
 	public void setUsername(String username) {
 		this.username = username;
 	}
@@ -187,6 +213,21 @@ public class RabbitProperties {
 		return this.password;
 	}
 
+	/**
+	 * If addresses have been set and the first address has a password it is returned.
+	 * Otherwise returns the result of calling {@code getPassword()}.
+	 * @return the password or {@code null}
+	 * @see #setAddresses(String)
+	 * @see #getPassword()
+	 */
+	public String determinePassword() {
+		if (CollectionUtils.isEmpty(this.parsedAddresses)) {
+			return getPassword();
+		}
+		Address address = this.parsedAddresses.get(0);
+		return address.password == null ? getPassword() : address.password;
+	}
+
 	public void setPassword(String password) {
 		this.password = password;
 	}
@@ -199,6 +240,21 @@ public class RabbitProperties {
 		return this.virtualHost;
 	}
 
+	/**
+	 * If addresses have been set and the first address has a virtual host it is returned.
+	 * Otherwise returns the result of calling {@code getVirtualHost()}.
+	 * @return the password or {@code null}
+	 * @see #setAddresses(String)
+	 * @see #getVirtualHost()
+	 */
+	public String determineVirtualHost() {
+		if (CollectionUtils.isEmpty(this.parsedAddresses)) {
+			return getVirtualHost();
+		}
+		Address address = this.parsedAddresses.get(0);
+		return address.virtualHost == null ? getVirtualHost() : address.virtualHost;
+	}
+
 	public void setVirtualHost(String virtualHost) {
 		this.virtualHost = ("".equals(virtualHost) ? "/" : virtualHost);
 	}
@@ -652,4 +708,75 @@ public class RabbitProperties {
 
 	}
 
+	private static final class Address {
+
+		private static final String PREFIX_AMQP = "amqp://";
+
+		private static final int DEFAULT_PORT = 5672;
+
+		private String host;
+
+		private int port;
+
+		private String username;
+
+		private String password;
+
+		private String virtualHost;
+
+		private Address(String input) {
+			input = input.trim();
+			input = trimPrefix(input);
+			input = parseUsernameAndPassword(input);
+			input = parseVirtualHost(input);
+			parseHostAndPort(input);
+		}
+
+		private String trimPrefix(String input) {
+			if (input.startsWith(PREFIX_AMQP)) {
+				input = input.substring(PREFIX_AMQP.length());
+			}
+			return input;
+		}
+
+		private String parseUsernameAndPassword(String input) {
+			if (input.contains("@")) {
+				String[] split = StringUtils.split(input, "@");
+				String creds = split[0];
+				input = split[1];
+				split = StringUtils.split(creds, ":");
+				this.username = split[0];
+				if (split.length > 0) {
+					this.password = split[1];
+				}
+			}
+			return input;
+		}
+
+		private String parseVirtualHost(String input) {
+			int hostIndex = input.indexOf("/");
+			if (hostIndex >= 0 && hostIndex < input.length()) {
+				this.virtualHost = input.substring(hostIndex + 1);
+				if (this.virtualHost.length() == 0) {
+					this.virtualHost = "/";
+				}
+				input = input.substring(0, hostIndex);
+			}
+			return input;
+		}
+
+		private void parseHostAndPort(String input) {
+			int portIndex = input.indexOf(':');
+			if (portIndex == -1) {
+				this.host = input;
+				this.port = DEFAULT_PORT;
+			}
+			else {
+				this.host = input.substring(0, portIndex);
+				this.port = Integer.valueOf(input.substring(portIndex + 1));
+			}
+		}
+
+	}
+
 }

spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfigurationTests.java

@@ -19,6 +19,7 @@ package org.springframework.boot.autoconfigure.amqp;
 import javax.net.SocketFactory;
 import javax.net.ssl.SSLSocketFactory;
 
+import com.rabbitmq.client.Address;
 import org.aopalliance.aop.Advice;
 import org.junit.After;
 import org.junit.Rule;
@@ -112,6 +113,8 @@ public class RabbitAutoConfigurationTests {
 		com.rabbitmq.client.ConnectionFactory rcf = (com.rabbitmq.client.ConnectionFactory) dfa
 				.getPropertyValue("rabbitConnectionFactory");
 		assertThat(rcf.getConnectionTimeout()).isEqualTo(123);
+		assertThat((Address[]) dfa.getPropertyValue("addresses")).hasSize(1);
+
 	}
 
 	@Test

spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/amqp/RabbitPropertiesTests.java

@@ -31,91 +31,199 @@ public class RabbitPropertiesTests {
 	private final RabbitProperties properties = new RabbitProperties();
 
 	@Test
-	public void addressesNotSet() {
+	public void hostDefaultsToLocalhost() {
 		assertThat(this.properties.getHost()).isEqualTo("localhost");
+	}
+
+	@Test
+	public void customHost() {
+		this.properties.setHost("rabbit.example.com");
+		assertThat(this.properties.getHost()).isEqualTo("rabbit.example.com");
+	}
+
+	@Test
+	public void hostIsDeterminedFromFirstAddress() {
+		this.properties.setAddresses("rabbit1.example.com:1234,rabbit2.example.com:2345");
+		assertThat(this.properties.determineHost()).isEqualTo("rabbit1.example.com");
+	}
+
+	@Test
+	public void determineHostReturnsHostPropertyWhenNoAddresses() {
+		this.properties.setHost("rabbit.example.com");
+		assertThat(this.properties.determineHost()).isEqualTo("rabbit.example.com");
+	}
+
+	@Test
+	public void portDefaultsTo5672() {
 		assertThat(this.properties.getPort()).isEqualTo(5672);
 	}
 
 	@Test
-	public void addressesSingleValued() {
-		this.properties.setAddresses("myhost:9999");
-		assertThat(this.properties.getHost()).isEqualTo("myhost");
-		assertThat(this.properties.getPort()).isEqualTo(9999);
+	public void customPort() {
+		this.properties.setPort(1234);
+		assertThat(this.properties.getPort()).isEqualTo(1234);
 	}
 
 	@Test
-	public void addressesDoubleValued() {
-		this.properties.setAddresses("myhost:9999,otherhost:1111");
-		assertThat(this.properties.getHost()).isNull();
-		assertThat(this.properties.getPort()).isEqualTo(9999);
+	public void determinePortReturnsPortOfFirstAddress() {
+		this.properties.setAddresses("rabbit1.example.com:1234,rabbit2.example.com:2345");
+		assertThat(this.properties.determinePort()).isEqualTo(1234);
 	}
 
 	@Test
-	public void addressesDoubleValuedWithCredentials() {
-		this.properties.setAddresses("myhost:9999,root:password@otherhost:1111/host");
-		assertThat(this.properties.getHost()).isNull();
-		assertThat(this.properties.getPort()).isEqualTo(9999);
-		assertThat(this.properties.getUsername()).isEqualTo("root");
-		assertThat(this.properties.getVirtualHost()).isEqualTo("host");
+	public void determinePortReturnsPortPropertyWhenNoAddresses() {
+		this.properties.setPort(1234);
+		assertThat(this.properties.determinePort()).isEqualTo(1234);
 	}
 
 	@Test
-	public void addressesDoubleValuedPreservesOrder() {
-		this.properties.setAddresses("myhost:9999,ahost:1111/host");
-		assertThat(this.properties.getHost()).isNull();
-		assertThat(this.properties.getAddresses()).isEqualTo("myhost:9999,ahost:1111");
+	public void determinePortReturnsDefaultAmqpPortWhenFirstAddressHasNoExplicitPort() {
+		this.properties.setPort(1234);
+		this.properties.setAddresses("rabbit1.example.com,rabbit2.example.com:2345");
+		assertThat(this.properties.determinePort()).isEqualTo(5672);
 	}
 
 	@Test
-	public void addressesSingleValuedWithCredentials() {
-		this.properties.setAddresses("amqp://root:password@otherhost:1111/host");
-		assertThat(this.properties.getHost()).isEqualTo("otherhost");
-		assertThat(this.properties.getPort()).isEqualTo(1111);
-		assertThat(this.properties.getUsername()).isEqualTo("root");
-		assertThat(this.properties.getVirtualHost()).isEqualTo("host");
+	public void virtualHostDefaultsToNull() {
+		assertThat(this.properties.getVirtualHost()).isNull();
 	}
 
 	@Test
-	public void addressesSingleValuedWithCredentialsDefaultPort() {
-		this.properties.setAddresses("amqp://root:password@lemur.cloudamqp.com/host");
-		assertThat(this.properties.getHost()).isEqualTo("lemur.cloudamqp.com");
-		assertThat(this.properties.getPort()).isEqualTo(5672);
-		assertThat(this.properties.getUsername()).isEqualTo("root");
-		assertThat(this.properties.getVirtualHost()).isEqualTo("host");
-		assertThat(this.properties.getAddresses()).isEqualTo("lemur.cloudamqp.com:5672");
+	public void customVirtualHost() {
+		this.properties.setVirtualHost("alpha");
+		assertThat(this.properties.getVirtualHost()).isEqualTo("alpha");
 	}
 
 	@Test
-	public void addressWithTrailingSlash() {
-		this.properties.setAddresses("amqp://root:password@otherhost:1111/");
-		assertThat(this.properties.getHost()).isEqualTo("otherhost");
-		assertThat(this.properties.getPort()).isEqualTo(1111);
-		assertThat(this.properties.getUsername()).isEqualTo("root");
-		assertThat(this.properties.getVirtualHost()).isEqualTo("/");
+	public void virtualHostRetainsALeadingSlash() {
+		this.properties.setVirtualHost("/alpha");
+		assertThat(this.properties.getVirtualHost()).isEqualTo("/alpha");
 	}
 
 	@Test
-	public void testDefaultVirtualHost() {
-		this.properties.setVirtualHost("/");
-		assertThat(this.properties.getVirtualHost()).isEqualTo("/");
+	public void determineVirtualHostReturnsVirtualHostOfFirstAddress() {
+		this.properties.setAddresses(
+				"rabbit1.example.com:1234/alpha,rabbit2.example.com:2345/bravo");
+		assertThat(this.properties.determineVirtualHost()).isEqualTo("alpha");
 	}
 
 	@Test
-	public void testEmptyVirtualHost() {
+	public void determineVirtualHostReturnsPropertyWhenNoAddresses() {
+		this.properties.setVirtualHost("alpha");
+		assertThat(this.properties.determineVirtualHost()).isEqualTo("alpha");
+	}
+
+	@Test
+	public void determineVirtualHostReturnsPropertyWhenFirstAddressHasNoVirtualHost() {
+		this.properties.setVirtualHost("alpha");
+		this.properties
+				.setAddresses("rabbit1.example.com:1234,rabbit2.example.com:2345/bravo");
+		assertThat(this.properties.determineVirtualHost()).isEqualTo("alpha");
+	}
+
+	@Test
+	public void determinedVirtualHostIsSlashWhenAddressHasTrailingSlash() {
+		this.properties.setAddresses("amqp://root:password@otherhost:1111/");
+		assertThat(this.properties.determineVirtualHost()).isEqualTo("/");
+	}
+
+	@Test
+	public void emptyVirtualHostIsCoercedToASlash() {
 		this.properties.setVirtualHost("");
 		assertThat(this.properties.getVirtualHost()).isEqualTo("/");
 	}
 
 	@Test
-	public void testCustomVirtualHost() {
-		this.properties.setVirtualHost("myvHost");
-		assertThat(this.properties.getVirtualHost()).isEqualTo("myvHost");
+	public void usernameDefaultsToNull() {
+		assertThat(this.properties.getUsername()).isNull();
+	}
+
+	@Test
+	public void customUsername() {
+		this.properties.setUsername("user");
+		assertThat(this.properties.getUsername()).isEqualTo("user");
+	}
+
+	@Test
+	public void determineUsernameReturnsUsernameOfFirstAddress() {
+		this.properties.setAddresses("user:secret@rabbit1.example.com:1234/alpha,"
+				+ "rabbit2.example.com:2345/bravo");
+		assertThat(this.properties.determineUsername()).isEqualTo("user");
+	}
+
+	@Test
+	public void determineUsernameReturnsPropertyWhenNoAddresses() {
+		this.properties.setUsername("alice");
+		assertThat(this.properties.determineUsername()).isEqualTo("alice");
+	}
+
+	@Test
+	public void determineUsernameReturnsPropertyWhenFirstAddressHasNoUsername() {
+		this.properties.setUsername("alice");
+		this.properties.setAddresses("rabbit1.example.com:1234/alpha,"
+				+ "user:secret@rabbit2.example.com:2345/bravo");
+		assertThat(this.properties.determineUsername()).isEqualTo("alice");
+	}
+
+	@Test
+	public void passwordDefaultsToNull() {
+		assertThat(this.properties.getPassword()).isNull();
+	}
+
+	@Test
+	public void customPassword() {
+		this.properties.setPassword("secret");
+		assertThat(this.properties.getPassword()).isEqualTo("secret");
+	}
+
+	@Test
+	public void determinePasswordReturnsPasswordOfFirstAddress() {
+		this.properties.setAddresses("user:secret@rabbit1.example.com:1234/alpha,"
+				+ "rabbit2.example.com:2345/bravo");
+		assertThat(this.properties.determinePassword()).isEqualTo("secret");
+	}
+
+	@Test
+	public void determinePasswordReturnsPropertyWhenNoAddresses() {
+		this.properties.setPassword("secret");
+		assertThat(this.properties.determinePassword()).isEqualTo("secret");
+	}
+
+	@Test
+	public void determinePasswordReturnsPropertyWhenFirstAddressHasNoPassword() {
+		this.properties.setPassword("12345678");
+		this.properties.setAddresses("rabbit1.example.com:1234/alpha,"
+				+ "user:secret@rabbit2.example.com:2345/bravo");
+		assertThat(this.properties.determinePassword()).isEqualTo("12345678");
+	}
+
+	@Test
+	public void addressesDefaultsToNull() {
+		assertThat(this.properties.getAddresses()).isEqualTo(null);
+	}
+
+	@Test
+	public void customAddresses() {
+		this.properties.setAddresses(
+				"user:secrect@rabbit1.example.com:1234/alpha,rabbit2.example.com");
+		assertThat(this.properties.getAddresses()).isEqualTo(
+				"user:secrect@rabbit1.example.com:1234/alpha,rabbit2.example.com");
+	}
+
+	@Test
+	public void determineAddressesReturnsAddressesWithJustHostAndPort() {
+		this.properties.setAddresses(
+				"user:secrect@rabbit1.example.com:1234/alpha,rabbit2.example.com");
+		assertThat(this.properties.determineAddresses())
+				.isEqualTo("rabbit1.example.com:1234,rabbit2.example.com:5672");
 	}
 
 	@Test
-	public void testCustomFalsyVirtualHost() {
-		this.properties.setVirtualHost("/myvHost");
-		assertThat(this.properties.getVirtualHost()).isEqualTo("/myvHost");
+	public void determineAddressesUsesHostAndPortPropertiesWhenNoAddressesSet() {
+		this.properties.setHost("rabbit.example.com");
+		this.properties.setPort(1234);
+		assertThat(this.properties.determineAddresses())
+				.isEqualTo("rabbit.example.com:1234");
 	}
 
 }