snoopdave commented on code in PR #249:
URL: https://github.com/apache/roller/pull/249#discussion_r4571035658
##########
app/src/test/java/org/apache/roller/weblogger/business/search/lucene/SearchHitPagingTest.java:
##########
@@ -49,11 +51,24 @@ class SearchHitPagingTest {
private static final int HITS = 20;
@Test
- void firstPageLoadsOnlyItsEntries() throws Exception {
+ void firstPageLoadsItsEntriesAndOneMore() throws Exception {
WeblogEntryManager entries = entries(-1);
SearchResultList result = convert(entries, 0, 2);
assertEquals(2, result.getResults().size());
- verify(entries, times(2)).getWeblogEntry(anyString());
+ // the third load is the look ahead that says whether there is a next
page
+ verify(entries, times(3)).getWeblogEntry(anyString());
+ assertTrue(result.hasMore());
+ assertEquals(HITS, result.getTotalHits());
+ }
+
+ @Test
+ void lastPageHasNothingAfterIt() throws Exception {
Review Comment:
🐞Claude Issue: **Important:** Nothing in these tests covers the main change
in this PR, removing the 500-match limit. `SearchHitPagingTest` builds 20 mock
hits and `IndexManagerTest` indexes two entries, so both would pass with
`docLimit = 500` restored. Please add a test that indexes more than 500
matching entries (an in-memory Lucene index is enough, with `SearchOperation`
run against it). It should assert that `getResultsCount()` is above 500 and
that a page past entry 500 is returned.
##########
app/src/main/java/org/apache/roller/weblogger/business/search/lucene/SearchOperation.java:
##########
@@ -141,7 +140,9 @@ public void doRun() {
.build();
}
- searchresults = searcher.search(query, docLimit, SORTER);
+ // ask for every match, so that results past the first few hundred
+ // can still be paged to
+ searchresults = searcher.search(query, Math.max(1,
reader.maxDoc()), SORTER);
Review Comment:
🐞Claude Issue: **Important:** `reader.maxDoc()` as the top-N is a
request-sized cost on a public, unauthenticated endpoint.
`IndexSearcher.search(query, n, sort)` sizes its hit queue from `n` before it
collects anything. Each search now allocates and sorts for every match, and the
cost scales with the whole index, which holds every weblog on a multi-weblog
install. The old limit of 500 bounded it. Repeated searches for a common word
will do this work each time.
Suggest a bounded top-N that is still far above 500 (for example a cap of a
few thousand, as a named constant or a `roller.properties` setting). Take the
true total from `searchresults.totalHits` (`getResultsCount()`), which
`search(query, n, sort)` counts exactly, instead of from `hits.length`. Then
the page can still show the real count, and only the pageable depth is capped.
The PR text says results past 500 can be reached. A cap of a few thousand would
keep that true for nearly any site.
##########
app/src/main/java/org/apache/roller/weblogger/business/search/lucene/LuceneIndexManager.java:
##########
@@ -513,7 +514,9 @@ static SearchResultList convertHitsToEntryList(
categories = categorySet;
}
- return new SearchResultList(results, categories, limit, offset);
+ boolean more = visible.size() > offset + limit;
+ return new SearchResultList(results, categories, limit, offset,
+ hits.length, more);
Review Comment:
🐞Claude Issue: **Important:** `hits.length` counts index hits, not entries
the visitor can see. The loop above drops hits whose entry is a draft, a future
post, deleted, or not found because the index lags. The page can therefore say
"1,017 entries found" while paging reaches fewer. The PR's own comment notes
that the index can lag behind the entries.
The visible count can't be known without loading every entry, so either word
the count as matches ("1,017 matches") or note the limit in the Javadoc of
`getTotalHits()` and in the PR. Also add a test with a hidden hit in the middle
that asserts what `getTotalHits()` returns. `SearchHitPagingTest` has a draft
case, but it does not assert the total.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
