From 8f14dca164b100271a4a98247754825a50f8b785 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 17 Jun 2025 10:04:01 -0700 Subject: [PATCH] Restore IndexedElementsBinder knownIndexedChildren checking logic Restore knownIndexedChildren logic in IndexedElementsBinder to that of Spring Boot 3.4, effectively reverting commit 93113a415f15. The performance improvements unfortunately caused the unwanted side-effect of no longer checking the all indexed elements were fully bound. Performance was also actually reduced for non iterable property sources that used deeply nested structures. In order to keep some performance improvements, the updated code also makes the following changes: * A `Set` is used rather than `LinkedMultiValueMap` for tracking. The full list of unbound properties are now only calculated when the exception is thrown. * If possible, the source is filtered early and passed down for child element binding. This should help reduce repeated full scans of all configuration property names. * The `FilteredIterableConfigurationPropertiesSource` has been optimized for immutable `SpringIterableConfigurationPropertySource` use. Fixes gh-45994 See gh-45970 See gh-44867 --- .../bind/IndexedElementsBinder.java | 95 +++++++++---------- .../source/ConfigurationPropertyState.java | 23 +++++ ...IterableConfigurationPropertiesSource.java | 35 ++++++- ...ngIterableConfigurationPropertySource.java | 4 +- .../bind/CollectionBinderTests.java | 26 ++++- ...bleConfigurationPropertiesSourceTests.java | 57 +++++++++-- 6 files changed, 176 insertions(+), 64 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/IndexedElementsBinder.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/IndexedElementsBinder.java index 1ff5f119cfa..65795ca8a53 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/IndexedElementsBinder.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/IndexedElementsBinder.java @@ -18,10 +18,11 @@ package org.springframework.boot.context.properties.bind; import java.lang.annotation.Annotation; import java.util.Collection; -import java.util.List; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; import java.util.TreeSet; import java.util.function.Supplier; -import java.util.stream.Collectors; import org.springframework.boot.context.properties.bind.Binder.Context; import org.springframework.boot.context.properties.source.ConfigurationProperty; @@ -30,8 +31,6 @@ import org.springframework.boot.context.properties.source.ConfigurationPropertyN import org.springframework.boot.context.properties.source.ConfigurationPropertySource; import org.springframework.boot.context.properties.source.IterableConfigurationPropertySource; import org.springframework.core.ResolvableType; -import org.springframework.util.LinkedMultiValueMap; -import org.springframework.util.MultiValueMap; /** * Base class for {@link AggregateBinder AggregateBinders} that read a sequential run of @@ -106,23 +105,52 @@ abstract class IndexedElementsBinder extends AggregateBinder { private void bindIndexed(ConfigurationPropertySource source, ConfigurationPropertyName root, AggregateElementBinder elementBinder, IndexedCollectionSupplier collection, ResolvableType elementType) { - int firstUnboundIndex = 0; - boolean hasBindingGap = false; + Set knownIndexedChildren = Collections.emptySet(); + if (source instanceof IterableConfigurationPropertySource iterableSource) { + source = iterableSource.filter(root::isAncestorOf); + knownIndexedChildren = getKnownIndexedChildren(iterableSource, root); + } for (int i = 0; i < Integer.MAX_VALUE; i++) { ConfigurationPropertyName name = appendIndex(root, i); Object value = elementBinder.bind(name, Bindable.of(elementType), source); - if (value != null) { - collection.get().add(value); - hasBindingGap = hasBindingGap || firstUnboundIndex > 0; - continue; - } - firstUnboundIndex = (firstUnboundIndex <= 0) ? i : firstUnboundIndex; - if (i - firstUnboundIndex > 10) { + if (value == null) { break; } + knownIndexedChildren.remove(name.getLastElement(Form.UNIFORM)); + collection.get().add(value); } - if (hasBindingGap) { - assertNoUnboundChildren(source, root, firstUnboundIndex); + if (source instanceof IterableConfigurationPropertySource iterableSource) { + assertNoUnboundChildren(knownIndexedChildren, iterableSource, root); + } + } + + private Set getKnownIndexedChildren(IterableConfigurationPropertySource filteredSource, + ConfigurationPropertyName root) { + Set knownIndexedChildren = new HashSet<>(); + for (ConfigurationPropertyName name : filteredSource) { + ConfigurationPropertyName choppedName = name.chop(root.getNumberOfElements() + 1); + if (choppedName.isLastElementIndexed()) { + knownIndexedChildren.add(choppedName.getLastElement(Form.UNIFORM)); + } + } + return knownIndexedChildren; + } + + private void assertNoUnboundChildren(Set unboundIndexedChildren, + IterableConfigurationPropertySource filteredSource, ConfigurationPropertyName root) { + if (unboundIndexedChildren.isEmpty()) { + return; + } + Set unboundProperties = new TreeSet<>(); + for (ConfigurationPropertyName name : filteredSource) { + ConfigurationPropertyName choppedName = name.chop(root.getNumberOfElements() + 1); + if (choppedName.isLastElementIndexed() + && unboundIndexedChildren.contains(choppedName.getLastElement(Form.UNIFORM))) { + unboundProperties.add(filteredSource.getConfigurationProperty(name)); + } + } + if (!unboundProperties.isEmpty()) { + throw new UnboundConfigurationPropertiesException(unboundProperties); } } @@ -130,43 +158,6 @@ abstract class IndexedElementsBinder extends AggregateBinder { return root.append((i < INDEXES.length) ? INDEXES[i] : "[" + i + "]"); } - private void assertNoUnboundChildren(ConfigurationPropertySource source, ConfigurationPropertyName root, - int firstUnboundIndex) { - MultiValueMap knownIndexedChildren = getKnownIndexedChildren(source, root); - for (int i = 0; i < firstUnboundIndex; i++) { - ConfigurationPropertyName name = appendIndex(root, i); - knownIndexedChildren.remove(name.getLastElement(Form.UNIFORM)); - } - assertNoUnboundChildren(source, knownIndexedChildren); - } - - private MultiValueMap getKnownIndexedChildren(ConfigurationPropertySource source, - ConfigurationPropertyName root) { - MultiValueMap children = new LinkedMultiValueMap<>(); - if (!(source instanceof IterableConfigurationPropertySource iterableSource)) { - return children; - } - for (ConfigurationPropertyName name : iterableSource.filter(root::isAncestorOf)) { - ConfigurationPropertyName choppedName = name.chop(root.getNumberOfElements() + 1); - if (choppedName.isLastElementIndexed()) { - String key = choppedName.getLastElement(Form.UNIFORM); - children.add(key, name); - } - } - return children; - } - - private void assertNoUnboundChildren(ConfigurationPropertySource source, - MultiValueMap children) { - if (!children.isEmpty()) { - throw new UnboundConfigurationPropertiesException(children.values() - .stream() - .flatMap(List::stream) - .map(source::getConfigurationProperty) - .collect(Collectors.toCollection(TreeSet::new))); - } - } - private C convert(Object value, ResolvableType type, Annotation... annotations) { value = getContext().getPlaceholdersResolver().resolvePlaceholders(value); return getContext().getConverter().convert(value, type, annotations); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyState.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyState.java index 4bb7b2c5241..7e369b0be62 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyState.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertyState.java @@ -66,4 +66,27 @@ public enum ConfigurationPropertyState { return ABSENT; } + /** + * Search the given iterable using a predicate to determine if content is + * {@link #PRESENT} or {@link #ABSENT}. + * @param the data type + * @param source the source iterable to search + * @param startInclusive the first index to cover + * @param endExclusive index immediately past the last index to cover + * @param predicate the predicate used to test for presence + * @return {@link #PRESENT} if the iterable contains a matching item, otherwise + * {@link #ABSENT}. + */ + static ConfigurationPropertyState search(T[] source, int startInclusive, int endExclusive, + Predicate predicate) { + Assert.notNull(source, "'source' must not be null"); + Assert.notNull(predicate, "'predicate' must not be null"); + for (int i = startInclusive; i < endExclusive; i++) { + if (predicate.test(source[i])) { + return PRESENT; + } + } + return ABSENT; + } + } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSource.java index fc357ded160..a5e0ef2af62 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSource.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2025 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. @@ -16,6 +16,7 @@ package org.springframework.boot.context.properties.source; +import java.util.Arrays; import java.util.function.Predicate; import java.util.stream.Stream; @@ -28,13 +29,41 @@ import java.util.stream.Stream; class FilteredIterableConfigurationPropertiesSource extends FilteredConfigurationPropertiesSource implements IterableConfigurationPropertySource { + private ConfigurationPropertyName[] filteredNames; + + private int numerOfFilteredNames; + FilteredIterableConfigurationPropertiesSource(IterableConfigurationPropertySource source, Predicate filter) { super(source, filter); + ConfigurationPropertyName[] filterableNames = getFilterableNames(source); + if (filterableNames != null) { + this.filteredNames = new ConfigurationPropertyName[filterableNames.length]; + this.numerOfFilteredNames = 0; + for (ConfigurationPropertyName name : filterableNames) { + if (filter.test(name)) { + this.filteredNames[this.numerOfFilteredNames++] = name; + } + } + } + } + + private ConfigurationPropertyName[] getFilterableNames(IterableConfigurationPropertySource source) { + if (source instanceof SpringIterableConfigurationPropertySource springPropertySource + && springPropertySource.isImmutablePropertySource()) { + return springPropertySource.getConfigurationPropertyNames(); + } + if (source instanceof FilteredIterableConfigurationPropertiesSource filteredSource) { + return filteredSource.filteredNames; + } + return null; } @Override public Stream stream() { + if (this.filteredNames != null) { + return Arrays.stream(this.filteredNames, 0, this.numerOfFilteredNames); + } return getSource().stream().filter(getFilter()); } @@ -45,6 +74,10 @@ class FilteredIterableConfigurationPropertiesSource extends FilteredConfiguratio @Override public ConfigurationPropertyState containsDescendantOf(ConfigurationPropertyName name) { + if (this.filteredNames != null) { + return ConfigurationPropertyState.search(this.filteredNames, 0, this.numerOfFilteredNames, + name::isAncestorOf); + } return ConfigurationPropertyState.search(this, name::isAncestorOf); } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java index 44bd2e09cae..d902d0224c4 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java @@ -169,7 +169,7 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope return false; } - private ConfigurationPropertyName[] getConfigurationPropertyNames() { + ConfigurationPropertyName[] getConfigurationPropertyNames() { if (!isImmutablePropertySource()) { return getCache().getConfigurationPropertyNames(getPropertySource().getPropertyNames()); } @@ -197,7 +197,7 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope return cache; } - private boolean isImmutablePropertySource() { + boolean isImmutablePropertySource() { EnumerablePropertySource source = getPropertySource(); if (source instanceof OriginLookup originLookup) { return originLookup.isImmutable(); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java index 8d3ca49f126..bf4a8db3298 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2023 the original author or authors. + * Copyright 2012-2025 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. @@ -123,6 +123,26 @@ class CollectionBinderTests { }); } + @Test + void bindToCollectionWhenNonKnownIndexedChildNotBoundThrowsException() { + // gh-45994 + MockConfigurationPropertySource source = new MockConfigurationPropertySource(); + source.put("foo[0].first", "Spring"); + source.put("foo[0].last", "Boot"); + source.put("foo[1].missing", "bad"); + this.sources.add(source); + assertThatExceptionOfType(BindException.class) + .isThrownBy(() -> this.binder.bind("foo", Bindable.listOf(Name.class))) + .satisfies((ex) -> { + Set unbound = ((UnboundConfigurationPropertiesException) ex.getCause()) + .getUnboundProperties(); + assertThat(unbound).hasSize(1); + ConfigurationProperty property = unbound.iterator().next(); + assertThat(property.getName()).hasToString("foo[1].missing"); + assertThat(property.getValue()).isEqualTo("bad"); + }); + } + @Test void bindToNonScalarCollectionWhenNonSequentialShouldThrowException() { MockConfigurationPropertySource source = new MockConfigurationPropertySource(); @@ -562,4 +582,8 @@ class CollectionBinderTests { } + record Name(String first, String last) { + + } + } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSourceTests.java index 0b36cbbfd42..c8367b65415 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSourceTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/FilteredIterableConfigurationPropertiesSourceTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2023 the original author or authors. + * Copyright 2012-2025 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. @@ -16,8 +16,15 @@ package org.springframework.boot.context.properties.source; +import java.util.LinkedHashMap; +import java.util.Map; + +import org.assertj.core.extractor.Extractors; import org.junit.jupiter.api.Test; +import org.springframework.boot.env.OriginTrackedMapPropertySource; +import org.springframework.core.env.PropertySource; + import static org.assertj.core.api.Assertions.assertThat; /** @@ -29,7 +36,7 @@ import static org.assertj.core.api.Assertions.assertThat; class FilteredIterableConfigurationPropertiesSourceTests extends FilteredConfigurationPropertiesSourceTests { @Test - void iteratorShouldFilterNames() { + void iteratorFiltersNames() { MockConfigurationPropertySource source = (MockConfigurationPropertySource) createTestSource(); IterableConfigurationPropertySource filtered = source.filter(this::noBrackets); assertThat(filtered.iterator()).toIterable() @@ -37,13 +44,8 @@ class FilteredIterableConfigurationPropertiesSourceTests extends FilteredConfigu .containsExactly("a", "b", "c"); } - @Override - protected ConfigurationPropertySource convertSource(MockConfigurationPropertySource source) { - return source; - } - @Test - void containsDescendantOfShouldUseContents() { + void containsDescendantOfUsesContents() { MockConfigurationPropertySource source = new MockConfigurationPropertySource(); source.put("foo.bar.baz", "1"); source.put("foo.bar[0]", "1"); @@ -55,6 +57,45 @@ class FilteredIterableConfigurationPropertiesSourceTests extends FilteredConfigu .isEqualTo(ConfigurationPropertyState.ABSENT); } + @Test + void iteratorWhenSpringPropertySourceFiltersNames() { + IterableConfigurationPropertySource testSource = (IterableConfigurationPropertySource) createTestSource(); + Map map = new LinkedHashMap<>(); + for (ConfigurationPropertyName name : testSource) { + map.put(name.toString(), testSource.getConfigurationProperty(name).getValue()); + } + PropertySource propertySource = new OriginTrackedMapPropertySource("test", map, true); + SpringConfigurationPropertySource source = SpringConfigurationPropertySource.from(propertySource); + IterableConfigurationPropertySource filtered = (IterableConfigurationPropertySource) source + .filter(this::noBrackets); + assertThat(Extractors.byName("filteredNames").apply(filtered)).isNotNull(); + assertThat(filtered.iterator()).toIterable() + .extracting(ConfigurationPropertyName::toString) + .containsExactly("a", "b", "c"); + } + + @Test + void containsDescendantOfWhenSpringPropertySourceUsesContents() { + Map map = new LinkedHashMap<>(); + map.put("foo.bar.baz", "1"); + map.put("foo.bar[0]", "1"); + map.put("faf.bar[0]", "1"); + PropertySource propertySource = new OriginTrackedMapPropertySource("test", map, true); + SpringConfigurationPropertySource source = SpringConfigurationPropertySource.from(propertySource); + IterableConfigurationPropertySource filtered = (IterableConfigurationPropertySource) source + .filter(this::noBrackets); + assertThat(Extractors.byName("filteredNames").apply(filtered)).isNotNull(); + assertThat(filtered.containsDescendantOf(ConfigurationPropertyName.of("foo"))) + .isEqualTo(ConfigurationPropertyState.PRESENT); + assertThat(filtered.containsDescendantOf(ConfigurationPropertyName.of("faf"))) + .isEqualTo(ConfigurationPropertyState.ABSENT); + } + + @Override + protected ConfigurationPropertySource convertSource(MockConfigurationPropertySource source) { + return source; + } + private boolean noBrackets(ConfigurationPropertyName name) { return !name.toString().contains("["); }