From 6578239f679f90d3404fff98f439c1eb61b9a75d Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Thu, 29 Oct 2015 10:29:41 +0100 Subject: [PATCH] Fix binding of Collection of enum As there is no way to copy all the converters of a `ConversionService` to another, `RelaxedConversionService` uses a fallback `ConversionService` when the user-provided one failed. That fallback is taking care of converting `String` to `Enum` in a case insensitive way but it has no registered converter to convert a comma separated String to a collection of something. Ironically, our current test suite has plenty of cases where we map a `String` to a collection of enums and they all pass. This is because the tests do not provide a custom `ConverterService` so we end up immediately in the fallback scenario. Since no converter is able to convert the String to a collection, the property editor support of the binder takes care of that for us and try to convert each individual value. In a regular use case however, a `ConversionService` is provided and fails to map the collection if the String value(s) don't have the exact same case as the annotations they represent. Since the original `ConversionService` has claimed it was able to convert a collection, the raw `String` value is passed to the fallback converter and that one fails to convert the raw String. The fallback converter now registers the necessary converters to convert collections. Additional tests have been added to test that in a more explicit way. Closes gh-4322 --- .../boot/bind/RelaxedConversionService.java | 5 ++- .../boot/bind/RelaxedDataBinderTests.java | 18 +++++++++++ ...onPropertiesBindingPostProcessorTests.java | 31 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/bind/RelaxedConversionService.java b/spring-boot/src/main/java/org/springframework/boot/bind/RelaxedConversionService.java index 2c143186cf..2ba672915f 100644 --- a/spring-boot/src/main/java/org/springframework/boot/bind/RelaxedConversionService.java +++ b/spring-boot/src/main/java/org/springframework/boot/bind/RelaxedConversionService.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 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. @@ -24,6 +24,7 @@ import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.TypeDescriptor; import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.ConverterFactory; +import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.core.convert.support.GenericConversionService; import org.springframework.util.Assert; @@ -32,6 +33,7 @@ import org.springframework.util.Assert; * additional relaxed conversion. * * @author Phillip Webb + * @author Stephane Nicoll * @since 1.1.0 */ class RelaxedConversionService implements ConversionService { @@ -47,6 +49,7 @@ class RelaxedConversionService implements ConversionService { RelaxedConversionService(ConversionService conversionService) { this.conversionService = conversionService; this.additionalConverters = new GenericConversionService(); + DefaultConversionService.addCollectionConverters(this.additionalConverters); this.additionalConverters .addConverterFactory(new StringToEnumIgnoringCaseConverterFactory()); this.additionalConverters.addConverter(new StringToCharArrayConverter()); diff --git a/spring-boot/src/test/java/org/springframework/boot/bind/RelaxedDataBinderTests.java b/spring-boot/src/test/java/org/springframework/boot/bind/RelaxedDataBinderTests.java index 9a61da8e61..84761a634a 100644 --- a/spring-boot/src/test/java/org/springframework/boot/bind/RelaxedDataBinderTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/bind/RelaxedDataBinderTests.java @@ -53,6 +53,7 @@ import org.springframework.validation.DataBinder; import org.springframework.validation.FieldError; import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; @@ -671,6 +672,14 @@ public class RelaxedDataBinderTests { result = bind(target, "bingo: The_Other"); assertThat(result.getErrorCount(), equalTo(0)); assertThat(target.getBingo(), equalTo(Bingo.THE_OTHER)); + + result = bind(target, "bingos: The_Other"); + assertThat(result.getErrorCount(), equalTo(0)); + assertThat(target.getBingos(), contains(Bingo.THE_OTHER)); + + result = bind(target, "bingos: The_Other, that"); + assertThat(result.getErrorCount(), equalTo(0)); + assertThat(target.getBingos(), contains(Bingo.THE_OTHER, Bingo.THAT)); } private BindingResult bind(Object target, String values) throws Exception { @@ -995,6 +1004,8 @@ public class RelaxedDataBinderTests { private Bingo bingo; + private List bingos; + public char[] getBar() { return this.bar; } @@ -1043,6 +1054,13 @@ public class RelaxedDataBinderTests { this.bingo = bingo; } + public List getBingos() { + return this.bingos; + } + + public void setBingos(List bingos) { + this.bingos = bingos; + } } enum Bingo { diff --git a/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java b/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java index 74c1d2ec63..81ad7c3dba 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java @@ -16,6 +16,8 @@ package org.springframework.boot.context.properties; +import java.util.List; + import javax.annotation.PostConstruct; import javax.validation.constraints.NotNull; @@ -43,6 +45,7 @@ import org.springframework.validation.Errors; import org.springframework.validation.ValidationUtils; import org.springframework.validation.Validator; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertEquals; @@ -175,6 +178,24 @@ public class ConfigurationPropertiesBindingPostProcessorTests { this.context.close(); } + @Test + public void testRelaxedPropertyWithSetOfEnum() { + doEnumSetTest("test.the-values:foo,bar", FooEnum.FOO, FooEnum.BAR); + doEnumSetTest("test.the-values:foo", FooEnum.FOO); + doEnumSetTest("TEST_THE_VALUES:FoO", FooEnum.FOO); + doEnumSetTest("test_the_values:BaR,FoO", FooEnum.BAR, FooEnum.FOO); + } + + private void doEnumSetTest(String property, FooEnum... expected) { + this.context = new AnnotationConfigApplicationContext(); + EnvironmentTestUtils.addEnvironment(this.context, property); + this.context.register(PropertyWithEnum.class); + this.context.refresh(); + assertThat(this.context.getBean(PropertyWithEnum.class).getTheValues(), + contains(expected)); + this.context.close(); + } + @Test public void testValueBindingForDefaults() throws Exception { this.context = new AnnotationConfigApplicationContext(); @@ -524,6 +545,8 @@ public class ConfigurationPropertiesBindingPostProcessorTests { private FooEnum theValue; + private List theValues; + public void setTheValue(FooEnum value) { this.theValue = value; } @@ -532,6 +555,14 @@ public class ConfigurationPropertiesBindingPostProcessorTests { return this.theValue; } + public List getTheValues() { + return this.theValues; + } + + public void setTheValues(List theValues) { + this.theValues = theValues; + } + } enum FooEnum {