Check for duplicate keys in YAML map nodes

Snake YAML allows for duplicate keys in map nodes. See
https://code.google.com/p/snakeyaml/issues/detail?id=199

This commit uses a dedicated Constructor extension that explicitly
checks for such duplicate keys.

Issue: SPR-12318

spring-beans/src/main/java/org/springframework/beans/factory/config/YamlProcessor.java

@@ -17,6 +17,7 @@
 package org.springframework.beans.factory.config;
 
 import java.io.IOException;
+import java.util.AbstractMap;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -25,10 +26,14 @@ import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Properties;
+import java.util.Set;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.yaml.snakeyaml.Yaml;
+import org.yaml.snakeyaml.constructor.Constructor;
+import org.yaml.snakeyaml.nodes.MappingNode;
+import org.yaml.snakeyaml.parser.ParserException;
 
 import org.springframework.core.io.Resource;
 import org.springframework.util.Assert;
@@ -126,7 +131,7 @@ public abstract class YamlProcessor {
 	 * @param callback a callback to delegate to once matching documents are found
 	 */
 	protected void process(MatchCallback callback) {
-		Yaml yaml = new Yaml();
+		Yaml yaml = new Yaml(new StrictMapAppenderConstructor());
 		for (Resource resource : this.resources) {
 			boolean found = process(callback, yaml, resource);
 			if (this.resolutionMethod == ResolutionMethod.FIRST_FOUND && found) {
@@ -351,4 +356,44 @@ public abstract class YamlProcessor {
 		FIRST_FOUND
 	}
 
+
+	/**
+	 * A specialized {@link Constructor} that checks for duplicate keys.
+	 */
+	private static class StrictMapAppenderConstructor extends Constructor {
+
+		public 	StrictMapAppenderConstructor() {
+			super();
+		}
+
+		@Override
+		protected Map<Object, Object> constructMapping(MappingNode node) {
+			try {
+				return super.constructMapping(node);
+			} catch (IllegalStateException e) {
+				throw new ParserException("while parsing MappingNode",
+						node.getStartMark(), e.getMessage(), node.getEndMark());
+			}
+		}
+
+		@Override
+		protected Map<Object, Object> createDefaultMap() {
+			final Map<Object, Object> delegate = super.createDefaultMap();
+			return new AbstractMap<Object, Object>() {
+				@Override
+				public Object put(Object key, Object value) {
+					if (delegate.containsKey(key)) {
+						throw new IllegalStateException("duplicate key: " + key);
+					}
+					return delegate.put(key, value);
+				}
+				@Override
+				public Set<Entry<Object, Object>> entrySet() {
+					return delegate.entrySet();
+				}
+			};
+		}
+
+	}
+
 }

spring-beans/src/test/java/org/springframework/beans/factory/config/YamlMapFactoryBeanTests.java

@@ -24,11 +24,11 @@ import java.util.LinkedHashMap;
 import java.util.Map;
 
 import org.junit.Test;
+import org.yaml.snakeyaml.parser.ParserException;
 
 import org.springframework.core.io.AbstractResource;
 import org.springframework.core.io.ByteArrayResource;
 import org.springframework.core.io.FileSystemResource;
-import org.springframework.core.io.Resource;
 
 /**
  * Tests for {@link YamlMapFactoryBean}.
@@ -76,7 +76,7 @@ public class YamlMapFactoryBeanTests {
 	@Test
 	public void testFirstFound() throws Exception {
 		this.factory.setResolutionMethod(YamlProcessor.ResolutionMethod.FIRST_FOUND);
-		this.factory.setResources(new Resource[] {new AbstractResource() {
+		this.factory.setResources(new AbstractResource() {
 			@Override
 			public String getDescription() {
 				return "non-existent";
@@ -86,7 +86,7 @@ public class YamlMapFactoryBeanTests {
 			public InputStream getInputStream() throws IOException {
 				throw new IOException("planned");
 			}
-		}, new ByteArrayResource("foo:\n  spam: bar".getBytes())});
+		}, new ByteArrayResource("foo:\n  spam: bar".getBytes()));
 		assertEquals(1, this.factory.getObject().size());
 	}
 
@@ -105,4 +105,11 @@ public class YamlMapFactoryBeanTests {
 		assertEquals("value", sub.get("key1.key2"));
 	}
 
+	@Test(expected = ParserException.class)
+	public void testDuplicateKey() throws Exception {
+		this.factory.setResources(new ByteArrayResource[] {new ByteArrayResource(
+				"mymap:\n  foo: bar\nmymap:\n  bar: foo".getBytes())});
+		this.factory.getObject().get("mymap");
+	}
+
 }

spring-beans/src/test/java/org/springframework/beans/factory/config/YamlProcessorTests.java

@@ -28,7 +28,6 @@ import org.yaml.snakeyaml.parser.ParserException;
 import org.yaml.snakeyaml.scanner.ScannerException;
 
 import org.springframework.core.io.ByteArrayResource;
-import org.springframework.core.io.Resource;
 
 /**
  * Tests for {@link YamlProcessor}.
@@ -45,8 +44,8 @@ public class YamlProcessorTests {
 
 	@Test
 	public void arrayConvertedToIndexedBeanReference() {
-		this.processor.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\nbar: [1,2,3]".getBytes())});
+		this.processor.setResources(new ByteArrayResource(
+				"foo: bar\nbar: [1,2,3]".getBytes()));
 		this.processor.process(new MatchCallback() {
 			@Override
 			public void process(Properties properties, Map<String, Object> map) {
@@ -60,8 +59,8 @@ public class YamlProcessorTests {
 
 	@Test
 	public void testStringResource() throws Exception {
-		this.processor.setResources(new Resource[] {new ByteArrayResource(
-				"foo # a document that is a literal".getBytes())});
+		this.processor.setResources(new ByteArrayResource(
+				"foo # a document that is a literal".getBytes()));
 		this.processor.process(new MatchCallback() {
 			@Override
 			public void process(Properties properties, Map<String, Object> map) {
@@ -72,8 +71,8 @@ public class YamlProcessorTests {
 
 	@Test
 	public void testBadDocumentStart() throws Exception {
-		this.processor.setResources(new Resource[] {new ByteArrayResource(
-				"foo # a document\nbar: baz".getBytes())});
+		this.processor.setResources(new ByteArrayResource(
+				"foo # a document\nbar: baz".getBytes()));
 		this.exception.expect(ParserException.class);
 		this.exception.expectMessage("line 2, column 1");
 		this.processor.process(new MatchCallback() {
@@ -85,8 +84,8 @@ public class YamlProcessorTests {
 
 	@Test
 	public void testBadResource() throws Exception {
-		this.processor.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\ncd\nspam:\n  foo: baz".getBytes())});
+		this.processor.setResources(new ByteArrayResource(
+				"foo: bar\ncd\nspam:\n  foo: baz".getBytes()));
 		this.exception.expect(ScannerException.class);
 		this.exception.expectMessage("line 3, column 1");
 		this.processor.process(new MatchCallback() {
@@ -98,8 +97,8 @@ public class YamlProcessorTests {
 
 	@Test
 	public void mapConvertedToIndexedBeanReference() {
-		this.processor.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\nbar:\n spam: bucket".getBytes())});
+		this.processor.setResources(new ByteArrayResource(
+				"foo: bar\nbar:\n spam: bucket".getBytes()));
 		this.processor.process(new MatchCallback() {
 			@Override
 			public void process(Properties properties, Map<String, Object> map) {
@@ -112,8 +111,8 @@ public class YamlProcessorTests {
 
 	@Test
 	public void integerKeyBehaves() {
-		this.processor.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\n1: bar".getBytes())});
+		this.processor.setResources(new ByteArrayResource(
+				"foo: bar\n1: bar".getBytes()));
 		this.processor.process(new MatchCallback() {
 			@Override
 			public void process(Properties properties, Map<String, Object> map) {
@@ -125,8 +124,8 @@ public class YamlProcessorTests {
 
 	@Test
 	public void integerDeepKeyBehaves() {
-		this.processor.setResources(new Resource[] {new ByteArrayResource(
-				"foo:\n  1: bar".getBytes())});
+		this.processor.setResources(new ByteArrayResource(
+				"foo:\n  1: bar".getBytes()));
 		this.processor.process(new MatchCallback() {
 
 			@Override

spring-beans/src/test/java/org/springframework/beans/factory/config/YamlPropertiesFactoryBeanTests.java

@@ -28,11 +28,11 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.yaml.snakeyaml.Yaml;
+import org.yaml.snakeyaml.parser.ParserException;
 import org.yaml.snakeyaml.scanner.ScannerException;
 
 import org.springframework.core.io.ByteArrayResource;
 import org.springframework.core.io.ClassPathResource;
-import org.springframework.core.io.Resource;
 
 /**
  * Tests for {@link YamlPropertiesFactoryBean}.
@@ -47,8 +47,8 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testLoadResource() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\nspam:\n  foo: baz".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"foo: bar\nspam:\n  foo: baz".getBytes()));
 		Properties properties = factory.getObject();
 		assertThat(properties.getProperty("foo"), equalTo("bar"));
 		assertThat(properties.getProperty("spam.foo"), equalTo("baz"));
@@ -57,8 +57,8 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testBadResource() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\ncd\nspam:\n  foo: baz".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"foo: bar\ncd\nspam:\n  foo: baz".getBytes()));
 		this.exception.expect(ScannerException.class);
 		this.exception.expectMessage("line 3, column 1");
 		factory.getObject();
@@ -67,9 +67,9 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testLoadResourcesWithOverride() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {
+		factory.setResources(
 				new ByteArrayResource("foo: bar\nspam:\n  foo: baz".getBytes()),
-				new ByteArrayResource("foo:\n  bar: spam".getBytes())});
+				new ByteArrayResource("foo:\n  bar: spam".getBytes()));
 		Properties properties = factory.getObject();
 		assertThat(properties.getProperty("foo"), equalTo("bar"));
 		assertThat(properties.getProperty("spam.foo"), equalTo("baz"));
@@ -77,21 +77,20 @@ public class YamlPropertiesFactoryBeanTests {
 	}
 
 	@Test
-	@Ignore("We can't fail on duplicate keys because the Map is created by the YAML library")
 	public void testLoadResourcesWithInternalOverride() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\nspam:\n  foo: baz\nfoo: bucket".getBytes())});
-		Properties properties = factory.getObject();
-		assertThat(properties.getProperty("foo"), equalTo("bar"));
+		factory.setResources(new ByteArrayResource(
+				"foo: bar\nspam:\n  foo: baz\nfoo: bucket".getBytes()));
+		exception.expect(ParserException.class);
+		factory.getObject();
 	}
 
 	@Test
 	@Ignore("We can't fail on duplicate keys because the Map is created by the YAML library")
 	public void testLoadResourcesWithNestedInternalOverride() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"foo:\n  bar: spam\n  foo: baz\nbreak: it\nfoo: bucket".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"foo:\n  bar: spam\n  foo: baz\nbreak: it\nfoo: bucket".getBytes()));
 		Properties properties = factory.getObject();
 		assertThat(properties.getProperty("foo.bar"), equalTo("spam"));
 	}
@@ -99,8 +98,8 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testLoadResourceWithMultipleDocuments() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\nspam: baz\n---\nfoo: bag".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"foo: bar\nspam: baz\n---\nfoo: bag".getBytes()));
 		Properties properties = factory.getObject();
 		assertThat(properties.getProperty("foo"), equalTo("bag"));
 		assertThat(properties.getProperty("spam"), equalTo("baz"));
@@ -109,8 +108,8 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testLoadResourceWithSelectedDocuments() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"foo: bar\nspam: baz\n---\nfoo: bag\nspam: bad".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"foo: bar\nspam: baz\n---\nfoo: bag\nspam: bad".getBytes()));
 		factory.setDocumentMatchers(new DocumentMatcher() {
 			@Override
 			public MatchStatus matches(Properties properties) {
@@ -127,8 +126,8 @@ public class YamlPropertiesFactoryBeanTests {
 	public void testLoadResourceWithDefaultMatch() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
 		factory.setMatchDefault(true);
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"one: two\n---\nfoo: bar\nspam: baz\n---\nfoo: bag\nspam: bad".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"one: two\n---\nfoo: bar\nspam: baz\n---\nfoo: bag\nspam: bad".getBytes()));
 		factory.setDocumentMatchers(new DocumentMatcher() {
 			@Override
 			public MatchStatus matches(Properties properties) {
@@ -149,8 +148,8 @@ public class YamlPropertiesFactoryBeanTests {
 	public void testLoadResourceWithoutDefaultMatch() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
 		factory.setMatchDefault(false);
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"one: two\n---\nfoo: bar\nspam: baz\n---\nfoo: bag\nspam: bad".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"one: two\n---\nfoo: bar\nspam: baz\n---\nfoo: bag\nspam: bad".getBytes()));
 		factory.setDocumentMatchers(new DocumentMatcher() {
 			@Override
 			public MatchStatus matches(Properties properties) {
@@ -171,8 +170,8 @@ public class YamlPropertiesFactoryBeanTests {
 	public void testLoadResourceWithDefaultMatchSkippingMissedMatch() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
 		factory.setMatchDefault(true);
-		factory.setResources(new Resource[] {new ByteArrayResource(
-				"one: two\n---\nfoo: bag\nspam: bad\n---\nfoo: bar\nspam: baz".getBytes())});
+		factory.setResources(new ByteArrayResource(
+				"one: two\n---\nfoo: bag\nspam: bad\n---\nfoo: bar\nspam: baz".getBytes()));
 		factory.setDocumentMatchers(new DocumentMatcher() {
 			@Override
 			public MatchStatus matches(Properties properties) {
@@ -193,7 +192,7 @@ public class YamlPropertiesFactoryBeanTests {
 	public void testLoadNonExistentResource() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
 		factory.setResolutionMethod(ResolutionMethod.OVERRIDE_AND_IGNORE);
-		factory.setResources(new Resource[] {new ClassPathResource("no-such-file.yml")});
+		factory.setResources(new ClassPathResource("no-such-file.yml"));
 		Properties properties = factory.getObject();
 		assertThat(properties.size(), equalTo(0));
 	}
@@ -201,8 +200,8 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testLoadNull() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource("foo: bar\nspam:"
-				.getBytes())});
+		factory.setResources(new ByteArrayResource("foo: bar\nspam:"
+				.getBytes()));
 		Properties properties = factory.getObject();
 		assertThat(properties.getProperty("foo"), equalTo("bar"));
 		assertThat(properties.getProperty("spam"), equalTo(""));
@@ -211,8 +210,8 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testLoadArrayOfString() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource("foo:\n- bar\n- baz"
-				.getBytes())});
+		factory.setResources(new ByteArrayResource("foo:\n- bar\n- baz"
+				.getBytes()));
 		Properties properties = factory.getObject();
 		assertThat(properties.getProperty("foo[0]"), equalTo("bar"));
 		assertThat(properties.getProperty("foo[1]"), equalTo("baz"));
@@ -221,10 +220,10 @@ public class YamlPropertiesFactoryBeanTests {
 	@Test
 	public void testLoadArrayOfObject() throws Exception {
 		YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
-		factory.setResources(new Resource[] {new ByteArrayResource(
+		factory.setResources(new ByteArrayResource(
 				"foo:\n- bar:\n    spam: crap\n- baz\n- one: two\n  three: four"
 						.getBytes()
-		)});
+		));
 		Properties properties = factory.getObject();
 		assertThat(properties.getProperty("foo[0].bar.spam"), equalTo("crap"));
 		assertThat(properties.getProperty("foo[1]"), equalTo("baz"));