This is an automated email from the ASF dual-hosted git repository. diru pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git
commit 3aa38e2fc322b87e38d41cf9130909a6e1d51b0a Author: Dirk Rudolph <[email protected]> AuthorDate: Fri Jun 4 17:28:17 2021 +0200 always return all sitemap infos It is in the responsibility of the user level to filter sitemap infos further. On a fragmework level all should be returned, if any at all. --- .../java/org/apache/sling/sitemap/SitemapInfo.java | 7 +++ .../org/apache/sling/sitemap/SitemapService.java | 4 +- .../sling/sitemap/impl/SitemapServiceImpl.java | 54 ++++++++++--------- .../sling/sitemap/impl/SitemapServiceImplTest.java | 60 ++++++++++++++-------- 4 files changed, 78 insertions(+), 47 deletions(-) diff --git a/sitemap/src/main/java/org/apache/sling/sitemap/SitemapInfo.java b/sitemap/src/main/java/org/apache/sling/sitemap/SitemapInfo.java index ce3067b..da7caaa 100644 --- a/sitemap/src/main/java/org/apache/sling/sitemap/SitemapInfo.java +++ b/sitemap/src/main/java/org/apache/sling/sitemap/SitemapInfo.java @@ -50,6 +50,13 @@ public interface SitemapInfo { int getEntries(); /** + * Returns true when the url is pointing to a sitemap-index. + * + * @return + */ + boolean isIndex(); + + /** * Returns true if both size and entries are within the configured limits. * * @return diff --git a/sitemap/src/main/java/org/apache/sling/sitemap/SitemapService.java b/sitemap/src/main/java/org/apache/sling/sitemap/SitemapService.java index ef22d1d..10cf7f7 100644 --- a/sitemap/src/main/java/org/apache/sling/sitemap/SitemapService.java +++ b/sitemap/src/main/java/org/apache/sling/sitemap/SitemapService.java @@ -36,8 +36,8 @@ public interface SitemapService { /** * Returns the urls to the given {@link Resource}'s sitemaps, if any. * <p> - * The returned urls may point to a sitemap index when there are multiple sitemaps generated for the given sitemap - * root {@link Resource}. Or it may point to another sitemap root, if the sitemap is nested below a top level + * The returned urls may contain a sitemap index when there are multiple sitemaps generated for the given sitemap + * root {@link Resource}. Or it may contain urls to another sitemap root, if the sitemap is nested below a top level * sitemap root. * <p> * Numbers for size and entries can only be provided for sitemaps served from storage. For sitemap index or diff --git a/sitemap/src/main/java/org/apache/sling/sitemap/impl/SitemapServiceImpl.java b/sitemap/src/main/java/org/apache/sling/sitemap/impl/SitemapServiceImpl.java index cf93af1..f77a56b 100644 --- a/sitemap/src/main/java/org/apache/sling/sitemap/impl/SitemapServiceImpl.java +++ b/sitemap/src/main/java/org/apache/sling/sitemap/impl/SitemapServiceImpl.java @@ -98,32 +98,29 @@ public class SitemapServiceImpl implements SitemapService { String url = externalizer.externalize(sitemapRoot); Collection<String> names = generatorManager.getGenerators(sitemapRoot).keySet(); - if (url == null || names.isEmpty()) { + if (url == null) { LOG.debug("Could not get absolute url to sitemap: {}", resource.getPath()); return Collections.emptySet(); } - if (names.size() > 1 || requiresSitemapIndex(sitemapRoot)) { - return Collections.singletonList(newSitemapInfo( - url + '.' + SitemapServlet.SITEMAP_INDEX_SELECTOR + '.' + SitemapServlet.SITEMAP_EXTENSION, - -1, - -1 - )); - } else { - String sitemapSelector = getSitemapSelector(sitemapRoot, getTopLevelSitemapRoot(sitemapRoot), - names.iterator().next()); - return storage.getSitemaps(sitemapRoot, names).stream() - .filter(info -> info.getSitemapSelector().equals(sitemapSelector)) - .findFirst() - .map(storageInfo -> Collections.<SitemapInfo>singleton(newSitemapInfo( - sitemapSelector.equals(SitemapServlet.SITEMAP_SELECTOR) - ? url + '.' + SitemapServlet.SITEMAP_SELECTOR + '.' + SitemapServlet.SITEMAP_EXTENSION - : url + '.' + SitemapServlet.SITEMAP_SELECTOR + '.' + sitemapSelector + '.' + SitemapServlet.SITEMAP_EXTENSION, - storageInfo.getSize(), - storageInfo.getEntries() - ))) - .orElseGet(Collections::emptySet); + Collection<SitemapInfo> infos = new ArrayList<>(names.size() + 1); + + if (requiresSitemapIndex(sitemapRoot)) { + infos.add(newSitemapIndexInfo( + url + '.' + SitemapServlet.SITEMAP_INDEX_SELECTOR + '.' + SitemapServlet.SITEMAP_EXTENSION)); + } + + for (SitemapStorageInfo storageInfo : storage.getSitemaps(sitemapRoot, names)) { + String location = url + '.' + SitemapServlet.SITEMAP_SELECTOR + '.'; + if (storageInfo.getSitemapSelector().equals(SitemapServlet.SITEMAP_SELECTOR)) { + location += SitemapServlet.SITEMAP_EXTENSION; + } else { + location += storageInfo.getSitemapSelector() + '.' + SitemapServlet.SITEMAP_EXTENSION; + } + infos.add(newSitemapInfo(location, storageInfo.getSize(), storageInfo.getEntries())); } + + return infos; } @Override @@ -204,8 +201,12 @@ public class SitemapServiceImpl implements SitemapService { return infos; } + private SitemapInfo newSitemapIndexInfo(@NotNull String url) { + return new SitemapInfoImpl(url, -1, -1, true, true); + } + private SitemapInfo newSitemapInfo(@NotNull String url, int size, int entries) { - return new SitemapInfoImpl(url, size, entries, size <= maxSize && entries <= maxEntries); + return new SitemapInfoImpl(url, size, entries, false, size <= maxSize && entries <= maxEntries); } private static class SitemapInfoImpl implements SitemapInfo { @@ -213,12 +214,14 @@ public class SitemapServiceImpl implements SitemapService { private final String url; private final int size; private final int entries; + private final boolean isIndex; private final boolean withinLimits; - private SitemapInfoImpl(@NotNull String url, int size, int entries, boolean withinLimits) { + private SitemapInfoImpl(@NotNull String url, int size, int entries, boolean isIndex, boolean withinLimits) { this.url = url; this.size = size; this.entries = entries; + this.isIndex = isIndex; this.withinLimits = withinLimits; } @@ -239,6 +242,11 @@ public class SitemapServiceImpl implements SitemapService { } @Override + public boolean isIndex() { + return isIndex; + } + + @Override public boolean isWithinLimits() { return withinLimits; } diff --git a/sitemap/src/test/java/org/apache/sling/sitemap/impl/SitemapServiceImplTest.java b/sitemap/src/test/java/org/apache/sling/sitemap/impl/SitemapServiceImplTest.java index 07b9fed..3b188f4 100644 --- a/sitemap/src/test/java/org/apache/sling/sitemap/impl/SitemapServiceImplTest.java +++ b/sitemap/src/test/java/org/apache/sling/sitemap/impl/SitemapServiceImplTest.java @@ -22,8 +22,8 @@ import com.google.common.collect.ImmutableSet; import org.apache.sling.api.resource.Resource; import org.apache.sling.event.jobs.Job; import org.apache.sling.event.jobs.JobManager; +import org.apache.sling.serviceusermapping.ServiceUserMapped; import org.apache.sling.sitemap.SitemapInfo; -import org.apache.sling.sitemap.common.Externalizer; import org.apache.sling.sitemap.generator.SitemapGenerator; import org.apache.sling.testing.mock.jcr.MockJcr; import org.apache.sling.testing.mock.sling.ResourceResolverType; @@ -41,6 +41,8 @@ import org.mockito.junit.jupiter.MockitoExtension; import javax.jcr.Node; import javax.jcr.Session; import javax.jcr.query.Query; +import java.io.ByteArrayInputStream; +import java.io.IOException; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -62,14 +64,15 @@ public class SitemapServiceImplTest { public final SlingContext context = new SlingContext(ResourceResolverType.JCR_MOCK); private final SitemapServiceImpl subject = new SitemapServiceImpl(); + private final SitemapStorage storage = new SitemapStorage(); private final SitemapGeneratorManager generatorManager = new SitemapGeneratorManager(); @Mock + private ServiceUserMapped serviceUser; + @Mock private JobManager jobManager; @Mock private SitemapGenerator generator; - @Mock - private SitemapStorage storage; private Resource deRoot; private Resource enRoot; @@ -97,16 +100,19 @@ public class SitemapServiceImplTest { )); noRoot = context.create().resource("/content/site/nothing"); - context.registerService(SitemapStorage.class, storage); + context.registerService(ServiceUserMapped.class, serviceUser, "subServiceName", "sitemap-writer"); context.registerService(SitemapGenerator.class, generator); context.registerService(JobManager.class, jobManager); context.registerInjectActivateService(generatorManager); + context.registerInjectActivateService(storage); context.registerInjectActivateService(subject, "onDemandNames", "news"); } @Test - public void testIsPendingReturnsTrue() { + public void testIsPendingReturnsTrue() throws IOException { // given + storage.writeSitemap(deRoot, SitemapGenerator.DEFAULT_SITEMAP, new ByteArrayInputStream(new byte[0]), 0, 0); + when(generator.getNames(any())).thenReturn(Collections.singleton(SitemapGenerator.DEFAULT_SITEMAP)); when(jobManager.findJobs( eq(JobManager.QueryType.ALL), @@ -116,7 +122,6 @@ public class SitemapServiceImplTest { && arg.containsKey(SitemapGeneratorExecutor.JOB_PROPERTY_SITEMAP_ROOT) && arg.containsValue("/content/site/de")))) .thenReturn(Collections.singleton(mock(Job.class))); - when(storage.getSitemaps(eq(deRoot), any())).thenReturn(Collections.singleton(mock(SitemapStorageInfo.class))); // when, then // one in storage, one job @@ -124,8 +129,10 @@ public class SitemapServiceImplTest { } @Test - public void testIsPendingReturnsFalse() { + public void testIsPendingReturnsFalse() throws IOException { // given + storage.writeSitemap(deRoot, SitemapGenerator.DEFAULT_SITEMAP, new ByteArrayInputStream(new byte[0]), 0, 0); + when(generator.getNames(any())).thenReturn(ImmutableSet.of(SitemapGenerator.DEFAULT_SITEMAP)); when(generator.getNames(frRoot)).thenReturn(ImmutableSet.of("a", "b")); when(generator.getNames(enNews)).thenReturn(ImmutableSet.of("news")); @@ -137,7 +144,6 @@ public class SitemapServiceImplTest { && arg.containsKey(SitemapGeneratorExecutor.JOB_PROPERTY_SITEMAP_ROOT) && arg.containsValue("/content/site/de")))) .thenReturn(Collections.emptyList()); - when(storage.getSitemaps(eq(deRoot), any())).thenReturn(Collections.singleton(mock(SitemapStorageInfo.class))); MockJcr.setQueryResult( context.resourceResolver().adaptTo(Session.class), @@ -200,27 +206,31 @@ public class SitemapServiceImplTest { } @Test - public void testSitemapUrlReturnsProperSelectors() { + public void testSitemapUrlReturnsProperSelectors() throws IOException { // given - SitemapStorageInfo storageInfoSitemap = - new SitemapStorageInfo("", "sitemap", null, 100, 1); - SitemapStorageInfo storageInfoFoobarSitemap = - new SitemapStorageInfo("", "foobar-sitemap", null, 100, 1); - SitemapStorageInfo storageInfoNewsFooSitemap = - new SitemapStorageInfo("", "news-foo-sitemap", null, 100, 1); - SitemapStorageInfo storageInfoNewsBarSitemap = - new SitemapStorageInfo("", "news-bar-sitemap", null, 100, 1); + storage.writeSitemap(deRoot, SitemapGenerator.DEFAULT_SITEMAP, new ByteArrayInputStream(new byte[0]), 100, 1); + storage.writeSitemap(frRoot, "foobar", new ByteArrayInputStream(new byte[0]), 100, 1); + storage.writeSitemap(enRoot, SitemapGenerator.DEFAULT_SITEMAP, new ByteArrayInputStream(new byte[0]), 100, 1); + storage.writeSitemap(enNews, "foo", new ByteArrayInputStream(new byte[0]), 100, 1); + storage.writeSitemap(enNews, "bar", new ByteArrayInputStream(new byte[0]), 100, 1); when(generator.getNames(deRoot)).thenReturn(ImmutableSet.of(SitemapGenerator.DEFAULT_SITEMAP)); when(generator.getNames(frRoot)).thenReturn(ImmutableSet.of("foobar")); when(generator.getNames(enNews)).thenReturn(ImmutableSet.of("foo", "bar", "news")); - when(storage.getSitemaps(any(), any())).thenReturn(ImmutableSet.of( - storageInfoSitemap, storageInfoFoobarSitemap, storageInfoNewsFooSitemap, storageInfoNewsBarSitemap)); + when(generator.getNames(enRoot)).thenReturn(ImmutableSet.of(SitemapGenerator.DEFAULT_SITEMAP)); + + MockJcr.setQueryResult( + context.resourceResolver().adaptTo(Session.class), + "/jcr:root/content/site/en//*[@sitemapRoot=true]", + Query.XPATH, + Arrays.asList(enRoot.adaptTo(Node.class), enNews.adaptTo(Node.class)) + ); // when Collection<SitemapInfo> deInfos = subject.getSitemapInfo(deRoot); Collection<SitemapInfo> frInfos = subject.getSitemapInfo(frRoot); - Collection<SitemapInfo> enInfos = subject.getSitemapInfo(enNews); + Collection<SitemapInfo> enInfos = subject.getSitemapInfo(enRoot); + Collection<SitemapInfo> enNestedInfos = subject.getSitemapInfo(enNews); // no name assertThat(deInfos, hasSize(1)); @@ -229,13 +239,19 @@ public class SitemapServiceImplTest { assertThat(frInfos, hasSize(1)); assertThat(frInfos, hasItems(eqSitemapInfo("/site/fr.sitemap.foobar-sitemap.xml", 100, 1))); // nested with multiple names - assertThat(enInfos, hasSize(3)); - assertThat(enInfos, hasItems( + assertThat(enNestedInfos, hasSize(3)); + assertThat(enNestedInfos, hasItems( eqSitemapInfo("/site/en.sitemap.news-foo-sitemap.xml", 100, 1), eqSitemapInfo("/site/en.sitemap.news-bar-sitemap.xml", 100, 1), // on-demand eqSitemapInfo("/site/en.sitemap.news-news-sitemap.xml", -1, -1) )); + // nested => index and default self + assertThat(enInfos, hasSize(2)); + assertThat(enInfos, hasItems( + eqSitemapInfo("/site/en.sitemap-index.xml", -1, -1), + eqSitemapInfo("/site/en.sitemap.xml", 100, 1) + )); } private static Matcher<SitemapInfo> eqSitemapInfo(String url, int size, int entries) {
