DATAGEODE-8 - Ensure locators and servers and configured correctly when using <gfe:pool> attributes.

Related JIRA: https://jira.spring.io/browse/SGF-628.

(cherry picked from commit ffb8c704f0a785f6f441c871756fcc802fd5be81)
Signed-off-by: John Blum <jblum@pivotal.io>

src/main/java/org/springframework/data/gemfire/client/PoolFactoryBean.java

@@ -120,7 +120,7 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 	public void afterPropertiesSet() throws Exception {
 		if (!StringUtils.hasText(name)) {
 			Assert.hasText(beanName, "Pool 'name' is required");
-			name = beanName;
+			this.name = beanName;
 		}
 
 		// check for an existing, configured Pool with name first
@@ -131,15 +131,15 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 				log.debug(String.format("A Pool with name [%1$s] already exists; using existing Pool.", name));
 			}
 
-			springBasedPool = false;
+			this.springBasedPool = false;
-			pool = existingPool;
+			this.pool = existingPool;
 		}
 		else {
 			if (log.isDebugEnabled()) {
 				log.debug(String.format("No Pool with name [%1$s] was found. Creating new Pool.", name));
 			}
 
-			springBasedPool = true;
+			this.springBasedPool = true;
 		}
 	}
 
@@ -166,7 +166,7 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 	/* (non-Javadoc) */
 	@Override
 	public Pool getObject() throws Exception {
-		if (pool == null) {
+		if (this.pool == null) {
 			eagerlyInitializeClientCacheIfNotPresent();
 
 			PoolFactory poolFactory = createPoolFactory();
@@ -190,11 +190,11 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 			poolFactory.setSubscriptionRedundancy(subscriptionRedundancy);
 			poolFactory.setThreadLocalConnections(threadLocalConnections);
 
-			for (ConnectionEndpoint locator : locators) {
+			for (ConnectionEndpoint locator : this.locators) {
 				poolFactory.addLocator(locator.getHost(), locator.getPort());
 			}
 
-			for (ConnectionEndpoint server : servers) {
+			for (ConnectionEndpoint server : this.servers) {
 				poolFactory.addServer(server.getHost(), server.getPort());
 			}
 
@@ -204,6 +204,18 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 		return pool;
 	}
 
+	/**
+	 * Determines whether the GemFire DistributedSystem exists yet or not.
+	 *
+	 * @return a boolean value indicating whether the single, GemFire DistributedSystem has been created already.
+	 * @see org.springframework.data.gemfire.GemfireUtils#getDistributedSystem()
+	 * @see org.springframework.data.gemfire.GemfireUtils#isConnected(DistributedSystem)
+	 * @see org.apache.geode.distributed.DistributedSystem
+	 */
+	boolean isDistributedSystemPresent() {
+		return GemfireUtils.isConnected(GemfireUtils.getDistributedSystem());
+	}
+
 	/**
 	 * Attempts to eagerly initialize the GemFire {@link ClientCache} if not already present so that the single
 	 * {@link org.apache.geode.distributed.DistributedSystem} will exists, which is required to create
@@ -218,18 +230,6 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 		}
 	}
 
-	/**
-	 * Determines whether the GemFire DistributedSystem exists yet or not.
-	 *
-	 * @return a boolean value indicating whether the single, GemFire DistributedSystem has been created already.
-	 * @see org.springframework.data.gemfire.GemfireUtils#getDistributedSystem()
-	 * @see org.springframework.data.gemfire.GemfireUtils#isConnected(DistributedSystem)
-	 * @see org.apache.geode.distributed.DistributedSystem
-	 */
-	boolean isDistributedSystemPresent() {
-		return GemfireUtils.isConnected(GemfireUtils.getDistributedSystem());
-	}
-
 	/**
 	 * Creates an instance of the GemFire {@link PoolFactory} interface to construct, configure and initialize
 	 * a GemFire {@link Pool}.
@@ -245,7 +245,7 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 	/* (non-Javadoc) */
 	@Override
 	public Class<?> getObjectType() {
-		return (pool != null ? pool.getClass() : Pool.class);
+		return (this.pool != null ? this.pool.getClass() : Pool.class);
 	}
 
 	/* (non-Javadoc) */
@@ -595,4 +595,12 @@ public class PoolFactoryBean implements FactoryBean<Pool>, InitializingBean, Dis
 	public void setThreadLocalConnections(boolean threadLocalConnections) {
 		this.threadLocalConnections = threadLocalConnections;
 	}
+
+	/* (non-Javadoc; internal framework use only) */
+	public final void setLocatorsConfiguration(Object locatorsConfiguration) {
+	}
+
+	/* (non-Javadoc; internal framework use only) */
+	public final void setServersConfiguration(Object serversConfiguration) {
+	}
 }

src/main/java/org/springframework/data/gemfire/config/xml/PoolParser.java

@@ -128,8 +128,8 @@ class PoolParser extends AbstractSingleBeanDefinitionParser {
 			}
 		}
 
-		boolean locatorsSet = parseLocators(element, getRegistry(parserContext));
+		boolean locatorsSet = parseLocators(element, builder, getRegistry(parserContext));
-		boolean serversSet = parseServers(element, getRegistry(parserContext));
+		boolean serversSet = parseServers(element, builder, getRegistry(parserContext));
 
 		// NOTE: if neither Locators nor Servers were configured, then setup a connection to a Server
 		// running on localhost, listening on the default CacheServer port 40404
@@ -192,7 +192,7 @@ class PoolParser extends AbstractSingleBeanDefinitionParser {
 	}
 
 	/* (non-Javadoc) */
-	boolean parseLocators(Element element, BeanDefinitionRegistry registry) {
+	boolean parseLocators(Element element, BeanDefinitionBuilder poolBuilder, BeanDefinitionRegistry registry) {
 		String locatorsAttributeValue = element.getAttribute(LOCATORS_ATTRIBUTE_NAME);
 
 		if (StringUtils.hasText(locatorsAttributeValue)) {
@@ -207,7 +207,8 @@ class PoolParser extends AbstractSingleBeanDefinitionParser {
 			AbstractBeanDefinition addLocatorsMethodInvokingBean =
 				addLocatorsMethodInvokingBeanBuilder.getBeanDefinition();
 
-			BeanDefinitionReaderUtils.registerWithGeneratedName(addLocatorsMethodInvokingBean, registry);
+			poolBuilder.addPropertyReference("locatorsConfiguration",
+				BeanDefinitionReaderUtils.registerWithGeneratedName(addLocatorsMethodInvokingBean, registry));
 
 			return true;
 		}
@@ -222,7 +223,7 @@ class PoolParser extends AbstractSingleBeanDefinitionParser {
 	}
 
 	/* (non-Javadoc) */
-	boolean parseServers(Element element, BeanDefinitionRegistry registry) {
+	boolean parseServers(Element element, BeanDefinitionBuilder poolBuilder, BeanDefinitionRegistry registry) {
 		String serversAttributeValue = element.getAttribute(SERVERS_ATTRIBUTE_NAME);
 
 		if (StringUtils.hasText(serversAttributeValue)) {
@@ -237,7 +238,8 @@ class PoolParser extends AbstractSingleBeanDefinitionParser {
 			AbstractBeanDefinition addServersMethodInvokingBean =
 				addServersMethodInvokingBeanBuilder.getBeanDefinition();
 
-			BeanDefinitionReaderUtils.registerWithGeneratedName(addServersMethodInvokingBean, registry);
+			poolBuilder.addPropertyReference("serversConfiguration",
+				BeanDefinitionReaderUtils.registerWithGeneratedName(addServersMethodInvokingBean, registry));
 
 			return true;
 		}

src/test/java/org/springframework/data/gemfire/config/xml/PoolParserUnitTests.java

@@ -507,7 +507,9 @@ public class PoolParserUnitTests {
 		doAnswer(answer).when(mockRegistry).registerBeanDefinition(eq(generateBeanName(MethodInvokingBean.class)),
 			any(BeanDefinition.class));
 
-		assertThat(parser.parseLocators(mockElement, mockRegistry)).isTrue();
+		BeanDefinitionBuilder poolBuilder = BeanDefinitionBuilder.genericBeanDefinition();
+
+		assertThat(parser.parseLocators(mockElement, poolBuilder, mockRegistry)).isTrue();
 
 		verify(mockElement, times(1)).getAttribute(eq(PoolParser.ID_ATTRIBUTE));
 		verify(mockElement, times(1)).getAttribute(eq(PoolParser.LOCATORS_ATTRIBUTE_NAME));
@@ -557,7 +559,9 @@ public class PoolParserUnitTests {
 		doAnswer(answer).when(mockRegistry).registerBeanDefinition(eq(generateBeanName(MethodInvokingBean.class)),
 			any(BeanDefinition.class));
 
-		assertThat(parser.parseServers(mockElement, mockRegistry)).isTrue();
+		BeanDefinitionBuilder poolBuilder = BeanDefinitionBuilder.genericBeanDefinition();
+
+		assertThat(parser.parseServers(mockElement, poolBuilder, mockRegistry)).isTrue();
 
 		verify(mockElement, times(1)).getAttribute(eq(PoolParser.ID_ATTRIBUTE));
 		verify(mockElement, times(1)).getAttribute(eq(PoolParser.SERVERS_ATTRIBUTE_NAME));

src/test/resources/org/springframework/data/gemfire/client/ClientCacheVariableLocatorsTest-context.xml

@@ -12,7 +12,7 @@
 ">
 
 	<util:properties id="client.properties">
-		<prop key="gemfire.cache.client.locator.host-and-port">localhost[11235]</prop>
+		<prop key="gemfire.cache.client.pool.locator.hosts-and-ports">localhost[11235]</prop>
 	</util:properties>
 
 	<context:property-placeholder properties-ref="client.properties"/>
@@ -21,11 +21,12 @@
 		<prop key="log-level">warning</prop>
 	</util:properties>
 
-	<gfe:client-cache properties-ref="gemfireProperties" pool-name="locatorPool"/>
+	<gfe:client-cache properties-ref="gemfireProperties"/>
-
-	<gfe:pool id="locatorPool" locators="${gemfire.cache.client.locator.host-and-port}"/>
 
 	<gfe:client-region id="Example" pool-name="locatorPool" shortcut="PROXY"
 					   key-constraint="java.lang.String" value-constraint="java.lang.Integer"/>
 
+	<!-- Keep the definition of this GemFire Pool bean after the Region (Example) that depends on it! -->
+	<gfe:pool id="locatorPool" locators="${gemfire.cache.client.pool.locator.hosts-and-ports}"/>
+
 </beans>

src/test/resources/org/springframework/data/gemfire/client/ClientCacheVariableServersTest-context.xml

@@ -12,9 +12,9 @@
 ">
 
 	<util:properties id="clientProperties">
-		<prop key="gemfire.cache.client.server.hosts-and-ports">localhost[23579],localhost[23654]</prop>
+		<prop key="gemfire.cache.client.pool.server.hosts-and-ports">localhost[23579],localhost[23654]</prop>
-		<prop key="gemfire.cache.client.server.host.3">localhost</prop>
+		<prop key="gemfire.cache.client.pool.server.host">localhost</prop>
-		<prop key="gemfire.cache.client.server.port.3">24448</prop>
+		<prop key="gemfire.cache.client.pool.server.port">24448</prop>
 	</util:properties>
 
 	<context:property-placeholder properties-ref="clientProperties"/>
@@ -25,11 +25,12 @@
 
 	<gfe:client-cache properties-ref="gemfireProperties" pool-name="serverPool"/>
 
-	<gfe:pool id="serverPool" servers="${gemfire.cache.client.server.hosts-and-ports}">
-		<gfe:server host="${gemfire.cache.client.server.host.3}" port="${gemfire.cache.client.server.port.3}"/>
-	</gfe:pool>
-
 	<gfe:client-region id="Example" pool-name="serverPool" shortcut="PROXY"
 					   key-constraint="java.lang.String" value-constraint="java.lang.Integer"/>
 
+	<!-- Keep the definition of this GemFire Pool bean after the Region (Example) that depends on it! -->
+	<gfe:pool id="serverPool" servers="${gemfire.cache.client.pool.server.hosts-and-ports}">
+		<gfe:server host="${gemfire.cache.client.pool.server.host}" port="${gemfire.cache.client.pool.server.port}"/>
+	</gfe:pool>
+
 </beans>