This is an automated email from the ASF dual-hosted git repository.
magibney pushed a commit to branch branch_9_1
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9_1 by this push:
new 8093e782eee SOLR-16585: Fix NPE in MatchAllDocs pagination (#1236)
8093e782eee is described below
commit 8093e782eeef212f2d978aaf79e2a8d0aacba6bb
Author: Michael Gibney <[email protected]>
AuthorDate: Wed Dec 21 09:57:11 2022 -0500
SOLR-16585: Fix NPE in MatchAllDocs pagination (#1236)
(cherry picked from commit ced26f7132a4162dd7eaa96de2c87712bd8525fa)
---
solr/CHANGES.txt | 2 +
.../org/apache/solr/search/SolrIndexSearcher.java | 20 +++++----
.../collection1/conf/solrconfig-deeppaging.xml | 2 +-
.../apache/solr/search/TestMainQueryCaching.java | 50 +++++++++++++++++++---
4 files changed, 60 insertions(+), 14 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index bb24d8be9a7..13f18294b68 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -11,6 +11,8 @@ Bug Fixes
* SOLR-16589: Large fields with large=true can be truncated when using unicode
values (Kevin Risden)
+* SOLR-16585: Fixed NPE when paginating MatchAllDocs with non-zero start
offset, like q=*:*&start=10 (Michael Gibney)
+
================== 9.1.0 ==================
New Features
diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
index 219384d4ea6..de6302e6198 100644
--- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
+++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
@@ -2240,15 +2240,19 @@ public class SolrIndexSearcher extends IndexSearcher
implements Closeable, SolrI
private DocList constantScoreDocList(int offset, int length, DocSet docs) {
final int size = docs.size();
- if (length == 0 || size <= offset) {
- return new DocSlice(0, 0, new int[0], null, size, 0f,
TotalHits.Relation.EQUAL_TO);
- }
- final DocIterator iter = docs.iterator();
- for (int i = offset; i > 0; i--) {
- iter.nextDoc(); // discard
- }
- final int returnSize = Math.min(length, size - offset);
+
+ // NOTE: it would be possible to special-case `length == 0 || size <=
offset` here
+ // (returning a DocList backed by an empty array) -- but the cases that
would practically
+ // benefit from doing so would be extremely unusual, and likely
pathological:
+ // 1. length==0 in conjunction with offset>0 (why?)
+ // 2. specifying offset>size (paging beyond end of results)
+ // This would require special consideration in dealing with cache handling
(and generation
+ // of the final DocList via `DocSlice.subset(int, int)`), and it's just
not worth it.
+
+ final int returnSize = Math.min(offset + length, size);
final int[] docIds = new int[returnSize];
+
+ final DocIterator iter = docs.iterator();
for (int i = 0; i < returnSize; i++) {
docIds[i] = iter.nextDoc();
}
diff --git
a/solr/core/src/test-files/solr/collection1/conf/solrconfig-deeppaging.xml
b/solr/core/src/test-files/solr/collection1/conf/solrconfig-deeppaging.xml
index d4b501736fe..981b02f2b31 100644
--- a/solr/core/src/test-files/solr/collection1/conf/solrconfig-deeppaging.xml
+++ b/solr/core/src/test-files/solr/collection1/conf/solrconfig-deeppaging.xml
@@ -50,7 +50,7 @@
<!-- no autowarming, it screws up our ability to sanity check cache stats
in tests -->
<filterCache size="50" initialSize="50" autowarmCount="0"/>
<queryResultCache size="50" initialSize="50" autowarmCount="0"/>
- <queryResultWindowSize>50</queryResultWindowSize>
+
<queryResultWindowSize>${solr.test.queryResultWindowSize:50}</queryResultWindowSize>
<queryResultMaxDocsCached>500</queryResultMaxDocsCached>
<!-- randomized so we exercise cursors using various paths in
SolrIndexSearcher -->
<useFilterForSortedQuery>${solr.test.useFilterForSortedQuery}</useFilterForSortedQuery>
diff --git
a/solr/core/src/test/org/apache/solr/search/TestMainQueryCaching.java
b/solr/core/src/test/org/apache/solr/search/TestMainQueryCaching.java
index 99b6a14f9d1..8a298be2cb1 100644
--- a/solr/core/src/test/org/apache/solr/search/TestMainQueryCaching.java
+++ b/solr/core/src/test/org/apache/solr/search/TestMainQueryCaching.java
@@ -19,6 +19,7 @@ package org.apache.solr.search;
import static org.apache.solr.common.util.Utils.fromJSONString;
import java.util.Map;
+import java.util.Random;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
@@ -40,6 +41,8 @@ public class TestMainQueryCaching extends SolrTestCaseJ4 {
private static final int ALL_DOCS = MOST_DOCS + 1;
private static final String TEST_UFFSQ_PROPNAME =
"solr.test.useFilterForSortedQuery";
static String RESTORE_UFFSQ_PROP;
+ private static final String TEST_QRC_WINDOW_SIZE_PROPNAME =
"solr.test.queryResultWindowSize";
+ static String RESTORE_QRC_WINDOW_SIZE_PROP;
static boolean USE_FILTER_FOR_SORTED_QUERY;
@BeforeClass
@@ -47,6 +50,7 @@ public class TestMainQueryCaching extends SolrTestCaseJ4 {
USE_FILTER_FOR_SORTED_QUERY = random().nextBoolean();
RESTORE_UFFSQ_PROP =
System.setProperty(TEST_UFFSQ_PROPNAME,
Boolean.toString(USE_FILTER_FOR_SORTED_QUERY));
+ RESTORE_QRC_WINDOW_SIZE_PROP =
System.setProperty(TEST_QRC_WINDOW_SIZE_PROPNAME, "0");
initCore("solrconfig-deeppaging.xml", "schema-sorts.xml");
createIndex();
}
@@ -58,6 +62,11 @@ public class TestMainQueryCaching extends SolrTestCaseJ4 {
} else {
System.setProperty(TEST_UFFSQ_PROPNAME, RESTORE_UFFSQ_PROP);
}
+ if (RESTORE_QRC_WINDOW_SIZE_PROP == null) {
+ System.clearProperty(TEST_QRC_WINDOW_SIZE_PROPNAME);
+ } else {
+ System.setProperty(TEST_QRC_WINDOW_SIZE_PROPNAME,
RESTORE_QRC_WINDOW_SIZE_PROP);
+ }
}
public static void createIndex() {
@@ -79,14 +88,14 @@ public class TestMainQueryCaching extends SolrTestCaseJ4 {
h.reload();
}
- private static long coreToInserts(SolrCore core) {
+ private static long coreToInserts(SolrCore core, String cacheName) {
return (long)
((MetricsMap)
((SolrMetricManager.GaugeWrapper<?>)
core.getCoreMetricManager()
.getRegistry()
.getMetrics()
- .get("CACHE.searcher.filterCache"))
+ .get("CACHE.searcher.".concat(cacheName)))
.getGauge())
.getValue()
.get("inserts");
@@ -232,8 +241,36 @@ public class TestMainQueryCaching extends SolrTestCaseJ4 {
@Test
public void testMatchAllDocsPlain() throws Exception {
// plain request with "score" sort should skip sort even if `rows`
requested
- String response = JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true"));
- assertMetricCounts(response, true, 0, 0, 1);
+ Random r = random();
+ int[] counters = new int[2];
+ for (int i = 1; i < ALL_DOCS + 10; i++) {
+ // gradually expand the offset, otherwise we'd find that
queryResultCache would intercept
+ // requests, avoiding exercising the behavior we're most interested in
+ String offset = Integer.toString(r.nextInt(i));
+ // keep the window small (again, to avoid queryResultCache intercepting
requests)
+ String rows = Integer.toString(r.nextInt(2));
+ String response =
+ JQ(req("q", MATCH_ALL_DOCS_QUERY, "indent", "true", "start", offset,
"rows", rows));
+ assertMetricCounts(response, counters);
+ }
+ }
+
+ private static void assertMetricCounts(String response, int[]
expectCounters) {
+ Map<?, ?> res = (Map<?, ?>) fromJSONString(response);
+ Map<?, ?> body = (Map<?, ?>) (res.get("response"));
+ SolrCore core = h.getCore();
+ assertEquals("Bad matchAllDocs insert count", 1,
coreToMatchAllDocsInsertCount(core));
+ assertEquals("Bad filterCache insert count", 0, coreToInserts(core,
"filterCache"));
+ assertEquals("Bad full sort count", 0, coreToSortCount(core, "full"));
+ assertEquals("Should have exactly " + ALL_DOCS, ALL_DOCS, (long)
(body.get("numFound")));
+ long queryCacheInsertCount = coreToInserts(core, "queryResultCache");
+ if (queryCacheInsertCount == expectCounters[0]) {
+ // should be a hit, so all insert/sort-count metrics remain unchanged.
+ } else {
+ assertEquals(++expectCounters[0], queryCacheInsertCount);
+ expectCounters[1]++;
+ }
+ assertEquals("Bad skip sort count", expectCounters[1],
coreToSortCount(core, "skip"));
}
@Test
@@ -375,7 +412,10 @@ public class TestMainQueryCaching extends SolrTestCaseJ4 {
"Bad matchAllDocs insert count",
(matchAllDocs ? 1 : 0),
coreToMatchAllDocsInsertCount(core));
- assertEquals("Bad filterCache insert count", expectFilterCacheInsertCount,
coreToInserts(core));
+ assertEquals(
+ "Bad filterCache insert count",
+ expectFilterCacheInsertCount,
+ coreToInserts(core, "filterCache"));
assertEquals("Bad full sort count", expectFullSortCount,
coreToSortCount(core, "full"));
assertEquals("Bad skip sort count", expectSkipSortCount,
coreToSortCount(core, "skip"));
assertEquals(
