Return null correctly from MutablePropertySources#get

Prior to this commit, MutablePropertySources#get(String) would throw
IndexArrayOutOfBoundsException if the named property source does not
actually exist. This is a violation of the PropertySource#get contract
as described in its Javadoc.

The implementation now correctly checks for the existence of the named
property source, returning null if non-existent and otherwise returning
the associated PropertySource.

Other changes

 - Rename PropertySourcesTests => MutablePropertySourcesTests
 - Polish MutablePropertySourcesTests for style, formatting
 - Refactor MutablePropertySources for consistency

Issue: SPR-9185
Backport-Issue: SPR-9179
Backport-Commit: 15d1d82

Author: cbeams

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 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.
@@ -79,7 +79,8 @@ public boolean contains(String name) {
 	}
 
 	public PropertySource<?> get(String name) {
-		return this.propertySourceList.get(this.propertySourceList.indexOf(PropertySource.named(name)));
+		int index = this.propertySourceList.indexOf(PropertySource.named(name));
+		return index == -1 ? null : this.propertySourceList.get(index);
 	}
 
 	public Iterator<PropertySource<?>> iterator() {
@@ -146,10 +147,7 @@ public int precedenceOf(PropertySource<?> propertySource) {
 	public PropertySource<?> remove(String name) {
 		logger.debug(String.format("Removing [%s] PropertySource", name));
 		int index = this.propertySourceList.indexOf(PropertySource.named(name));
-		if (index >= 0) {
-			return this.propertySourceList.remove(index);
-		}
-		return null;
+		return index == -1 ? null : this.propertySourceList.remove(index);
 	}
 
 	/**
@@ -210,11 +208,13 @@ private void addAtIndex(int index, PropertySource<?> propertySource) {
 
 	/**
 	 * Assert that the named property source is present and return its index.
+	 * @param name the {@linkplain PropertySource#getName() name of the property source}
+	 * to find
 	 * @throws IllegalArgumentException if the named property source is not present
 	 */
-	private int assertPresentAndGetIndex(String propertySourceName) {
-		int index = this.propertySourceList.indexOf(PropertySource.named(propertySourceName));
-		Assert.isTrue(index >= 0, String.format(NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, propertySourceName));
+	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));
 		return index;
 	}
 

org.springframework.core/src/test/java/org/springframework/core/env/PropertySourcesTests.java renamed to org.springframework.core/src/test/java/org/springframework/core/env/MutablePropertySourcesTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2010 the original author or authors.
+ * Copyright 2002-2012 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,18 +16,21 @@
 
 package org.springframework.core.env;
 
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.not;
-import static org.hamcrest.CoreMatchers.nullValue;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.fail;
-
 import org.junit.Test;
 import org.springframework.mock.env.MockPropertySource;
 
-public class PropertySourcesTests {
+import static java.lang.String.*;
+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
+ */
+public class MutablePropertySourcesTests {
+
 	@Test
 	public void test() {
 		MutablePropertySources sources = new MutablePropertySources();
@@ -104,7 +107,7 @@ public void test() {
 			fail("expected non-existent PropertySource exception");
 		} catch (IllegalArgumentException ex) {
 			assertThat(ex.getMessage(),
-					equalTo(String.format(MutablePropertySources.NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, bogusPS)));
+					equalTo(format(NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, bogusPS)));
 		}
 
 		sources.addFirst(new MockPropertySource("a"));
@@ -126,24 +129,30 @@ public void test() {
 			fail("expected non-existent PropertySource exception");
 		} catch (IllegalArgumentException ex) {
 			assertThat(ex.getMessage(),
-					equalTo(String.format(MutablePropertySources.NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, bogusPS)));
+					equalTo(format(NON_EXISTENT_PROPERTY_SOURCE_MESSAGE, bogusPS)));
 		}
 
 		try {
 			sources.addBefore("b", new MockPropertySource("b"));
 			fail("expected exception");
 		} catch (IllegalArgumentException ex) {
 			assertThat(ex.getMessage(),
-					equalTo(String.format(MutablePropertySources.ILLEGAL_RELATIVE_ADDITION_MESSAGE, "b")));
+					equalTo(format(ILLEGAL_RELATIVE_ADDITION_MESSAGE, "b")));
 		}
 
 		try {
 			sources.addAfter("b", new MockPropertySource("b"));
 			fail("expected exception");
 		} catch (IllegalArgumentException ex) {
 			assertThat(ex.getMessage(),
-					equalTo(String.format(MutablePropertySources.ILLEGAL_RELATIVE_ADDITION_MESSAGE, "b")));
+					equalTo(format(ILLEGAL_RELATIVE_ADDITION_MESSAGE, "b")));
 		}
 	}
 
+	@Test
+	public void getNonExistentPropertySourceReturnsNull() {
+		MutablePropertySources sources = new MutablePropertySources();
+		assertThat(sources.get("bogus"), nullValue());
+	}
+
 }