Skip to content

Commit

Permalink
Improve SimpleAliasRegistryTests
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@sbrannen
sbrannen committed Jan 14, 2024
1 parent eeb145e commit 5d309d5
Showing 1 changed file with 197 additions and 35 deletions.
232 changes: 197 additions & 35 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 @@ -16,16 +16,19 @@

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;

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}.
Expand All @@ -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";


Expand Down Expand Up @@ -128,8 +129,7 @@ void checkForAliasCircle() {
// 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));
Expand All @@ -139,8 +139,7 @@ void checkForAliasCircle() {
.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()
Expand All @@ -154,13 +153,14 @@ void resolveAliasesPreconditions() {

@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);
Expand All @@ -174,46 +174,183 @@ void resolveAliasesWithoutPlaceholderReplacement() {

@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 resolveAliasesWithComplexPlaceholderReplacementAndConfigurationError() {
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
NAME3, NAME4,
ALIAS3, ALIAS4,
ALIAS4, ALIAS5
));

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):
// Note that duplicate entry (ALIAS4 -> NAME4) gets removed.
// ALIAS5 -> NAME5
// ALIAS4 -> NAME4

// 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 mock = mock();
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
ALIAS3, ALIAS1,
ALIAS4, ALIAS5,
ALIAS5, ALIAS2
));

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):
// ALIAS1 -> NAME3
// 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 (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) {
Expand All @@ -228,4 +365,29 @@ private void assertDoesNotHaveAlias(String name, String alias) {
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);
}

}

}

0 comments on commit 5d309d5

Please sign in to comment.