From 127b91fd4f148538562fc255ed1e22b5b74817dd Mon Sep 17 00:00:00 2001
From: Juergen Hoeller <jhoeller@vmware.com>
Date: Wed, 31 Jul 2013 17:50:44 +0200
Subject: [PATCH] PropertySourcesPlaceholderConfigurer's
 "ignoreUnresolvablePlaceholders" setting reliably applies to nested
 placeholders as well

Issue: SPR-10549
---
 .../PropertySourcesPlaceholderConfigurer.java |  8 +--
 ...rtyResourceConfigurerIntegrationTests.java | 19 ++++++
 ...ertySourcesPlaceholderConfigurerTests.java | 41 ++++++++++--
 .../core/env/AbstractPropertyResolver.java    | 66 +++++++++++--------
 .../core/env/PropertyResolver.java            | 10 +--
 .../env/PropertySourcesPropertyResolver.java  | 21 ++++--
 .../util/PropertyPlaceholderHelper.java       | 14 ++--
 .../PropertySourcesPropertyResolverTests.java |  8 +--
 8 files changed, 131 insertions(+), 56 deletions(-)

diff --git a/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java b/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java
index c2c6cc8cce..ba0d7d1916 100644
--- a/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java
+++ b/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java
@@ -112,8 +112,8 @@ public class PropertySourcesPlaceholderConfigurer extends PlaceholderConfigurerS
 	 * <p>Processing occurs by replacing ${...} placeholders in bean definitions by resolving each
 	 * against this configurer's set of {@link PropertySources}, which includes:
 	 * <ul>
