Skip to content

Commit

Permalink
Ensure alias resolution in SimpleAliasRegistry depends on registratio…
Browse files Browse the repository at this point in the history 
…n order

Closes spring-projectsgh-32024
  • Loading branch information
@sbrannen
sbrannen committed Jan 19, 2024
1 parent 6691ff2 commit 6238228
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 70 deletions.
17 changes: 14 additions & 3 deletions spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -50,6 +49,9 @@ public class SimpleAliasRegistry implements AliasRegistry {
/** Map from alias to canonical name. */
private final Map<String, String> aliasMap = new ConcurrentHashMap<>(16);

/** List of alias names, in registration order. */
private volatile List<String> aliasNames = new ArrayList<>(16);


@Override
public void registerAlias(String name, String alias) {
Expand All @@ -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");
}
Expand All @@ -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 + "'");
}
Expand Down Expand Up @@ -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");
}
Expand Down Expand Up @@ -155,19 +160,22 @@ private void retrieveAliases(String name, List<String> result) {
public void resolveAliases(StringValueResolver valueResolver) {
Assert.notNull(valueResolver, "StringValueResolver must not be null");
synchronized (this.aliasMap) {
Map<String, String> aliasCopy = new HashMap<>(this.aliasMap);
aliasCopy.forEach((alias, registeredName) -> {
List<String> 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);
if (existingName != null) {
if (existingName.equals(resolvedName)) {
// Pointing to existing alias - just remove placeholder
this.aliasMap.remove(alias);
this.aliasNames.remove(alias);
return;
}
throw new IllegalStateException(
Expand All @@ -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);
}
});
}
Expand Down
83 changes: 16 additions & 67 deletions spring-core/src/test/java/org/springframework/core/SimpleAliasRegistryTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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";

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 6238228

Please sign in to comment.