You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by di...@apache.org on 2021/06/04 15:29:12 UTC
[sling-whiteboard] 02/02: always return all sitemap infos
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 <di...@apache.org>
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) {