From b1f4320c17b742fb8a452138ed60f40d59e1fc98 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Thu, 25 Jul 2013 09:13:04 +0100 Subject: [PATCH] Added new logic to RelaxedDataBinder to deal with nested stuff * Leverages existing behaviour of BeanWrapperImpl where possible to autogrow collections and lists * Logic for scanning and converting bean paths encapsulated in BeanPath inner class [Fixes #53947797] [bs-249] @ConfigurationProperties cannot bind to Map> --- .../bootstrap/bind/RelaxedDataBinder.java | 272 ++++++++++++++---- .../bind/BindingPreparationTests.java | 240 ++++++++++++++++ .../bind/RelaxedDataBinderTests.java | 16 +- spring-cli/src/test/resources/logback.xml | 2 +- 4 files changed, 470 insertions(+), 60 deletions(-) create mode 100644 spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/BindingPreparationTests.java diff --git a/spring-bootstrap/src/main/java/org/springframework/bootstrap/bind/RelaxedDataBinder.java b/spring-bootstrap/src/main/java/org/springframework/bootstrap/bind/RelaxedDataBinder.java index c253767b310..a4f590e5fa9 100644 --- a/spring-bootstrap/src/main/java/org/springframework/bootstrap/bind/RelaxedDataBinder.java +++ b/spring-bootstrap/src/main/java/org/springframework/bootstrap/bind/RelaxedDataBinder.java @@ -17,6 +17,7 @@ package org.springframework.bootstrap.bind; import java.net.InetAddress; +import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -26,6 +27,7 @@ import org.springframework.beans.BeanWrapperImpl; import org.springframework.beans.InvalidPropertyException; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.PropertyValue; +import org.springframework.core.convert.TypeDescriptor; import org.springframework.util.StringUtils; import org.springframework.validation.DataBinder; @@ -129,66 +131,228 @@ public class RelaxedDataBinder extends DataBinder { private void modifyProperty(MutablePropertyValues propertyValues, BeanWrapper target, PropertyValue propertyValue, int index) { - - String name = propertyValue.getName(); - StringBuilder builder = new StringBuilder(); - Class type = target.getWrappedClass(); - - for (String key : StringUtils.delimitedListToStringArray(name, ".")) { - if (builder.length() != 0) { - builder.append("."); - } - String oldKey = key; - key = getActualPropertyName(target, builder.toString(), oldKey); - builder.append(key); - String base = builder.toString(); - if (!oldKey.equals(key)) { - propertyValues.setPropertyValueAt( - new PropertyValue(base, propertyValue.getValue()), index); - } - type = target.getPropertyType(base); - - // Any nested properties that are maps, are assumed to be simple nested - // maps of maps... - if (type != null && Map.class.isAssignableFrom(type)) { - Map nested = new LinkedHashMap(); - if (target.getPropertyValue(base) != null) { - @SuppressWarnings("unchecked") - Map existing = (Map) target - .getPropertyValue(base); - nested = existing; - } - else { - target.setPropertyValue(base, nested); - } - modifyPopertiesForMap(nested, propertyValues, index, base); - break; - } + String oldName = propertyValue.getName(); + String name = normalizePath(target, oldName); + if (!name.equals(oldName)) { + propertyValues.setPropertyValueAt( + new PropertyValue(name, propertyValue.getValue()), index); } } - private void modifyPopertiesForMap(Map target, - MutablePropertyValues propertyValues, int index, String base) { - PropertyValue propertyValue = propertyValues.getPropertyValueList().get(index); - String name = propertyValue.getName(); - String suffix = name.substring(base.length()); - Map value = new LinkedHashMap(); - String[] tree = StringUtils.delimitedListToStringArray( - suffix.startsWith(".") ? suffix.substring(1) : suffix, "."); - for (int j = 0; j < tree.length - 1; j++) { - if (!target.containsKey(tree[j])) { - target.put(tree[j], value); - } - target = value; - value = new LinkedHashMap(); - } - String refName = base + suffix.replaceAll("\\.([a-zA-Z0-9]*)", "[$1]"); - propertyValues.setPropertyValueAt( - new PropertyValue(refName, propertyValue.getValue()), index); + protected String normalizePath(BeanWrapper wrapper, String path) { + return initializePath(wrapper, new BeanPath(path), 0); + } + private static class BeanPath { + + private List nodes; + + public BeanPath(String path) { + this.nodes = splitPath(path); + } + + public void mapIndex(int index) { + PathNode node = this.nodes.get(index); + if (node instanceof PropertyNode) { + node = ((PropertyNode) node).mapIndex(); + } + this.nodes.set(index, node); + } + + public String prefix(int index) { + return range(0, index); + } + + public void rename(int index, String name) { + this.nodes.get(index).name = name; + } + + public String name(int index) { + if (index < this.nodes.size()) { + return this.nodes.get(index).name; + } + return null; + } + + public int length() { + return this.nodes.size(); + } + + private String range(int start, int end) { + StringBuilder builder = new StringBuilder(); + for (int i = start; i < end; i++) { + PathNode node = this.nodes.get(i); + builder.append(node); + } + if (builder.toString().startsWith(("."))) { + builder.replace(0, 1, ""); + } + return builder.toString(); + } + + public boolean isArrayIndex(int index) { + return this.nodes.get(index) instanceof ArrayIndexNode; + } + + public boolean isProperty(int index) { + return this.nodes.get(index) instanceof PropertyNode; + } + + @Override + public String toString() { + return prefix(this.nodes.size()); + } + + private static class PathNode { + + protected String name; + + public PathNode(String name) { + this.name = name; + } + + } + + private static class ArrayIndexNode extends PathNode { + + public ArrayIndexNode(String name) { + super(name); + } + + @Override + public String toString() { + return "[" + this.name + "]"; + } + + } + + private static class MapIndexNode extends PathNode { + + public MapIndexNode(String name) { + super(name); + } + + @Override + public String toString() { + return "[" + this.name + "]"; + } + } + + private static class PropertyNode extends PathNode { + + public PropertyNode(String name) { + super(name); + } + + public MapIndexNode mapIndex() { + return new MapIndexNode(this.name); + } + + @Override + public String toString() { + return "." + this.name; + } + } + + private List splitPath(String path) { + List nodes = new ArrayList(); + for (String name : StringUtils.delimitedListToStringArray(path, ".")) { + for (String sub : StringUtils.delimitedListToStringArray(name, "[")) { + if (StringUtils.hasText(sub)) { + if (sub.endsWith("]")) { + sub = sub.substring(0, sub.length() - 1); + if (sub.matches("[0-9]+")) { + nodes.add(new ArrayIndexNode(sub)); + } + else { + nodes.add(new MapIndexNode(sub)); + } + } + else { + nodes.add(new PropertyNode(sub)); + } + } + } + } + return nodes; + } + + } + + private String initializePath(BeanWrapper wrapper, BeanPath path, int index) { + String prefix = path.prefix(index); + String key = path.name(index); + if (key == null) { + return path.toString(); + } + if (path.isProperty(index)) { + key = getActualPropertyName(wrapper, prefix, key); + path.rename(index, key); + } + if (index >= path.length() - 1) { + return path.toString(); + } + String name = path.prefix(++index); + TypeDescriptor descriptor = wrapper.getPropertyTypeDescriptor(name); + if (descriptor == null || descriptor.isMap()) { + if (descriptor != null) { + wrapper.getPropertyValue(name + "[foo]"); + } + path.mapIndex(index); + extendMapIfNecessary(wrapper, path, index); + } + else if (descriptor.isCollection()) { + // TODO: test collection extension + extendCollectionIfNecessary(wrapper, path, index); + } + else if (descriptor.getType().equals(Object.class)) { + path.mapIndex(index); + name = path.prefix(index + 1); + if (wrapper.getPropertyValue(name) == null) { + wrapper.setPropertyValue(name, new LinkedHashMap()); + } + } + if (index < path.length()) { + return initializePath(wrapper, path, index); + } + return path.toString(); + } + + private void extendCollectionIfNecessary(BeanWrapper wrapper, BeanPath path, int index) { + String name = path.prefix(index); + TypeDescriptor elementDescriptor = wrapper.getPropertyTypeDescriptor(name) + .getElementTypeDescriptor(); + if (!elementDescriptor.isMap() && !elementDescriptor.isCollection() + && !elementDescriptor.getType().equals(Object.class)) { + return; + } + Object extend = new LinkedHashMap(); + if (!elementDescriptor.isMap() && path.isArrayIndex(index + 1)) { + extend = new ArrayList(); + } + wrapper.setPropertyValue(path.prefix(index + 1), extend); + } + + private void extendMapIfNecessary(BeanWrapper wrapper, BeanPath path, int index) { + String name = path.prefix(index); + TypeDescriptor parent = wrapper.getPropertyTypeDescriptor(name); + if (parent == null) { + return; + } + TypeDescriptor descriptor = parent.getMapValueTypeDescriptor(); + if (!descriptor.isMap() && !descriptor.isCollection() + && !descriptor.getType().equals(Object.class)) { + return; + } + Object extend = new LinkedHashMap(); + if (descriptor.isCollection()) { + extend = new ArrayList(); + } + wrapper.setPropertyValue(path.prefix(index + 1), extend); } private String getActualPropertyName(BeanWrapper target, String prefix, String name) { + prefix = StringUtils.hasText(prefix) ? prefix + "." : ""; for (Variation variation : Variation.values()) { for (Manipulation manipulation : Manipulation.values()) { // Apply all manipulations before attempting variations @@ -199,7 +363,7 @@ public class RelaxedDataBinder extends DataBinder { } } catch (InvalidPropertyException ex) { - // swallow and contrinue + // swallow and continue } } } diff --git a/spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/BindingPreparationTests.java b/spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/BindingPreparationTests.java new file mode 100644 index 00000000000..612c9e03343 --- /dev/null +++ b/spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/BindingPreparationTests.java @@ -0,0 +1,240 @@ +/* + * Copyright 2012-2013 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.bootstrap.bind; + +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.junit.Ignore; +import org.junit.Test; +import org.springframework.beans.BeanWrapperImpl; +import org.springframework.bootstrap.bind.RelaxedDataBinderTests.TargetWithNestedObject; +import org.springframework.context.expression.MapAccessor; +import org.springframework.core.convert.TypeDescriptor; +import org.springframework.expression.Expression; +import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.expression.spel.support.StandardEvaluationContext; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +/** + * @author Dave Syer + */ +public class BindingPreparationTests { + + @Test + public void testBeanWrapperCreatesNewMaps() throws Exception { + TargetWithNestedMap target = new TargetWithNestedMap(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + // For a nested map, you only have to get an element of it for it to be created + wrapper.getPropertyValue("nested[foo]"); + assertNotNull(wrapper.getPropertyValue("nested")); + } + + @Test + public void testBeanWrapperCreatesNewMapEntries() throws Exception { + TargetWithNestedMapOfBean target = new TargetWithNestedMapOfBean(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + // For a nested map, you only have to get an element of it for it to be created + wrapper.getPropertyValue("nested[foo]"); + wrapper.setPropertyValue("nested[foo].foo", "bar"); + assertNotNull(wrapper.getPropertyValue("nested")); + assertNotNull(wrapper.getPropertyValue("nested[foo]")); + } + + @Test + public void testAutoGrowWithFuzzyNameCapitals() throws Exception { + TargetWithNestedMap target = new TargetWithNestedMap(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + RelaxedDataBinder binder = new RelaxedDataBinder(target); + String result = binder.normalizePath(wrapper, "NESTED[foo][bar]"); + assertNotNull(wrapper.getPropertyValue("nested")); + assertEquals("nested[foo][bar]", result); + assertNotNull(wrapper.getPropertyValue("nested[foo][bar]")); + } + + @Test + public void testAutoGrowWithFuzzyNameUnderscores() throws Exception { + TargetWithNestedMap target = new TargetWithNestedMap(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + RelaxedDataBinder binder = new RelaxedDataBinder(target); + String result = binder.normalizePath(wrapper, "nes_ted[foo][bar]"); + assertNotNull(wrapper.getPropertyValue("nested")); + assertEquals("nested[foo][bar]", result); + assertNotNull(wrapper.getPropertyValue("nested[foo][bar]")); + } + + @Test + public void testAutoGrowNewNestedMapOfMaps() throws Exception { + TargetWithNestedMap target = new TargetWithNestedMap(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + RelaxedDataBinder binder = new RelaxedDataBinder(target); + String result = binder.normalizePath(wrapper, "nested[foo][bar]"); + assertNotNull(wrapper.getPropertyValue("nested")); + assertEquals("nested[foo][bar]", result); + assertNotNull(wrapper.getPropertyValue("nested[foo][bar]")); + } + + @Test + public void testAutoGrowNewNestedMapOfBeans() throws Exception { + TargetWithNestedMapOfBean target = new TargetWithNestedMapOfBean(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + RelaxedDataBinder binder = new RelaxedDataBinder(target); + String result = binder.normalizePath(wrapper, "nested[foo].foo"); + assertNotNull(wrapper.getPropertyValue("nested")); + assertEquals("nested[foo].foo", result); + assertNotNull(wrapper.getPropertyValue("nested[foo]")); + } + + @Test + public void testAutoGrowNewNestedMapOfBeansWithPeriod() throws Exception { + TargetWithNestedMapOfBean target = new TargetWithNestedMapOfBean(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + RelaxedDataBinder binder = new RelaxedDataBinder(target); + String result = binder.normalizePath(wrapper, "nested.foo.foo"); + assertNotNull(wrapper.getPropertyValue("nested")); + assertEquals("nested[foo].foo", result); + } + + @Test + public void testAutoGrowNewNestedMapOfListOfString() throws Exception { + TargetWithNestedMapOfListOfString target = new TargetWithNestedMapOfListOfString(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + RelaxedDataBinder binder = new RelaxedDataBinder(target); + binder.normalizePath(wrapper, "nested[foo][0]"); + assertNotNull(wrapper.getPropertyValue("nested")); + assertNotNull(wrapper.getPropertyValue("nested[foo]")); + } + + @Test + public void testBeanWrapperCreatesNewNestedMaps() throws Exception { + TargetWithNestedMap target = new TargetWithNestedMap(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + // For a nested map, you only have to get an element of it for it to be created + wrapper.getPropertyValue("nested[foo]"); + // To decide what type to create for nested[foo] we need to look ahead and see + // what the user is trying to bind it to, e.g. if nested[foo][bar] then it's a map + wrapper.setPropertyValue("nested[foo]", new LinkedHashMap()); + // But it might equally well be a collection, if nested[foo][0] + wrapper.setPropertyValue("nested[foo]", new ArrayList()); + // Then it would have to be actually bound to get the list to autogrow + wrapper.setPropertyValue("nested[foo][0]", "bar"); + assertNotNull(wrapper.getPropertyValue("nested[foo][0]")); + } + + @Test + public void testBeanWrapperCreatesNewObjects() throws Exception { + TargetWithNestedObject target = new TargetWithNestedObject(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + // For a nested object, you have to set a property for it to be created + wrapper.setPropertyValue("nested.foo", "bar"); + wrapper.getPropertyValue("nested"); + assertNotNull(wrapper.getPropertyValue("nested")); + } + + @Test + public void testBeanWrapperLists() throws Exception { + TargetWithNestedMapOfListOfString target = new TargetWithNestedMapOfListOfString(); + BeanWrapperImpl wrapper = new BeanWrapperImpl(target); + wrapper.setAutoGrowNestedPaths(true); + TypeDescriptor descriptor = wrapper.getPropertyTypeDescriptor("nested"); + assertTrue(descriptor.isMap()); + wrapper.getPropertyValue("nested[foo]"); + assertNotNull(wrapper.getPropertyValue("nested")); + // You also need to bind to a value here + wrapper.setPropertyValue("nested[foo][0]", "bar"); + wrapper.getPropertyValue("nested[foo][0]"); + assertNotNull(wrapper.getPropertyValue("nested[foo]")); + } + + @Test + @Ignore("Work in progress") + public void testExpressionLists() throws Exception { + TargetWithNestedMapOfListOfString target = new TargetWithNestedMapOfListOfString(); + LinkedHashMap> map = new LinkedHashMap>(); + // map.put("foo", Arrays.asList("bar")); + target.setNested(map); + SpelExpressionParser parser = new SpelExpressionParser(); + StandardEvaluationContext context = new StandardEvaluationContext(target); + context.addPropertyAccessor(new MapAccessor()); + Expression expression = parser.parseExpression("nested.foo"); + assertNotNull(expression.getValue(context)); + } + + public static class TargetWithNestedMap { + private Map nested; + + public Map getNested() { + return this.nested; + } + + public void setNested(Map nested) { + this.nested = nested; + } + } + + public static class TargetWithNestedMapOfListOfString { + private Map> nested; + + public Map> getNested() { + return this.nested; + } + + public void setNested(Map> nested) { + this.nested = nested; + } + } + + public static class TargetWithNestedMapOfBean { + private Map nested; + + public Map getNested() { + return this.nested; + } + + public void setNested(Map nested) { + this.nested = nested; + } + } + + public static class VanillaTarget { + + private String foo; + + public String getFoo() { + return this.foo; + } + + public void setFoo(String foo) { + this.foo = foo; + } + } +} diff --git a/spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/RelaxedDataBinderTests.java b/spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/RelaxedDataBinderTests.java index df1d0aaebe8..85ccfe7545c 100644 --- a/spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/RelaxedDataBinderTests.java +++ b/spring-bootstrap/src/test/java/org/springframework/bootstrap/bind/RelaxedDataBinderTests.java @@ -36,7 +36,6 @@ import javax.validation.ConstraintValidatorContext; import javax.validation.Payload; import javax.validation.constraints.NotNull; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -76,12 +75,19 @@ public class RelaxedDataBinderTests { } @Test - public void testBindUnderscore() throws Exception { + public void testBindUnderscoreInActualPropertyName() throws Exception { VanillaTarget target = new VanillaTarget(); bind(target, "foo-bar: bar"); assertEquals("bar", target.getFoo_bar()); } + @Test + public void testBindHyphen() throws Exception { + VanillaTarget target = new VanillaTarget(); + bind(target, "foo-baz: bar"); + assertEquals("bar", target.getFooBaz()); + } + @Test public void testBindCamelCase() throws Exception { VanillaTarget target = new VanillaTarget(); @@ -208,7 +214,7 @@ public class RelaxedDataBinderTests { } @Test - @Ignore("Should be possible but currently not supported") + // @Ignore("Should be possible but currently not supported") // FIXME: bind to map containing beans public void testBindNestedMapOfBean() throws Exception { TargetWithNestedMapOfBean target = new TargetWithNestedMapOfBean(); @@ -218,7 +224,7 @@ public class RelaxedDataBinderTests { } @Test - @Ignore("Should be possible but currently not supported") + // @Ignore("Should be possible but currently not supported") // FIXME: bind to map containing beans public void testBindNestedMapOfListOfBean() throws Exception { TargetWithNestedMapOfListOfBean target = new TargetWithNestedMapOfListOfBean(); @@ -265,7 +271,7 @@ public class RelaxedDataBinderTests { } @Test - public void testBindMapNestedInMap() throws Exception { + public void testBindMapNestedMap() throws Exception { Map target = new LinkedHashMap(); BindingResult result = bind(target, "spam: bar\n" + "vanilla.foo.value: 123", "vanilla"); diff --git a/spring-cli/src/test/resources/logback.xml b/spring-cli/src/test/resources/logback.xml index 407f13b26aa..845832cda61 100644 --- a/spring-cli/src/test/resources/logback.xml +++ b/spring-cli/src/test/resources/logback.xml @@ -14,6 +14,6 @@ - +