-	 * <li>all {@linkplain org.springframework.core.env.ConfigurableEnvironment#getPropertySources environment property sources}, if an
-	 * {@code Environment} {@linkplain #setEnvironment is present}
+	 * <li>all {@linkplain org.springframework.core.env.ConfigurableEnvironment#getPropertySources
+	 * environment property sources}, if an {@code Environment} {@linkplain #setEnvironment is present}
 	 * <li>{@linkplain #mergeProperties merged local properties}, if {@linkplain #setLocation any}
 	 * {@linkplain #setLocations have} {@linkplain #setProperties been}
 	 * {@linkplain #setPropertiesArray specified}
@@ -139,7 +139,7 @@ public class PropertySourcesPlaceholderConfigurer extends PlaceholderConfigurerS
 			}
 			try {
 				PropertySource<?> localPropertySource =
-					new PropertiesPropertySource(LOCAL_PROPERTIES_PROPERTY_SOURCE_NAME, this.mergeProperties());
+					new PropertiesPropertySource(LOCAL_PROPERTIES_PROPERTY_SOURCE_NAME, mergeProperties());
 				if (this.localOverride) {
 					this.propertySources.addFirst(localPropertySource);
 				}
@@ -152,7 +152,7 @@ public class PropertySourcesPlaceholderConfigurer extends PlaceholderConfigurerS
 			}
 		}
 
-		this.processProperties(beanFactory, new PropertySourcesPropertyResolver(this.propertySources));
+		processProperties(beanFactory, new PropertySourcesPropertyResolver(this.propertySources));
 		this.appliedPropertySources = this.propertySources;
 	}
 
diff --git a/spring-context/src/test/java/org/springframework/context/support/PropertyResourceConfigurerIntegrationTests.java b/spring-context/src/test/java/org/springframework/context/support/PropertyResourceConfigurerIntegrationTests.java
index 24185526d6..cb85d50d51 100644
--- a/spring-context/src/test/java/org/springframework/context/support/PropertyResourceConfigurerIntegrationTests.java
+++ b/spring-context/src/test/java/org/springframework/context/support/PropertyResourceConfigurerIntegrationTests.java
@@ -153,6 +153,25 @@ public class PropertyResourceConfigurerIntegrationTests {
 		}
 	}
 
+	@Test
+	public void testPropertyPlaceholderConfigurerWithNestedUnresolvableReference() {
+		StaticApplicationContext ac = new StaticApplicationContext();
+		MutablePropertyValues pvs = new MutablePropertyValues();
+		pvs.add("name", "name${var}");
+		ac.registerSingleton("tb1", TestBean.class, pvs);
+		pvs = new MutablePropertyValues();
+		pvs.add("properties", "var=${m}var\nm=${var2}\nvar2=${m2}");
+		ac.registerSingleton("configurer1", PropertyPlaceholderConfigurer.class, pvs);
+		try {
+			ac.refresh();
+			fail("Should have thrown BeanDefinitionStoreException");
+		}
+		catch (BeanDefinitionStoreException ex) {
+			// expected
+			ex.printStackTrace();
+		}
+	}
+
 	@Ignore // this test was breaking after the 3.0 repackaging
 	@Test
 	public void testPropertyPlaceholderConfigurerWithAutowireByType() {
diff --git a/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java b/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java
index 9da942c3ed..ecfea14839 100644
--- a/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java
+++ b/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java
@@ -37,8 +37,6 @@ import static org.junit.Assert.*;
 import static org.springframework.beans.factory.support.BeanDefinitionBuilder.*;
 
 /**
- * Unit tests for {@link PropertySourcesPlaceholderConfigurer}.
- *
  * @author Chris Beams
  * @since 3.1
  */
@@ -143,7 +141,9 @@ public class PropertySourcesPlaceholderConfigurerTests {
 
 		PropertySourcesPlaceholderConfigurer pc = new PropertySourcesPlaceholderConfigurer();
 		pc.setPropertySources(propertySources);
-		pc.setProperties(new Properties() {{ put("my.name", "local"); }});
+		pc.setProperties(new Properties() {{
+			put("my.name", "local");
+		}});
 		pc.setIgnoreUnresolvablePlaceholders(true);
 		pc.postProcessBeanFactory(bf);
 		assertThat(bf.getBean(TestBean.class).getName(), equalTo("${my.name}"));
@@ -176,6 +176,38 @@ public class PropertySourcesPlaceholderConfigurerTests {
 		assertThat(bf.getBean(TestBean.class).getName(), equalTo("${my.name}"));
 	}
 
+	@Test(expected=BeanDefinitionStoreException.class)
+	public void nestedUnresolvablePlaceholder() {
+		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
+		bf.registerBeanDefinition("testBean",
+				genericBeanDefinition(TestBean.class)
+						.addPropertyValue("name", "${my.name}")
+						.getBeanDefinition());
+
+		PropertySourcesPlaceholderConfigurer pc = new PropertySourcesPlaceholderConfigurer();
+		pc.setProperties(new Properties() {{
+			put("my.name", "${bogus}");
+		}});
+		pc.postProcessBeanFactory(bf); // should throw
+	}
+
+	@Test
+	public void ignoredNestedUnresolvablePlaceholder() {
+		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
+		bf.registerBeanDefinition("testBean",
+				genericBeanDefinition(TestBean.class)
+						.addPropertyValue("name", "${my.name}")
+						.getBeanDefinition());
+
+		PropertySourcesPlaceholderConfigurer pc = new PropertySourcesPlaceholderConfigurer();
+		pc.setProperties(new Properties() {{
+			put("my.name", "${bogus}");
+		}});
+		pc.setIgnoreUnresolvablePlaceholders(true);
+		pc.postProcessBeanFactory(bf);
+		assertThat(bf.getBean(TestBean.class).getName(), equalTo("${bogus}"));
+	}
+
 	@Test
 	public void withNonEnumerablePropertySource() {
 		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
@@ -217,7 +249,8 @@ public class PropertySourcesPlaceholderConfigurerTests {
 		ppc.postProcessBeanFactory(bf);
 		if (override) {
 			assertThat(bf.getBean(TestBean.class).getName(), equalTo("local"));
-		} else {
+		}
+		else {
 			assertThat(bf.getBean(TestBean.class).getName(), equalTo("enclosing"));
 		}
 	}
diff --git a/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java b/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java
index 77bf33ab21..d8bbd20d96 100644
--- a/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java
+++ b/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 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,22 @@
 
 package org.springframework.core.env;
 
-import static java.lang.String.format;
-import static org.springframework.util.SystemPropertyUtils.PLACEHOLDER_PREFIX;
-import static org.springframework.util.SystemPropertyUtils.PLACEHOLDER_SUFFIX;
-import static org.springframework.util.SystemPropertyUtils.VALUE_SEPARATOR;
-
 import java.util.LinkedHashSet;
 import java.util.Set;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+
 import org.springframework.core.convert.support.ConfigurableConversionService;
 import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.util.PropertyPlaceholderHelper;
-import org.springframework.util.PropertyPlaceholderHelper.PlaceholderResolver;
+import org.springframework.util.SystemPropertyUtils;
 
 /**
  * Abstract base class for resolving properties against any underlying source.
  *
  * @author Chris Beams
+ * @author Juergen Hoeller
  * @since 3.1
  */
 public abstract class AbstractPropertyResolver implements ConfigurablePropertyResolver {
@@ -44,15 +41,20 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe
 	protected ConfigurableConversionService conversionService = new DefaultConversionService();
 
 	private PropertyPlaceholderHelper nonStrictHelper;
+
 	private PropertyPlaceholderHelper strictHelper;
+
 	private boolean ignoreUnresolvableNestedPlaceholders = false;
 
-	private String placeholderPrefix = PLACEHOLDER_PREFIX;
-	private String placeholderSuffix = PLACEHOLDER_SUFFIX;
-	private String valueSeparator = VALUE_SEPARATOR;
+	private String placeholderPrefix = SystemPropertyUtils.PLACEHOLDER_PREFIX;
+
+	private String placeholderSuffix = SystemPropertyUtils.PLACEHOLDER_SUFFIX;
+
+	private String valueSeparator = SystemPropertyUtils.VALUE_SEPARATOR;
 
 	private final Set<String> requiredProperties = new LinkedHashSet<String>();
 
+
 	@Override
 	public ConfigurableConversionService getConversionService() {
 		return this.conversionService;
@@ -66,13 +68,13 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe
 	@Override
 	public String getProperty(String key, String defaultValue) {
 		String value = getProperty(key);
-		return value == null ? defaultValue : value;
+		return (value != null ? value : defaultValue);
 	}
 
 	@Override
 	public <T> T getProperty(String key, Class<T> targetType, T defaultValue) {
 		T value = getProperty(key, targetType);
-		return value == null ? defaultValue : value;
+		return (value != null ? value : defaultValue);
 	}
 
 	@Override
@@ -99,7 +101,7 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe
 	public String getRequiredProperty(String key) throws IllegalStateException {
 		String value = getProperty(key);
 		if (value == null) {
-			throw new IllegalStateException(format("required key [%s] not found", key));
+			throw new IllegalStateException(String.format("required key [%s] not found", key));
 		}
 		return value;
 	}
@@ -108,7 +110,7 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe
 	public <T> T getRequiredProperty(String key, Class<T> valueType) throws IllegalStateException {
 		T value = getProperty(key, valueType);
 		if (value == null) {
-			throw new IllegalStateException(format("required key [%s] not found", key));
+			throw new IllegalStateException(String.format("required key [%s] not found", key));
 		}
 		return value;
 	}
@@ -142,18 +144,18 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe
 
 	@Override
 	public String resolvePlaceholders(String text) {
-		if (nonStrictHelper == null) {
-			nonStrictHelper = createPlaceholderHelper(true);
+		if (this.nonStrictHelper == null) {
+			this.nonStrictHelper = createPlaceholderHelper(true);
 		}
-		return doResolvePlaceholders(text, nonStrictHelper);
+		return doResolvePlaceholders(text, this.nonStrictHelper);
 	}
 
 	@Override
 	public String resolveRequiredPlaceholders(String text) throws IllegalArgumentException {
-		if (strictHelper == null) {
-			strictHelper = createPlaceholderHelper(false);
+		if (this.strictHelper == null) {
+			this.strictHelper = createPlaceholderHelper(false);
 		}
-		return doResolvePlaceholders(text, strictHelper);
+		return doResolvePlaceholders(text, this.strictHelper);
 	}
 
 	/**
@@ -168,15 +170,19 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe
 
 	/**
 	 * Resolve placeholders within the given string, deferring to the value of
-	 * {@link #setIgnoreUnresolvableNestedPlaceholders(boolean)} to determine whether any
+	 * {@link #setIgnoreUnresolvableNestedPlaceholders} to determine whether any
 	 * unresolvable placeholders should raise an exception or be ignored.
+	 * <p>Invoked from {@link #getProperty} and its variants, implicitly resolving
+	 * nested placeholders. In contrast, {@link #resolvePlaceholders} and
+	 * {@link #resolveRequiredPlaceholders} do <emphasis>not</emphasis> delegate
+	 * to this method but rather perform their own handling of unresolvable
+	 * placeholders, as specified by each of those methods.
 	 * @since 3.2
-	 * @see #setIgnoreUnresolvableNestedPlaceholders(boolean)
+	 * @see #setIgnoreUnresolvableNestedPlaceholders
 	 */
 	protected String resolveNestedPlaceholders(String value) {
 		return this.ignoreUnresolvableNestedPlaceholders ?
-				this.resolvePlaceholders(value) :
-				this.resolveRequiredPlaceholders(value);
+				resolvePlaceholders(value) : resolveRequiredPlaceholders(value);
 	}
 
 	private PropertyPlaceholderHelper createPlaceholderHelper(boolean ignoreUnresolvablePlaceholders) {
@@ -185,12 +191,20 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe
 	}
 
 	private String doResolvePlaceholders(String text, PropertyPlaceholderHelper helper) {
-		return helper.replacePlaceholders(text, new PlaceholderResolver() {
+		return helper.replacePlaceholders(text, new PropertyPlaceholderHelper.PlaceholderResolver() {
 			@Override
 			public String resolvePlaceholder(String placeholderName) {
-				return getProperty(placeholderName);
+				return getPropertyAsRawString(placeholderName);
 			}
 		});
 	}
 
+	/**
+	 * Retrieve the specified property as a raw String,
+	 * i.e. without resolution of nested placeholders.
+	 * @param key the property name to resolve
+	 * @return the property value or {@code null} if none found
+	 */
+	protected abstract String getPropertyAsRawString(String key);
+
 }
diff --git a/spring-core/src/main/java/org/springframework/core/env/PropertyResolver.java b/spring-core/src/main/java/org/springframework/core/env/PropertyResolver.java
index cf94472c0c..b3ce15b6f6 100644
--- a/spring-core/src/main/java/org/springframework/core/env/PropertyResolver.java
+++ b/spring-core/src/main/java/org/springframework/core/env/PropertyResolver.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 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.
@@ -74,8 +74,9 @@ public interface PropertyResolver {
 	/**
 	 * Convert the property value associated with the given key to a {@code Class}
 	 * of type {@code T} or {@code null} if the key cannot be resolved.
-	 * @throws ConversionException if class specified by property value cannot be found
-	 * or loaded or if targetType is not assignable from class specified by property value
+	 * @throws org.springframework.core.convert.ConversionException if class specified
+	 * by property value cannot be found  or loaded or if targetType is not assignable
+	 * from class specified by property value
 	 * @see #getProperty(String, Class)
 	 */
 	<T> Class<T> getPropertyAsClass(String key, Class<T> targetType);
@@ -113,8 +114,9 @@ public interface PropertyResolver {
 	 * no default value will cause an IllegalArgumentException to be thrown.
 	 * @return the resolved String (never {@code null})
 	 * @throws IllegalArgumentException if given text is {@code null}
-	 * @throws IllegalArgumentException if any placeholders are unresolvable
+	 * or if any placeholders are unresolvable
 	 * @see org.springframework.util.SystemPropertyUtils#resolvePlaceholders(String, boolean)
 	 */
 	String resolveRequiredPlaceholders(String text) throws IllegalArgumentException;
+
 }
diff --git a/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java b/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java
index 1f5998c0c8..9e33628448 100644
--- a/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java
+++ b/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java
@@ -57,11 +57,20 @@ public class PropertySourcesPropertyResolver extends AbstractPropertyResolver {
 
 	@Override
 	public String getProperty(String key) {
-		return getProperty(key, String.class);
+		return getProperty(key, String.class, true);
 	}
 
 	@Override
 	public <T> T getProperty(String key, Class<T> targetValueType) {
+		return getProperty(key, targetValueType, true);
+	}
+
+	@Override
+	protected String getPropertyAsRawString(String key) {
+		return getProperty(key, String.class, false);
+	}
+
+	protected <T> T getProperty(String key, Class<T> targetValueType, boolean resolveNestedPlaceholders) {
 		boolean debugEnabled = logger.isDebugEnabled();
 		if (logger.isTraceEnabled()) {
 			logger.trace(String.format("getProperty(\"%s\", %s)", key, targetValueType.getSimpleName()));
@@ -74,8 +83,8 @@ public class PropertySourcesPropertyResolver extends AbstractPropertyResolver {
 				Object value;
 				if ((value = propertySource.getProperty(key)) != null) {
 					Class<?> valueType = value.getClass();
-					if (String.class.equals(valueType)) {
-						value = this.resolveNestedPlaceholders((String) value);
+					if (resolveNestedPlaceholders && value instanceof String) {
+						value = resolveNestedPlaceholders((String) value);
 					}
 					if (debugEnabled) {
 						logger.debug(String.format("Found key '%s' in [%s] with type [%s] and value '%s'",
@@ -86,7 +95,7 @@ public class PropertySourcesPropertyResolver extends AbstractPropertyResolver {
 								"Cannot convert value [%s] from source type [%s] to target type [%s]",
 								value, valueType.getSimpleName(), targetValueType.getSimpleName()));
 					}
-					return conversionService.convert(value, targetValueType);
+					return this.conversionService.convert(value, targetValueType);
 				}
 			}
 		}
@@ -115,10 +124,10 @@ public class PropertySourcesPropertyResolver extends AbstractPropertyResolver {
 					Class<?> clazz;
 					if (value instanceof String) {
 						try {
-							clazz = ClassUtils.forName((String)value, null);
+							clazz = ClassUtils.forName((String) value, null);
 						}
 						catch (Exception ex) {
-							throw new ClassConversionException((String)value, targetValueType, ex);
+							throw new ClassConversionException((String) value, targetValueType, ex);
 						}
 					}
 					else if (value instanceof Class) {
diff --git a/spring-core/src/main/java/org/springframework/util/PropertyPlaceholderHelper.java b/spring-core/src/main/java/org/springframework/util/PropertyPlaceholderHelper.java
index 99d666e5a5..65b600ac15 100644
--- a/spring-core/src/main/java/org/springframework/util/PropertyPlaceholderHelper.java
+++ b/spring-core/src/main/java/org/springframework/util/PropertyPlaceholderHelper.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 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.
@@ -98,8 +98,8 @@ public class PropertyPlaceholderHelper {
 
 
 	/**
-	 * Replaces all placeholders of format {@code ${name}} with the corresponding property
-	 * from the supplied {@link Properties}.
+	 * Replaces all placeholders of format {@code ${name}} with the corresponding
+	 * property from the supplied {@link Properties}.
 	 * @param value the value containing the placeholders to be replaced.
 	 * @param properties the {@code Properties} to use for replacement.
 	 * @return the supplied value with placeholders replaced inline.
@@ -115,8 +115,8 @@ public class PropertyPlaceholderHelper {
 	}
 
 	/**
-	 * Replaces all placeholders of format {@code ${name}} with the value returned from the supplied
-	 * {@link PlaceholderResolver}.
+	 * Replaces all placeholders of format {@code ${name}} with the value returned
+	 * from the supplied {@link PlaceholderResolver}.
 	 * @param value the value containing the placeholders to be replaced.
 	 * @param placeholderResolver the {@code PlaceholderResolver} to use for replacement.
 	 * @return the supplied value with placeholders replaced inline.
@@ -217,8 +217,8 @@ public class PropertyPlaceholderHelper {
 
 		/**
 		 * Resolves the supplied placeholder name into the replacement value.
-		 * @param placeholderName the name of the placeholder to resolve.
-		 * @return the replacement value or {@code null} if no replacement is to be made.
+		 * @param placeholderName the name of the placeholder to resolve
+		 * @return the replacement value or {@code null} if no replacement is to be made
 		 */
 		String resolvePlaceholder(String placeholderName);
 	}
diff --git a/spring-core/src/test/java/org/springframework/core/env/PropertySourcesPropertyResolverTests.java b/spring-core/src/test/java/org/springframework/core/env/PropertySourcesPropertyResolverTests.java
index f998cc5366..84f6fce08a 100644
--- a/spring-core/src/test/java/org/springframework/core/env/PropertySourcesPropertyResolverTests.java
+++ b/spring-core/src/test/java/org/springframework/core/env/PropertySourcesPropertyResolverTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 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.
@@ -30,8 +30,6 @@ import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 
 /**
- * Unit tests for {@link PropertySourcesPropertyResolver}.
- *
  * @author Chris Beams
  * @since 3.1
  */
@@ -387,8 +385,8 @@ public class PropertySourcesPropertyResolverTests {
 		try {
 			pr.getProperty("pL");
 		}
-		catch (StackOverflowError ex) {
-			// no explicit handling for cyclic references for now
+		catch (IllegalArgumentException ex) {
+			assertTrue(ex.getMessage().toLowerCase().contains("circular"));
 		}
 	}