Skip to content

Commit f39d4d6

Browse files
sbrannensbrannen
committed
Ensure alias resolution in SimpleAliasRegistry depends on registration order
Closes spring-projectsgh-32024
1 parent e1236a8 commit f39d4d6

File tree

2 files changed

+30
-68
lines changed

2 files changed

+30
-68
lines changed

Diff for: ‎spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java

+14-3
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package org.springframework.core;
1818

1919
import java.util.ArrayList;
20-
import java.util.HashMap;
2120
import java.util.List;
2221
import java.util.Map;
2322
import java.util.concurrent.ConcurrentHashMap;
@@ -50,6 +49,9 @@ public class SimpleAliasRegistry implements AliasRegistry {
5049
/** Map from alias to canonical name. */
5150
private final Map<String, String> aliasMap = new ConcurrentHashMap<>(16);
5251

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

5456
@Override
5557
public void registerAlias(String name, String alias) {
@@ -58,6 +60,7 @@ public void registerAlias(String name, String alias) {
5860
synchronized (this.aliasMap) {
5961
if (alias.equals(name)) {
6062
this.aliasMap.remove(alias);
63+
this.aliasNames.remove(alias);
6164
if (logger.isDebugEnabled()) {
6265
logger.debug("Alias definition '" + alias + "' ignored since it points to same name");
6366
}
@@ -80,6 +83,7 @@ public void registerAlias(String name, String alias) {
8083
}
8184
checkForAliasCircle(name, alias);
8285
this.aliasMap.put(alias, name);
86+
this.aliasNames.add(alias);
8387
if (logger.isTraceEnabled()) {
8488
logger.trace("Alias definition '" + alias + "' registered for name '" + name + "'");
8589
}
@@ -111,6 +115,7 @@ public boolean hasAlias(String name, String alias) {
111115
public void removeAlias(String alias) {
112116
synchronized (this.aliasMap) {
113117
String name = this.aliasMap.remove(alias);
118+
this.aliasNames.remove(alias);
114119
if (name == null) {
115120
throw new IllegalStateException("No alias '" + alias + "' registered");
116121
}
@@ -155,19 +160,22 @@ private void retrieveAliases(String name, List<String> result) {
155160
public void resolveAliases(StringValueResolver valueResolver) {
156161
Assert.notNull(valueResolver, "StringValueResolver must not be null");
157162
synchronized (this.aliasMap) {
158-
Map<String, String> aliasCopy = new HashMap<>(this.aliasMap);
159-
aliasCopy.forEach((alias, registeredName) -> {
163+
List<String> aliasNamesCopy = new ArrayList<>(this.aliasNames);
164+
aliasNamesCopy.forEach(alias -> {
165+
String registeredName = this.aliasMap.get(alias);
160166
String resolvedAlias = valueResolver.resolveStringValue(alias);
161167
String resolvedName = valueResolver.resolveStringValue(registeredName);
162168
if (resolvedAlias == null || resolvedName == null || resolvedAlias.equals(resolvedName)) {
163169
this.aliasMap.remove(alias);
170+
this.aliasNames.remove(alias);
164171
}
165172
else if (!resolvedAlias.equals(alias)) {
166173
String existingName = this.aliasMap.get(resolvedAlias);
167174
if (existingName != null) {
168175
if (existingName.equals(resolvedName)) {
169176
// Pointing to existing alias - just remove placeholder
170177
this.aliasMap.remove(alias);
178+
this.aliasNames.remove(alias);
171179
return;
172180
}
173181
throw new IllegalStateException(
@@ -177,10 +185,13 @@ else if (!resolvedAlias.equals(alias)) {
177185
}
178186
checkForAliasCircle(resolvedName, resolvedAlias);
179187
this.aliasMap.remove(alias);
188+
this.aliasNames.remove(alias);
180189
this.aliasMap.put(resolvedAlias, resolvedName);
190+
this.aliasNames.add(resolvedAlias);
181191
}
182192
else if (!registeredName.equals(resolvedName)) {
183193
this.aliasMap.put(alias, resolvedName);
194+
this.aliasNames.add(alias);
184195
}
185196
});
186197
}

Diff for: ‎spring-core/src/test/java/org/springframework/core/SimpleAliasRegistryTests.java

+16-65
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import java.util.Map;
2020

21-
import org.junit.jupiter.api.Disabled;
2221
import org.junit.jupiter.api.Test;
2322
import org.junit.jupiter.params.ParameterizedTest;
2423
import org.junit.jupiter.params.provider.ValueSource;
@@ -49,9 +48,6 @@ class SimpleAliasRegistryTests {
4948
private static final String ALIAS1 = "alias1";
5049
private static final String ALIAS2 = "alias2";
5150
private static final String ALIAS3 = "alias3";
52-
// TODO Determine if we can make SimpleAliasRegistry.resolveAliases() reliable.
53-
// When ALIAS4 is changed to "test", various tests fail due to the iteration
54-
// order of the entries in the aliasMap in SimpleAliasRegistry.
5551
private static final String ALIAS4 = "alias4";
5652
private static final String ALIAS5 = "alias5";
5753

@@ -212,35 +208,37 @@ void resolveAliasesWithPlaceholderReplacementConflict() {
212208
"It is already registered for name '%s'.", ALIAS2, ALIAS1, NAME1, NAME2);
213209
}
214210

215-
@Test
216-
void resolveAliasesWithComplexPlaceholderReplacement() {
211+
@ParameterizedTest
212+
@ValueSource(strings = {"alias4", "test", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"})
213+
void resolveAliasesWithComplexPlaceholderReplacementWithAliasSwitching(String aliasX) {
217214
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
218215
ALIAS3, ALIAS1,
219-
ALIAS4, ALIAS5,
216+
aliasX, ALIAS5,
220217
ALIAS5, ALIAS2
221218
));
222219

220+
// Since SimpleAliasRegistry ensures that aliases are processed in declaration
221+
// order, we need to register ALIAS5 *before* aliasX to support our use case.
223222
registerAlias(NAME3, ALIAS3);
224-
registerAlias(NAME4, ALIAS4);
225223
registerAlias(NAME5, ALIAS5);
224+
registerAlias(NAME4, aliasX);
226225

227226
// Original state:
228-
// WARNING: Based on ConcurrentHashMap iteration order!
229227
// ALIAS3 -> NAME3
230228
// ALIAS5 -> NAME5
231-
// ALIAS4 -> NAME4
229+
// aliasX -> NAME4
232230

233231
// State after processing original entry (ALIAS3 -> NAME3):
234232
// ALIAS1 -> NAME3
235233
// ALIAS5 -> NAME5
236-
// ALIAS4 -> NAME4
234+
// aliasX -> NAME4
237235

238236
// State after processing original entry (ALIAS5 -> NAME5):
239237
// ALIAS1 -> NAME3
240238
// ALIAS2 -> NAME5
241-
// ALIAS4 -> NAME4
239+
// aliasX -> NAME4
242240

243-
// State after processing original entry (ALIAS4 -> NAME4):
241+
// State after processing original entry (aliasX -> NAME4):
244242
// ALIAS1 -> NAME3
245243
// ALIAS2 -> NAME5
246244
// ALIAS5 -> NAME4
@@ -251,71 +249,24 @@ void resolveAliasesWithComplexPlaceholderReplacement() {
251249
assertThat(registry.getAliases(NAME5)).containsExactly(ALIAS2);
252250
}
253251

254-
// TODO Remove this test once we have implemented reliable processing in SimpleAliasRegistry.resolveAliases().
255-
// This method effectively duplicates the @ParameterizedTest version below,
256-
// with aliasX hard coded to ALIAS4; however, this method also hard codes
257-
// a different outcome that passes based on ConcurrentHashMap iteration order!
258-
@Test
259-
void resolveAliasesWithComplexPlaceholderReplacementAndNameSwitching() {
260-
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
261-
NAME3, NAME4,
262-
NAME4, NAME3,
263-
ALIAS3, ALIAS1,
264-
ALIAS4, ALIAS5,
265-
ALIAS5, ALIAS2
266-
));
267-
268-
registerAlias(NAME3, ALIAS3);
269-
registerAlias(NAME4, ALIAS4);
270-
registerAlias(NAME5, ALIAS5);
271-
272-
// Original state:
273-
// WARNING: Based on ConcurrentHashMap iteration order!
274-
// ALIAS3 -> NAME3
275-
// ALIAS5 -> NAME5
276-
// ALIAS4 -> NAME4
277-
278-
// State after processing original entry (ALIAS3 -> NAME3):
279-
// ALIAS1 -> NAME4
280-
// ALIAS5 -> NAME5
281-
// ALIAS4 -> NAME4
282-
283-
// State after processing original entry (ALIAS5 -> NAME5):
284-
// ALIAS1 -> NAME4
285-
// ALIAS2 -> NAME5
286-
// ALIAS4 -> NAME4
287-
288-
// State after processing original entry (ALIAS4 -> NAME4):
289-
// ALIAS1 -> NAME4
290-
// ALIAS2 -> NAME5
291-
// ALIAS5 -> NAME3
292-
293-
registry.resolveAliases(valueResolver);
294-
assertThat(registry.getAliases(NAME3)).containsExactly(ALIAS5);
295-
assertThat(registry.getAliases(NAME4)).containsExactly(ALIAS1);
296-
assertThat(registry.getAliases(NAME5)).containsExactly(ALIAS2);
297-
}
298-
299-
@Disabled("Fails for some values unless alias registration order is honored")
300252
@ParameterizedTest
301253
@ValueSource(strings = {"alias4", "test", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"})
302-
void resolveAliasesWithComplexPlaceholderReplacementAndNameSwitching(String aliasX) {
254+
void resolveAliasesWithComplexPlaceholderReplacementWithAliasAndNameSwitching(String aliasX) {
303255
StringValueResolver valueResolver = new StubStringValueResolver(Map.of(
304-
NAME3, NAME4,
305-
NAME4, NAME3,
306256
ALIAS3, ALIAS1,
307257
aliasX, ALIAS5,
308-
ALIAS5, ALIAS2
258+
ALIAS5, ALIAS2,
259+
NAME3, NAME4,
260+
NAME4, NAME3
309261
));
310262

311-
// If SimpleAliasRegistry ensures that aliases are processed in declaration
263+
// Since SimpleAliasRegistry ensures that aliases are processed in declaration
312264
// order, we need to register ALIAS5 *before* aliasX to support our use case.
313265
registerAlias(NAME3, ALIAS3);
314266
registerAlias(NAME5, ALIAS5);
315267
registerAlias(NAME4, aliasX);
316268

317269
// Original state:
318-
// WARNING: Based on LinkedHashMap iteration order!
319270
// ALIAS3 -> NAME3
320271
// ALIAS5 -> NAME5
321272
// aliasX -> NAME4

0 commit comments

Comments
 (0)