From bb72876f0b05a957ba3718bca668c82ed01dd073 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 6 Oct 2026 17:56:54 -0500 Subject: [PATCH 1/5] fix(#16): page search results by accessible hits With record locks on, offset now skips accessible results, read in doubling batches from position 0, and per-table mode honors offset in each table. Every OpenSearch request keeps from + size within the 10,000 result window. Closes #16 --- .../actions/QuickSearchAction.java | 184 +++++++++++------- .../quicksearch/actions/QuickSearchInput.java | 5 +- .../OpenSearchIntegrationTest.java | 13 ++ .../QuickSearchActionSecurityTest.java | 152 ++++++++++++++- 4 files changed, 277 insertions(+), 77 deletions(-) diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java index aeb2dfc..aebd649 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java @@ -67,6 +67,13 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** limit is capped at maxSearchLimit and offset + limit at OpenSearch's result ** window. tableNames restricts the search; limitPerTable searches each table ** separately and returns up to that many hits per table. + ** + ** offset counts results the caller can see. Without locks it is passed + ** straight to OpenSearch. With locks, hits are read from position 0 in + ** batches that double in size, filtered, the first offset accessible results + ** are skipped and limit are kept, so consecutive pages neither overlap nor + ** skip an accessible hit. No request reads past MAX_RESULT_WINDOW (10,000 + ** raw hits), so accessible results beyond it are not reachable. *******************************************************************************/ public class QuickSearchAction { @@ -118,11 +125,7 @@ public class QuickSearchAction int limit = (input.getLimit() == null || input.getLimit() <= 0) ? DEFAULT_LIMIT : Math.min(input.getLimit(), maxLimit); int offset = (input.getOffset() == null || input.getOffset() < 0) ? 0 : input.getOffset(); - if(offset + limit > QuickSearchOpenSearchClient.MAX_RESULT_WINDOW) - { - limit = Math.max(0, QuickSearchOpenSearchClient.MAX_RESULT_WINDOW - offset); - } - if(limit == 0) + if(offset >= QuickSearchOpenSearchClient.MAX_RESULT_WINDOW) { return (emptyOutput()); } @@ -139,56 +142,20 @@ public class QuickSearchAction if(input.getLimitPerTable() != null && input.getLimitPerTable() > 0) { - return (searchPerTable(client, normalizedTerm, allowedTables, Math.min(input.getLimitPerTable(), maxLimit), tableConfigs, applyLocks)); + return (searchPerTable(client, normalizedTerm, allowedTables, offset, Math.min(input.getLimitPerTable(), maxLimit), tableConfigs, applyLocks)); } - int fetchSize = applyLocks ? Math.min(limit * 2, Math.max(limit, QuickSearchOpenSearchClient.MAX_RESULT_WINDOW - offset)) : limit; - - SearchResponse response = runSearch(client, normalizedTerm, allowedTables, fetchSize, offset, tableConfigs); - List results = toResults(response); - int rawHits = results.size(); - - if(applyLocks) - { - results = filterByRecordAccess(results, tableConfigs); - } - - boolean truncated = results.size() > limit; - if(truncated) - { - results = new ArrayList<>(results.subList(0, limit)); - } - - long totalHits = response.hits().total() != null ? response.hits().total().value() : results.size(); - boolean lowerBound = response.hits().total() != null && response.hits().total().relation() == TotalHitsRelation.Gte; - boolean hasMore = truncated || (offset + rawHits) < totalHits; - - if(applyLocks) - { - //////////////////////////////////////////////////////////////////////////// - // the raw total would reveal how many matching rows the session may not // - // see; report only what was verified accessible. hasMore still follows // - // the raw window: it says only that more candidates exist past it, so a // - // caller can page on to accessible rows instead of stopping short. // - //////////////////////////////////////////////////////////////////////////// - totalHits = offset + results.size() + (truncated ? 1 : 0); - lowerBound = truncated || hasMore; - hasMore = truncated || hasMore; - } - - return (new QuickSearchOutput() - .withResults(results) - .withTotalHits(totalHits) - .withTotalHitsIsLowerBound(lowerBound) - .withHasMore(hasMore)); + Page page = searchPage(client, normalizedTerm, allowedTables, offset, limit, tableConfigs, applyLocks); + return (page.toOutput()); } /******************************************************************************* - ** Search each allowed table separately, up to limitPerTable hits each. + ** Search each allowed table separately, skipping offset results and + ** returning up to limitPerTable hits in each table. *******************************************************************************/ - private QuickSearchOutput searchPerTable(QuickSearchOpenSearchClient client, String term, List allowedTables, int limitPerTable, List tableConfigs, boolean applyLocks) throws QException + private QuickSearchOutput searchPerTable(QuickSearchOpenSearchClient client, String term, List allowedTables, int offset, int limitPerTable, List tableConfigs, boolean applyLocks) throws QException { List results = new ArrayList<>(); long totalHits = 0; @@ -197,31 +164,11 @@ public class QuickSearchAction for(String tableName : allowedTables) { - int fetchSize = applyLocks ? limitPerTable * 2 : limitPerTable; - SearchResponse response = runSearch(client, term, List.of(tableName), fetchSize, 0, tableConfigs); - List tableHits = toResults(response); - - if(applyLocks) - { - tableHits = filterByRecordAccess(tableHits, tableConfigs); - lowerBound = true; - } - if(tableHits.size() > limitPerTable) - { - tableHits = new ArrayList<>(tableHits.subList(0, limitPerTable)); - hasMore = true; - } - - long tableTotal = response.hits().total() != null ? response.hits().total().value() : tableHits.size(); - if(applyLocks) - { - tableTotal = tableHits.size(); - } - totalHits += tableTotal; - hasMore |= tableHits.size() < tableTotal; - lowerBound |= response.hits().total() != null && response.hits().total().relation() == TotalHitsRelation.Gte; - - results.addAll(tableHits); + Page page = searchPage(client, term, List.of(tableName), offset, limitPerTable, tableConfigs, applyLocks); + results.addAll(page.results()); + totalHits += page.totalHits(); + hasMore |= page.hasMore(); + lowerBound |= page.lowerBound(); } return (new QuickSearchOutput().withResults(results).withTotalHits(totalHits).withTotalHitsIsLowerBound(lowerBound).withHasMore(hasMore)); @@ -229,6 +176,83 @@ public class QuickSearchAction + /******************************************************************************* + ** One page over the given tables: skip offset results, keep up to limit. + ** Without locks offset goes straight to OpenSearch; with locks offset and + ** limit count accessible results (see the class comment). Every request + ** keeps from + size within MAX_RESULT_WINDOW. + *******************************************************************************/ + private Page searchPage(QuickSearchOpenSearchClient client, String term, List tables, int offset, int limit, List tableConfigs, boolean applyLocks) throws QException + { + int window = QuickSearchOpenSearchClient.MAX_RESULT_WINDOW; + int pageSize = Math.min(limit, window - offset); + if(pageSize <= 0) + { + return (new Page(Collections.emptyList(), 0L, false, false)); + } + + if(!applyLocks) + { + SearchResponse response = runSearch(client, term, tables, pageSize, offset, tableConfigs); + List results = toResults(response); + int rawHits = results.size(); + + boolean truncated = results.size() > pageSize; + if(truncated) + { + results = new ArrayList<>(results.subList(0, pageSize)); + } + + long totalHits = response.hits().total() != null ? response.hits().total().value() : results.size(); + boolean lowerBound = response.hits().total() != null && response.hits().total().relation() == TotalHitsRelation.Gte; + return (new Page(results, totalHits, lowerBound, truncated || (offset + rawHits) < totalHits)); + } + + //////////////////////////////////////////////////////////////////////////// + // read candidates from position 0 until offset + pageSize accessible // + // results are known, the hits run out, or the result window is reached. // + // each batch over-reads 2x what is still needed and at least doubles the // + // last one, so a mostly-locked result set costs a bounded number of // + // round trips (about log2 of the window). // + //////////////////////////////////////////////////////////////////////////// + int target = offset + pageSize; + List accessible = new ArrayList<>(); + int position = 0; + int batchSize = 0; + boolean exhausted = false; + boolean rawFloor = false; + + while(accessible.size() < target && !exhausted && position < window) + { + batchSize = Math.min(window - position, Math.max(2 * (target - accessible.size()), 2 * batchSize)); + + SearchResponse response = runSearch(client, term, tables, batchSize, position, tableConfigs); + int rawHits = response.hits().hits().size(); + long rawTotal = response.hits().total() != null ? response.hits().total().value() : position + rawHits; + rawFloor = response.hits().total() != null && response.hits().total().relation() == TotalHitsRelation.Gte; + + accessible.addAll(filterByRecordAccess(toResults(response), tableConfigs)); + position += rawHits; + exhausted = rawHits < batchSize || position >= rawTotal; + } + + //////////////////////////////////////////////////////////////////////////// + // the raw total would reveal how many matching rows the session may not // + // see; report only what was verified accessible. hasMore says only that // + // more candidates exist, so a caller can page on to accessible rows. // + //////////////////////////////////////////////////////////////////////////// + boolean truncated = accessible.size() > target; + boolean hasMore = truncated || (!exhausted && position < window); + + List results = accessible.size() > offset + ? new ArrayList<>(accessible.subList(offset, Math.min(target, accessible.size()))) + : new ArrayList<>(); + + return (new Page(results, (long) accessible.size(), hasMore || rawFloor, hasMore)); + } + + + /******************************************************************************* ** Requested tables (tableNames, else tableName, else all configured) that ** are configured, enabled, and readable by the session. @@ -407,6 +431,22 @@ public class QuickSearchAction } + /******************************************************************************* + ** One page of results with its total and paging flags. + *******************************************************************************/ + private record Page(List results, Long totalHits, Boolean lowerBound, Boolean hasMore) + { + /******************************************************************************* + ** + *******************************************************************************/ + QuickSearchOutput toOutput() + { + return (new QuickSearchOutput().withResults(results).withTotalHits(totalHits).withTotalHitsIsLowerBound(lowerBound).withHasMore(hasMore)); + } + } + + + /******************************************************************************* ** *******************************************************************************/ diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java index c12bf92..d716514 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java @@ -122,7 +122,10 @@ public class QuickSearchInput extends AbstractActionInput /******************************************************************************* - ** Getter for offset. + ** Getter for offset: the number of results to skip, counting only results + ** the session can see (with record security locks on, hits filtered out + ** by a lock are not counted). With limitPerTable it applies within each + ** table. Results are reachable only within the first 10,000 raw hits. *******************************************************************************/ public Integer getOffset() { diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java index f5f40fe..34fd220 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java @@ -329,6 +329,19 @@ class OpenSearchIntegrationTest List page2Ids = page2.getResults().stream().map(r -> r.getRecordId()).toList(); assertThat(page1Ids).doesNotContainAnyElementsOf(page2Ids); + + //////////////////////////////////////////////////// + // per-table mode honors offset the same way // + //////////////////////////////////////////////////// + QuickSearchOutput perTable1 = new QuickSearchAction().execute( + new QuickSearchInput().withSearchTerm("widget").withLimitPerTable(2).withOffset(0)); + QuickSearchOutput perTable2 = new QuickSearchAction().execute( + new QuickSearchInput().withSearchTerm("widget").withLimitPerTable(2).withOffset(2)); + + List perTable1Ids = perTable1.getResults().stream().map(r -> r.getTableName() + ":" + r.getRecordId()).toList(); + List perTable2Ids = perTable2.getResults().stream().map(r -> r.getTableName() + ":" + r.getRecordId()).toList(); + + assertThat(perTable1Ids).doesNotContainAnyElementsOf(perTable2Ids); } } diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java index 8e28805..6bff0ea 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java @@ -17,8 +17,10 @@ package com.kingsrook.qbits.quicksearch.actions; +import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.stream.IntStream; import com.kingsrook.qqq.backend.core.actions.permissions.PermissionsHelper; import com.kingsrook.qqq.backend.core.actions.permissions.TablePermissionSubType; import com.kingsrook.qqq.backend.core.actions.tables.InsertAction; @@ -29,11 +31,13 @@ import com.kingsrook.qqq.backend.core.model.data.QRecord; import com.kingsrook.qqq.backend.core.model.metadata.permissions.PermissionLevel; import com.kingsrook.qqq.backend.core.model.metadata.permissions.QPermissionRules; import com.kingsrook.qbits.quicksearch.BaseQuickSearchTest; +import com.kingsrook.qbits.quicksearch.QuickSearchQBitConfig; import com.kingsrook.qbits.quicksearch.QuickSearchQBitContext; import com.kingsrook.qbits.quicksearch.opensearch.OpenSearchDocument; import com.kingsrook.qbits.quicksearch.opensearch.QuickSearchOpenSearchClient; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.opensearch.client.opensearch.core.SearchResponse; import org.opensearch.client.opensearch.core.search.Hit; import org.opensearch.client.opensearch.core.search.HitsMetadata; @@ -46,6 +50,7 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -257,22 +262,161 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest /******************************************************************************* - ** Test: limitPerTable with locks on reports the filtered count per table - ** and flags the total as a lower bound. + ** Test: limitPerTable with locks on reports the filtered count per table; + ** with every candidate read, the count is exact. *******************************************************************************/ @Test void testLimitPerTable_withLocks_totalIsFilteredCount() throws QException { insertEntities(1); - stubSearch(List.of(hitFor("1"), hitFor("2")), 9L); + stubSearch(List.of(hitFor("1"), hitFor("2")), 2L); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withLimitPerTable(5)); verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(10), eq(0), anyList()); assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1"); assertThat(output.getTotalHits()).isEqualTo(1L); - assertThat(output.getTotalHitsIsLowerBound()).isTrue(); + assertThat(output.getTotalHitsIsLowerBound()).isFalse(); assertThat(output.getHasMore()).isFalse(); } + + + /******************************************************************************* + ** Stub the client search over an ordered list of testEntity record ids: + ** each call returns the slice at its from/size, as OpenSearch would. + ** Fails the call if from + size passes the result window. + *******************************************************************************/ + private void stubPagedSearch(List recordIds) throws QException + { + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenAnswer(invocation -> + { + int size = invocation.getArgument(2); + int from = invocation.getArgument(3); + assertThat(from + size).isLessThanOrEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); + + List> hits = new ArrayList<>(); + for(int i = from; i < Math.min(recordIds.size(), from + size); i++) + { + hits.add(hitFor(recordIds.get(i))); + } + return (buildMockResponse(hits, recordIds.size())); + }); + } + + + + /******************************************************************************* + ** Run one search page and return its output. + *******************************************************************************/ + private QuickSearchOutput page(Integer offset, Integer limit) throws QException + { + return (action.execute(new QuickSearchInput().withSearchTerm("entity").withOffset(offset).withLimit(limit))); + } + + + + /******************************************************************************* + ** Test: with locks on, offset counts accessible results, so consecutive + ** pages neither overlap nor skip an accessible hit when the post-filter + ** removes hits, and hasMore is false only on the last page. + *******************************************************************************/ + @Test + void testRecordLock_pagesByAccessibleResults_noOverlapNoSkip() throws QException + { + insertEntities(1, 3, 5, 7, 9); + stubPagedSearch(IntStream.rangeClosed(1, 10).mapToObj(String::valueOf).toList()); + + QuickSearchOutput page1 = page(0, 2); + QuickSearchOutput page2 = page(2, 2); + QuickSearchOutput page3 = page(4, 2); + + assertThat(page1.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1", "3"); + assertThat(page2.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("5", "7"); + assertThat(page3.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("9"); + assertThat(page1.getHasMore()).isTrue(); + assertThat(page2.getHasMore()).isTrue(); + assertThat(page3.getHasMore()).isFalse(); + assertThat(page3.getTotalHits()).isEqualTo(5L); + assertThat(page3.getTotalHitsIsLowerBound()).isFalse(); + } + + + + /******************************************************************************* + ** Test: an offset past the last accessible result returns an empty page + ** with hasMore false. + *******************************************************************************/ + @Test + void testRecordLock_offsetPastAccessibleResults_emptyPage() throws QException + { + insertEntities(1, 3); + stubPagedSearch(IntStream.rangeClosed(1, 10).mapToObj(String::valueOf).toList()); + + QuickSearchOutput output = page(5, 2); + + assertThat(output.getResults()).isEmpty(); + assertThat(output.getHasMore()).isFalse(); + assertThat(output.getTotalHits()).isEqualTo(2L); + } + + + + /******************************************************************************* + ** Test: per-table mode skips offset accessible results in each table. + *******************************************************************************/ + @Test + void testLimitPerTable_withLocks_honorsOffsetPerTable() throws QException + { + insertEntities(1, 3, 5, 7, 9); + stubPagedSearch(IntStream.rangeClosed(1, 10).mapToObj(String::valueOf).toList()); + + QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withOffset(2).withLimitPerTable(2)); + + assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("5", "7"); + assertThat(output.getHasMore()).isTrue(); + } + + + + /******************************************************************************* + ** Test: per-table mode without locks passes offset straight to OpenSearch. + *******************************************************************************/ + @Test + void testLimitPerTable_locksDisabled_passesOffset() throws QException + { + QuickSearchQBitContext.getConfig().withApplyRecordSecurityLocks(false); + stubPagedSearch(IntStream.rangeClosed(1, 10).mapToObj(String::valueOf).toList()); + + QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withOffset(8).withLimitPerTable(5)); + + verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(5), eq(8), anyList()); + assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("9", "10"); + assertThat(output.getHasMore()).isFalse(); + } + + + + /******************************************************************************* + ** Test: with a maxSearchLimit above half the result window, no request + ** asks OpenSearch for more than the window (from + size at most 10,000). + *******************************************************************************/ + @Test + void testLargeMaxSearchLimit_requestsCappedAtResultWindow() throws QException + { + QuickSearchQBitConfig config = QuickSearchQBitContext.getConfig().withMaxSearchLimit(8_000); + stubPagedSearch(IntStream.rangeClosed(1, 20).mapToObj(String::valueOf).toList()); + + action.execute(new QuickSearchInput().withSearchTerm("entity").withLimitPerTable(8_000)); + action.execute(new QuickSearchInput().withSearchTerm("entity").withLimit(8_000)); + config.withApplyRecordSecurityLocks(false); + action.execute(new QuickSearchInput().withSearchTerm("entity").withLimitPerTable(8_000).withOffset(5_000)); + + ArgumentCaptor sizes = ArgumentCaptor.forClass(Integer.class); + ArgumentCaptor offsets = ArgumentCaptor.forClass(Integer.class); + verify(mockClient, times(3)).search(anyString(), any(), sizes.capture(), offsets.capture(), anyList()); + assertThat(sizes.getAllValues()).containsExactly(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW, QuickSearchOpenSearchClient.MAX_RESULT_WINDOW, 5_000); + assertThat(offsets.getAllValues()).containsExactly(0, 0, 5_000); + } + } -- 2.54.0 From 2bc9ac48aec231b53a9214519f024ab28709e80f Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 6 Oct 2026 18:03:23 -0500 Subject: [PATCH 2/5] fix(#16): bound the locked search scan per request One search now shares a budget of 10,000 raw hits and 32 OpenSearch round trips across every table it scans, so locked-out hits cannot amplify work. Refs #16 --- .../actions/QuickSearchAction.java | 66 ++++++++++++--- .../QuickSearchActionSecurityTest.java | 82 +++++++++++++++++++ 2 files changed, 137 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java index aebd649..82bddff 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java @@ -74,6 +74,12 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** are skipped and limit are kept, so consecutive pages neither overlap nor ** skip an accessible hit. No request reads past MAX_RESULT_WINDOW (10,000 ** raw hits), so accessible results beyond it are not reachable. + ** + ** With locks, one execute call has a hard budget shared by every table it + ** searches (per-table mode included): at most MAX_RESULT_WINDOW raw hits + ** fetched in total and at most MAX_LOCKED_ROUND_TRIPS OpenSearch requests, + ** each followed by one QueryAction per table in the batch. When the budget + ** runs out the scan stops and reports hasMore and a lower-bound total. *******************************************************************************/ public class QuickSearchAction { @@ -84,6 +90,8 @@ public class QuickSearchAction public static final int MAX_TERM_LENGTH = 100; public static final int MAX_LIMIT_PER_TABLE = 100; + public static final int MAX_LOCKED_ROUND_TRIPS = 32; + /******************************************************************************* @@ -145,7 +153,7 @@ public class QuickSearchAction return (searchPerTable(client, normalizedTerm, allowedTables, offset, Math.min(input.getLimitPerTable(), maxLimit), tableConfigs, applyLocks)); } - Page page = searchPage(client, normalizedTerm, allowedTables, offset, limit, tableConfigs, applyLocks); + Page page = searchPage(client, normalizedTerm, allowedTables, offset, limit, tableConfigs, applyLocks, new ScanBudget()); return (page.toOutput()); } @@ -161,10 +169,11 @@ public class QuickSearchAction long totalHits = 0; boolean hasMore = false; boolean lowerBound = false; + ScanBudget budget = new ScanBudget(); for(String tableName : allowedTables) { - Page page = searchPage(client, term, List.of(tableName), offset, limitPerTable, tableConfigs, applyLocks); + Page page = searchPage(client, term, List.of(tableName), offset, limitPerTable, tableConfigs, applyLocks, budget); results.addAll(page.results()); totalHits += page.totalHits(); hasMore |= page.hasMore(); @@ -179,10 +188,11 @@ public class QuickSearchAction /******************************************************************************* ** One page over the given tables: skip offset results, keep up to limit. ** Without locks offset goes straight to OpenSearch; with locks offset and - ** limit count accessible results (see the class comment). Every request - ** keeps from + size within MAX_RESULT_WINDOW. + ** limit count accessible results (see the class comment), and the locked + ** scan draws on budget. Every request keeps from + size within + ** MAX_RESULT_WINDOW. *******************************************************************************/ - private Page searchPage(QuickSearchOpenSearchClient client, String term, List tables, int offset, int limit, List tableConfigs, boolean applyLocks) throws QException + private Page searchPage(QuickSearchOpenSearchClient client, String term, List tables, int offset, int limit, List tableConfigs, boolean applyLocks, ScanBudget budget) throws QException { int window = QuickSearchOpenSearchClient.MAX_RESULT_WINDOW; int pageSize = Math.min(limit, window - offset); @@ -210,10 +220,10 @@ public class QuickSearchAction //////////////////////////////////////////////////////////////////////////// // read candidates from position 0 until offset + pageSize accessible // - // results are known, the hits run out, or the result window is reached. // - // each batch over-reads 2x what is still needed and at least doubles the // - // last one, so a mostly-locked result set costs a bounded number of // - // round trips (about log2 of the window). // + // results are known, the hits run out, the result window is reached, or // + // the request's budget is spent. each batch over-reads 2x what is still // + // needed and at least doubles the last one, so even a mostly-locked // + // result set takes about log2 of the window round trips. // //////////////////////////////////////////////////////////////////////////// int target = offset + pageSize; List accessible = new ArrayList<>(); @@ -222,9 +232,9 @@ public class QuickSearchAction boolean exhausted = false; boolean rawFloor = false; - while(accessible.size() < target && !exhausted && position < window) + while(accessible.size() < target && !exhausted && position < window && budget.hasRemaining()) { - batchSize = Math.min(window - position, Math.max(2 * (target - accessible.size()), 2 * batchSize)); + batchSize = Math.min(Math.min(window - position, budget.remainingHits), Math.max(2 * (target - accessible.size()), 2 * batchSize)); SearchResponse response = runSearch(client, term, tables, batchSize, position, tableConfigs); int rawHits = response.hits().hits().size(); @@ -233,6 +243,7 @@ public class QuickSearchAction accessible.addAll(filterByRecordAccess(toResults(response), tableConfigs)); position += rawHits; + budget.spend(rawHits); exhausted = rawHits < batchSize || position >= rawTotal; } @@ -431,6 +442,39 @@ public class QuickSearchAction } + /******************************************************************************* + ** Work budget for the locked scans of one execute call: raw hits fetched + ** and OpenSearch round trips, shared by every table searched. + *******************************************************************************/ + private static final class ScanBudget + { + private Integer remainingHits = QuickSearchOpenSearchClient.MAX_RESULT_WINDOW; + private Integer remainingRoundTrips = MAX_LOCKED_ROUND_TRIPS; + + + + /******************************************************************************* + ** + *******************************************************************************/ + boolean hasRemaining() + { + return (remainingHits > 0 && remainingRoundTrips > 0); + } + + + + /******************************************************************************* + ** Record one round trip that fetched the given number of raw hits. + *******************************************************************************/ + void spend(int rawHits) + { + remainingHits -= rawHits; + remainingRoundTrips--; + } + } + + + /******************************************************************************* ** One page of results with its total and paging flags. *******************************************************************************/ diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java index 6bff0ea..a99f073 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java @@ -28,11 +28,15 @@ import com.kingsrook.qqq.backend.core.context.QContext; import com.kingsrook.qqq.backend.core.exceptions.QException; import com.kingsrook.qqq.backend.core.model.actions.tables.insert.InsertInput; import com.kingsrook.qqq.backend.core.model.data.QRecord; +import com.kingsrook.qqq.backend.core.model.metadata.fields.QFieldMetaData; +import com.kingsrook.qqq.backend.core.model.metadata.fields.QFieldType; import com.kingsrook.qqq.backend.core.model.metadata.permissions.PermissionLevel; import com.kingsrook.qqq.backend.core.model.metadata.permissions.QPermissionRules; +import com.kingsrook.qqq.backend.core.model.metadata.tables.QTableMetaData; import com.kingsrook.qbits.quicksearch.BaseQuickSearchTest; import com.kingsrook.qbits.quicksearch.QuickSearchQBitConfig; import com.kingsrook.qbits.quicksearch.QuickSearchQBitContext; +import com.kingsrook.qbits.quicksearch.QuickSearchableTableConfig; import com.kingsrook.qbits.quicksearch.opensearch.OpenSearchDocument; import com.kingsrook.qbits.quicksearch.opensearch.QuickSearchOpenSearchClient; import org.junit.jupiter.api.BeforeEach; @@ -48,6 +52,8 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -67,6 +73,7 @@ import static org.mockito.Mockito.when; class QuickSearchActionSecurityTest extends BaseQuickSearchTest { private static final String TABLE_LABEL = "Test Entity"; + private static final String OTHER_TABLE = "otherEntity"; private QuickSearchOpenSearchClient mockClient; private QuickSearchAction action; @@ -419,4 +426,79 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest assertThat(offsets.getAllValues()).containsExactly(0, 0, 5_000); } + + + /******************************************************************************* + ** Assert the searches since the last clear stayed within one request's + ** budget and read up to, but not past, the result window. + *******************************************************************************/ + private void assertScanWithinBudget() throws QException + { + ArgumentCaptor sizes = ArgumentCaptor.forClass(Integer.class); + ArgumentCaptor offsets = ArgumentCaptor.forClass(Integer.class); + verify(mockClient, atLeastOnce()).search(anyString(), any(), sizes.capture(), offsets.capture(), anyList()); + + List sizeValues = sizes.getAllValues(); + List offsetValues = offsets.getAllValues(); + assertThat(sizeValues.size()).isLessThanOrEqualTo(QuickSearchAction.MAX_LOCKED_ROUND_TRIPS); + assertThat(sizeValues.stream().mapToInt(Integer::intValue).sum()).isEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); + assertThat(offsetValues.get(offsetValues.size() - 1) + sizeValues.get(sizeValues.size() - 1)).isEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); + } + + + + /******************************************************************************* + ** Test: when every hit is filtered out by a lock, one search stops at the + ** request budget: at most MAX_RESULT_WINDOW raw hits and + ** MAX_LOCKED_ROUND_TRIPS requests, in shared and per-table mode, with an + ** empty page and an honest hasMore. + *******************************************************************************/ + @Test + void testRecordLock_allHitsFiltered_workIsBounded() throws QException + { + stubPagedSearch(IntStream.rangeClosed(1, 50_000).mapToObj(String::valueOf).toList()); + + QuickSearchOutput shared = action.execute(new QuickSearchInput().withSearchTerm("entity").withLimit(1)); + assertScanWithinBudget(); + clearInvocations(mockClient); + + QuickSearchOutput perTable = action.execute(new QuickSearchInput().withSearchTerm("entity").withOffset(3).withLimitPerTable(1)); + assertScanWithinBudget(); + + assertThat(shared.getResults()).isEmpty(); + assertThat(perTable.getResults()).isEmpty(); + assertThat(shared.getTotalHits()).isEqualTo(0L); + assertThat(shared.getHasMore()).isFalse(); + assertThat(perTable.getHasMore()).isFalse(); + } + + + + /******************************************************************************* + ** Test: per-table mode shares one budget across tables, so a second fully + ** locked table is not scanned once the first has spent the window, and + ** hasMore says results may remain. + *******************************************************************************/ + @Test + void testLimitPerTable_withLocks_budgetSharedAcrossTables() throws QException + { + QContext.getQInstance().addTable(new QTableMetaData() + .withName(OTHER_TABLE) + .withBackendName(TEST_BACKEND_NAME) + .withPrimaryKeyField("id") + .withField(new QFieldMetaData("id", QFieldType.INTEGER))); + QuickSearchQBitContext.setDiscoveredTables(List.of( + new QuickSearchableTableConfig().withTableName(TEST_ENTITY_TABLE).withPrimaryKeyField("id").withEnabledByDefault(true), + new QuickSearchableTableConfig().withTableName(OTHER_TABLE).withPrimaryKeyField("id").withEnabledByDefault(true))); + stubPagedSearch(IntStream.rangeClosed(1, 50_000).mapToObj(String::valueOf).toList()); + + QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withTableNames(List.of(TEST_ENTITY_TABLE, OTHER_TABLE)).withLimitPerTable(1)); + + assertScanWithinBudget(); + verify(mockClient, never()).search(anyString(), eq(List.of(OTHER_TABLE)), anyInt(), anyInt(), anyList()); + assertThat(output.getResults()).isEmpty(); + assertThat(output.getHasMore()).isTrue(); + assertThat(output.getTotalHitsIsLowerBound()).isTrue(); + } + } -- 2.54.0 From 4b85a77bbeb59bb05fdda0dca5deecffeec32132 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 6 Oct 2026 18:16:25 -0500 Subject: [PATCH 3/5] fix(#16): give each table its own scan budget and cap batches Each locked scan now has its own budget of 10,000 raw hits in batches of at most 1,000, deduplicates hits and pins batches to one shard copy, and reports an exact total once every hit is read. Refs #16 --- README.md | 2 + .../actions/QuickSearchAction.java | 90 ++++---- .../QuickSearchOpenSearchClient.java | 14 +- .../OpenSearchIntegrationTest.java | 1 + .../QuickSearchActionSecurityTest.java | 199 +++++++++++++++--- .../actions/QuickSearchActionTest.java | 99 +++++---- 6 files changed, 284 insertions(+), 121 deletions(-) diff --git a/README.md b/README.md index b8499f8..5d93093 100644 --- a/README.md +++ b/README.md @@ -120,6 +120,8 @@ output.getTotalHits(); output.getTotalHitsIsLowerBound(); output.getHasMore(); `limitPerTable` returns up to that many hits from each table instead of one ranked list. Terms shorter than 2 characters return nothing; longer than 100 characters are rejected. `limit` is capped at `maxSearchLimit` and `offset + limit` at 10,000. +`offset` skips N accessible results: with record security locks on, hits a lock hides are not counted, so consecutive pages never overlap or skip a visible hit, and with `limitPerTable` the offset applies within each table. With locks on, `totalHits` counts hits confirmed accessible so far; `totalHitsIsLowerBound` is true when the scan stopped before reading every candidate (page full, result window, or scan budget). Each scan reads at most 10,000 raw hits in batches of at most 1,000, so one request reads at most 10,000 raw hits per searched table. + Search runs as the current `QContext` session: tables the session cannot read are skipped, and with `applyRecordSecurityLocks` (default) hits are re-read through `QueryAction` so record security locks apply. ### Processes diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java index 82bddff..3ca40a0 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java @@ -26,6 +26,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; import com.kingsrook.qqq.backend.core.actions.permissions.PermissionsHelper; import com.kingsrook.qqq.backend.core.actions.permissions.TablePermissionSubType; import com.kingsrook.qqq.backend.core.actions.tables.QueryAction; @@ -75,11 +76,17 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** skip an accessible hit. No request reads past MAX_RESULT_WINDOW (10,000 ** raw hits), so accessible results beyond it are not reachable. ** - ** With locks, one execute call has a hard budget shared by every table it - ** searches (per-table mode included): at most MAX_RESULT_WINDOW raw hits - ** fetched in total and at most MAX_LOCKED_ROUND_TRIPS OpenSearch requests, - ** each followed by one QueryAction per table in the batch. When the budget - ** runs out the scan stops and reports hasMore and a lower-bound total. + ** With locks, each scan (the whole search, or one table in per-table mode) + ** has a hard budget: at most MAX_RESULT_WINDOW raw hits, in batches of at + ** most MAX_LOCKED_BATCH_SIZE, over at most MAX_LOCKED_ROUND_TRIPS OpenSearch + ** requests, each followed by one QueryAction of at most + ** MAX_LOCKED_BATCH_SIZE ids per table in the batch. One request therefore + ** reads at most (searched tables) x MAX_RESULT_WINDOW raw hits however + ** large offset or limit is, and a fully locked table cannot starve another. + ** When a budget runs out the scan stops and reports hasMore and a + ** lower-bound total. Hits are deduplicated by table and record id within a + ** call; batches are separate from/size requests without a tiebreak sort, so + ** a hit that moves between batches during a refresh can still be missed. *******************************************************************************/ public class QuickSearchAction { @@ -91,6 +98,7 @@ public class QuickSearchAction public static final int MAX_LIMIT_PER_TABLE = 100; public static final int MAX_LOCKED_ROUND_TRIPS = 32; + public static final int MAX_LOCKED_BATCH_SIZE = 1_000; @@ -153,7 +161,7 @@ public class QuickSearchAction return (searchPerTable(client, normalizedTerm, allowedTables, offset, Math.min(input.getLimitPerTable(), maxLimit), tableConfigs, applyLocks)); } - Page page = searchPage(client, normalizedTerm, allowedTables, offset, limit, tableConfigs, applyLocks, new ScanBudget()); + Page page = searchPage(client, normalizedTerm, allowedTables, offset, limit, tableConfigs, applyLocks); return (page.toOutput()); } @@ -169,11 +177,10 @@ public class QuickSearchAction long totalHits = 0; boolean hasMore = false; boolean lowerBound = false; - ScanBudget budget = new ScanBudget(); for(String tableName : allowedTables) { - Page page = searchPage(client, term, List.of(tableName), offset, limitPerTable, tableConfigs, applyLocks, budget); + Page page = searchPage(client, term, List.of(tableName), offset, limitPerTable, tableConfigs, applyLocks); results.addAll(page.results()); totalHits += page.totalHits(); hasMore |= page.hasMore(); @@ -188,11 +195,10 @@ public class QuickSearchAction /******************************************************************************* ** One page over the given tables: skip offset results, keep up to limit. ** Without locks offset goes straight to OpenSearch; with locks offset and - ** limit count accessible results (see the class comment), and the locked - ** scan draws on budget. Every request keeps from + size within - ** MAX_RESULT_WINDOW. + ** limit count accessible results within one scan budget (see the class + ** comment). Every request keeps from + size within MAX_RESULT_WINDOW. *******************************************************************************/ - private Page searchPage(QuickSearchOpenSearchClient client, String term, List tables, int offset, int limit, List tableConfigs, boolean applyLocks, ScanBudget budget) throws QException + private Page searchPage(QuickSearchOpenSearchClient client, String term, List tables, int offset, int limit, List tableConfigs, boolean applyLocks) throws QException { int window = QuickSearchOpenSearchClient.MAX_RESULT_WINDOW; int pageSize = Math.min(limit, window - offset); @@ -203,27 +209,20 @@ public class QuickSearchAction if(!applyLocks) { - SearchResponse response = runSearch(client, term, tables, pageSize, offset, tableConfigs); + SearchResponse response = runSearch(client, term, tables, pageSize, offset, tableConfigs, null); List results = toResults(response); - int rawHits = results.size(); - - boolean truncated = results.size() > pageSize; - if(truncated) - { - results = new ArrayList<>(results.subList(0, pageSize)); - } long totalHits = response.hits().total() != null ? response.hits().total().value() : results.size(); boolean lowerBound = response.hits().total() != null && response.hits().total().relation() == TotalHitsRelation.Gte; - return (new Page(results, totalHits, lowerBound, truncated || (offset + rawHits) < totalHits)); + return (new Page(results, totalHits, lowerBound, (offset + results.size()) < totalHits)); } //////////////////////////////////////////////////////////////////////////// // read candidates from position 0 until offset + pageSize accessible // // results are known, the hits run out, the result window is reached, or // - // the request's budget is spent. each batch over-reads 2x what is still // - // needed and at least doubles the last one, so even a mostly-locked // - // result set takes about log2 of the window round trips. // + // the scan budget is spent. each batch over-reads 2x what is still // + // needed and at least doubles the last one, up to MAX_LOCKED_BATCH_SIZE, // + // so a fully locked table reaches the window in 19 round trips. // //////////////////////////////////////////////////////////////////////////// int target = offset + pageSize; List accessible = new ArrayList<>(); @@ -231,17 +230,28 @@ public class QuickSearchAction int batchSize = 0; boolean exhausted = false; boolean rawFloor = false; + ScanBudget budget = new ScanBudget(); + Set seen = new HashSet<>(); + String preference = "quick-search-" + UUID.randomUUID(); while(accessible.size() < target && !exhausted && position < window && budget.hasRemaining()) { - batchSize = Math.min(Math.min(window - position, budget.remainingHits), Math.max(2 * (target - accessible.size()), 2 * batchSize)); + batchSize = Math.min(Math.min(window - position, budget.remainingHits), Math.min(MAX_LOCKED_BATCH_SIZE, Math.max(2 * (target - accessible.size()), 2 * batchSize))); - SearchResponse response = runSearch(client, term, tables, batchSize, position, tableConfigs); + SearchResponse response = runSearch(client, term, tables, batchSize, position, tableConfigs, preference); int rawHits = response.hits().hits().size(); long rawTotal = response.hits().total() != null ? response.hits().total().value() : position + rawHits; rawFloor = response.hits().total() != null && response.hits().total().relation() == TotalHitsRelation.Gte; - accessible.addAll(filterByRecordAccess(toResults(response), tableConfigs)); + List candidates = new ArrayList<>(); + for(QuickSearchResult result : toResults(response)) + { + if(seen.add(result.getTableName() + ":" + result.getRecordId())) + { + candidates.add(result); + } + } + accessible.addAll(filterByRecordAccess(candidates, tableConfigs)); position += rawHits; budget.spend(rawHits); exhausted = rawHits < batchSize || position >= rawTotal; @@ -259,7 +269,7 @@ public class QuickSearchAction ? new ArrayList<>(accessible.subList(offset, Math.min(target, accessible.size()))) : new ArrayList<>(); - return (new Page(results, (long) accessible.size(), hasMore || rawFloor, hasMore)); + return (new Page(results, (long) accessible.size(), !exhausted || rawFloor, hasMore)); } @@ -348,14 +358,17 @@ public class QuickSearchAction ids.add(result.getRecordId()); } - QueryInput queryInput = new QueryInput(); - queryInput.setTableName(table.getName()); - queryInput.setFilter(new QQueryFilter(new QFilterCriteria(table.getPrimaryKeyField(), QCriteriaOperator.IN, ids))); - Set found = new HashSet<>(); - for(QRecord record : CollectionUtils.nonNullList(new QueryAction().execute(queryInput).getRecords())) + for(int start = 0; start < ids.size(); start += MAX_LOCKED_BATCH_SIZE) { - found.add(record.getValueString(table.getPrimaryKeyField())); + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(table.getName()); + queryInput.setFilter(new QQueryFilter(new QFilterCriteria(table.getPrimaryKeyField(), QCriteriaOperator.IN, ids.subList(start, Math.min(ids.size(), start + MAX_LOCKED_BATCH_SIZE))))); + + for(QRecord record : CollectionUtils.nonNullList(new QueryAction().execute(queryInput).getRecords())) + { + found.add(record.getValueString(table.getPrimaryKeyField())); + } } for(QuickSearchResult result : entry.getValue()) @@ -429,11 +442,11 @@ public class QuickSearchAction ** Run one search, surfacing an OpenSearch client error as a QException so ** callers never see an unchecked OpenSearchException. *******************************************************************************/ - private static SearchResponse runSearch(QuickSearchOpenSearchClient client, String term, List tables, int fetchSize, int offset, List tableConfigs) throws QException + private static SearchResponse runSearch(QuickSearchOpenSearchClient client, String term, List tables, int fetchSize, int offset, List tableConfigs, String preference) throws QException { try { - return (client.search(term, tables, fetchSize, offset, tableConfigs)); + return (client.search(term, tables, fetchSize, offset, tableConfigs, preference)); } catch(OpenSearchException e) { @@ -442,9 +455,10 @@ public class QuickSearchAction } + /******************************************************************************* - ** Work budget for the locked scans of one execute call: raw hits fetched - ** and OpenSearch round trips, shared by every table searched. + ** Work budget for one locked scan: raw hits fetched and OpenSearch round + ** trips. *******************************************************************************/ private static final class ScanBudget { diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/QuickSearchOpenSearchClient.java b/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/QuickSearchOpenSearchClient.java index 8e99828..df93a3a 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/QuickSearchOpenSearchClient.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/QuickSearchOpenSearchClient.java @@ -638,6 +638,17 @@ public class QuickSearchOpenSearchClient implements Closeable ** in allowedTables are searched; an empty set returns no hits. *******************************************************************************/ public SearchResponse search(String searchTerm, Collection allowedTables, int limit, int offset, List tableConfigs) throws QException + { + return (search(searchTerm, allowedTables, limit, offset, tableConfigs, null)); + } + + + + /******************************************************************************* + ** As search above; a non-null preference routes every request that shares + ** it to the same shard copies, so batches of one scan see one ordering. + *******************************************************************************/ + public SearchResponse search(String searchTerm, Collection allowedTables, int limit, int offset, List tableConfigs, String preference) throws QException { try { @@ -657,7 +668,8 @@ public class QuickSearchOpenSearchClient implements Closeable .highlight(highlight) .trackTotalHits(t -> t.count(MAX_RESULT_WINDOW)) .from(offset) - .size(limit)); + .size(limit) + .preference(preference)); return (client.search(request, OpenSearchDocument.class)); } diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java index 34fd220..ab17806 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/OpenSearchIntegrationTest.java @@ -341,6 +341,7 @@ class OpenSearchIntegrationTest List perTable1Ids = perTable1.getResults().stream().map(r -> r.getTableName() + ":" + r.getRecordId()).toList(); List perTable2Ids = perTable2.getResults().stream().map(r -> r.getTableName() + ":" + r.getRecordId()).toList(); + assertThat(perTable2Ids).isNotEmpty(); assertThat(perTable1Ids).doesNotContainAnyElementsOf(perTable2Ids); } } diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java index a99f073..c51332a 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java @@ -18,6 +18,7 @@ package com.kingsrook.qbits.quicksearch.actions; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.stream.IntStream; @@ -46,6 +47,7 @@ import org.opensearch.client.opensearch.core.SearchResponse; import org.opensearch.client.opensearch.core.search.Hit; import org.opensearch.client.opensearch.core.search.HitsMetadata; import org.opensearch.client.opensearch.core.search.TotalHits; +import org.opensearch.client.opensearch.core.search.TotalHitsRelation; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; @@ -119,7 +121,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest private void stubSearch(List> hits, long totalHits) throws QException { SearchResponse response = buildMockResponse(hits, totalHits); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); } @@ -128,9 +130,19 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest ** Build a mocked hit for a testEntity record. *******************************************************************************/ private Hit hitFor(String recordId) + { + return (hitFor(TEST_ENTITY_TABLE, recordId)); + } + + + + /******************************************************************************* + ** Build a mocked hit for a record of the given table. + *******************************************************************************/ + private Hit hitFor(String tableName, String recordId) { Hit hit = mock(Hit.class); - when(hit.source()).thenReturn(new OpenSearchDocument().withSourceTable(TEST_ENTITY_TABLE).withRecordId(recordId).withRecordLabel("Entity " + recordId)); + when(hit.source()).thenReturn(new OpenSearchDocument().withSourceTable(tableName).withRecordId(recordId).withRecordLabel("Entity " + recordId)); when(hit.score()).thenReturn(1.0); when(hit.highlight()).thenReturn(Map.of()); return (hit); @@ -163,7 +175,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity")); - verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(QuickSearchAction.DEFAULT_LIMIT * 2), eq(0), anyList()); + verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(QuickSearchAction.DEFAULT_LIMIT * 2), eq(0), anyList(), any()); assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1"); assertThat(output.getTotalHits()).isEqualTo(1L); assertThat(output.getTotalHitsIsLowerBound()).isFalse(); @@ -173,8 +185,8 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest /******************************************************************************* - ** Test: accessible hits beyond the limit are truncated and the total is - ** reported as a lower bound. + ** Test: accessible hits beyond the limit are truncated; every hit was read, + ** so the total is exact. *******************************************************************************/ @Test void testRecordLock_moreAccessibleHitsThanLimit_truncatedWithLowerBoundTotal() throws QException @@ -184,10 +196,10 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withLimit(2)); - verify(mockClient).search(anyString(), any(), eq(4), eq(0), anyList()); + verify(mockClient).search(anyString(), any(), eq(4), eq(0), anyList(), any()); assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1", "2"); assertThat(output.getTotalHits()).isEqualTo(3L); - assertThat(output.getTotalHitsIsLowerBound()).isTrue(); + assertThat(output.getTotalHitsIsLowerBound()).isFalse(); assertThat(output.getHasMore()).isTrue(); } @@ -205,7 +217,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withLimit(10)); - verify(mockClient).search(anyString(), any(), eq(10), eq(0), anyList()); + verify(mockClient).search(anyString(), any(), eq(10), eq(0), anyList(), any()); assertThat(output.getResults()).hasSize(2); assertThat(output.getTotalHits()).isEqualTo(40L); assertThat(output.getHasMore()).isTrue(); @@ -226,7 +238,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest assertThat(output.getResults()).isEmpty(); assertThat(output.getTotalHits()).isEqualTo(0L); - verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList()); + verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); } @@ -280,7 +292,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withLimitPerTable(5)); - verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(10), eq(0), anyList()); + verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(10), eq(0), anyList(), any()); assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1"); assertThat(output.getTotalHits()).isEqualTo(1L); assertThat(output.getTotalHitsIsLowerBound()).isFalse(); @@ -296,18 +308,38 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest *******************************************************************************/ private void stubPagedSearch(List recordIds) throws QException { - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenAnswer(invocation -> + stubPagedSearch(Map.of(TEST_ENTITY_TABLE, recordIds), false); + } + + + + /******************************************************************************* + ** Paged stub over per-table id lists (a request naming several tables + ** reads the test entity list); totalIsFloor reports the total as Gte. + *******************************************************************************/ + private void stubPagedSearch(Map> idsByTable, boolean totalIsFloor) throws QException + { + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenAnswer(invocation -> { - int size = invocation.getArgument(2); - int from = invocation.getArgument(3); + List tables = new ArrayList<>(invocation.>getArgument(1)); + String table = tables.size() == 1 ? tables.get(0) : TEST_ENTITY_TABLE; + List ids = idsByTable.getOrDefault(table, List.of()); + int size = invocation.getArgument(2); + int from = invocation.getArgument(3); assertThat(from + size).isLessThanOrEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); List> hits = new ArrayList<>(); - for(int i = from; i < Math.min(recordIds.size(), from + size); i++) + for(int i = from; i < Math.min(ids.size(), from + size); i++) { - hits.add(hitFor(recordIds.get(i))); + hits.add(hitFor(table, ids.get(i))); } - return (buildMockResponse(hits, recordIds.size())); + + SearchResponse response = buildMockResponse(hits, ids.size()); + if(totalIsFloor) + { + when(response.hits().total().relation()).thenReturn(TotalHitsRelation.Gte); + } + return (response); }); } @@ -397,7 +429,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withOffset(8).withLimitPerTable(5)); - verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(5), eq(8), anyList()); + verify(mockClient).search(anyString(), eq(List.of(TEST_ENTITY_TABLE)), eq(5), eq(8), anyList(), any()); assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("9", "10"); assertThat(output.getHasMore()).isFalse(); } @@ -406,7 +438,8 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest /******************************************************************************* ** Test: with a maxSearchLimit above half the result window, no request - ** asks OpenSearch for more than the window (from + size at most 10,000). + ** asks OpenSearch for more than the window (from + size at most 10,000), + ** and a locked batch is at most MAX_LOCKED_BATCH_SIZE. *******************************************************************************/ @Test void testLargeMaxSearchLimit_requestsCappedAtResultWindow() throws QException @@ -421,8 +454,8 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest ArgumentCaptor sizes = ArgumentCaptor.forClass(Integer.class); ArgumentCaptor offsets = ArgumentCaptor.forClass(Integer.class); - verify(mockClient, times(3)).search(anyString(), any(), sizes.capture(), offsets.capture(), anyList()); - assertThat(sizes.getAllValues()).containsExactly(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW, QuickSearchOpenSearchClient.MAX_RESULT_WINDOW, 5_000); + verify(mockClient, times(3)).search(anyString(), any(), sizes.capture(), offsets.capture(), anyList(), any()); + assertThat(sizes.getAllValues()).containsExactly(QuickSearchAction.MAX_LOCKED_BATCH_SIZE, QuickSearchAction.MAX_LOCKED_BATCH_SIZE, 5_000); assertThat(offsets.getAllValues()).containsExactly(0, 0, 5_000); } @@ -436,11 +469,12 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest { ArgumentCaptor sizes = ArgumentCaptor.forClass(Integer.class); ArgumentCaptor offsets = ArgumentCaptor.forClass(Integer.class); - verify(mockClient, atLeastOnce()).search(anyString(), any(), sizes.capture(), offsets.capture(), anyList()); + verify(mockClient, atLeastOnce()).search(anyString(), any(), sizes.capture(), offsets.capture(), anyList(), any()); List sizeValues = sizes.getAllValues(); List offsetValues = offsets.getAllValues(); assertThat(sizeValues.size()).isLessThanOrEqualTo(QuickSearchAction.MAX_LOCKED_ROUND_TRIPS); + assertThat(sizeValues).allMatch(size -> size <= QuickSearchAction.MAX_LOCKED_BATCH_SIZE); assertThat(sizeValues.stream().mapToInt(Integer::intValue).sum()).isEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); assertThat(offsetValues.get(offsetValues.size() - 1) + sizeValues.get(sizeValues.size() - 1)).isEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); } @@ -456,7 +490,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest @Test void testRecordLock_allHitsFiltered_workIsBounded() throws QException { - stubPagedSearch(IntStream.rangeClosed(1, 50_000).mapToObj(String::valueOf).toList()); + stubPagedSearch(Map.of(TEST_ENTITY_TABLE, IntStream.rangeClosed(1, 50_000).mapToObj(String::valueOf).toList()), true); QuickSearchOutput shared = action.execute(new QuickSearchInput().withSearchTerm("entity").withLimit(1)); assertScanWithinBudget(); @@ -469,36 +503,137 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest assertThat(perTable.getResults()).isEmpty(); assertThat(shared.getTotalHits()).isEqualTo(0L); assertThat(shared.getHasMore()).isFalse(); + assertThat(shared.getTotalHitsIsLowerBound()).isTrue(); assertThat(perTable.getHasMore()).isFalse(); } /******************************************************************************* - ** Test: per-table mode shares one budget across tables, so a second fully - ** locked table is not scanned once the first has spent the window, and - ** hasMore says results may remain. + ** Add a second memory table and make both quick-searchable. *******************************************************************************/ - @Test - void testLimitPerTable_withLocks_budgetSharedAcrossTables() throws QException + private void addOtherTable() { QContext.getQInstance().addTable(new QTableMetaData() .withName(OTHER_TABLE) .withBackendName(TEST_BACKEND_NAME) .withPrimaryKeyField("id") - .withField(new QFieldMetaData("id", QFieldType.INTEGER))); + .withField(new QFieldMetaData("id", QFieldType.INTEGER)) + .withField(new QFieldMetaData("name", QFieldType.STRING))); QuickSearchQBitContext.setDiscoveredTables(List.of( new QuickSearchableTableConfig().withTableName(TEST_ENTITY_TABLE).withPrimaryKeyField("id").withEnabledByDefault(true), new QuickSearchableTableConfig().withTableName(OTHER_TABLE).withPrimaryKeyField("id").withEnabledByDefault(true))); - stubPagedSearch(IntStream.rangeClosed(1, 50_000).mapToObj(String::valueOf).toList()); + } - QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withTableNames(List.of(TEST_ENTITY_TABLE, OTHER_TABLE)).withLimitPerTable(1)); - assertScanWithinBudget(); - verify(mockClient, never()).search(anyString(), eq(List.of(OTHER_TABLE)), anyInt(), anyInt(), anyList()); + + /******************************************************************************* + ** Test: in per-table mode each table has its own scan budget, so a table + ** with many hits all locked out cannot starve the next table, which still + ** returns its accessible hits. + *******************************************************************************/ + @Test + void testLimitPerTable_withLocks_lockedTableDoesNotStarveOthers() throws QException + { + addOtherTable(); + InsertInput insertInput = new InsertInput(); + insertInput.setTableName(OTHER_TABLE); + insertInput.setRecords(List.of(new QRecord().withValue("id", 1).withValue("name", "Other 1"), new QRecord().withValue("id", 2).withValue("name", "Other 2"))); + new InsertAction().execute(insertInput); + + stubPagedSearch(Map.of( + TEST_ENTITY_TABLE, IntStream.rangeClosed(1, 12_000).mapToObj(String::valueOf).toList(), + OTHER_TABLE, List.of("1", "2")), false); + + QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withTableNames(List.of(TEST_ENTITY_TABLE, OTHER_TABLE)).withLimitPerTable(5)); + + verify(mockClient, atLeastOnce()).search(anyString(), eq(List.of(OTHER_TABLE)), anyInt(), anyInt(), anyList(), any()); + assertThat(output.getResults()).extracting(QuickSearchResult::getTableName).containsOnly(OTHER_TABLE); + assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1", "2"); + assertThat(output.getTotalHits()).isEqualTo(2L); + } + + + + /******************************************************************************* + ** Test: with both tables fully locked, each table's scan stays within its + ** own budget, so the request's work is bounded by the number of tables. + *******************************************************************************/ + @Test + void testLimitPerTable_withLocks_eachTableHasOwnBudget() throws QException + { + addOtherTable(); + List ids = IntStream.rangeClosed(1, 50_000).mapToObj(String::valueOf).toList(); + stubPagedSearch(Map.of(TEST_ENTITY_TABLE, ids, OTHER_TABLE, ids), false); + + QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withTableNames(List.of(TEST_ENTITY_TABLE, OTHER_TABLE)).withOffset(5_000).withLimitPerTable(5)); + + for(String table : List.of(TEST_ENTITY_TABLE, OTHER_TABLE)) + { + ArgumentCaptor sizes = ArgumentCaptor.forClass(Integer.class); + verify(mockClient, atLeastOnce()).search(anyString(), eq(List.of(table)), sizes.capture(), anyInt(), anyList(), any()); + assertThat(sizes.getAllValues().size()).isLessThanOrEqualTo(QuickSearchAction.MAX_LOCKED_ROUND_TRIPS); + assertThat(sizes.getAllValues()).allMatch(size -> size <= QuickSearchAction.MAX_LOCKED_BATCH_SIZE); + assertThat(sizes.getAllValues().stream().mapToInt(Integer::intValue).sum()).isEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); + } assertThat(output.getResults()).isEmpty(); + assertThat(output.getHasMore()).isFalse(); + } + + + + /******************************************************************************* + ** Test: a deep offset under locks reads in capped batches rather than one + ** request (and one record-lock query) the size of the window. + *******************************************************************************/ + @Test + void testRecordLock_deepOffset_batchesCapped() throws QException + { + stubPagedSearch(IntStream.rangeClosed(1, 20_000).mapToObj(String::valueOf).toList()); + + action.execute(new QuickSearchInput().withSearchTerm("entity").withOffset(4_975).withLimit(25)); + + ArgumentCaptor sizes = ArgumentCaptor.forClass(Integer.class); + verify(mockClient, atLeastOnce()).search(anyString(), any(), sizes.capture(), anyInt(), anyList(), any()); + assertThat(sizes.getAllValues().get(0)).isEqualTo(QuickSearchAction.MAX_LOCKED_BATCH_SIZE); + assertThat(sizes.getAllValues()).allMatch(size -> size <= QuickSearchAction.MAX_LOCKED_BATCH_SIZE); + } + + + + /******************************************************************************* + ** Test: accessible hits found only in later batches fill the page, and + ** unread candidates keep hasMore true with a lower-bound total. + *******************************************************************************/ + @Test + void testRecordLock_accessibleHitsInLaterBatches() throws QException + { + insertEntities(60, 70, 80); + stubPagedSearch(IntStream.rangeClosed(1, 100).mapToObj(String::valueOf).toList()); + + QuickSearchOutput output = page(1, 2); + + verify(mockClient, times(4)).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); + assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("70", "80"); assertThat(output.getHasMore()).isTrue(); + assertThat(output.getTotalHits()).isEqualTo(3L); assertThat(output.getTotalHitsIsLowerBound()).isTrue(); } + + + /******************************************************************************* + ** Test: a hit returned by two batches appears once on the page. + *******************************************************************************/ + @Test + void testRecordLock_duplicateAcrossBatches_returnedOnce() throws QException + { + insertEntities(1, 2); + stubPagedSearch(List.of("9", "1", "1", "2")); + + QuickSearchOutput output = page(0, 2); + + assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1", "2"); + } + } diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionTest.java index 8bbb8ef..f7834df 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionTest.java @@ -201,7 +201,7 @@ class QuickSearchActionTest Hit hit = buildMockHit(doc, 1.5, Map.of("searchableText", List.of("Order 42"))); SearchResponse response = buildMockResponse(List.of(hit), 1L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchInput input = new QuickSearchInput() .withSearchTerm("42") @@ -237,7 +237,7 @@ class QuickSearchActionTest Hit hit = buildMockHit(doc, 1.0, Map.of("searchableText", List.of("fragment one", "fragment two"))); SearchResponse response = buildMockResponse(List.of(hit), 1L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("one")); @@ -260,7 +260,7 @@ class QuickSearchActionTest Hit hit = buildMockHit(doc, 0.9, null); SearchResponse response = buildMockResponse(List.of(hit), 1L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("order")); @@ -276,11 +276,11 @@ class QuickSearchActionTest void testNullLimit_defaultsTo25() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withLimit(null)); - verify(mockClient).search(anyString(), any(), eq(25), anyInt(), anyList()); + verify(mockClient).search(anyString(), any(), eq(25), anyInt(), anyList(), any()); } @@ -292,11 +292,11 @@ class QuickSearchActionTest void testZeroLimit_defaultsTo25() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withLimit(0)); - verify(mockClient).search(anyString(), any(), eq(25), anyInt(), anyList()); + verify(mockClient).search(anyString(), any(), eq(25), anyInt(), anyList(), any()); } @@ -308,11 +308,11 @@ class QuickSearchActionTest void testNegativeLimit_defaultsTo25() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withLimit(-5)); - verify(mockClient).search(anyString(), any(), eq(25), anyInt(), anyList()); + verify(mockClient).search(anyString(), any(), eq(25), anyInt(), anyList(), any()); } @@ -324,11 +324,11 @@ class QuickSearchActionTest void testNullOffset_defaultsTo0() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withOffset(null)); - verify(mockClient).search(anyString(), any(), anyInt(), eq(0), anyList()); + verify(mockClient).search(anyString(), any(), anyInt(), eq(0), anyList(), any()); } @@ -340,11 +340,11 @@ class QuickSearchActionTest void testNegativeOffset_defaultsTo0() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withOffset(-1)); - verify(mockClient).search(anyString(), any(), anyInt(), eq(0), anyList()); + verify(mockClient).search(anyString(), any(), anyInt(), eq(0), anyList(), any()); } @@ -362,7 +362,7 @@ class QuickSearchActionTest // 1 result, offset=0, totalHits=5 → hasMore = (0+1) < 5 = true SearchResponse response = buildMockResponse(hits, 5L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput() .withSearchTerm("order") @@ -392,7 +392,7 @@ class QuickSearchActionTest // 2 results, offset=3, totalHits=5 → hasMore = (3+2) < 5 = false SearchResponse response = buildMockResponse(hits, 5L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput() .withSearchTerm("order") @@ -412,13 +412,13 @@ class QuickSearchActionTest void testTableNameFilter_passedToClient() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput() .withSearchTerm("widget") .withTableName("orders")); - verify(mockClient).search(anyString(), eq(List.of("orders")), anyInt(), anyInt(), anyList()); + verify(mockClient).search(anyString(), eq(List.of("orders")), anyInt(), anyInt(), anyList(), any()); } @@ -430,13 +430,13 @@ class QuickSearchActionTest void testNullTableName_searchesAllConfiguredTables() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput() .withSearchTerm("widget") .withTableName(null)); - verify(mockClient).search(anyString(), anyCollection(), anyInt(), anyInt(), anyList()); + verify(mockClient).search(anyString(), anyCollection(), anyInt(), anyInt(), anyList(), any()); } @@ -449,12 +449,12 @@ class QuickSearchActionTest void testSearchTerm_isNormalizedBeforeClientCall() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm(" HELLO WORLD ")); ArgumentCaptor termCaptor = ArgumentCaptor.forClass(String.class); - verify(mockClient).search(termCaptor.capture(), any(), anyInt(), anyInt(), anyList()); + verify(mockClient).search(termCaptor.capture(), any(), anyInt(), anyInt(), anyList(), any()); assertThat(termCaptor.getValue()).isEqualTo("HELLO WORLD"); } @@ -472,7 +472,7 @@ class QuickSearchActionTest assertThat(output.getResults()).isEmpty(); assertThat(output.getTotalHits()).isEqualTo(0L); - verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList()); + verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); } @@ -489,7 +489,7 @@ class QuickSearchActionTest .isInstanceOf(QException.class) .hasMessageContaining("too long"); - verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList()); + verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); } @@ -501,11 +501,11 @@ class QuickSearchActionTest void testTermOfExactly100Characters_accepted() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("x".repeat(QuickSearchAction.MAX_TERM_LENGTH))); - verify(mockClient).search(anyString(), any(), anyInt(), anyInt(), anyList()); + verify(mockClient).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); } @@ -518,11 +518,11 @@ class QuickSearchActionTest { QuickSearchQBitContext.setConfig(new QuickSearchQBitConfig().withApplyRecordSecurityLocks(false).withMaxSearchLimit(40)); SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withLimit(500)); - verify(mockClient).search(anyString(), any(), eq(40), anyInt(), anyList()); + verify(mockClient).search(anyString(), any(), eq(40), anyInt(), anyList(), any()); } @@ -535,14 +535,14 @@ class QuickSearchActionTest { QuickSearchQBitContext.setConfig(null); SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withLimit(500)); //////////////////////////////////////////////////////////////////////// // no config means locks default to on, so the fetch size is doubled // //////////////////////////////////////////////////////////////////////// - verify(mockClient).search(anyString(), any(), eq(200), anyInt(), anyList()); + verify(mockClient).search(anyString(), any(), eq(200), anyInt(), anyList(), any()); } @@ -555,11 +555,11 @@ class QuickSearchActionTest void testOffsetPlusLimit_beyondResultWindow_limitShrunk() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("test").withLimit(25).withOffset(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW - 10)); - verify(mockClient).search(anyString(), any(), eq(10), eq(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW - 10), anyList()); + verify(mockClient).search(anyString(), any(), eq(10), eq(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW - 10), anyList(), any()); } @@ -575,7 +575,7 @@ class QuickSearchActionTest assertThat(output.getResults()).isEmpty(); assertThat(output.getHasMore()).isFalse(); - verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList()); + verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); } @@ -587,11 +587,11 @@ class QuickSearchActionTest void testTableNames_intersectedWithConfiguredTables() throws QException { SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("widget").withTableNames(List.of("orders", "notConfigured"))); - verify(mockClient).search(anyString(), eq(List.of("orders")), anyInt(), anyInt(), anyList()); + verify(mockClient).search(anyString(), eq(List.of("orders")), anyInt(), anyInt(), anyList(), any()); } @@ -606,11 +606,11 @@ class QuickSearchActionTest new QuickSearchableTableConfig().withTableName("orders").withPrimaryKeyField("id").withSearchableFields(List.of("orderNumber")), new QuickSearchableTableConfig().withTableName("customers").withPrimaryKeyField("id").withSearchableFields(List.of("name")))); SearchResponse response = buildMockResponse(Collections.emptyList(), 0L); - when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); action.execute(new QuickSearchInput().withSearchTerm("widget").withTableName("orders").withTableNames(List.of("customers"))); - verify(mockClient).search(anyString(), eq(List.of("customers")), anyInt(), anyInt(), anyList()); + verify(mockClient).search(anyString(), eq(List.of("customers")), anyInt(), anyInt(), anyList(), any()); } @@ -625,7 +625,7 @@ class QuickSearchActionTest QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("widget").withTableNames(List.of("notConfigured"))); assertThat(output.getResults()).isEmpty(); - verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList()); + verify(mockClient, never()).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); } @@ -646,13 +646,13 @@ class QuickSearchActionTest SearchResponse orderResponse = buildMockResponse(List.of(orderHit), 7L); SearchResponse customerResponse = buildMockResponse(List.of(customerHit), 1L); - when(mockClient.search(anyString(), eq(List.of("orders")), anyInt(), anyInt(), anyList())).thenReturn(orderResponse); - when(mockClient.search(anyString(), eq(List.of("customers")), anyInt(), anyInt(), anyList())).thenReturn(customerResponse); + when(mockClient.search(anyString(), eq(List.of("orders")), anyInt(), anyInt(), anyList(), any())).thenReturn(orderResponse); + when(mockClient.search(anyString(), eq(List.of("customers")), anyInt(), anyInt(), anyList(), any())).thenReturn(customerResponse); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("widget").withLimitPerTable(3)); - verify(mockClient).search(anyString(), eq(List.of("orders")), eq(3), eq(0), anyList()); - verify(mockClient).search(anyString(), eq(List.of("customers")), eq(3), eq(0), anyList()); + verify(mockClient).search(anyString(), eq(List.of("orders")), eq(3), eq(0), anyList(), any()); + verify(mockClient).search(anyString(), eq(List.of("customers")), eq(3), eq(0), anyList(), any()); assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1", "9"); assertThat(output.getTotalHits()).isEqualTo(8L); @@ -665,20 +665,19 @@ class QuickSearchActionTest ** limitPerTable is capped at maxSearchLimit and trims each table's hits. *******************************************************************************/ @Test - void testLimitPerTable_aboveMax_clampedAndHitsTrimmed() throws QException + void testLimitPerTable_aboveMax_clamped() throws QException { QuickSearchQBitContext.setConfig(new QuickSearchQBitConfig().withApplyRecordSecurityLocks(false).withMaxSearchLimit(2)); List> hits = List.of( buildMockHit(new OpenSearchDocument().withSourceTable("orders").withRecordId("1"), 3.0, null), - buildMockHit(new OpenSearchDocument().withSourceTable("orders").withRecordId("2"), 2.0, null), - buildMockHit(new OpenSearchDocument().withSourceTable("orders").withRecordId("3"), 1.0, null)); + buildMockHit(new OpenSearchDocument().withSourceTable("orders").withRecordId("2"), 2.0, null)); SearchResponse response = buildMockResponse(hits, 3L); - when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), anyCollection(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("widget").withLimitPerTable(50)); - verify(mockClient).search(anyString(), eq(List.of("orders")), eq(2), eq(0), anyList()); + verify(mockClient).search(anyString(), eq(List.of("orders")), eq(2), eq(0), anyList(), any()); assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1", "2"); assertThat(output.getHasMore()).isTrue(); } @@ -694,7 +693,7 @@ class QuickSearchActionTest { Hit hit = buildMockHit(new OpenSearchDocument().withSourceTable("orders").withRecordId("1"), 1.0, null); SearchResponse response = buildMockResponse(List.of(hit), 1L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("widget")); @@ -711,7 +710,7 @@ class QuickSearchActionTest { SearchResponse response = buildMockResponse(Collections.emptyList(), 10000L); when(response.hits().total().relation()).thenReturn(TotalHitsRelation.Gte); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("widget")); @@ -728,7 +727,7 @@ class QuickSearchActionTest { Hit hit = buildMockHit(null, 1.0, null); SearchResponse response = buildMockResponse(List.of(hit), 1L); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenReturn(response); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenReturn(response); QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("widget")); @@ -761,7 +760,7 @@ class QuickSearchActionTest OpenSearchException openSearchException = new OpenSearchException(ErrorResponse.of(e -> e .status(500) .error(c -> c.type("search_phase_execution_exception").reason("all shards failed")))); - when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList())).thenThrow(openSearchException); + when(mockClient.search(anyString(), any(), anyInt(), anyInt(), anyList(), any())).thenThrow(openSearchException); assertThatThrownBy(() -> action.execute(new QuickSearchInput().withSearchTerm("widget"))) .isInstanceOf(QException.class) -- 2.54.0 From 8ec173f66cb32c834e63365b61d0d69a865a7bb8 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 6 Oct 2026 18:22:12 -0500 Subject: [PATCH 4/5] fix(#16): share the per-table scan budget across tables Each of T tables now gets an equal share of the scan budget, so one call reads at most max(10,000, T x 2 x limitPerTable) raw hits over max(32, 2T) requests. Refs #16 --- README.md | 2 +- .../actions/QuickSearchAction.java | 51 +++++++++++++------ .../quicksearch/actions/QuickSearchInput.java | 5 +- .../QuickSearchActionSecurityTest.java | 17 ++++--- 4 files changed, 51 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 5d93093..9d9d2ba 100644 --- a/README.md +++ b/README.md @@ -120,7 +120,7 @@ output.getTotalHits(); output.getTotalHitsIsLowerBound(); output.getHasMore(); `limitPerTable` returns up to that many hits from each table instead of one ranked list. Terms shorter than 2 characters return nothing; longer than 100 characters are rejected. `limit` is capped at `maxSearchLimit` and `offset + limit` at 10,000. -`offset` skips N accessible results: with record security locks on, hits a lock hides are not counted, so consecutive pages never overlap or skip a visible hit, and with `limitPerTable` the offset applies within each table. With locks on, `totalHits` counts hits confirmed accessible so far; `totalHitsIsLowerBound` is true when the scan stopped before reading every candidate (page full, result window, or scan budget). Each scan reads at most 10,000 raw hits in batches of at most 1,000, so one request reads at most 10,000 raw hits per searched table. +`offset` skips N accessible results: with record security locks on, hits a lock hides are not counted, so consecutive pages never overlap or skip a visible hit, and with `limitPerTable` the offset applies within each table. With locks on, `totalHits` counts hits confirmed accessible so far; `totalHitsIsLowerBound` is true when the scan stopped before reading every candidate (page full, result window, or scan budget). A single-list search reads at most 10,000 raw hits in batches of at most 1,000. In per-table mode the T tables share that budget, each getting max(10,000 / T, 2 x `limitPerTable`) hits, so one call reads at most max(10,000, T x 2 x `limitPerTable`) raw hits; deep pages of a heavily locked table may then be unreachable, and `hasMore` and `totalHitsIsLowerBound` stay true. Search runs as the current `QContext` session: tables the session cannot read are skipped, and with `applyRecordSecurityLocks` (default) hits are re-read through `QueryAction` so record security locks apply. diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java index 3ca40a0..a6cb3fb 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java @@ -76,15 +76,19 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** skip an accessible hit. No request reads past MAX_RESULT_WINDOW (10,000 ** raw hits), so accessible results beyond it are not reachable. ** - ** With locks, each scan (the whole search, or one table in per-table mode) - ** has a hard budget: at most MAX_RESULT_WINDOW raw hits, in batches of at - ** most MAX_LOCKED_BATCH_SIZE, over at most MAX_LOCKED_ROUND_TRIPS OpenSearch - ** requests, each followed by one QueryAction of at most - ** MAX_LOCKED_BATCH_SIZE ids per table in the batch. One request therefore - ** reads at most (searched tables) x MAX_RESULT_WINDOW raw hits however - ** large offset or limit is, and a fully locked table cannot starve another. + ** With locks, each scan has a hard budget of raw hits, read in batches of + ** at most MAX_LOCKED_BATCH_SIZE, and of OpenSearch requests, each followed + ** by one QueryAction per table in the batch. A single-list search gets + ** MAX_RESULT_WINDOW hits and MAX_LOCKED_ROUND_TRIPS requests. In per-table + ** mode each of the T tables gets its own equal share, + ** max(MAX_RESULT_WINDOW / T, min(MAX_RESULT_WINDOW, 2 x limitPerTable)) hits + ** and max(2, MAX_LOCKED_ROUND_TRIPS / T) requests, so a fully locked table + ** cannot starve another and one call reads at most + ** max(MAX_RESULT_WINDOW, T x 2 x limitPerTable) raw hits over at most + ** max(MAX_LOCKED_ROUND_TRIPS, 2 x T) requests; offset never raises it. ** When a budget runs out the scan stops and reports hasMore and a - ** lower-bound total. Hits are deduplicated by table and record id within a + ** lower-bound total, so deep pages of a heavily locked table may be + ** unreachable in per-table mode. Hits are deduplicated by table and record id within a ** call; batches are separate from/size requests without a tiebreak sort, so ** a hit that moves between batches during a refresh can still be missed. *******************************************************************************/ @@ -161,7 +165,7 @@ public class QuickSearchAction return (searchPerTable(client, normalizedTerm, allowedTables, offset, Math.min(input.getLimitPerTable(), maxLimit), tableConfigs, applyLocks)); } - Page page = searchPage(client, normalizedTerm, allowedTables, offset, limit, tableConfigs, applyLocks); + Page page = searchPage(client, normalizedTerm, allowedTables, offset, limit, tableConfigs, applyLocks, new ScanBudget(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW, MAX_LOCKED_ROUND_TRIPS)); return (page.toOutput()); } @@ -178,9 +182,13 @@ public class QuickSearchAction boolean hasMore = false; boolean lowerBound = false; + int window = QuickSearchOpenSearchClient.MAX_RESULT_WINDOW; + int tableHits = Math.max(window / allowedTables.size(), Math.min(window, 2 * limitPerTable)); + int tableTrips = Math.max(2, MAX_LOCKED_ROUND_TRIPS / allowedTables.size()); + for(String tableName : allowedTables) { - Page page = searchPage(client, term, List.of(tableName), offset, limitPerTable, tableConfigs, applyLocks); + Page page = searchPage(client, term, List.of(tableName), offset, limitPerTable, tableConfigs, applyLocks, new ScanBudget(tableHits, tableTrips)); results.addAll(page.results()); totalHits += page.totalHits(); hasMore |= page.hasMore(); @@ -198,7 +206,7 @@ public class QuickSearchAction ** limit count accessible results within one scan budget (see the class ** comment). Every request keeps from + size within MAX_RESULT_WINDOW. *******************************************************************************/ - private Page searchPage(QuickSearchOpenSearchClient client, String term, List tables, int offset, int limit, List tableConfigs, boolean applyLocks) throws QException + private Page searchPage(QuickSearchOpenSearchClient client, String term, List tables, int offset, int limit, List tableConfigs, boolean applyLocks, ScanBudget budget) throws QException { int window = QuickSearchOpenSearchClient.MAX_RESULT_WINDOW; int pageSize = Math.min(limit, window - offset); @@ -222,7 +230,7 @@ public class QuickSearchAction // results are known, the hits run out, the result window is reached, or // // the scan budget is spent. each batch over-reads 2x what is still // // needed and at least doubles the last one, up to MAX_LOCKED_BATCH_SIZE, // - // so a fully locked table reaches the window in 19 round trips. // + // and the last round trip the budget allows takes all it has left. // //////////////////////////////////////////////////////////////////////////// int target = offset + pageSize; List accessible = new ArrayList<>(); @@ -230,13 +238,13 @@ public class QuickSearchAction int batchSize = 0; boolean exhausted = false; boolean rawFloor = false; - ScanBudget budget = new ScanBudget(); Set seen = new HashSet<>(); String preference = "quick-search-" + UUID.randomUUID(); while(accessible.size() < target && !exhausted && position < window && budget.hasRemaining()) { - batchSize = Math.min(Math.min(window - position, budget.remainingHits), Math.min(MAX_LOCKED_BATCH_SIZE, Math.max(2 * (target - accessible.size()), 2 * batchSize))); + int ceiling = Math.min(MAX_LOCKED_BATCH_SIZE, Math.min(window - position, budget.remainingHits)); + batchSize = budget.remainingRoundTrips == 1 ? ceiling : Math.min(ceiling, Math.max(2 * (target - accessible.size()), 2 * batchSize)); SearchResponse response = runSearch(client, term, tables, batchSize, position, tableConfigs, preference); int rawHits = response.hits().hits().size(); @@ -462,8 +470,19 @@ public class QuickSearchAction *******************************************************************************/ private static final class ScanBudget { - private Integer remainingHits = QuickSearchOpenSearchClient.MAX_RESULT_WINDOW; - private Integer remainingRoundTrips = MAX_LOCKED_ROUND_TRIPS; + private Integer remainingHits; + private Integer remainingRoundTrips; + + + + /******************************************************************************* + ** Constructor + *******************************************************************************/ + ScanBudget(Integer hits, Integer roundTrips) + { + this.remainingHits = hits; + this.remainingRoundTrips = roundTrips; + } diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java index d716514..be54f04 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java @@ -125,7 +125,10 @@ public class QuickSearchInput extends AbstractActionInput ** Getter for offset: the number of results to skip, counting only results ** the session can see (with record security locks on, hits filtered out ** by a lock are not counted). With limitPerTable it applies within each - ** table. Results are reachable only within the first 10,000 raw hits. + ** table. Results are reachable only within the first 10,000 raw hits; in + ** per-table mode with locks each table gets an equal share of that scan + ** budget, so deep pages of a heavily locked table may be unreachable + ** (hasMore and totalHitsIsLowerBound stay true). *******************************************************************************/ public Integer getOffset() { diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java index c51332a..2bae465 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java @@ -556,11 +556,12 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest /******************************************************************************* - ** Test: with both tables fully locked, each table's scan stays within its - ** own budget, so the request's work is bounded by the number of tables. + ** Test: with both tables fully locked, each table stops at its equal share + ** and the raw hits fetched across tables stay within + ** max(10,000, T x 2 x limitPerTable), reported as a lower bound. *******************************************************************************/ @Test - void testLimitPerTable_withLocks_eachTableHasOwnBudget() throws QException + void testLimitPerTable_withLocks_totalWorkWithinBound() throws QException { addOtherTable(); List ids = IntStream.rangeClosed(1, 50_000).mapToObj(String::valueOf).toList(); @@ -568,16 +569,20 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest QuickSearchOutput output = action.execute(new QuickSearchInput().withSearchTerm("entity").withTableNames(List.of(TEST_ENTITY_TABLE, OTHER_TABLE)).withOffset(5_000).withLimitPerTable(5)); + int total = 0; for(String table : List.of(TEST_ENTITY_TABLE, OTHER_TABLE)) { ArgumentCaptor sizes = ArgumentCaptor.forClass(Integer.class); verify(mockClient, atLeastOnce()).search(anyString(), eq(List.of(table)), sizes.capture(), anyInt(), anyList(), any()); - assertThat(sizes.getAllValues().size()).isLessThanOrEqualTo(QuickSearchAction.MAX_LOCKED_ROUND_TRIPS); + assertThat(sizes.getAllValues().size()).isLessThanOrEqualTo(QuickSearchAction.MAX_LOCKED_ROUND_TRIPS / 2); assertThat(sizes.getAllValues()).allMatch(size -> size <= QuickSearchAction.MAX_LOCKED_BATCH_SIZE); - assertThat(sizes.getAllValues().stream().mapToInt(Integer::intValue).sum()).isEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW); + assertThat(sizes.getAllValues().stream().mapToInt(Integer::intValue).sum()).isEqualTo(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW / 2); + total += sizes.getAllValues().stream().mapToInt(Integer::intValue).sum(); } + assertThat(total).isLessThanOrEqualTo(Math.max(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW, 2 * 2 * 5)); assertThat(output.getResults()).isEmpty(); - assertThat(output.getHasMore()).isFalse(); + assertThat(output.getHasMore()).isTrue(); + assertThat(output.getTotalHitsIsLowerBound()).isTrue(); } -- 2.54.0 From 8b1c96166a626751cfdbe35a332564656f5e0332 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 6 Oct 2026 18:32:21 -0500 Subject: [PATCH 5/5] fix(#16): end paging when a table's scan share runs out A spent scan budget now ends paging like the result window, so hasMore turns false while the total stays a lower bound. Refs #16 --- README.md | 2 +- .../actions/QuickSearchAction.java | 15 +++--- .../quicksearch/actions/QuickSearchInput.java | 5 +- .../QuickSearchActionSecurityTest.java | 48 ++++++++++++++++--- 4 files changed, 53 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 9d9d2ba..3eed0fc 100644 --- a/README.md +++ b/README.md @@ -120,7 +120,7 @@ output.getTotalHits(); output.getTotalHitsIsLowerBound(); output.getHasMore(); `limitPerTable` returns up to that many hits from each table instead of one ranked list. Terms shorter than 2 characters return nothing; longer than 100 characters are rejected. `limit` is capped at `maxSearchLimit` and `offset + limit` at 10,000. -`offset` skips N accessible results: with record security locks on, hits a lock hides are not counted, so consecutive pages never overlap or skip a visible hit, and with `limitPerTable` the offset applies within each table. With locks on, `totalHits` counts hits confirmed accessible so far; `totalHitsIsLowerBound` is true when the scan stopped before reading every candidate (page full, result window, or scan budget). A single-list search reads at most 10,000 raw hits in batches of at most 1,000. In per-table mode the T tables share that budget, each getting max(10,000 / T, 2 x `limitPerTable`) hits, so one call reads at most max(10,000, T x 2 x `limitPerTable`) raw hits; deep pages of a heavily locked table may then be unreachable, and `hasMore` and `totalHitsIsLowerBound` stay true. +`offset` skips N accessible results: with record security locks on, hits a lock hides are not counted, so consecutive pages never overlap or skip a visible hit, and with `limitPerTable` the offset applies within each table. With locks on, `totalHits` counts hits confirmed accessible so far; `totalHitsIsLowerBound` is true when the scan stopped before reading every candidate (page full, result window, or scan budget). A single-list search reads at most 10,000 raw hits in batches of at most 1,000. In per-table mode the T tables share that budget, each getting max(10,000 / T, 2 x `limitPerTable`) hits, so one call reads at most max(10,000, T x 2 x `limitPerTable`) raw hits. Paging within a table stops past that share of raw hits, as it does past 10,000 in a single list: `hasMore` turns false and `totalHitsIsLowerBound` stays true to signal unreachable matches. Search runs as the current `QContext` session: tables the session cannot read are skipped, and with `applyRecordSecurityLocks` (default) hits are re-read through `QueryAction` so record security locks apply. diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java index a6cb3fb..774f606 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchAction.java @@ -86,11 +86,12 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** cannot starve another and one call reads at most ** max(MAX_RESULT_WINDOW, T x 2 x limitPerTable) raw hits over at most ** max(MAX_LOCKED_ROUND_TRIPS, 2 x T) requests; offset never raises it. - ** When a budget runs out the scan stops and reports hasMore and a - ** lower-bound total, so deep pages of a heavily locked table may be - ** unreachable in per-table mode. Hits are deduplicated by table and record id within a - ** call; batches are separate from/size requests without a tiebreak sort, so - ** a hit that moves between batches during a refresh can still be missed. + ** A budget ends a scan like the result window does: hasMore is false and + ** the total is a lower bound, so in per-table mode paging within a table + ** stops past max(MAX_RESULT_WINDOW / T, 2 x limitPerTable) raw hits. Hits + ** are deduplicated by table and record id within a call; batches are + ** separate from/size requests without a tiebreak sort, so a hit that moves + ** between batches during a refresh can still be missed. *******************************************************************************/ public class QuickSearchAction { @@ -230,7 +231,7 @@ public class QuickSearchAction // results are known, the hits run out, the result window is reached, or // // the scan budget is spent. each batch over-reads 2x what is still // // needed and at least doubles the last one, up to MAX_LOCKED_BATCH_SIZE, // - // and the last round trip the budget allows takes all it has left. // + // and the last round trip takes up to MAX_LOCKED_BATCH_SIZE of the rest. // //////////////////////////////////////////////////////////////////////////// int target = offset + pageSize; List accessible = new ArrayList<>(); @@ -271,7 +272,7 @@ public class QuickSearchAction // more candidates exist, so a caller can page on to accessible rows. // //////////////////////////////////////////////////////////////////////////// boolean truncated = accessible.size() > target; - boolean hasMore = truncated || (!exhausted && position < window); + boolean hasMore = truncated || (!exhausted && position < window && budget.hasRemaining()); List results = accessible.size() > offset ? new ArrayList<>(accessible.subList(offset, Math.min(target, accessible.size()))) diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java index be54f04..33428d8 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchInput.java @@ -127,8 +127,9 @@ public class QuickSearchInput extends AbstractActionInput ** by a lock are not counted). With limitPerTable it applies within each ** table. Results are reachable only within the first 10,000 raw hits; in ** per-table mode with locks each table gets an equal share of that scan - ** budget, so deep pages of a heavily locked table may be unreachable - ** (hasMore and totalHitsIsLowerBound stay true). + ** budget, so paging within a table stops past + ** max(10,000 / T, 2 x limitPerTable) raw hits (hasMore turns false and + ** totalHitsIsLowerBound stays true). *******************************************************************************/ public Integer getOffset() { diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java index 2bae465..6cf7d32 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/actions/QuickSearchActionSecurityTest.java @@ -558,7 +558,8 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest /******************************************************************************* ** Test: with both tables fully locked, each table stops at its equal share ** and the raw hits fetched across tables stay within - ** max(10,000, T x 2 x limitPerTable), reported as a lower bound. + ** max(10,000, T x 2 x limitPerTable); the share ends paging like the + ** window, so hasMore is false and the total is a lower bound. *******************************************************************************/ @Test void testLimitPerTable_withLocks_totalWorkWithinBound() throws QException @@ -581,7 +582,7 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest } assertThat(total).isLessThanOrEqualTo(Math.max(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW, 2 * 2 * 5)); assertThat(output.getResults()).isEmpty(); - assertThat(output.getHasMore()).isTrue(); + assertThat(output.getHasMore()).isFalse(); assertThat(output.getTotalHitsIsLowerBound()).isTrue(); } @@ -628,17 +629,50 @@ class QuickSearchActionSecurityTest extends BaseQuickSearchTest /******************************************************************************* - ** Test: a hit returned by two batches appears once on the page. + ** Test: a hit returned again by a later batch is counted once, so the + ** offset skips it once and the page holds the next accessible hits. *******************************************************************************/ @Test void testRecordLock_duplicateAcrossBatches_returnedOnce() throws QException { - insertEntities(1, 2); - stubPagedSearch(List.of("9", "1", "1", "2")); + insertEntities(1, 2, 3); + stubPagedSearch(List.of("9", "8", "7", "6", "5", "1", "1", "2", "3")); - QuickSearchOutput output = page(0, 2); + QuickSearchOutput output = page(1, 2); - assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("1", "2"); + verify(mockClient, times(2)).search(anyString(), any(), anyInt(), anyInt(), anyList(), any()); + assertThat(output.getResults()).extracting(QuickSearchResult::getRecordId).containsExactly("2", "3"); + } + + + + /******************************************************************************* + ** Test: paging a table with offset += results until hasMore is false + ** terminates at the table's share, with every result returned once. + *******************************************************************************/ + @Test + void testLimitPerTable_withLocks_pagingUntilNoMoreTerminates() throws QException + { + addOtherTable(); + QuickSearchQBitContext.getConfig().withMaxSearchLimit(1_000); + insertEntities(IntStream.rangeClosed(1, 6_000).boxed().toArray(Integer[]::new)); + stubPagedSearch(Map.of(TEST_ENTITY_TABLE, IntStream.rangeClosed(1, 12_000).mapToObj(String::valueOf).toList()), false); + + List seen = new ArrayList<>(); + int pages = 0; + QuickSearchOutput output; + do + { + output = action.execute(new QuickSearchInput().withSearchTerm("entity").withTableNames(List.of(TEST_ENTITY_TABLE, OTHER_TABLE)).withOffset(seen.size()).withLimitPerTable(1_000)); + output.getResults().forEach(result -> seen.add(result.getRecordId())); + pages++; + } + while(output.getHasMore() && pages < 20); + + assertThat(output.getHasMore()).isFalse(); + assertThat(output.getTotalHitsIsLowerBound()).isTrue(); + assertThat(pages).isEqualTo(5); + assertThat(seen).doesNotHaveDuplicates().hasSize(QuickSearchOpenSearchClient.MAX_RESULT_WINDOW / 2); } } -- 2.54.0