From 5ae1798ec54c03e6f576f4b367dee3d20fcc7eb3 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 1 May 2017 20:49:14 -0700 Subject: [PATCH] Don't rely on ConfigurationPropertyName exceptions Update `ConfigurationPropertySourcesPropertySource` to no longer use `try/catch` when checking for valid names. A new `isValid` method has been introduced to `ConfigurationPropertyName` which is offers better performance. Fixes gh-9058 --- .../source/ConfigurationPropertyName.java | 61 ++++++++++++++++++- .../ConfigurationPropertyNameBuilder.java | 14 +---- ...gurationPropertySourcesPropertySource.java | 6 +- .../ConfigurationPropertyNameTests.java | 20 ++++++ 4 files changed, 87 insertions(+), 14 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java b/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java index a7fa9d8a69..98c412984f 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyName.java @@ -226,6 +226,45 @@ public final class ConfigurationPropertyName return this; } + /** + * Returns if the given name is valid. If this method returns {@code true} then the + * name may be used with {@link #of(String)} without throwing an exception. + * @param name the name to test + * @return {@code true} if the name is valid + */ + public static boolean isValid(String name) { + if (name == null) { + return false; + } + boolean indexed = false; + int charIndex = 0; + for (int i = 0; i < name.length(); i++) { + char ch = name.charAt(i); + if (!indexed) { + if (ch == '[') { + indexed = true; + charIndex = 1; + } + else if (ch == '.') { + charIndex = 0; + } + else { + if (!Element.isValid(charIndex, ch)) { + return false; + } + charIndex++; + } + } + else { + if (ch == ']') { + indexed = false; + charIndex = 0; + } + } + } + return true; + } + /** * Return a {@link ConfigurationPropertyName} for the specified string. * @param name the source name @@ -335,7 +374,27 @@ public final class ConfigurationPropertyName return this.value[form.ordinal()]; } - public static boolean isIndexed(String value) { + static boolean isValid(String value) { + if (!isIndexed(value)) { + for (int i = 0; i < value.length(); i++) { + if (!isValid(i, value.charAt(i))) { + return false; + } + } + } + return true; + } + + static boolean isValid(int index, char ch) { + boolean isAlpha = ch >= 'a' && ch <= 'z'; + boolean isNumeric = ch >= '0' && ch <= '9'; + if (index == 0) { + return isAlpha; + } + return isAlpha || isNumeric || ch == '-'; + } + + private static boolean isIndexed(String value) { return value.startsWith("[") && value.endsWith("]"); } diff --git a/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameBuilder.java b/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameBuilder.java index 4cbad8d8e4..888ac2fa54 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameBuilder.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameBuilder.java @@ -142,7 +142,6 @@ class ConfigurationPropertyNameBuilder { * An processor that will be applied to element values. Can be used to manipulate or * restrict the values that are used. */ - @FunctionalInterface public interface ElementValueProcessor { /** @@ -169,16 +168,9 @@ class ConfigurationPropertyNameBuilder { default ElementValueProcessor withValidName() { return (value) -> { value = apply(value); - if (!Element.isIndexed(value)) { - for (int i = 0; i < value.length(); i++) { - char ch = value.charAt(i); - boolean isAlpha = ch >= 'a' && ch <= 'z'; - boolean isNumeric = ch >= '0' && ch <= '9'; - if (i == 0 && !isAlpha || !(isAlpha || isNumeric || ch == '-')) { - throw new IllegalArgumentException( - "Element value '" + value + "' is not valid"); - } - } + if (!Element.isValid(value)) { + throw new IllegalArgumentException( + "Element value '" + value + "' is not valid"); } return value; }; diff --git a/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySourcesPropertySource.java b/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySourcesPropertySource.java index eff2cb2469..dfe3f8bad4 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySourcesPropertySource.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySourcesPropertySource.java @@ -56,11 +56,13 @@ class ConfigurationPropertySourcesPropertySource private ConfigurationProperty findConfigurationProperty(String name) { try { - return findConfigurationProperty(ConfigurationPropertyName.of(name)); + if (ConfigurationPropertyName.isValid(name)) { + return findConfigurationProperty(ConfigurationPropertyName.of(name)); + } } catch (Exception ex) { - return null; } + return null; } private ConfigurationProperty findConfigurationProperty( diff --git a/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java b/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java index 695995515a..e97e59f020 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/properties/source/ConfigurationPropertyNameTests.java @@ -332,4 +332,24 @@ public class ConfigurationPropertyNameTests { assertThat(name.append("foo").toString()).isEqualTo("foo"); } + @Test + public void isValidWhenValidShouldReturnTrue() throws Exception { + assertThat(ConfigurationPropertyName.isValid("")).isTrue(); + assertThat(ConfigurationPropertyName.isValid("foo")).isTrue(); + assertThat(ConfigurationPropertyName.isValid("foo.bar")).isTrue(); + assertThat(ConfigurationPropertyName.isValid("foo[0]")).isTrue(); + assertThat(ConfigurationPropertyName.isValid("foo[0].baz")).isTrue(); + assertThat(ConfigurationPropertyName.isValid("foo.b1")).isTrue(); + assertThat(ConfigurationPropertyName.isValid("foo.b-a-r")).isTrue(); + assertThat(ConfigurationPropertyName.isValid("foo[FooBar].baz")).isTrue(); + } + + @Test + public void isValidWhenNotValidShouldReturnFalse() throws Exception { + assertThat(ConfigurationPropertyName.isValid(null)).isFalse(); + assertThat(ConfigurationPropertyName.isValid("1foo")).isFalse(); + assertThat(ConfigurationPropertyName.isValid("FooBar")).isFalse(); + assertThat(ConfigurationPropertyName.isValid("foo!bar")).isFalse(); + } + }