This is an automated email from the ASF dual-hosted git repository. rombert pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
commit 259f895d1e9a31635d2f7be64dd6317f6a9ada1e Author: Robert Munteanu <[email protected]> AuthorDate: Fri Aug 7 13:31:39 2020 +0200 SLING-9620 - ResourceMapperImpl.getAllMappings does not respect multi-valued sling:alias Generate all possible alias paths in case of multiple aliases. --- .../resourceresolver/impl/mapping/PathBuilder.java | 44 +++---- .../impl/mapping/ResourceMapperImpl.java | 127 +++++++++++---------- .../impl/mapping/PathBuilderTest.java | 74 ++++++++---- .../impl/mapping/ResourceMapperImplTest.java | 4 +- 4 files changed, 144 insertions(+), 105 deletions(-) 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 @@ import org.jetbrains.annotations.Nullable; */ 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 @@ public class PathBuilder { 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 @@ public class PathBuilder { return out; } - private List<String> segments = new ArrayList<>(); + private List<List<String>> segments = new ArrayList<>(); private String toAppend; /** @@ -65,8 +72,14 @@ public class PathBuilder { * @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 @@ public class PathBuilder { * * @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 @@ public class ResourceMapperImpl implements ResourceMapper { // 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 @@ public class ResourceMapperImpl implements ResourceMapper { 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 @@ public class ResourceMapperImpl implements ResourceMapper { 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 @@ public class ResourceMapperImpl implements ResourceMapper { 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 @@ public class ResourceMapperImpl implements ResourceMapper { } // 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 @@ public class PathBuilderTest { @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")); - } - - @Test - public void cartesianJoin_simple() { - List<String> paths = PathBuilder.cartesianJoin(Arrays.asList( Arrays.asList("1"), Arrays.asList("2") )); + + List<String> paths = builder.toPaths(); + assertThat(paths, Matchers.hasSize(1)); - assertThat(paths, Matchers.hasItem("/1/2")); + assertThat(paths, Matchers.hasItem("/foo/bar/baz")); } - + @Test - public void cartesianJoin_multiple() { - List<String> paths = PathBuilder.cartesianJoin(Arrays.asList( Arrays.asList("1a", "1b"), Arrays.asList("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("/1a/2", "/1b/2")); + assertThat(paths, Matchers.hasItems("/alias1/bar", "/alias2/bar")); } - + @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.is; 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 @@ public class ResourceMapperImplTest { * @throws LoginException */ @Test - @Ignore("SLING-9620") public void mapResourceWithMultivaluedAlias() { + + assumeFalse(optimiseAliasResolution); ExpectedMappings.existingResource("/there-multiple") .singleMapping("/alias-value-3")
