From 326a56da01e1e32f3b4b3123983f982b090cc6ad Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Sun, 13 Sep 2020 10:49:08 -0700 Subject: [PATCH] Support validation of bound map key entries Update `ValidationBindHandler` so that pushed fields that reference map keys can be used. This fixes a regression that was introduced in commit 4483f417 when we switched to a `AbstractBindingResult` that no longer required public getters/setters. Closes gh-20350 --- .../bind/DataObjectPropertyName.java | 29 +++++-- .../validation/ValidationBindHandler.java | 45 ++++++++--- .../source/ConfigurationPropertyName.java | 16 +++- .../bind/DataObjectPropertyNameTests.java | 10 ++- .../ValidationBindHandlerTests.java | 79 +++++++++++++++++++ .../ConfigurationPropertyNameTests.java | 12 ++- 6 files changed, 171 insertions(+), 20 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DataObjectPropertyName.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DataObjectPropertyName.java index 4bee580ad13..041a439b2f9 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DataObjectPropertyName.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DataObjectPropertyName.java @@ -35,14 +35,29 @@ public abstract class DataObjectPropertyName { * @return the dashed from */ public static String toDashedForm(String name) { - StringBuilder result = new StringBuilder(); - String replaced = name.replace('_', '-'); - for (int i = 0; i < replaced.length(); i++) { - char ch = replaced.charAt(i); - if (Character.isUpperCase(ch) && result.length() > 0 && result.charAt(result.length() - 1) != '-') { - result.append('-'); + StringBuilder result = new StringBuilder(name.length()); + boolean inIndex = false; + for (int i = 0; i < name.length(); i++) { + char ch = name.charAt(i); + if (inIndex) { + result.append(ch); + if (ch == ']') { + inIndex = false; + } + } + else { + if (ch == '[') { + inIndex = true; + result.append(ch); + } + else { + ch = (ch != '_') ? ch : '-'; + if (Character.isUpperCase(ch) && result.length() > 0 && result.charAt(result.length() - 1) != '-') { + result.append('-'); + } + result.append(Character.toLowerCase(ch)); + } } - result.append(Character.toLowerCase(ch)); } return result.toString(); } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandler.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandler.java index 914138bf18a..e4196a1c31a 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandler.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandler.java @@ -29,7 +29,9 @@ import org.springframework.boot.context.properties.bind.Bindable; import org.springframework.boot.context.properties.bind.DataObjectPropertyName; import org.springframework.boot.context.properties.source.ConfigurationProperty; import org.springframework.boot.context.properties.source.ConfigurationPropertyName; +import org.springframework.boot.context.properties.source.ConfigurationPropertyName.Form; import org.springframework.core.ResolvableType; +import org.springframework.util.ObjectUtils; import org.springframework.validation.AbstractBindingResult; import org.springframework.validation.Validator; @@ -165,28 +167,53 @@ public class ValidationBindHandler extends AbstractBindHandler { @Override public Class getFieldType(String field) { - try { - ResolvableType type = ValidationBindHandler.this.boundTypes.get(getName(field)); - Class resolved = (type != null) ? type.resolve() : null; - if (resolved != null) { - return resolved; - } - } - catch (Exception ex) { + ResolvableType type = getBoundField(ValidationBindHandler.this.boundTypes, field); + Class resolved = (type != null) ? type.resolve() : null; + if (resolved != null) { + return resolved; } return super.getFieldType(field); } @Override protected Object getActualFieldValue(String field) { + return getBoundField(ValidationBindHandler.this.boundResults, field); + } + + private T getBoundField(Map boundFields, String field) { try { - return ValidationBindHandler.this.boundResults.get(getName(field)); + ConfigurationPropertyName name = getName(field); + T bound = boundFields.get(name); + if (bound != null) { + return bound; + } + if (name.hasIndexedElement()) { + for (Map.Entry entry : boundFields.entrySet()) { + if (isFieldNameMatch(entry.getKey(), name)) { + return entry.getValue(); + } + } + } } catch (Exception ex) { } return null; } + private boolean isFieldNameMatch(ConfigurationPropertyName name, ConfigurationPropertyName fieldName) { + if (name.getNumberOfElements() != fieldName.getNumberOfElements()) { + return false; + } + for (int i = 0; i < name.getNumberOfElements(); i++) { + String element = name.getElement(i, Form.ORIGINAL); + String fieldElement = fieldName.getElement(i, Form.ORIGINAL); + if (!ObjectUtils.nullSafeEquals(element, fieldElement)) { + return false; + } + } + return true; + } + private ConfigurationPropertyName getName(String field) { return this.name.append(DataObjectPropertyName.toDashedForm(field)); } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java index cb63a5b8fde..361b8d29e46 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 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. @@ -86,6 +86,20 @@ public final class ConfigurationPropertyName implements Comparable 0 && isIndexed(size - 1)); } + /** + * Return {@code true} if any element in the name is indexed. + * @return if the element has one or more indexed elements + * @since 2.2.10 + */ + public boolean hasIndexedElement() { + for (int i = 0; i < getNumberOfElements(); i++) { + if (isIndexed(i)) { + return true; + } + } + return false; + } + /** * Return if the element in the name is indexed. * @param elementIndex the index of the element diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DataObjectPropertyNameTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DataObjectPropertyNameTests.java index dcdeeebf4b6..468454623ce 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DataObjectPropertyNameTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DataObjectPropertyNameTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 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. @@ -29,7 +29,7 @@ import static org.assertj.core.api.Assertions.assertThat; class DataObjectPropertyNameTests { @Test - void toDashedCaseShouldConvertValue() { + void toDashedCaseConvertsValue() { assertThat(DataObjectPropertyName.toDashedForm("Foo")).isEqualTo("foo"); assertThat(DataObjectPropertyName.toDashedForm("foo")).isEqualTo("foo"); assertThat(DataObjectPropertyName.toDashedForm("fooBar")).isEqualTo("foo-bar"); @@ -38,4 +38,10 @@ class DataObjectPropertyNameTests { assertThat(DataObjectPropertyName.toDashedForm("foo_Bar")).isEqualTo("foo-bar"); } + @Test + void toDashedFormWhenContainsIndexedAddsNoDashToIndex() throws Exception { + assertThat(DataObjectPropertyName.toDashedForm("test[fooBar]")).isEqualTo("test[fooBar]"); + assertThat(DataObjectPropertyName.toDashedForm("testAgain[fooBar]")).isEqualTo("test-again[fooBar]"); + } + } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandlerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandlerTests.java index 82c4133b882..450e1e79e77 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandlerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/validation/ValidationBindHandlerTests.java @@ -17,7 +17,9 @@ package org.springframework.boot.context.properties.bind.validation; import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.Set; import javax.validation.Valid; @@ -35,11 +37,16 @@ import org.springframework.boot.context.properties.bind.Binder; import org.springframework.boot.context.properties.source.ConfigurationProperty; import org.springframework.boot.context.properties.source.ConfigurationPropertyName; import org.springframework.boot.context.properties.source.ConfigurationPropertySource; +import org.springframework.boot.context.properties.source.ConfigurationPropertySources; import org.springframework.boot.context.properties.source.MockConfigurationPropertySource; import org.springframework.boot.origin.Origin; import org.springframework.core.convert.ConverterNotFoundException; +import org.springframework.core.env.MapPropertySource; +import org.springframework.validation.Errors; import org.springframework.validation.FieldError; import org.springframework.validation.ObjectError; +import org.springframework.validation.ValidationUtils; +import org.springframework.validation.Validator; import org.springframework.validation.annotation.Validated; import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; @@ -210,6 +217,54 @@ class ValidationBindHandlerTests { assertThat(fieldError.getField()).isEqualTo("personAge"); } + @Test + void validateMapValues() throws Exception { + MockConfigurationPropertySource source = new MockConfigurationPropertySource(); + source.put("test.items.[itemOne].number", "one"); + source.put("test.items.[ITEM2].number", "two"); + this.sources.add(source); + Validator validator = getMapValidator(); + this.handler = new ValidationBindHandler(validator); + this.binder.bind(ConfigurationPropertyName.of("test"), Bindable.of(ExampleWithMap.class), this.handler); + } + + @Test + void validateMapValuesWithNonUniformSource() throws Exception { + Map map = new LinkedHashMap<>(); + map.put("test.items.itemOne.number", "one"); + map.put("test.items.ITEM2.number", "two"); + this.sources.add(ConfigurationPropertySources.from(new MapPropertySource("test", map)).iterator().next()); + Validator validator = getMapValidator(); + this.handler = new ValidationBindHandler(validator); + this.binder.bind(ConfigurationPropertyName.of("test"), Bindable.of(ExampleWithMap.class), this.handler); + } + + private Validator getMapValidator() { + return new Validator() { + + @Override + public boolean supports(Class clazz) { + return ExampleWithMap.class == clazz; + + } + + @Override + public void validate(Object target, Errors errors) { + ExampleWithMap value = (ExampleWithMap) target; + value.getItems().forEach((k, v) -> { + try { + errors.pushNestedPath("items[" + k + "]"); + ValidationUtils.rejectIfEmptyOrWhitespace(errors, "number", "NUMBER_ERR"); + } + finally { + errors.popNestedPath(); + } + }); + } + + }; + } + private BindValidationException bindAndExpectValidationError(Runnable action) { try { action.run(); @@ -358,6 +413,30 @@ class ValidationBindHandlerTests { } + static class ExampleWithMap { + + private Map items = new LinkedHashMap<>(); + + Map getItems() { + return this.items; + } + + } + + static class ExampleMapValue { + + private String number; + + String getNumber() { + return this.number; + } + + void setNumber(String number) { + this.number = number; + } + + } + static class TestHandler extends AbstractBindHandler { private Object result; diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java index cbb66a50e6f..755edab8d33 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 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. @@ -618,4 +618,14 @@ class ConfigurationPropertyNameTests { assertThat(ConfigurationPropertyName.isValid("foo!bar")).isFalse(); } + @Test + void hasIndexedElementWhenHasIndexedElementReturnsTrue() throws Exception { + assertThat(ConfigurationPropertyName.of("foo[bar]").hasIndexedElement()).isTrue(); + } + + @Test + void hasIndexedElementWhenHasNoIndexedElementReturnsFalse() throws Exception { + assertThat(ConfigurationPropertyName.of("foo.bar").hasIndexedElement()).isFalse(); + } + }