Fix Hazelcast Cache auto-configuration ordering

Spring Boot supports the automatic configuration of an additional
HazelcastInstance if one already exists and an explicit property has been
set to use a different configuration for caching. So three cases are
supported really: no `HazelcastInstance` exists so we need to create one
anyway or an `HazelcastInstance` already exists; in that latter case, we
should either reuse it or create a new one.

Unfortunately, the conditions that checked those three use cases were
not ordered consistently and we could easily get in a situation where
both conditions were evaluated.

This commit makes sure that we  first check if an `HazelcastInstance`
exists and then (and only then) we create the missing `HazelcastInstance`
used for caching. The tests have also been improved to validate the
proper `HazelcastInstance` is used for caching.

Closes gh-5181

spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/cache/HazelcastCacheConfiguration.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2015 the original author or authors.
+ * Copyright 2012-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,25 +16,17 @@
 
 package org.springframework.boot.autoconfigure.cache;
 
-import java.io.Closeable;
-import java.io.IOException;
-
-import com.hazelcast.core.Hazelcast;
 import com.hazelcast.core.HazelcastInstance;
 import com.hazelcast.spring.cache.HazelcastCacheManager;
 
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
-import org.springframework.boot.autoconfigure.condition.ConditionalOnSingleCandidate;
 import org.springframework.boot.autoconfigure.hazelcast.HazelcastAutoConfiguration;
 import org.springframework.boot.autoconfigure.hazelcast.HazelcastConfigResourceCondition;
-import org.springframework.boot.autoconfigure.hazelcast.HazelcastInstanceFactory;
 import org.springframework.cache.CacheManager;
-import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Conditional;
 import org.springframework.context.annotation.Configuration;
-import org.springframework.core.io.Resource;
+import org.springframework.context.annotation.Import;
 
 /**
  * Hazelcast cache configuration. Can either reuse the {@link HazelcastInstance} that has
@@ -49,84 +41,10 @@ import org.springframework.core.io.Resource;
  * @see HazelcastConfigResourceCondition
  */
 @Configuration
-@ConditionalOnClass({ HazelcastInstance.class, HazelcastCacheManager.class })
+@ConditionalOnClass({HazelcastInstance.class, HazelcastCacheManager.class})
 @ConditionalOnMissingBean(CacheManager.class)
 @Conditional(CacheCondition.class)
+@Import({HazelcastInstanceConfiguration.Existing.class, HazelcastInstanceConfiguration.Specific.class})
 class HazelcastCacheConfiguration {
 
-	@Configuration
-	@ConditionalOnSingleCandidate(HazelcastInstance.class)
-	static class ExistingHazelcastInstanceConfiguration {
-
-		@Autowired
-		private CacheProperties cacheProperties;
-
-		@Bean
-		public HazelcastCacheManager cacheManager(
-				HazelcastInstance existingHazelcastInstance) throws IOException {
-			Resource config = this.cacheProperties.getHazelcast().getConfig();
-			Resource location = this.cacheProperties.resolveConfigLocation(config);
-			if (location != null) {
-				HazelcastInstance cacheHazelcastInstance = new HazelcastInstanceFactory(
-						location).getHazelcastInstance();
-				return new CloseableHazelcastCacheManager(cacheHazelcastInstance);
-			}
-			return new HazelcastCacheManager(existingHazelcastInstance);
-		}
-	}
-
-	@Configuration
-	@ConditionalOnMissingBean(HazelcastInstance.class)
-	@Conditional(ConfigAvailableCondition.class)
-	static class DefaultHazelcastInstanceConfiguration {
-
-		@Autowired
-		private CacheProperties cacheProperties;
-
-		@Bean
-		public HazelcastInstance hazelcastInstance() throws IOException {
-			Resource config = this.cacheProperties.getHazelcast().getConfig();
-			Resource location = this.cacheProperties.resolveConfigLocation(config);
-			if (location != null) {
-				new HazelcastInstanceFactory(location).getHazelcastInstance();
-			}
-			return Hazelcast.newHazelcastInstance();
-		}
-
-		@Bean
-		public HazelcastCacheManager cacheManager() throws IOException {
-			return new HazelcastCacheManager(hazelcastInstance());
-		}
-
-	}
-
-	/**
-	 * {@link HazelcastConfigResourceCondition} that checks if the
-	 * {@code spring.cache.hazelcast.config} configuration key is defined.
-	 */
-	static class ConfigAvailableCondition extends HazelcastConfigResourceCondition {
-
-		ConfigAvailableCondition() {
-			super("spring.cache.hazelcast", "config");
-		}
-
-	}
-
-	private static class CloseableHazelcastCacheManager extends HazelcastCacheManager
-			implements Closeable {
-
-		private final HazelcastInstance hazelcastInstance;
-
-		CloseableHazelcastCacheManager(HazelcastInstance hazelcastInstance) {
-			super(hazelcastInstance);
-			this.hazelcastInstance = hazelcastInstance;
-		}
-
-		@Override
-		public void close() throws IOException {
-			this.hazelcastInstance.shutdown();
-		}
-
-	}
-
 }

spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/cache/HazelcastInstanceConfiguration.java

@@ -0,0 +1,120 @@
+/*
+ * Copyright 2012-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.boot.autoconfigure.cache;
+
+import java.io.Closeable;
+import java.io.IOException;
+
+import com.hazelcast.core.Hazelcast;
+import com.hazelcast.core.HazelcastInstance;
+import com.hazelcast.spring.cache.HazelcastCacheManager;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnSingleCandidate;
+import org.springframework.boot.autoconfigure.hazelcast.HazelcastConfigResourceCondition;
+import org.springframework.boot.autoconfigure.hazelcast.HazelcastInstanceFactory;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Conditional;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.core.io.Resource;
+
+/**
+ * Actual {@link HazelcastInstance} configurations imported by
+ * {@link HazelcastCacheConfiguration}.
+ *
+ * @author Stephane Nicoll
+ */
+abstract class HazelcastInstanceConfiguration {
+
+	@Configuration
+	@ConditionalOnSingleCandidate(HazelcastInstance.class)
+	static class Existing {
+
+		@Autowired
+		private CacheProperties cacheProperties;
+
+		@Bean
+		public HazelcastCacheManager cacheManager(
+				HazelcastInstance existingHazelcastInstance) throws IOException {
+			Resource config = this.cacheProperties.getHazelcast().getConfig();
+			Resource location = this.cacheProperties.resolveConfigLocation(config);
+			if (location != null) {
+				HazelcastInstance cacheHazelcastInstance = new HazelcastInstanceFactory(
+						location).getHazelcastInstance();
+				return new CloseableHazelcastCacheManager(cacheHazelcastInstance);
+			}
+			return new HazelcastCacheManager(existingHazelcastInstance);
+		}
+	}
+
+	@Configuration
+	@ConditionalOnMissingBean(HazelcastInstance.class)
+	@Conditional(ConfigAvailableCondition.class)
+	static class Specific {
+
+		@Autowired
+		private CacheProperties cacheProperties;
+
+		@Bean
+		public HazelcastInstance hazelcastInstance() throws IOException {
+			Resource config = this.cacheProperties.getHazelcast().getConfig();
+			Resource location = this.cacheProperties.resolveConfigLocation(config);
+			if (location != null) {
+				return new HazelcastInstanceFactory(location).getHazelcastInstance();
+			}
+			return Hazelcast.newHazelcastInstance();
+		}
+
+		@Bean
+		public HazelcastCacheManager cacheManager() throws IOException {
+			return new HazelcastCacheManager(hazelcastInstance());
+		}
+
+	}
+
+
+	/**
+	 * {@link HazelcastConfigResourceCondition} that checks if the
+	 * {@code spring.cache.hazelcast.config} configuration key is defined.
+	 */
+	static class ConfigAvailableCondition extends HazelcastConfigResourceCondition {
+
+		ConfigAvailableCondition() {
+			super("spring.cache.hazelcast", "config");
+		}
+
+	}
+
+	private static class CloseableHazelcastCacheManager extends HazelcastCacheManager
+			implements Closeable {
+
+		private final HazelcastInstance hazelcastInstance;
+
+		CloseableHazelcastCacheManager(HazelcastInstance hazelcastInstance) {
+			super(hazelcastInstance);
+			this.hazelcastInstance = hazelcastInstance;
+		}
+
+		@Override
+		public void close() throws IOException {
+			this.hazelcastInstance.shutdown();
+		}
+
+	}
+
+}

spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/cache/CacheAutoConfigurationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2015 the original author or authors.
+ * Copyright 2012-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -70,6 +70,7 @@ import org.springframework.data.redis.core.RedisTemplate;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.sameInstance;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.empty;
@@ -344,16 +345,21 @@ public class CacheAutoConfigurationTests {
 		assertThat(cacheManager.getCacheNames(), containsInAnyOrder("defaultCache"));
 		assertThat(cacheManager.getCacheNames(), hasSize(1));
 		assertThat(this.context.getBean(HazelcastInstance.class),
-				equalTo(new DirectFieldAccessor(cacheManager)
+				equalTo(getHazelcastInstance(cacheManager)));
-						.getPropertyValue("hazelcastInstance")));
 	}
 
 	@Test
-	public void hazelcastCacheWithConfig() {
+	public void hazelcastCacheWithConfig() throws IOException {
 		load(DefaultCacheConfiguration.class, "spring.cache.type=hazelcast",
 				"spring.cache.hazelcast.config=org/springframework/boot/autoconfigure/cache/hazelcast-specific.xml");
+		HazelcastInstance hazelcastInstance = this.context
+				.getBean(HazelcastInstance.class);
 		HazelcastCacheManager cacheManager = validateCacheManager(
 				HazelcastCacheManager.class);
+		HazelcastInstance actual = getHazelcastInstance(cacheManager);
+		assertThat(actual, sameInstance(hazelcastInstance));
+		assertThat(actual.getConfig().getConfigurationUrl(), equalTo(new ClassPathResource(
+				"org/springframework/boot/autoconfigure/cache/hazelcast-specific.xml").getURL()));
 		cacheManager.getCache("foobar");
 		assertThat(cacheManager.getCacheNames(), containsInAnyOrder("foobar"));
 		assertThat(cacheManager.getCacheNames(), hasSize(1));
@@ -372,9 +378,7 @@ public class CacheAutoConfigurationTests {
 		load(HazelcastCustomHazelcastInstance.class, "spring.cache.type=hazelcast");
 		HazelcastCacheManager cacheManager = validateCacheManager(
 				HazelcastCacheManager.class);
-		assertThat(
+		assertThat(getHazelcastInstance(cacheManager),
-				new DirectFieldAccessor(cacheManager)
-						.getPropertyValue("hazelcastInstance"),
 				equalTo(this.context.getBean("customHazelcastInstance")));
 	}
 
@@ -392,8 +396,7 @@ public class CacheAutoConfigurationTests {
 				HazelcastCacheManager.class);
 		HazelcastInstance hazelcastInstance = this.context
 				.getBean(HazelcastInstance.class);
-		assertThat(new DirectFieldAccessor(cacheManager).getPropertyValue(
+		assertThat(getHazelcastInstance(cacheManager), equalTo((Object) hazelcastInstance));
-				"hazelcastInstance"), equalTo((Object) hazelcastInstance));
 		assertThat(hazelcastInstance.getConfig().getConfigurationFile(),
 				equalTo(new ClassPathResource(mainConfig).getFile()));
 	}
@@ -416,8 +419,7 @@ public class CacheAutoConfigurationTests {
 				.getBean(HazelcastInstance.class);
 		HazelcastCacheManager cacheManager = validateCacheManager(
 				HazelcastCacheManager.class);
-		HazelcastInstance cacheHazelcastInstance = (HazelcastInstance) new DirectFieldAccessor(
+		HazelcastInstance cacheHazelcastInstance = getHazelcastInstance(cacheManager);
-				cacheManager).getPropertyValue("hazelcastInstance");
 		assertThat(cacheHazelcastInstance, not(hazelcastInstance)); // Our custom
 		assertThat(hazelcastInstance.getConfig().getConfigurationFile(),
 				equalTo(new ClassPathResource(mainConfig).getFile()));
@@ -570,6 +572,11 @@ public class CacheAutoConfigurationTests {
 		this.context = applicationContext;
 	}
 
+	private static HazelcastInstance getHazelcastInstance(HazelcastCacheManager cacheManager) {
+		return (HazelcastInstance) new DirectFieldAccessor(cacheManager)
+				.getPropertyValue("hazelcastInstance");
+	}
+
 	@Configuration
 	static class EmptyConfiguration {