diff --git a/spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java b/spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java index a9d84542f03e..1ec7d5c3a18e 100644 --- a/spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java +++ b/spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java @@ -17,7 +17,6 @@ package org.springframework.core; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -50,6 +49,9 @@ public class SimpleAliasRegistry implements AliasRegistry { /** Map from alias to canonical name. */ private final Map aliasMap = new ConcurrentHashMap<>(16); + /** List of alias names, in registration order. */ + private volatile List aliasNames = new ArrayList<>(16); + @Override public void registerAlias(String name, String alias) { @@ -58,6 +60,7 @@ public void registerAlias(String name, String alias) { synchronized (this.aliasMap) { if (alias.equals(name)) { this.aliasMap.remove(alias); + this.aliasNames.remove(alias); if (logger.isDebugEnabled()) { logger.debug("Alias definition '" + alias + "' ignored since it points to same name"); } @@ -80,6 +83,7 @@ public void registerAlias(String name, String alias) { } checkForAliasCircle(name, alias); this.aliasMap.put(alias, name); + this.aliasNames.add(alias); if (logger.isTraceEnabled()) { logger.trace("Alias definition '" + alias + "' registered for name '" + name + "'"); } @@ -111,6 +115,7 @@ public boolean hasAlias(String name, String alias) { public void removeAlias(String alias) { synchronized (this.aliasMap) { String name = this.aliasMap.remove(alias); + this.aliasNames.remove(alias); if (name == null) { throw new IllegalStateException("No alias '" + alias + "' registered"); } @@ -155,12 +160,14 @@ private void retrieveAliases(String name, List result) { public void resolveAliases(StringValueResolver valueResolver) { Assert.notNull(valueResolver, "StringValueResolver must not be null"); synchronized (this.aliasMap) { - Map aliasCopy = new HashMap<>(this.aliasMap); - aliasCopy.forEach((alias, registeredName) -> { + List aliasNamesCopy = new ArrayList<>(this.aliasNames); + aliasNamesCopy.forEach(alias -> { + String registeredName = this.aliasMap.get(alias); String resolvedAlias = valueResolver.resolveStringValue(alias); String resolvedName = valueResolver.resolveStringValue(registeredName); if (resolvedAlias == null || resolvedName == null || resolvedAlias.equals(resolvedName)) { this.aliasMap.remove(alias); + this.aliasNames.remove(alias); } else if (!resolvedAlias.equals(alias)) { String existingName = this.aliasMap.get(resolvedAlias); @@ -168,6 +175,7 @@ else if (!resolvedAlias.equals(alias)) { if (existingName.equals(resolvedName)) { // Pointing to existing alias - just remove placeholder this.aliasMap.remove(alias); + this.aliasNames.remove(alias); return; } throw new IllegalStateException( @@ -177,10 +185,13 @@ else if (!resolvedAlias.equals(alias)) { } checkForAliasCircle(resolvedName, resolvedAlias); this.aliasMap.remove(alias); + this.aliasNames.remove(alias); this.aliasMap.put(resolvedAlias, resolvedName); + this.aliasNames.add(resolvedAlias); } else if (!registeredName.equals(resolvedName)) { this.aliasMap.put(alias, resolvedName); + this.aliasNames.add(alias); } }); } diff --git a/spring-core/src/test/java/org/springframework/core/SimpleAliasRegistryTests.java b/spring-core/src/test/java/org/springframework/core/SimpleAliasRegistryTests.java index 9e2a6e531838..6b2106152bb6 100644 --- a/spring-core/src/test/java/org/springframework/core/SimpleAliasRegistryTests.java +++ b/spring-core/src/test/java/org/springframework/core/SimpleAliasRegistryTests.java @@ -18,7 +18,6 @@ import java.util.Map; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -49,10 +48,6 @@ class SimpleAliasRegistryTests { private static final String ALIAS1 = "alias1"; private static final String ALIAS2 = "alias2"; private static final String ALIAS3 = "alias3"; - // TODO Determine if we can make SimpleAliasRegistry.resolveAliases() reliable. - // See https://github.com/spring-projects/spring-framework/issues/32024. - // When ALIAS4 is changed to "test", various tests fail due to the iteration - // order of the entries in the aliasMap in SimpleAliasRegistry. private static final String ALIAS4 = "alias4"; private static final String ALIAS5 = "alias5"; @@ -213,35 +208,37 @@ void resolveAliasesWithPlaceholderReplacementConflict() { "It is already registered for name '%s'.", ALIAS2, ALIAS1, NAME1, NAME2); } - @Test - void resolveAliasesWithComplexPlaceholderReplacement() { + @ParameterizedTest + @ValueSource(strings = {"alias4", "test", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"}) + void resolveAliasesWithComplexPlaceholderReplacementWithAliasSwitching(String aliasX) { StringValueResolver valueResolver = new StubStringValueResolver(Map.of( ALIAS3, ALIAS1, - ALIAS4, ALIAS5, + aliasX, ALIAS5, ALIAS5, ALIAS2 )); + // Since SimpleAliasRegistry ensures that aliases are processed in declaration + // order, we need to register ALIAS5 *before* aliasX to support our use case. registerAlias(NAME3, ALIAS3); - registerAlias(NAME4, ALIAS4); registerAlias(NAME5, ALIAS5); + registerAlias(NAME4, aliasX); // Original state: - // WARNING: Based on ConcurrentHashMap iteration order! // ALIAS3 -> NAME3 // ALIAS5 -> NAME5 - // ALIAS4 -> NAME4 + // aliasX -> NAME4 // State after processing original entry (ALIAS3 -> NAME3): // ALIAS1 -> NAME3 // ALIAS5 -> NAME5 - // ALIAS4 -> NAME4 + // aliasX -> NAME4 // State after processing original entry (ALIAS5 -> NAME5): // ALIAS1 -> NAME3 // ALIAS2 -> NAME5 - // ALIAS4 -> NAME4 + // aliasX -> NAME4 - // State after processing original entry (ALIAS4 -> NAME4): + // State after processing original entry (aliasX -> NAME4): // ALIAS1 -> NAME3 // ALIAS2 -> NAME5 // ALIAS5 -> NAME4 @@ -252,72 +249,24 @@ void resolveAliasesWithComplexPlaceholderReplacement() { assertThat(registry.getAliases(NAME5)).containsExactly(ALIAS2); } - // TODO Remove this test once we have implemented reliable processing in SimpleAliasRegistry.resolveAliases(). - // See https://github.com/spring-projects/spring-framework/issues/32024. - // This method effectively duplicates the @ParameterizedTest version below, - // with aliasX hard coded to ALIAS4; however, this method also hard codes - // a different outcome that passes based on ConcurrentHashMap iteration order! - @Test - void resolveAliasesWithComplexPlaceholderReplacementAndNameSwitching() { - StringValueResolver valueResolver = new StubStringValueResolver(Map.of( - NAME3, NAME4, - NAME4, NAME3, - ALIAS3, ALIAS1, - ALIAS4, ALIAS5, - ALIAS5, ALIAS2 - )); - - registerAlias(NAME3, ALIAS3); - registerAlias(NAME4, ALIAS4); - registerAlias(NAME5, ALIAS5); - - // Original state: - // WARNING: Based on ConcurrentHashMap iteration order! - // ALIAS3 -> NAME3 - // ALIAS5 -> NAME5 - // ALIAS4 -> NAME4 - - // State after processing original entry (ALIAS3 -> NAME3): - // ALIAS1 -> NAME4 - // ALIAS5 -> NAME5 - // ALIAS4 -> NAME4 - - // State after processing original entry (ALIAS5 -> NAME5): - // ALIAS1 -> NAME4 - // ALIAS2 -> NAME5 - // ALIAS4 -> NAME4 - - // State after processing original entry (ALIAS4 -> NAME4): - // ALIAS1 -> NAME4 - // ALIAS2 -> NAME5 - // ALIAS5 -> NAME3 - - registry.resolveAliases(valueResolver); - assertThat(registry.getAliases(NAME3)).containsExactly(ALIAS5); - assertThat(registry.getAliases(NAME4)).containsExactly(ALIAS1); - assertThat(registry.getAliases(NAME5)).containsExactly(ALIAS2); - } - - @Disabled("Fails for some values unless alias registration order is honored") @ParameterizedTest // gh-32024 @ValueSource(strings = {"alias4", "test", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"}) - void resolveAliasesWithComplexPlaceholderReplacementAndNameSwitching(String aliasX) { + void resolveAliasesWithComplexPlaceholderReplacementWithAliasAndNameSwitching(String aliasX) { StringValueResolver valueResolver = new StubStringValueResolver(Map.of( - NAME3, NAME4, - NAME4, NAME3, ALIAS3, ALIAS1, aliasX, ALIAS5, - ALIAS5, ALIAS2 + ALIAS5, ALIAS2, + NAME3, NAME4, + NAME4, NAME3 )); - // If SimpleAliasRegistry ensures that aliases are processed in declaration + // Since SimpleAliasRegistry ensures that aliases are processed in declaration // order, we need to register ALIAS5 *before* aliasX to support our use case. registerAlias(NAME3, ALIAS3); registerAlias(NAME5, ALIAS5); registerAlias(NAME4, aliasX); // Original state: - // WARNING: Based on LinkedHashMap iteration order! // ALIAS3 -> NAME3 // ALIAS5 -> NAME5 // aliasX -> NAME4