From 7ada32ca778bc3f34d27920d3f973cfb39a69ed6 Mon Sep 17 00:00:00 2001 From: Moritz Halbritter Date: Tue, 16 Sep 2025 09:39:44 +0200 Subject: [PATCH] Make ConfigDataLocation.of non-nullable Update `ConfigDataLocation.of` to return an empty location rather than null. Filtering of empty elements now happens in `ConfigDataProperties` and `ConfigDataEnvironment` which allows us to simplify `ConfigDataLocationBindHandler`. Closes gh-47221 Co-authored-by: Phillip Webb --- .../context/config/ConfigDataEnvironment.java | 4 +- .../context/config/ConfigDataLocation.java | 50 ++++++++----------- .../config/ConfigDataLocationBindHandler.java | 43 +++++++++------- .../context/config/ConfigDataProperties.java | 3 +- .../ConfigDataLocationBindHandlerTests.java | 28 ++++++++--- .../config/ConfigDataLocationTests.java | 15 +++--- 6 files changed, 79 insertions(+), 64 deletions(-) diff --git a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironment.java b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironment.java index f8277380a27..a1028a6cffa 100644 --- a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironment.java +++ b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironment.java @@ -214,7 +214,9 @@ class ConfigDataEnvironment { private void addInitialImportContributors(List initialContributors, ConfigDataLocation[] locations) { for (int i = locations.length - 1; i >= 0; i--) { - initialContributors.add(createInitialImportContributor(locations[i])); + if (ConfigDataLocation.isNotEmpty(locations[i])) { + initialContributors.add(createInitialImportContributor(locations[i])); + } } } diff --git a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocation.java b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocation.java index ea7dd9f2c68..7a5d68c1933 100644 --- a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocation.java +++ b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocation.java @@ -38,6 +38,8 @@ import org.springframework.util.StringUtils; */ public final class ConfigDataLocation implements OriginProvider { + private static final ConfigDataLocation EMPTY = new ConfigDataLocation(false, "", null); + /** * Prefix used to indicate that a {@link ConfigDataResource} is optional. */ @@ -89,10 +91,7 @@ public final class ConfigDataLocation implements OriginProvider { * @return the value with the prefix removed */ public String getNonPrefixedValue(String prefix) { - if (hasPrefix(prefix)) { - return this.value.substring(prefix.length()); - } - return this.value; + return (!hasPrefix(prefix)) ? this.value : this.value.substring(prefix.length()); } @Override @@ -118,17 +117,26 @@ public final class ConfigDataLocation implements OriginProvider { * @since 2.4.7 */ public ConfigDataLocation[] split(String delimiter) { + Assert.state(!this.value.isEmpty(), "Unable to split empty locations"); String[] values = StringUtils.delimitedListToStringArray(toString(), delimiter); ConfigDataLocation[] result = new ConfigDataLocation[values.length]; for (int i = 0; i < values.length; i++) { int index = i; ConfigDataLocation configDataLocation = of(values[index]); - Assert.state(configDataLocation != null, () -> "Unable to parse '%s'".formatted(values[index])); result[i] = configDataLocation.withOrigin(getOrigin()); } return result; } + /** + * Create a new {@link ConfigDataLocation} with a specific {@link Origin}. + * @param origin the origin to set + * @return a new {@link ConfigDataLocation} instance. + */ + ConfigDataLocation withOrigin(@Nullable Origin origin) { + return new ConfigDataLocation(this.optional, this.value, origin); + } + @Override public boolean equals(Object obj) { if (this == obj) { @@ -151,35 +159,19 @@ public final class ConfigDataLocation implements OriginProvider { return (!this.optional) ? this.value : OPTIONAL_PREFIX + this.value; } - /** - * Create a new {@link ConfigDataLocation} with a specific {@link Origin}. - * @param origin the origin to set - * @return a new {@link ConfigDataLocation} instance. - */ - ConfigDataLocation withOrigin(@Nullable Origin origin) { - return new ConfigDataLocation(this.optional, this.value, origin); - } - /** * Factory method to create a new {@link ConfigDataLocation} from a string. * @param location the location string - * @return a {@link ConfigDataLocation} instance or {@code null} if no location was - * provided + * @return the {@link ConfigDataLocation} (which may be empty) */ - public static @Nullable ConfigDataLocation of(@Nullable String location) { + public static ConfigDataLocation of(@Nullable String location) { boolean optional = location != null && location.startsWith(OPTIONAL_PREFIX); - String value; - if (optional) { - Assert.state(location != null, "'location' can't be null here"); - value = location.substring(OPTIONAL_PREFIX.length()); - } - else { - value = location; - } - if (!StringUtils.hasText(value)) { - return null; - } - return new ConfigDataLocation(optional, value, null); + String value = (location != null && optional) ? location.substring(OPTIONAL_PREFIX.length()) : location; + return (StringUtils.hasText(value)) ? new ConfigDataLocation(optional, value, null) : EMPTY; + } + + static boolean isNotEmpty(@Nullable ConfigDataLocation location) { + return (location != null) && !location.getValue().isEmpty(); } } diff --git a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocationBindHandler.java b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocationBindHandler.java index 54dcbcb8b64..5cdeca8471f 100644 --- a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocationBindHandler.java +++ b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataLocationBindHandler.java @@ -19,13 +19,15 @@ package org.springframework.boot.context.config; import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.Objects; import java.util.stream.Collectors; +import org.jspecify.annotations.Nullable; + import org.springframework.boot.context.properties.bind.AbstractBindHandler; import org.springframework.boot.context.properties.bind.BindContext; import org.springframework.boot.context.properties.bind.BindHandler; import org.springframework.boot.context.properties.bind.Bindable; +import org.springframework.boot.context.properties.source.ConfigurationProperty; import org.springframework.boot.context.properties.source.ConfigurationPropertyName; import org.springframework.boot.origin.Origin; @@ -39,32 +41,35 @@ import org.springframework.boot.origin.Origin; class ConfigDataLocationBindHandler extends AbstractBindHandler { @Override - public Object onSuccess(ConfigurationPropertyName name, Bindable target, BindContext context, Object result) { + public @Nullable Object onSuccess(ConfigurationPropertyName name, Bindable target, BindContext context, + Object result) { + OriginMapper originMapper = new OriginMapper(context.getConfigurationProperty()); if (result instanceof ConfigDataLocation location) { - return withOrigin(context, location); + return originMapper.map(location); } - if (result instanceof List list) { - return list.stream() - .filter(Objects::nonNull) - .map((element) -> (element instanceof ConfigDataLocation location) ? withOrigin(context, location) - : element) - .collect(Collectors.toCollection(ArrayList::new)); + if (result instanceof List locations) { + return locations.stream().map(originMapper::mapIfPossible).collect(Collectors.toCollection(ArrayList::new)); } - if (result instanceof ConfigDataLocation[] unfilteredLocations) { - return Arrays.stream(unfilteredLocations) - .filter(Objects::nonNull) - .map((element) -> withOrigin(context, element)) - .toArray(ConfigDataLocation[]::new); + if (result instanceof ConfigDataLocation[] locations) { + return Arrays.stream(locations).map(originMapper::mapIfPossible).toArray(ConfigDataLocation[]::new); } return result; } - private ConfigDataLocation withOrigin(BindContext context, ConfigDataLocation result) { - if (result.getOrigin() != null) { - return result; + private record OriginMapper(@Nullable ConfigurationProperty property) { + + @Nullable Object mapIfPossible(@Nullable Object object) { + return (object instanceof ConfigDataLocation location) ? map(location) : object; } - Origin origin = Origin.from(context.getConfigurationProperty()); - return result.withOrigin(origin); + + @Nullable ConfigDataLocation map(@Nullable ConfigDataLocation location) { + if (location == null) { + return null; + } + Origin origin = Origin.from(location); + return (origin != null) ? location : location.withOrigin(Origin.from(property())); + } + } } diff --git a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataProperties.java b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataProperties.java index 3727de94854..a9f74cdc47d 100644 --- a/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataProperties.java +++ b/core/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataProperties.java @@ -53,7 +53,8 @@ class ConfigDataProperties { * @param activate the activate properties */ ConfigDataProperties(@Nullable @Name("import") List imports, @Nullable Activate activate) { - this.imports = (imports != null) ? imports : Collections.emptyList(); + this.imports = (imports != null) ? imports.stream().filter(ConfigDataLocation::isNotEmpty).toList() + : Collections.emptyList(); this.activate = activate; } diff --git a/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationBindHandlerTests.java b/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationBindHandlerTests.java index f8b519d0275..d6e20747bf7 100644 --- a/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationBindHandlerTests.java +++ b/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationBindHandlerTests.java @@ -56,18 +56,24 @@ class ConfigDataLocationBindHandlerTests { } @Test - void bindToArrayFromCommaStringPropertyIgnoresEmptyElements() { + void bindToArrayFromCommaStringPropertyDoesNotFailOnEmptyElements() { MapConfigurationPropertySource source = new MapConfigurationPropertySource(); source.put("locations", ",a,,b,c,"); Binder binder = new Binder(source); ConfigDataLocation[] bound = binder.bind("locations", ARRAY, this.handler).get(); String expectedLocation = "\"locations\" from property source \"source\""; - assertThat(bound[0]).hasToString("a"); + assertThat(bound[0]).hasToString(""); assertThat(bound[0].getOrigin()).hasToString(expectedLocation); - assertThat(bound[1]).hasToString("b"); + assertThat(bound[1]).hasToString("a"); assertThat(bound[1].getOrigin()).hasToString(expectedLocation); - assertThat(bound[2]).hasToString("c"); + assertThat(bound[2]).hasToString(""); assertThat(bound[2].getOrigin()).hasToString(expectedLocation); + assertThat(bound[3]).hasToString("b"); + assertThat(bound[3].getOrigin()).hasToString(expectedLocation); + assertThat(bound[4]).hasToString("c"); + assertThat(bound[4].getOrigin()).hasToString(expectedLocation); + assertThat(bound[5]).hasToString(""); + assertThat(bound[5].getOrigin()).hasToString(expectedLocation); } @Test @@ -102,18 +108,24 @@ class ConfigDataLocationBindHandlerTests { } @Test - void bindToValueObjectFromCommaStringPropertyIgnoresEmptyElements() { + void bindToValueObjectFromCommaStringPropertyDoesNotFailOnEmptyElements() { MapConfigurationPropertySource source = new MapConfigurationPropertySource(); source.put("test.locations", ",a,b,,c,"); Binder binder = new Binder(source); ValueObject bound = binder.bind("test", VALUE_OBJECT, this.handler).get(); String expectedLocation = "\"test.locations\" from property source \"source\""; - assertThat(bound.getLocation(0)).hasToString("a"); + assertThat(bound.getLocation(0)).hasToString(""); assertThat(bound.getLocation(0).getOrigin()).hasToString(expectedLocation); - assertThat(bound.getLocation(1)).hasToString("b"); + assertThat(bound.getLocation(1)).hasToString("a"); assertThat(bound.getLocation(1).getOrigin()).hasToString(expectedLocation); - assertThat(bound.getLocation(2)).hasToString("c"); + assertThat(bound.getLocation(2)).hasToString("b"); assertThat(bound.getLocation(2).getOrigin()).hasToString(expectedLocation); + assertThat(bound.getLocation(3)).hasToString(""); + assertThat(bound.getLocation(3).getOrigin()).hasToString(expectedLocation); + assertThat(bound.getLocation(4)).hasToString("c"); + assertThat(bound.getLocation(4).getOrigin()).hasToString(expectedLocation); + assertThat(bound.getLocation(5)).hasToString(""); + assertThat(bound.getLocation(5).getOrigin()).hasToString(expectedLocation); } @Test diff --git a/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationTests.java b/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationTests.java index 26e54dbe84d..aea6c793f3b 100644 --- a/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationTests.java +++ b/core/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataLocationTests.java @@ -115,18 +115,21 @@ class ConfigDataLocationTests { } @Test - void ofWhenNullValueReturnsNull() { - assertThat(ConfigDataLocation.of(null)).isNull(); + void ofWhenNullValueReturnsEmptyLocation() { + ConfigDataLocation location = ConfigDataLocation.of(null); + assertThat(location.getValue().isEmpty()).isTrue(); } @Test - void ofWhenEmptyValueReturnsNull() { - assertThat(ConfigDataLocation.of("")).isNull(); + void ofWhenEmptyValueReturnsEmptyLocation() { + ConfigDataLocation location = ConfigDataLocation.of(""); + assertThat(location.getValue().isEmpty()).isTrue(); } @Test - void ofWhenEmptyOptionalValueReturnsNull() { - assertThat(ConfigDataLocation.of("optional:")).isNull(); + void ofWhenEmptyOptionalValueReturnsEmptyLocation() { + ConfigDataLocation location = ConfigDataLocation.of("optional:"); + assertThat(location.getValue().isEmpty()).isTrue(); } @Test