MutablePropertySources uses an internal CopyOnWriteArrayList for defensiveness against concurrent modifications

Issue: SPR-12428

Author: jhoeller

spring-core/src/main/java/org/springframework/core/env/MutablePropertySources.java

@@ -17,12 +17,12 @@
 package org.springframework.core.env;
 
 import java.util.Iterator;
-import java.util.LinkedList;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 
 /**
@@ -35,24 +35,22 @@
  * will be searched when resolving a given property with a {@link PropertyResolver}.
  *
  * @author Chris Beams
+ * @author Juergen Hoeller
  * @since 3.1
  * @see PropertySourcesPropertyResolver
  */
 public class MutablePropertySources implements PropertySources {
 
-	static final String NON_EXISTENT_PROPERTY_SOURCE_MESSAGE = "PropertySource named [%s] does not exist";
-	static final String ILLEGAL_RELATIVE_ADDITION_MESSAGE = "PropertySource named [%s] cannot be added relative to itself";
-
 	private final Log logger;
 
-	private final LinkedList<PropertySource<?>> propertySourceList = new LinkedList<PropertySource<?>>();
+	private final List<PropertySource<?>> propertySourceList = new CopyOnWriteArrayList<PropertySource<?>>();
 
 
 	/**
 	 * Create a new {@link MutablePropertySources} object.
 	 */
 	public MutablePropertySources() {
-		this.logger = LogFactory.getLog(this.getClass());
+		this.logger = LogFactory.getLog(getClass());
 	}
 
 	/**
@@ -62,7 +60,7 @@ public MutablePropertySources() {
 	public MutablePropertySources(PropertySources propertySources) {
 		this();
 		for (PropertySource<?> propertySource : propertySources) {
-			this.addLast(propertySource);
+			addLast(propertySource);
 		}
 	}
 
@@ -83,7 +81,7 @@ public boolean contains(String name) {
 	@Override
 	public PropertySource<?> get(String name) {
 		int index = this.propertySourceList.indexOf(PropertySource.named(name));
-		return index == -1 ? null : this.propertySourceList.get(index);
+		return (index != -1 ? this.propertySourceList.get(index) : null);
 	}
 
 	@Override
@@ -100,7 +98,7 @@ public void addFirst(PropertySource<?> propertySource) {
 					propertySource.getName()));
 		}
 		removeIfPresent(propertySource);
-		this.propertySourceList.addFirst(propertySource);
+		this.propertySourceList.add(0, propertySource);
 	}
 
 	/**
@@ -112,7 +110,7 @@ public void addLast(PropertySource<?> propertySource) {
 					propertySource.getName()));
 		}
 		removeIfPresent(propertySource);
-		this.propertySourceList.addLast(propertySource);
+		this.propertySourceList.add(propertySource);
 	}
 
 	/**
@@ -161,7 +159,7 @@ public PropertySource<?> remove(String name) {
 			logger.debug(String.format("Removing [%s] PropertySource", name));
 		}
 		int index = this.propertySourceList.indexOf(PropertySource.named(name));
-		return index == -1 ? null : this.propertySourceList.remove(index);
+		return (index != -1 ? this.propertySourceList.remove(index) : null);
 	}
 
 	/**
@@ -190,7 +188,7 @@ public int size() {
 	@Override
 	public String toString() {
 		String[] names = new String[this.size()];
-		for (int i=0; i < size(); i++) {
+		for (int i = 0; i < size(); i++) {
 			names[i] = this.propertySourceList.get(i).getName();
 		}
 		return String.format("[%s]", StringUtils.arrayToCommaDelimitedString(names));
@@ -201,17 +199,17 @@ public String toString() {
 	 */
 	protected void assertLegalRelativeAddition(String relativePropertySourceName, PropertySource<?> propertySource) {
 		String newPropertySourceName = propertySource.getName();
-		Assert.isTrue(!relativePropertySourceName.equals(newPropertySourceName),
-				String.format(ILLEGAL_RELATIVE_ADDITION_MESSAGE, newPropertySourceName));
+		if (relativePropertySourceName.equals(newPropertySourceName)) {
+			throw new IllegalArgumentException(
+					String.format("PropertySource named [%s] cannot be added relative to itself", newPropertySourceName));
+		}
 	}
 
 	/**
 	 * Remove the given property source if it is present.
 	 */
 	protected void removeIfPresent(PropertySource<?> propertySource) {
-		if (this.propertySourceList.contains(propertySource)) {
-			this.propertySourceList.remove(propertySource);
-		}
+		this.propertySourceList.remove(propertySource);
 	}
 
 	/**
@@ -230,7 +228,9 @@ private void addAtIndex(int index, PropertySource<?> propertySource) {
 	 */
 	private int assertPresentAndGetIndex(String name) {
 		int index = this.propertySourceList.indexOf(PropertySource.named(name));
-		Assert.isTrue(index >= 0, String.format(NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, name));
+		if (index == -1) {
+			throw new IllegalArgumentException(String.format("PropertySource named [%s] does not exist", name));
+		}
 		return index;
 	}
 

spring-core/src/test/java/org/springframework/core/env/MutablePropertySourcesTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -20,15 +20,12 @@
 
 import org.springframework.mock.env.MockPropertySource;
 
-import static java.lang.String.format;
 import static org.hamcrest.CoreMatchers.*;
 import static org.junit.Assert.*;
-import static org.springframework.core.env.MutablePropertySources.*;
 
 /**
- * Unit tests for {@link MutablePropertySources}
- *
  * @author Chris Beams
+ * @author Juergen Hoeller
  */
 public class MutablePropertySourcesTests {
 
@@ -106,9 +103,9 @@ public void test() {
 		try {
 			sources.addAfter(bogusPS, new MockPropertySource("h"));
 			fail("expected non-existent PropertySource exception");
-		} catch (IllegalArgumentException ex) {
-			assertThat(ex.getMessage(),
-					equalTo(format(NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, bogusPS)));
+		}
+		catch (IllegalArgumentException ex) {
+			assertTrue(ex.getMessage().contains("does not exist"));
 		}
 
 		sources.addFirst(new MockPropertySource("a"));
@@ -128,25 +125,25 @@ public void test() {
 		try {
 			sources.replace(bogusPS, new MockPropertySource("bogus-replaced"));
 			fail("expected non-existent PropertySource exception");
-		} catch (IllegalArgumentException ex) {
-			assertThat(ex.getMessage(),
-					equalTo(format(NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, bogusPS)));
+		}
+		catch (IllegalArgumentException ex) {
+			assertTrue(ex.getMessage().contains("does not exist"));
 		}
 
 		try {
 			sources.addBefore("b", new MockPropertySource("b"));
 			fail("expected exception");
-		} catch (IllegalArgumentException ex) {
-			assertThat(ex.getMessage(),
-					equalTo(format(ILLEGAL_RELATIVE_ADDITION_MESSAGE, "b")));
+		}
+		catch (IllegalArgumentException ex) {
+			assertTrue(ex.getMessage().contains("cannot be added relative to itself"));
 		}
 
 		try {
 			sources.addAfter("b", new MockPropertySource("b"));
 			fail("expected exception");
-		} catch (IllegalArgumentException ex) {
-			assertThat(ex.getMessage(),
-					equalTo(format(ILLEGAL_RELATIVE_ADDITION_MESSAGE, "b")));
+		}
+		catch (IllegalArgumentException ex) {
+			assertTrue(ex.getMessage().contains("cannot be added relative to itself"));
 		}
 	}