From 859e1e800345d528b17f23e74dfaf8bf4185b070 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 14 Oct 2014 15:58:35 +0200 Subject: [PATCH] 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 --- .../beans/factory/config/YamlProcessor.java | 47 +++++++++++++- .../config/YamlMapFactoryBeanTests.java | 13 +++- .../factory/config/YamlProcessorTests.java | 29 +++++---- .../YamlPropertiesFactoryBeanTests.java | 61 +++++++++---------- 4 files changed, 100 insertions(+), 50 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/config/YamlProcessor.java b/spring-beans/src/main/java/org/springframework/beans/factory/config/YamlProcessor.java index a2c822f81ed..9e0e58ba361 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/config/YamlProcessor.java +++ b/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 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 createDefaultMap() { + final Map delegate = super.createDefaultMap(); + return new AbstractMap() { + @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> entrySet() { + return delegate.entrySet(); + } + }; + } + + } + } diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlMapFactoryBeanTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlMapFactoryBeanTests.java index a69762fd78a..69ff8067fc0 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlMapFactoryBeanTests.java +++ b/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"); + } + } diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlProcessorTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlProcessorTests.java index fca1edfdfca..49587f48ca4 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlProcessorTests.java +++ b/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 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 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 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 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 diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlPropertiesFactoryBeanTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlPropertiesFactoryBeanTests.java index 3c2c711b643..2487c4330dc 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/config/YamlPropertiesFactoryBeanTests.java +++ b/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"));