From 07acc4af08a8001487d5196a617a02441245ae4f Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 1 Jul 2019 21:57:23 -0700 Subject: [PATCH] Consistently coerce booleans to enums Rename `StringToEnumIgnoringCaseConverterFactory` to `LenientStringToEnumConverterFactory` and extended it to support binding of YAML style 'true'/'false' values to 'ON'/'OFF'. Closes gh-17385 --- .../logging/LoggingApplicationListener.java | 20 +++------ .../convert/ApplicationConversionService.java | 2 +- ... LenientStringToEnumConverterFactory.java} | 41 ++++++++++++++++--- .../boot/SpringApplicationTests.java | 8 ++++ .../LoggingApplicationListenerTests.java | 7 ++-- ...entStringToEnumConverterFactoryTests.java} | 35 ++++++++++++++-- 6 files changed, 86 insertions(+), 27 deletions(-) rename spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/{StringToEnumIgnoringCaseConverterFactory.java => LenientStringToEnumConverterFactory.java} (64%) rename spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/{StringToEnumIgnoringCaseConverterFactoryTests.java => LenientStringToEnumConverterFactoryTests.java} (66%) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java index 9502422769..4d8de4265d 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java @@ -19,7 +19,6 @@ package org.springframework.boot.context.logging; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; -import java.util.Locale; import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; @@ -93,7 +92,8 @@ public class LoggingApplicationListener implements GenericApplicationListener { private static final ConfigurationPropertyName LOGGING_GROUP = ConfigurationPropertyName.of("logging.group"); - private static final Bindable> STRING_STRING_MAP = Bindable.mapOf(String.class, String.class); + private static final Bindable> STRING_LOGLEVEL_MAP = Bindable.mapOf(String.class, + LogLevel.class); private static final Bindable> STRING_STRINGS_MAP = Bindable.mapOf(String.class, String[].class); @@ -326,7 +326,7 @@ public class LoggingApplicationListener implements GenericApplicationListener { Binder binder = Binder.get(environment); Map groups = getGroups(); binder.bind(LOGGING_GROUP, STRING_STRINGS_MAP.withExistingValue(groups)); - Map levels = binder.bind(LOGGING_LEVEL, STRING_STRING_MAP).orElseGet(Collections::emptyMap); + Map levels = binder.bind(LOGGING_LEVEL, STRING_LOGLEVEL_MAP).orElseGet(Collections::emptyMap); levels.forEach((name, level) -> { String[] groupedNames = groups.get(name); if (ObjectUtils.isEmpty(groupedNames)) { @@ -344,30 +344,22 @@ public class LoggingApplicationListener implements GenericApplicationListener { return groups; } - private void setLogLevel(LoggingSystem system, String[] names, String level) { + private void setLogLevel(LoggingSystem system, String[] names, LogLevel level) { for (String name : names) { setLogLevel(system, name, level); } } - private void setLogLevel(LoggingSystem system, String name, String level) { + private void setLogLevel(LoggingSystem system, String name, LogLevel level) { try { name = name.equalsIgnoreCase(LoggingSystem.ROOT_LOGGER_NAME) ? null : name; - system.setLogLevel(name, coerceLogLevel(level)); + system.setLogLevel(name, level); } catch (RuntimeException ex) { this.logger.error("Cannot set level '" + level + "' for '" + name + "'"); } } - private LogLevel coerceLogLevel(String level) { - String trimmedLevel = level.trim(); - if ("false".equalsIgnoreCase(trimmedLevel)) { - return LogLevel.OFF; - } - return LogLevel.valueOf(trimmedLevel.toUpperCase(Locale.ENGLISH)); - } - private void registerShutdownHookIfNecessary(Environment environment, LoggingSystem loggingSystem) { boolean registerShutdownHook = environment.getProperty(REGISTER_SHUTDOWN_HOOK_PROPERTY, Boolean.class, false); if (registerShutdownHook) { diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/ApplicationConversionService.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/ApplicationConversionService.java index 15e059a52c..885009abaa 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/ApplicationConversionService.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/ApplicationConversionService.java @@ -115,7 +115,7 @@ public class ApplicationConversionService extends FormattingConversionService { registry.addConverter(new DurationToNumberConverter()); registry.addConverter(new StringToDataSizeConverter()); registry.addConverter(new NumberToDataSizeConverter()); - registry.addConverterFactory(new StringToEnumIgnoringCaseConverterFactory()); + registry.addConverterFactory(new LenientStringToEnumConverterFactory()); } /** diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/StringToEnumIgnoringCaseConverterFactory.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/LenientStringToEnumConverterFactory.java similarity index 64% rename from spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/StringToEnumIgnoringCaseConverterFactory.java rename to spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/LenientStringToEnumConverterFactory.java index 5f9a7f88a4..2521afe234 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/StringToEnumIgnoringCaseConverterFactory.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/convert/LenientStringToEnumConverterFactory.java @@ -16,21 +16,41 @@ package org.springframework.boot.convert; +import java.util.Collections; import java.util.EnumSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; import java.util.Set; import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.ConverterFactory; import org.springframework.util.Assert; +import org.springframework.util.LinkedMultiValueMap; +import org.springframework.util.MultiValueMap; /** - * Converts from a String to a {@link java.lang.Enum} by calling searching matching enum - * names (ignoring case). + * Converts from a String to a {@link java.lang.Enum} with lenient conversion rules. + * Specifically: + *
    + *
  • Uses a case insensitive search
    • + *
  • Does not consider {@code '_'}, {@code '$'} or other special characters
    • + *
  • Allows mapping of YAML style {@code "false"} and {@code "true"} to enums {@code ON} + * and {@code OFF}
    • + *
* * @author Phillip Webb */ @SuppressWarnings({ "unchecked", "rawtypes" }) -final class StringToEnumIgnoringCaseConverterFactory implements ConverterFactory { +final class LenientStringToEnumConverterFactory implements ConverterFactory { + + private static Map> ALIASES; + static { + MultiValueMap aliases = new LinkedMultiValueMap<>(); + aliases.add("true", "on"); + aliases.add("false", "off"); + ALIASES = Collections.unmodifiableMap(aliases); + } @Override public Converter getConverter(Class targetType) { @@ -65,10 +85,19 @@ final class StringToEnumIgnoringCaseConverterFactory implements ConverterFactory } private T findEnum(String source) { - String name = getLettersAndDigits(source); + Map candidates = new LinkedHashMap(); for (T candidate : (Set) EnumSet.allOf(this.enumType)) { - if (getLettersAndDigits(candidate.name()).equals(name)) { - return candidate; + candidates.put(getLettersAndDigits(candidate.name()), candidate); + } + String name = getLettersAndDigits(source); + T result = candidates.get(name); + if (result != null) { + return result; + } + for (String alias : ALIASES.getOrDefault(name, Collections.emptyList())) { + result = candidates.get(alias); + if (result != null) { + return result; } } throw new IllegalArgumentException("No enum constant " + this.enumType.getCanonicalName() + "." + source); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java index 5b2843962d..1125a1f7be 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java @@ -288,6 +288,14 @@ class SpringApplicationTests { assertThat(application).hasFieldOrPropertyWithValue("bannerMode", Banner.Mode.OFF); } + @Test + void bindsYamlStyleBannerModeToSpringApplication() { + SpringApplication application = new SpringApplication(ExampleConfig.class); + application.setWebApplicationType(WebApplicationType.NONE); + this.context = application.run("--spring.main.banner-mode=false"); + assertThat(application).hasFieldOrPropertyWithValue("bannerMode", Banner.Mode.OFF); + } + @Test void customId() { SpringApplication application = new SpringApplication(ExampleConfig.class); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java index 209f54b4fe..0263538f99 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java @@ -44,6 +44,7 @@ import org.slf4j.impl.StaticLoggerBinder; import org.springframework.boot.SpringApplication; import org.springframework.boot.context.event.ApplicationFailedEvent; import org.springframework.boot.context.event.ApplicationStartingEvent; +import org.springframework.boot.context.properties.bind.BindException; import org.springframework.boot.context.properties.source.ConfigurationPropertySources; import org.springframework.boot.logging.AbstractLoggingSystem; import org.springframework.boot.logging.LogFile; @@ -69,6 +70,7 @@ import org.springframework.core.env.MutablePropertySources; import org.springframework.test.util.ReflectionTestUtils; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.not; @@ -369,9 +371,8 @@ public class LoggingApplicationListenerTests { public void parseLevelsFails() { this.logger.setLevel(Level.INFO); addPropertiesToEnvironment(this.context, "logging.level.org.springframework.boot=GARBAGE"); - this.initializer.initialize(this.context.getEnvironment(), this.context.getClassLoader()); - this.logger.debug("testatdebug"); - assertThat(this.outputCapture.toString()).doesNotContain("testatdebug").contains("Cannot set level 'GARBAGE'"); + assertThatExceptionOfType(BindException.class).isThrownBy( + () -> this.initializer.initialize(this.context.getEnvironment(), this.context.getClassLoader())); } @Test diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/StringToEnumIgnoringCaseConverterFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/LenientStringToEnumConverterFactoryTests.java similarity index 66% rename from spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/StringToEnumIgnoringCaseConverterFactoryTests.java rename to spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/LenientStringToEnumConverterFactoryTests.java index dbbe120c06..0d2b9b2d88 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/StringToEnumIgnoringCaseConverterFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/convert/LenientStringToEnumConverterFactoryTests.java @@ -26,11 +26,11 @@ import org.springframework.core.convert.ConversionService; import static org.assertj.core.api.Assertions.assertThat; /** - * Tests for {@link StringToEnumIgnoringCaseConverterFactory}. + * Tests for {@link LenientStringToEnumConverterFactory}. * * @author Phillip Webb */ -class StringToEnumIgnoringCaseConverterFactoryTests { +class LenientStringToEnumConverterFactoryTests { @ConversionServiceTest void canConvertFromStringToEnumShouldReturnTrue(ConversionService conversionService) { @@ -75,9 +75,26 @@ class StringToEnumIgnoringCaseConverterFactoryTests { } } + @ConversionServiceTest + void convertFromStringToEnumWhenYamlBooleanShouldConvertValue(ConversionService conversionService) { + assertThat(conversionService.convert("one", TestOnOffEnum.class)).isEqualTo(TestOnOffEnum.ONE); + assertThat(conversionService.convert("two", TestOnOffEnum.class)).isEqualTo(TestOnOffEnum.TWO); + assertThat(conversionService.convert("true", TestOnOffEnum.class)).isEqualTo(TestOnOffEnum.ON); + assertThat(conversionService.convert("false", TestOnOffEnum.class)).isEqualTo(TestOnOffEnum.OFF); + assertThat(conversionService.convert("TRUE", TestOnOffEnum.class)).isEqualTo(TestOnOffEnum.ON); + assertThat(conversionService.convert("FALSE", TestOnOffEnum.class)).isEqualTo(TestOnOffEnum.OFF); + assertThat(conversionService.convert("fA_lsE", TestOnOffEnum.class)).isEqualTo(TestOnOffEnum.OFF); + assertThat(conversionService.convert("one", TestTrueFalseEnum.class)).isEqualTo(TestTrueFalseEnum.ONE); + assertThat(conversionService.convert("two", TestTrueFalseEnum.class)).isEqualTo(TestTrueFalseEnum.TWO); + assertThat(conversionService.convert("true", TestTrueFalseEnum.class)).isEqualTo(TestTrueFalseEnum.TRUE); + assertThat(conversionService.convert("false", TestTrueFalseEnum.class)).isEqualTo(TestTrueFalseEnum.FALSE); + assertThat(conversionService.convert("TRUE", TestTrueFalseEnum.class)).isEqualTo(TestTrueFalseEnum.TRUE); + assertThat(conversionService.convert("FALSE", TestTrueFalseEnum.class)).isEqualTo(TestTrueFalseEnum.FALSE); + } + static Stream conversionServices() { return ConversionServiceArguments - .with((service) -> service.addConverterFactory(new StringToEnumIgnoringCaseConverterFactory())); + .with((service) -> service.addConverterFactory(new LenientStringToEnumConverterFactory())); } enum TestEnum { @@ -86,6 +103,18 @@ class StringToEnumIgnoringCaseConverterFactoryTests { } + enum TestOnOffEnum { + + ONE, TWO, ON, OFF + + } + + enum TestTrueFalseEnum { + + ONE, TWO, TRUE, FALSE, ON, OFF + + } + enum LocaleSensitiveEnum { ACCEPT_CASE_INSENSITIVE_PROPERTIES