This is an automated email from the ASF dual-hosted git repository.
reschke pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
The following commit(s) were added to refs/heads/master by this push:
new a0270a0 SLING-12384: ResourceResolver: paged query for sling:alias
will miss entries when 'first(sling:alias)' is empty (#119)
a0270a0 is described below
commit a0270a0cff7d9d892906293cd13fd8f0b7d8f9be
Author: Julian Reschke <[email protected]>
AuthorDate: Tue Jul 23 16:36:04 2024 +0200
SLING-12384: ResourceResolver: paged query for sling:alias will miss
entries when 'first(sling:alias)' is empty (#119)
* SLING-12384: ResourceResolver: paged query for sling:alias will miss
entries where 'first(sling:alias)' is empty
* SLING-12384: adapt test case
* SLING-12384: rewrite pageing logic so that no resources are skipped on
page boundaries, add more diagnostics
* SLING-12384: rewrite pageing logic so that no resources are skipped on
page boundaries, add more diagnostics - cleanup
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: empty search result
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: broken query
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: simple query
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: paged query skipping resource with empty key
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: broken paging
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: detect broken sort order
* SLING-12384: include tests from SLING-12387, add detection of sort issue
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: sonar mangling
* SLING-12387: ResourceResolver: improve test coverage for
PagedQueryIterator: align test with SLING-12384
* SLING-12384: fix page number in debug log
* SLING-12384: report stats about last page as well (ack: Yannick Poireault)
* SLING-12384: make the paging warning a WARN log message
* SLING-12384: test the warning message
* SLING-12384: test cleanup
* SLING-12384: cleanup
* SLING-12384: avoid huge literal array init
* SLING-12384: doPageStats -> updatePageStats
* SLING-12384: fix string format for iterator warning message
* SLING-12384: MapEntriesTest - adjust string const for VP query
---
.../resourceresolver/impl/mapping/MapEntries.java | 85 +++++++++++++++++++---
.../impl/mapping/MapEntriesTest.java | 4 +-
.../impl/mapping/PagedQueryIteratorTest.java | 54 +++++++++-----
3 files changed, 112 insertions(+), 31 deletions(-)
diff --git
a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
index f9fa751..792ec7b 100644
---
a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
+++
b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
@@ -358,6 +358,7 @@ public class MapEntries implements
long initElapsed = System.nanoTime() - initStart;
long resourcesPerSecond = (vanityResourcesOnStartup.get() *
TimeUnit.SECONDS.toNanos(1) / (initElapsed == 0 ? 1 : initElapsed));
+
log.info(
"vanity path initialization - completed, processed {}
resources with sling:vanityPath properties in {}ms (~{} resource/s)",
vanityResourcesOnStartup.get(),
TimeUnit.NANOSECONDS.toMillis(initElapsed), resourcesPerSecond);
@@ -1160,7 +1161,7 @@ public class MapEntries implements
Iterator<Resource> it;
try {
- final String queryStringWithSort = baseQueryString + " AND
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+ final String queryStringWithSort = baseQueryString + " AND
FIRST([sling:alias]) >= '%s' ORDER BY FIRST([sling:alias])";
it = new PagedQueryIterator("alias", "sling:alias", resolver,
queryStringWithSort, 2000);
} catch (QuerySyntaxException ex) {
log.debug("sort with first() not supported, falling back to base
query", ex);
@@ -1179,8 +1180,20 @@ public class MapEntries implements
}
long processElapsed = System.nanoTime() - processStart;
long resourcePerSecond = (count * TimeUnit.SECONDS.toNanos(1) /
(processElapsed == 0 ? 1 : processElapsed));
- log.info("alias initialization - completed, processed {} resources
with sling:alias properties in {}ms (~{} resource/s)",
- count, TimeUnit.NANOSECONDS.toMillis(processElapsed),
resourcePerSecond);
+
+ String diagnostics = "";
+ if (it instanceof PagedQueryIterator) {
+ PagedQueryIterator pit = (PagedQueryIterator)it;
+
+ if (!pit.getWarning().isEmpty()) {
+ log.warn(pit.getWarning());
+ }
+
+ diagnostics = pit.getStatistics();
+ }
+
+ log.info("alias initialization - completed, processed {} resources
with sling:alias properties in {}ms (~{} resource/s){}",
+ count, TimeUnit.NANOSECONDS.toMillis(processElapsed),
resourcePerSecond, diagnostics);
this.aliasResourcesOnStartup.set(count);
@@ -1316,17 +1329,22 @@ public class MapEntries implements
private String subject;
private String propertyName;
private String query;
- private String lastValue = "";
+ private String lastKey = "";
+ private String lastValue = null;
private Iterator<Resource> it;
private int count = 0;
private int page = 0;
private int pageSize;
private Resource next = null;
private String[] defaultValue = new String[0];
+ private int largestPage = 0;
+ private String largestKeyValue = "";
+ private int largestKeyCount = 0;
+ private int currentKeyCount = 0;
/**
* @param subject name of the query, will be used only for logging
- * @param propertyName name of multivalued string property to query on
+ * @param propertyName name of multivalued string property to query on
(used for diagnostics)
* @param resolver resource resolver
* @param query query string in SQL2 syntax
* @param pageSize page size (start a new query after page size is
exceeded)
@@ -1342,7 +1360,7 @@ public class MapEntries implements
private void nextPage() {
count = 0;
- String tquery = String.format(query, queryLiteral(lastValue));
+ String tquery = String.format(query, queryLiteral(lastKey));
log.debug("start {} query (page {}): {}", subject, page, tquery);
long queryStart = System.nanoTime();
this.it = resolver.findResources(tquery, "JCR-SQL2");
@@ -1355,20 +1373,41 @@ public class MapEntries implements
Resource resource = it.next();
count += 1;
final String[] values = resource.getValueMap().get(propertyName,
defaultValue);
+
if (values.length > 0) {
String value = values[0];
- if (value.compareTo(lastValue) < 0) {
+ if (value.compareTo(lastKey) < 0) {
String message = String.format("unexpected query result in
page %d, %s of '%s' despite querying for > '%s'",
+ (page - 1), propertyName, value, lastKey);
+ log.error(message);
+ throw new RuntimeException(message);
+ }
+ if (lastValue != null && value.compareTo(lastValue) < 0) {
+ String message = String.format("unexpected query result in
page %d, property name '%s', got '%s', last value was '%s'",
(page - 1), propertyName, value, lastValue);
log.error(message);
throw new RuntimeException(message);
}
+
+ // keep information about large key counts
+ if (value.equals(lastValue)) {
+ currentKeyCount += 1;
+ } else {
+ if (currentKeyCount > largestKeyCount) {
+ largestKeyCount = currentKeyCount + 1;
+ largestKeyValue = lastValue;
+ }
+ currentKeyCount = 0;
+ }
+
// start next page?
if (count > pageSize && !value.equals(lastValue)) {
- log.debug("read {} query (page {}); {} entries", subject,
page, count);
- lastValue = value;
+ updatePageStats();
+ lastKey = value;
nextPage();
+ return getNext();
}
+ lastValue = value;
}
return resource;
@@ -1380,6 +1419,11 @@ public class MapEntries implements
try {
next = getNext();
} catch (NoSuchElementException ex) {
+ if (currentKeyCount > largestKeyCount) {
+ largestKeyCount = currentKeyCount + 1;
+ largestKeyValue = lastValue;
+ }
+ updatePageStats();
// there are no more
next = null;
}
@@ -1393,6 +1437,27 @@ public class MapEntries implements
next = null;
return result;
}
+
+ private void updatePageStats() {
+ largestPage = Math.max(largestPage, count - 1);
+ log.debug("read {} query (page {}); {} entries, last key was: {},
largest page so far: {}", subject, page - 1,
+ count, lastKey, largestPage);
+ }
+
+ public @NotNull String getStatistics() {
+ return String.format(" (max. page size: %d, number of pages: %d)",
largestPage, page);
+ }
+
+ public @NotNull String getWarning() {
+ int warnAt = pageSize * 10;
+ if (largestKeyCount > warnAt) {
+ return String.format(
+ "Largest number of aliases with the same 'first'
selector exceeds expectation of %d (value '%s' appears %d times)",
+ warnAt, largestKeyValue, largestKeyCount);
+ } else {
+ return "";
+ }
+ }
}
/**
@@ -1408,7 +1473,7 @@ public class MapEntries implements
boolean supportsSort = true;
Iterator<Resource> it;
try {
- final String queryStringWithSort = baseQueryString + " AND
FIRST([sling:vanityPath]) > '%s' ORDER BY FIRST([sling:vanityPath])";
+ final String queryStringWithSort = baseQueryString + " AND
FIRST([sling:vanityPath]) >= '%s' ORDER BY FIRST([sling:vanityPath])";
it = new PagedQueryIterator("vanity path", PROP_VANITY_PATH,
resolver, queryStringWithSort, 2000);
} catch (QuerySyntaxException ex) {
log.debug("sort with first() not supported, falling back to base
query", ex);
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
index b82baaf..47917e7 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
@@ -2279,7 +2279,7 @@ public class MapEntriesTest extends
AbstractMappingMapEntriesTest {
@Override
public Iterator<Resource> answer(InvocationOnMock invocation)
throws Throwable {
String query =
StringUtils.trim((String)invocation.getArguments()[0]);
- assertEquals("SELECT [sling:alias] FROM [nt:base] WHERE NOT
isdescendantnode('/jcr:system') AND [sling:alias] IS NOT NULL AND
FIRST([sling:alias]) > '' ORDER BY FIRST([sling:alias])", query);
+ assertEquals("SELECT [sling:alias] FROM [nt:base] WHERE NOT
isdescendantnode('/jcr:system') AND [sling:alias] IS NOT NULL AND
FIRST([sling:alias]) >= '' ORDER BY FIRST([sling:alias])", query);
return Collections.<Resource> emptySet().iterator();
}
});
@@ -2299,7 +2299,7 @@ public class MapEntriesTest extends
AbstractMappingMapEntriesTest {
// utilities for testing vanity path queries
- private static String VPQSTART = "SELECT [sling:vanityPath],
[sling:redirect], [sling:redirectStatus] FROM [nt:base] WHERE NOT
isdescendantnode('/jcr:system') AND [sling:vanityPath] IS NOT NULL AND
FIRST([sling:vanityPath]) > '";
+ private static String VPQSTART = "SELECT [sling:vanityPath],
[sling:redirect], [sling:redirectStatus] FROM [nt:base] WHERE NOT
isdescendantnode('/jcr:system') AND [sling:vanityPath] IS NOT NULL AND
FIRST([sling:vanityPath]) >= '";
private static String VPQEND = "' ORDER BY FIRST([sling:vanityPath])";
private boolean matchesPagedQuery(String query) {
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java
index 4bd41d0..7e23e1f 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java
@@ -40,8 +40,8 @@ import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ValueMap;
import org.apache.sling.api.resource.path.Path;
import org.apache.sling.resourceresolver.impl.ResourceResolverMetrics;
+import
org.apache.sling.resourceresolver.impl.mapping.MapEntries.PagedQueryIterator;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Test;
import org.mockito.MockitoAnnotations;
@@ -86,14 +86,14 @@ public class PagedQueryIteratorTest extends
AbstractMappingMapEntriesTest {
String[] expected = new String[] { "a", "b", "c" };
Collection<Resource> expectedResources = toResourceList(expected);
when(resourceResolver.findResources(eq("simple"),
eq("JCR-SQL2"))).thenReturn(expectedResources.iterator());
- Iterator<Resource> it = mapEntries.new PagedQueryIterator("alias",
PROPNAME, resourceResolver, "simple", 2000);
+ PagedQueryIterator it = mapEntries.new PagedQueryIterator("alias",
PROPNAME, resourceResolver, "simple", 2000);
for (String key : expected) {
assertEquals(key, getFirstValueOf(it.next(), PROPNAME));
}
assertFalse(it.hasNext());
+ assertEquals("", it.getWarning());
}
- @Ignore("SLING-12384: detection of incorrect sort order fails")
@Test(expected = RuntimeException.class)
public void testSimpleWrongOrder() {
String[] expected = new String[] { "a", "b", "d", "c" };
@@ -106,7 +106,6 @@ public class PagedQueryIteratorTest extends
AbstractMappingMapEntriesTest {
}
}
- @Ignore("SLING-12384: resources with empty keys lost")
@Test
public void testPagedWithEmpty() {
String[] expected = new String[] { "", "a", "b", "c", "d" };
@@ -114,15 +113,29 @@ public class PagedQueryIteratorTest extends
AbstractMappingMapEntriesTest {
Collection<Resource> expectedFilteredResources = filter("",
expectedResources);
when(resourceResolver.findResources(eq("testPagedWithEmpty ''"),
eq("JCR-SQL2")))
.thenReturn(expectedFilteredResources.iterator());
- Iterator<Resource> it = mapEntries.new PagedQueryIterator("alias",
PROPNAME, resourceResolver, "testPagedWithEmpty '%s'",
+ PagedQueryIterator it = mapEntries.new PagedQueryIterator("alias",
PROPNAME, resourceResolver, "testPagedWithEmpty '%s'",
2000);
- for (String key : expected) {
- assertEquals(key, getFirstValueOf(it.next(), PROPNAME));
- }
- assertFalse(it.hasNext());
+ checkResult(it, expected);
+ assertEquals("", it.getWarning());
+ }
+
+ @Test
+ public void testPagedLargePage() {
+ final int cnt = 140;
+ final int pageSize = 5;
+ String[] expected = new String[cnt];
+ Arrays.fill(expected,"a");
+ Collection<Resource> expectedResources = toResourceList(expected);
+ Collection<Resource> expectedFilteredResources = filter("",
expectedResources);
+ when(resourceResolver.findResources(eq("testPagedLargePage ''"),
eq("JCR-SQL2")))
+ .thenReturn(expectedFilteredResources.iterator());
+ PagedQueryIterator it = mapEntries.new PagedQueryIterator("alias",
PROPNAME, resourceResolver, "testPagedLargePage '%s'",
+ pageSize);
+ checkResult(it, expected);
+ assertEquals("Largest number of aliases with the same 'first' selector
exceeds expectation of " + pageSize * 10
+ + " (value 'a' appears " + cnt + " times)", it.getWarning());
}
- @Ignore("SLING-12384: broken paging")
@Test
public void testPagedResourcesOnPageBoundaryLost() {
String[] expected = new String[] { "a", "a", "a", "a", "a", "a", "b",
"c", "d" };
@@ -144,12 +157,8 @@ public class PagedQueryIteratorTest extends
AbstractMappingMapEntriesTest {
.thenReturn(expectedFilteredResourcesD.iterator());
Iterator<Resource> it = mapEntries.new PagedQueryIterator("alias",
PROPNAME, resourceResolver,
"testPagedResourcesOnPageBoundaryLost '%s'", 5);
- int pos = 0;
- for (String key : expected) {
- assertEquals("expects " + key + " at position " + pos, key,
getFirstValueOf(it.next(), PROPNAME));
- pos += 1;
- }
- assertFalse(it.hasNext());
+
+ checkResult(it, expected);
}
private static Collection<Resource> toResourceList(String... keys) {
@@ -165,13 +174,20 @@ public class PagedQueryIteratorTest extends
AbstractMappingMapEntriesTest {
}
private static Collection<Resource> filter(String key,
Collection<Resource> input) {
- // this emulates the ">" condition used by PagedQueryIterator prior to
- // resolution of SLING-12384
- Predicate<Resource> filter = r -> getFirstValueOf(r,
PROPNAME).compareTo(key) > 0;
+ Predicate<Resource> filter = r -> getFirstValueOf(r,
PROPNAME).compareTo(key) >= 0;
return input.stream().filter(filter).collect(Collectors.toList());
}
private static String getFirstValueOf(Resource r, String propname) {
return r.getValueMap().get(propname, new String[0])[0];
}
+
+ private static void checkResult(Iterator<Resource> it, String...expected )
{
+ int pos = 0;
+ for (String key : expected) {
+ assertEquals("expects " + key + " at position " + pos, key,
getFirstValueOf(it.next(), PROPNAME));
+ pos += 1;
+ }
+ assertFalse(it.hasNext());
+ }
}
