SLING-9620 - ResourceMapperImpl.getAllMappings does not respect multi-valued sling:alias
Generate all possible alias paths in case of multiple aliases.
diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilder.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilder.java
index bc37a86..207e557 100644
--- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilder.java
+++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilder.java
@@ -19,6 +19,7 @@
package org.apache.sling.resourceresolver.impl.mapping;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
@@ -31,10 +32,15 @@
*/
public class PathBuilder {
- // visible for testing
- static List<String> cartesianJoin(List<List<String>> segments) {
+ private static List<String> cartesianJoin(List<List<String>> segments, String toAppend) {
return cartesianJoin0(0, segments).stream()
+ .map ( sb -> {
+ if ( toAppend != null )
+ sb.append(toAppend);
+
+ return sb;
+ })
.map( StringBuilder::toString )
.collect( Collectors.toList() );
}
@@ -42,12 +48,13 @@
private static List<StringBuilder> cartesianJoin0(int index, List<List<String>> segments) {
List<StringBuilder> out = new ArrayList<>();
if ( index == segments.size() ) {
- out.add(new StringBuilder());
+ out.add(new StringBuilder("/"));
} else {
for ( String segment : segments.get(index) ) {
for (StringBuilder sb : cartesianJoin0(index + 1, segments) ) {
- // TODO - this is sub-optimal, as we are copying the array for each move
- sb.insert(0, '/' + segment);
+ sb.append(segment);
+ if ( index != 0 )
+ sb.append('/');
out.add(sb);
}
}
@@ -56,7 +63,7 @@
return out;
}
- private List<String> segments = new ArrayList<>();
+ private List<List<String>> segments = new ArrayList<>();
private String toAppend;
/**
@@ -65,8 +72,14 @@
* @param alias the alias, ignored if null or empty
* @param name the name
*/
- public void insertSegment(@Nullable String alias, @NotNull String name) {
- segments.add(alias != null && alias.length() != 0 ? alias : name);
+ public void insertSegment(@NotNull List<String> alias, @NotNull String name) {
+
+ // TODO - can we avoid filtering here?
+ List<String> filtered = alias.stream()
+ .filter( e -> e != null && ! e.isEmpty() )
+ .collect(Collectors.toList());
+
+ segments.add(!filtered.isEmpty() ? alias : Collections.singletonList(name));
}
/**
@@ -83,18 +96,7 @@
*
* @return a path in string form
*/
- public String toPath() {
- StringBuilder sb = new StringBuilder();
- sb.append('/');
- for ( int i = segments.size() - 1 ; i >= 0 ; i-- ) {
- sb.append(segments.get(i));
- if ( i == 1 )
- sb.append('/');
- }
-
- if ( toAppend != null )
- sb.append(toAppend);
-
- return sb.toString();
+ public List<String> toPaths() {
+ return cartesianJoin(segments, toAppend);
}
}
diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java
index 6fdd883..acc4707 100644
--- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java
+++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImpl.java
@@ -19,6 +19,7 @@
package org.apache.sling.resourceresolver.impl.mapping;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashSet;
@@ -95,21 +96,21 @@
// Therefore we take care to add the entries in a very particular order, which preserves backwards
// compatibility with the existing implementation. Previously the order was
//
- // resource path → alias → mapping (with alias potentially being null)
+ // resource path → aliases → mapping (with aliases potentially being empty)
//
// To ensure we keep the same behaviour but expose all possible mappings, we now have the following
// flow
//
// resource path → mapping
- // resource path → alias
- // alias → mapping
+ // resource path → aliases
+ // aliases → mappings
//
// After all are processed we reverse the order to preserve the logic of the old ResourceResolver.map() method (last
// found wins) and also make sure that no duplicates are added.
//
// There is some room for improvement here by using a data structure that does not need reversing ( ArrayList
// .add moves the elements every time ) or reversal of duplicates but since we will have a small number of
- // entries ( <= 4 ) the time spent here should be negligible.
+ // entries ( <= 4 in case of single aliases) the time spent here should be negligible.
List<String> mappings = new ArrayList<>();
@@ -141,18 +142,18 @@
mappings.add(mappedPath);
// 3. load mappings from the resource path
- populateMappingsFromMapEntries(mappings, mappedPath, requestContext);
-
+ populateMappingsFromMapEntries(mappings, Collections.singletonList(mappedPath), requestContext);
// 4. load aliases
final Resource nonDecoratedResource = resolver.resolveInternal(parsed.getRawPath(), parsed.getParameters());
if (nonDecoratedResource != null) {
- String alias = loadAliasIfApplicable(nonDecoratedResource);
+ List<String> aliases = loadAliasesIfApplicable(nonDecoratedResource);
+ // ensure that the first declared alias will be returned first
+ Collections.reverse(aliases);
// 5. load mappings for alias
- if ( alias != null )
- mappings.add(alias);
- populateMappingsFromMapEntries(mappings, alias, requestContext);
+ mappings.addAll(aliases);
+ populateMappingsFromMapEntries(mappings, aliases, requestContext);
}
// 6. apply context path if needed
@@ -172,7 +173,7 @@
return new LinkedHashSet<>(mappings);
}
- private String loadAliasIfApplicable(final Resource nonDecoratedResource) {
+ private List<String> loadAliasesIfApplicable(final Resource nonDecoratedResource) {
//Invoke the decorator for the resolved resource
Resource res = resourceDecorator.decorate(nonDecoratedResource);
@@ -192,13 +193,13 @@
Resource current = res;
String path = res.getPath();
while (path != null) {
- String alias = null;
+ List<String> aliases = Collections.emptyList();
// read alias only if we can read the resources and it's not a jcr:content leaf
if (current != null && !path.endsWith(ResourceResolverImpl.JCR_CONTENT_LEAF)) {
- alias = readAlias(path, current);
+ aliases = readAliases(path, current);
}
// build the path from the name segments or aliases
- pathBuilder.insertSegment(alias, ResourceUtil.getName(path));
+ pathBuilder.insertSegment(aliases, ResourceUtil.getName(path));
path = ResourceUtil.getParent(path);
if ("/".equals(path)) {
path = null;
@@ -208,82 +209,90 @@
}
// and then we have the mapped path to work on
- String mappedPath = pathBuilder.toPath();
+ List<String> mappedPaths = pathBuilder.toPaths();
- logger.debug("map: Alias mapping resolves to path {}", mappedPath);
+ logger.debug("map: Alias mapping resolves to paths {}", mappedPaths);
- return mappedPath;
+ return mappedPaths;
}
- private String readAlias(String path, Resource current) {
+ private List<String> readAliases(String path, Resource current) {
if (optimizedAliasResolutionEnabled) {
logger.debug("map: Optimize Alias Resolution is Enabled");
String parentPath = ResourceUtil.getParent(path);
if ( parentPath == null )
- return null;
+ return Collections.emptyList();
final Map<String, String> aliases = mapEntries.getAliasMap(parentPath);
if ( aliases == null )
- return null;
+ return Collections.emptyList();
if ( aliases.containsValue(current.getName()) ) {
for ( Map.Entry<String,String> entry : aliases.entrySet() ) {
if (current.getName().equals(entry.getValue())) {
- return entry.getKey();
+ // TODO - support multi-valued entries
+ return Collections.singletonList(entry.getKey());
}
}
}
- return null;
+ return Collections.emptyList();
} else {
logger.debug("map: Optimize Alias Resolution is Disabled");
- return ResourceResolverControl.getProperty(current, ResourceResolverImpl.PROP_ALIAS);
+ String[] aliases = ResourceResolverControl.getProperty(current, ResourceResolverImpl.PROP_ALIAS, String[].class);
+ if ( aliases == null || aliases.length == 0 )
+ return Collections.emptyList();
+ if ( aliases.length == 1 )
+ return Collections.singletonList(aliases[0]);
+ return Arrays.asList(aliases);
}
}
- private void populateMappingsFromMapEntries(List<String> mappings, String mappedPath,
+ private void populateMappingsFromMapEntries(List<String> mappings, List<String> mappedPathList,
final RequestContext requestContext) {
boolean mappedPathIsUrl = false;
- for (final MapEntry mapEntry : mapEntries.getMapMaps()) {
- final String[] mappedPaths = mapEntry.replace(mappedPath);
- if (mappedPaths != null) {
-
- logger.debug("map: Match for Entry {}", mapEntry);
-
- mappedPathIsUrl = !mapEntry.isInternal();
-
- if (mappedPathIsUrl && requestContext.hasUri() ) {
-
- mappedPath = null;
-
- for (final String candidate : mappedPaths) {
- if (candidate.startsWith(requestContext.getUri())) {
- mappedPath = candidate.substring(requestContext.getUri().length() - 1);
- mappedPathIsUrl = false;
- logger.debug("map: Found host specific mapping {} resolving to {}", candidate, mappedPath);
- break;
- } else if (candidate.startsWith(requestContext.getSchemeWithPrefix()) && mappedPath == null) {
- mappedPath = candidate;
+ for ( String mappedPath : mappedPathList ) {
+ for (final MapEntry mapEntry : mapEntries.getMapMaps()) {
+ final String[] mappedPaths = mapEntry.replace(mappedPath);
+ if (mappedPaths != null) {
+
+ logger.debug("map: Match for Entry {}", mapEntry);
+
+ mappedPathIsUrl = !mapEntry.isInternal();
+
+ if (mappedPathIsUrl && requestContext.hasUri() ) {
+
+ mappedPath = null;
+
+ for (final String candidate : mappedPaths) {
+ if (candidate.startsWith(requestContext.getUri())) {
+ mappedPath = candidate.substring(requestContext.getUri().length() - 1);
+ mappedPathIsUrl = false;
+ logger.debug("map: Found host specific mapping {} resolving to {}", candidate, mappedPath);
+ break;
+ } else if (candidate.startsWith(requestContext.getSchemeWithPrefix()) && mappedPath == null) {
+ mappedPath = candidate;
+ }
}
- }
-
- if (mappedPath == null) {
+
+ if (mappedPath == null) {
+ mappedPath = mappedPaths[0];
+ }
+
+ } else {
+
+ // we can only go with assumptions selecting the first entry
mappedPath = mappedPaths[0];
+
}
-
- } else {
-
- // we can only go with assumptions selecting the first entry
- mappedPath = mappedPaths[0];
-
+
+ logger.debug("map: MapEntry {} matches, mapped path is {}", mapEntry, mappedPath);
+
+ mappings.add(mappedPath);
+
+ break;
}
-
- logger.debug("map: MapEntry {} matches, mapped path is {}", mapEntry, mappedPath);
-
- mappings.add(mappedPath);
-
- break;
}
}
}
diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilderTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilderTest.java
index 6f4ba78..79b454b 100644
--- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilderTest.java
+++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PathBuilderTest.java
@@ -18,9 +18,11 @@
*/
package org.apache.sling.resourceresolver.impl.mapping;
+import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
import static org.junit.Assert.*;
-import java.util.Arrays;
import java.util.List;
import org.hamcrest.Matchers;
@@ -30,51 +32,75 @@
@Test
public void buildRootPath() {
- assertThat(new PathBuilder().toPath(), Matchers.equalTo("/"));
+
+ List<String> paths = new PathBuilder().toPaths();
+
+ assertThat(paths, Matchers.hasSize(1));
+ assertThat(paths, Matchers.hasItem("/"));
}
@Test
public void buildSubPathWithMissingAliases() {
+
PathBuilder builder = new PathBuilder();
- builder.insertSegment(null, "bar");
- builder.insertSegment("", "foo");
- assertThat(builder.toPath(), Matchers.equalTo("/foo/bar"));
+ builder.insertSegment(singletonList(null), "bar");
+ builder.insertSegment(singletonList(""), "foo");
+ List<String> paths = builder.toPaths();
+
+ assertThat(paths, Matchers.hasSize(1));
+ assertThat(paths, Matchers.hasItem("/foo/bar"));
}
@Test
public void buildSubPathWithMixedAliases() {
+
PathBuilder builder = new PathBuilder();
- builder.insertSegment(null, "bar");
- builder.insertSegment("super", "foo");
- assertThat(builder.toPath(), Matchers.equalTo("/super/bar"));
+ builder.insertSegment(emptyList(), "bar");
+ builder.insertSegment(singletonList("super"), "foo");
+ List<String> paths = builder.toPaths();
+
+ assertThat(paths, Matchers.hasSize(1));
+ assertThat(paths, Matchers.hasItem("/super/bar"));
}
@Test
public void buildSubPathWithResolutionInfo() {
+
PathBuilder builder = new PathBuilder();
- builder.insertSegment(null, "bar");
- builder.insertSegment(null, "foo");
+ builder.insertSegment(emptyList(), "bar");
+ builder.insertSegment(emptyList(), "foo");
builder.setResolutionPathInfo("/baz");
- assertThat(builder.toPath(), Matchers.equalTo("/foo/bar/baz"));
+
+ List<String> paths = builder.toPaths();
+
+ assertThat(paths, Matchers.hasSize(1));
+ assertThat(paths, Matchers.hasItem("/foo/bar/baz"));
}
@Test
- public void cartesianJoin_simple() {
- List<String> paths = PathBuilder.cartesianJoin(Arrays.asList( Arrays.asList("1"), Arrays.asList("2") ));
- assertThat(paths, Matchers.hasSize(1));
- assertThat(paths, Matchers.hasItem("/1/2"));
+ public void buildSubPathWithMultipleAliases() {
+
+ PathBuilder builder = new PathBuilder();
+ builder.insertSegment(emptyList(), "bar");
+ builder.insertSegment(asList("alias1", "alias2"), "foo");
+
+ List<String> paths = builder.toPaths();
+
+ assertThat(paths, Matchers.hasSize(2));
+ assertThat(paths, Matchers.hasItems("/alias1/bar", "/alias2/bar"));
}
@Test
- public void cartesianJoin_multiple() {
- List<String> paths = PathBuilder.cartesianJoin(Arrays.asList( Arrays.asList("1a", "1b"), Arrays.asList("2") ));
- assertThat(paths, Matchers.hasSize(2));
- assertThat(paths, Matchers.hasItems("/1a/2", "/1b/2"));
- }
-
- @Test
- public void cartesianJoin_complex() {
- List<String> paths = PathBuilder.cartesianJoin(Arrays.asList( Arrays.asList("1a", "1b"), Arrays.asList("2a", "2b"), Arrays.asList("3"), Arrays.asList("4a", "4b", "4c") ));
+ public void buildSubPathWithComplexAliasesSetup() {
+
+ PathBuilder builder = new PathBuilder();
+ builder.insertSegment(asList("4a", "4b", "4c"), "4");
+ builder.insertSegment(emptyList(), "3");
+ builder.insertSegment(asList("2a", "2b"), "2");
+ builder.insertSegment(asList("1a", "1b"), "1");
+
+ List<String> paths = builder.toPaths();
+
assertThat(paths, Matchers.hasSize(12));
assertThat(paths, Matchers.hasItems(
"/1a/2a/3/4a",
diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
index 5754143..e267be5 100644
--- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
+++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
@@ -25,6 +25,7 @@
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;
+import static org.junit.Assume.assumeFalse;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -205,8 +206,9 @@
* @throws LoginException
*/
@Test
- @Ignore("SLING-9620")
public void mapResourceWithMultivaluedAlias() {
+
+ assumeFalse(optimiseAliasResolution);
ExpectedMappings.existingResource("/there-multiple")
.singleMapping("/alias-value-3")
