Spring/spring-framework
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Improve SimpleAliasRegistryTests

Browse Source 
This commit improves SimpleAliasRegistryTests in the following ways.

- Some existing methods have been split up into smaller test methods
  which focus on a single use case.

- The use of Mockito mocks has been replaced by a hand-crafted
  StubStringValueResolver which ensures that existing aliases and names
  are not accidentally replaced by null thereby removing their removing
  there mappings inadvertently.

- Several test methods now include inline comments that document the
  current state of the aliasMap in order to clarify what is happening
  behind the scenes in the ConcurrentHashMap.

- Several test methods warn that the current expectations are based on
  ConcurrentHashMap iteration order!

- New @⁠ParameterizedTest which is currently @⁠Disabled but
  demonstrates that complex use cases involving placeholder replacement
  can be supported consistently -- regardless of the values of the
  aliases involved -- but only if alias registration order is honored.

Closes gh-31353
This commit is contained in:
Sam Brannen
2024-01-14 14:21:40 +01:00
parent eeb145ee0b
commit 5d309d5724
1 changed files with 198 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

234
spring-core/src/test/java/org/springframework/core/SimpleAliasRegistryTests.java
View File

@@ -16,7 +16,12 @@
package org.springframework.core;
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;
import org.springframework.util.StringValueResolver;
@@ -24,8 +29,6 @@ import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
import static org.assertj.core.api.Assertions.assertThatNoException;
import static org.mockito.BDDMockito.given;
import static org.mockito.BDDMockito.mock;
/**
* Tests for {@link SimpleAliasRegistry}.
@@ -46,12 +49,10 @@ class SimpleAliasRegistryTests {
private static final String ALIAS1 = "alias1";
private static final String ALIAS2 = "alias2";
private static final String ALIAS3 = "alias3";
// TODO Change ALIAS4 to "alias4".
// When ALIAS4 is "testAlias4", resolveAliasesWithComplexPlaceholderReplacement() passes.
// If you change ALIAS4 to "alias4", resolveAliasesWithComplexPlaceholderReplacement() fails.
// Those assertions pass for values such as "x", "xx", "xxx", and "xxxx" but fail with values
// such as "xxxxx", "testAli", ...
private static final String ALIAS4 = "testAlias4";
// TODO Determine if we can make SimpleAliasRegistry.resolveAliases() reliable.
// 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";
@@ -128,8 +129,7 @@ class SimpleAliasRegistryTests {
// No aliases registered, so no cycles possible.
assertThatNoException().isThrownBy(() -> registry.checkForAliasCircle(NAME1, ALIAS1));
// NAME1 -> ALIAS1
registerAlias(NAME1, ALIAS1);
registerAlias(NAME1, ALIAS1); // ALIAS1 -> NAME1
// No cycles possible.
assertThatNoException().isThrownBy(() -> registry.checkForAliasCircle(NAME1, ALIAS1));
@@ -139,8 +139,7 @@ class SimpleAliasRegistryTests {
.isThrownBy(() -> registerAlias(ALIAS1, NAME1)) // internally invokes checkForAliasCircle()
.withMessageContaining("'%s' is a direct or indirect alias for '%s'", ALIAS1, NAME1);
// NAME1 -> ALIAS1 -> ALIAS2
registerAlias(ALIAS1, ALIAS2);
registerAlias(ALIAS1, ALIAS2); // ALIAS2 -> ALIAS1 -> NAME1
assertThatIllegalStateException()
// NAME1 -> ALIAS1 -> ALIAS2 -> NAME1
.isThrownBy(() -> registerAlias(ALIAS2, NAME1)) // internally invokes checkForAliasCircle()
@@ -154,13 +153,14 @@ class SimpleAliasRegistryTests {
@Test
void resolveAliasesWithoutPlaceholderReplacement() {
// Resolver returns input unmodified.
StringValueResolver valueResolver = str -> str;
StringValueResolver valueResolver = new StubStringValueResolver();
registerAlias(NAME1, ALIAS1);
registerAlias(NAME1, ALIAS3);
registerAlias(NAME2, ALIAS2);
registerAlias(NAME2, ALIAS4);
assertThat(registry.getAliases(NAME1)).containsExactlyInAnyOrder(ALIAS1, ALIAS3);
assertThat(registry.getAliases(NAME2)).containsExactlyInAnyOrder(ALIAS2, ALIAS4);
registry.resolveAliases(valueResolver);
assertThat(registry.getAliases(NAME1)).containsExactlyInAnyOrder(ALIAS1, ALIAS3);
@@ -174,46 +174,183 @@ class SimpleAliasRegistryTests {
@Test
void resolveAliasesWithPlaceholderReplacement() {
StringValueResolver mock = mock();
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
NAME1, NAME2,
ALIAS1, ALIAS2
));
registerAlias(NAME1, ALIAS1);
assertThat(registry.getAliases(NAME1)).containsExactly(ALIAS1);
given(mock.resolveStringValue(NAME1)).willReturn(NAME2);
given(mock.resolveStringValue(ALIAS1)).willReturn(ALIAS2);
registry.resolveAliases(mock);
registry.resolveAliases(valueResolver);
assertThat(registry.getAliases(NAME1)).isEmpty();
assertThat(registry.getAliases(NAME2)).containsExactly(ALIAS2);
registry.removeAlias(ALIAS2);
assertThat(registry.getAliases(NAME1)).isEmpty();
assertThat(registry.getAliases(NAME2)).isEmpty();
}
@Test
void resolveAliasesWithComplexPlaceholderReplacement() {
StringValueResolver mock = mock();
void resolveAliasesWithComplexPlaceholderReplacementAndConfigurationError() {
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
NAME3, NAME4,
ALIAS3, ALIAS4,
ALIAS4, ALIAS5
));
registerAlias(NAME3, ALIAS3);
registerAlias(NAME4, ALIAS4);
registerAlias(NAME5, ALIAS5);
given(mock.resolveStringValue(NAME3)).willReturn(NAME4);
given(mock.resolveStringValue(NAME4)).willReturn(NAME4);
given(mock.resolveStringValue(NAME5)).willReturn(NAME5);
given(mock.resolveStringValue(ALIAS3)).willReturn(ALIAS4);
given(mock.resolveStringValue(ALIAS4)).willReturn(ALIAS5);
given(mock.resolveStringValue(ALIAS5)).willReturn(ALIAS5);
assertThatIllegalStateException().isThrownBy(() -> registry.resolveAliases(mock));
// Original state:
// WARNING: Based on ConcurrentHashMap iteration order!
// ALIAS3 -> NAME3
// ALIAS5 -> NAME5
// ALIAS4 -> NAME4
given(mock.resolveStringValue(NAME4)).willReturn(NAME5);
given(mock.resolveStringValue(ALIAS4)).willReturn(ALIAS4);
assertThatIllegalStateException().isThrownBy(() -> registry.resolveAliases(mock));
// State after processing original entry (ALIAS3 -> NAME3):
// Note that duplicate entry (ALIAS4 -> NAME4) gets removed.
// ALIAS5 -> NAME5
// ALIAS4 -> NAME4
given(mock.resolveStringValue(NAME4)).willReturn(NAME4);
given(mock.resolveStringValue(ALIAS4)).willReturn(ALIAS5);
registry.resolveAliases(mock);
assertThat(registry.getAliases(NAME4)).containsExactly(ALIAS4);
assertThat(registry.getAliases(NAME5)).containsExactly(ALIAS5);
// State after processing original entry (ALIAS4 -> NAME4):
// ALIAS5 -> NAME5
// ALIAS4 -> NAME4
// ALIAS5 -> NAME4 --> Conflict: entry for ALIAS5 already exists
assertThatIllegalStateException()
.isThrownBy(() -> registry.resolveAliases(valueResolver))
.withMessageStartingWith("Cannot register resolved alias");
}
@Test
void resolveAliasesWithComplexPlaceholderReplacement() {
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
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 -> NAME3
// ALIAS5 -> NAME5
// ALIAS4 -> NAME4
// State after processing original entry (ALIAS5 -> NAME5):
// ALIAS1 -> NAME3
// ALIAS2 -> NAME5
// ALIAS4 -> NAME4
// State after processing original entry (ALIAS4 -> NAME4):
// ALIAS1 -> NAME3
// ALIAS2 -> NAME5
// ALIAS5 -> NAME4
registry.resolveAliases(valueResolver);
assertThat(registry.getAliases(NAME3)).containsExactly(ALIAS1);
assertThat(registry.getAliases(NAME4)).containsExactly(ALIAS5);
assertThat(registry.getAliases(NAME5)).containsExactly(ALIAS2);
}
// TODO Remove this test once we have implemented reliable processing in SimpleAliasRegistry.resolveAliases().
// 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
@ValueSource(strings = {"alias4", "test", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"})
void resolveAliasesWithComplexPlaceholderReplacementAndNameSwitching(String aliasX) {
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
NAME3, NAME4,
NAME4, NAME3,
ALIAS3, ALIAS1,
aliasX, ALIAS5,
ALIAS5, ALIAS2
));
// If 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
// State after processing original entry (ALIAS3 -> NAME3):
// ALIAS5 -> NAME5
// aliasX -> NAME4
// ALIAS1 -> NAME4
// State after processing original entry (ALIAS5 -> NAME5):
// aliasX -> NAME4
// ALIAS1 -> NAME4
// ALIAS2 -> NAME5
// State after processing original entry (aliasX -> 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);
}
private void registerAlias(String name, String alias) {
@@ -228,4 +365,29 @@ class SimpleAliasRegistryTests {
assertThat(registry.hasAlias(name, alias)).isFalse();
}
/**
* {@link StringValueResolver} that replaces each value with a supplied
* placeholder and otherwise returns the original value if no placeholder
* is configured.
*/
private static class StubStringValueResolver implements StringValueResolver {
private final Map<String, String> placeholders;
StubStringValueResolver() {
this(Map.of());
}
StubStringValueResolver(Map<String, String> placeholders) {
this.placeholders = placeholders;
}
@Override
public String resolveStringValue(String str) {
return (this.placeholders.containsKey(str) ? this.placeholders.get(str) : str);
}
}
}