YamlProcessor embraces SnakeYAML 1.18+ duplicate key handling

Includes removal of StrictMapAppenderConstructor for compatibility with SnakeYAML 1.21.

Issue: SPR-16791

Author: jhoeller

spring-beans/spring-beans.gradle

@@ -5,7 +5,7 @@ apply plugin: "groovy"
 dependencies {
 	compile(project(':spring-core'))
 	optional("javax.inject:javax.inject:1")
-	optional("org.yaml:snakeyaml:1.20")
+	optional("org.yaml:snakeyaml:1.21")
     optional("org.codehaus.groovy:groovy-all:${groovyVersion}")
 	optional("org.jetbrains.kotlin:kotlin-reflect:${kotlinVersion}")
 	optional("org.jetbrains.kotlin:kotlin-stdlib:${kotlinVersion}")

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -64,6 +64,8 @@
  * Note that the value of "foo" in the first document is not simply replaced
  * with the value in the second, but its nested values are merged.
  *
+ * <p>Requires SnakeYAML 1.18 or higher, as of Spring Framework 5.0.6.
+ *
  * @author Dave Syer
  * @author Juergen Hoeller
  * @since 4.1

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

@@ -18,22 +18,18 @@
 
 import java.io.IOException;
 import java.io.Reader;
-import java.util.AbstractMap;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
-import java.util.Set;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.yaml.snakeyaml.LoaderOptions;
 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.yaml.snakeyaml.reader.UnicodeReader;
 
 import org.springframework.core.CollectionFactory;
@@ -45,6 +41,8 @@
 /**
  * Base class for YAML factories.
  *
+ * <p>Requires SnakeYAML 1.18 or higher, as of Spring Framework 5.0.6.
+ *
  * @author Dave Syer
  * @author Juergen Hoeller
  * @since 4.1
@@ -144,9 +142,14 @@ protected void process(MatchCallback callback) {
 
 	/**
 	 * Create the {@link Yaml} instance to use.
+	 * <p>The default implementation sets the "allowDuplicateKeys" flag to {@code false},
+	 * enabling built-in duplicate key handling in SnakeYAML 1.18+.
+	 * @see LoaderOptions#setAllowDuplicateKeys(boolean)
 	 */
 	protected Yaml createYaml() {
-		return new Yaml(new StrictMapAppenderConstructor());
+		LoaderOptions options = new LoaderOptions();
+		options.setAllowDuplicateKeys(false);
+		return new Yaml(options);
 	}
 
 	private boolean process(MatchCallback callback, Yaml yaml, Resource resource) {
@@ -390,45 +393,4 @@ public enum ResolutionMethod {
 		FIRST_FOUND
 	}
 
-
-	/**
-	 * A specialized {@link Constructor} that checks for duplicate keys.
-	 */
-	protected static class StrictMapAppenderConstructor extends Constructor {
-
-		// Declared as public for use in subclasses
-		public StrictMapAppenderConstructor() {
-			super();
-		}
-
-		@Override
-		protected Map<Object, Object> constructMapping(MappingNode node) {
-			try {
-				return super.constructMapping(node);
-			}
-			catch (IllegalStateException ex) {
-				throw new ParserException("while parsing MappingNode",
-						node.getStartMark(), ex.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/main/java/org/springframework/beans/factory/config/YamlPropertiesFactoryBean.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 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,6 +74,8 @@
  * servers[1]=foo.bar.com
  * </pre>
  *
+ * <p>Requires SnakeYAML 1.18 or higher, as of Spring Framework 5.0.6.
+ *
  * @author Dave Syer
  * @author Stephane Nicoll
  * @author Juergen Hoeller

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

@@ -19,10 +19,10 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 
 import org.junit.Test;
+import org.yaml.snakeyaml.constructor.DuplicateKeyException;
 
 import org.springframework.core.io.AbstractResource;
 import org.springframework.core.io.ByteArrayResource;
@@ -42,27 +42,27 @@ public class YamlMapFactoryBeanTests {
 
 
 	@Test
-	public void testSetIgnoreResourceNotFound() throws Exception {
+	public void testSetIgnoreResourceNotFound() {
 		this.factory.setResolutionMethod(YamlMapFactoryBean.ResolutionMethod.OVERRIDE_AND_IGNORE);
 		this.factory.setResources(new FileSystemResource("non-exsitent-file.yml"));
 		assertEquals(0, this.factory.getObject().size());
 	}
 
 	@Test(expected = IllegalStateException.class)
-	public void testSetBarfOnResourceNotFound() throws Exception {
+	public void testSetBarfOnResourceNotFound() {
 		this.factory.setResources(new FileSystemResource("non-exsitent-file.yml"));
 		assertEquals(0, this.factory.getObject().size());
 	}
 
 	@Test
-	public void testGetObject() throws Exception {
+	public void testGetObject() {
 		this.factory.setResources(new ByteArrayResource("foo: bar".getBytes()));
 		assertEquals(1, this.factory.getObject().size());
 	}
 
 	@SuppressWarnings("unchecked")
 	@Test
-	public void testOverrideAndRemoveDefaults() throws Exception {
+	public void testOverrideAndRemoveDefaults() {
 		this.factory.setResources(new ByteArrayResource("foo:\n  bar: spam".getBytes()),
 				new ByteArrayResource("foo:\n  spam: bar".getBytes()));
 
@@ -71,7 +71,7 @@ public void testOverrideAndRemoveDefaults() throws Exception {
 	}
 
 	@Test
-	public void testFirstFound() throws Exception {
+	public void testFirstFound() {
 		this.factory.setResolutionMethod(YamlProcessor.ResolutionMethod.FIRST_FOUND);
 		this.factory.setResources(new AbstractResource() {
 			@Override
@@ -88,7 +88,7 @@ public InputStream getInputStream() throws IOException {
 	}
 
 	@Test
-	public void testMapWithPeriodsInKey() throws Exception {
+	public void testMapWithPeriodsInKey() {
 		this.factory.setResources(new ByteArrayResource("foo:\n  ? key1.key2\n  : value".getBytes()));
 		Map<String, Object> map = this.factory.getObject();
 
@@ -103,7 +103,7 @@ public void testMapWithPeriodsInKey() throws Exception {
 	}
 
 	@Test
-	public void testMapWithIntegerValue() throws Exception {
+	public void testMapWithIntegerValue() {
 		this.factory.setResources(new ByteArrayResource("foo:\n  ? key1.key2\n  : 3".getBytes()));
 		Map<String, Object> map = this.factory.getObject();
 
@@ -117,33 +117,10 @@ public void testMapWithIntegerValue() throws Exception {
 		assertEquals(Integer.valueOf(3), sub.get("key1.key2"));
 	}
 
-	@Test
-	public void mapWithEmptyArrayValue() {
-		this.factory.setResources(new ByteArrayResource("a: alpha\ntest: []".getBytes()));
-		assertTrue(this.factory.getObject().containsKey("test"));
-		assertEquals(((List<?>)this.factory.getObject().get("test")).size(), 0);
-	}
-
-	@Test
-	public void mapWithEmptyValue() {
-		this.factory.setResources(new ByteArrayResource("a: alpha\ntest:".getBytes()));
-		assertTrue(this.factory.getObject().containsKey("test"));
-		assertNull(this.factory.getObject().get("test"));
-	}
-
-	@Test
-	public void testDuplicateKey() throws Exception {
+	@Test(expected = DuplicateKeyException.class)
+	public void testDuplicateKey() {
 		this.factory.setResources(new ByteArrayResource("mymap:\n  foo: bar\nmymap:\n  bar: foo".getBytes()));
-		Map<String, Object> map = this.factory.getObject();
-
-		assertEquals(1, map.size());
-		assertTrue(map.containsKey("mymap"));
-		Object object = map.get("mymap");
-		assertTrue(object instanceof LinkedHashMap);
-		@SuppressWarnings("unchecked")
-		Map<String, Object> sub = (Map<String, Object>) object;
-		assertEquals(1, sub.size());
-		assertEquals("foo", sub.get("bar"));
+		this.factory.getObject().get("mymap");
 	}
 
 }