From 5b23e1bbecb87b50a8218f59eae602dcedb4d860 Mon Sep 17 00:00:00 2001 From: TipzCM Date: Thu, 17 Oct 2024 10:46:21 -0400 Subject: [PATCH] fixing bug with fulltext and jpa search filters (#6374) --- .../ca/uhn/fhir/util/TaskChunkerTest.java | 1 - ...-lucene-and-jpa-search-combine-issues.yaml | 8 ++ .../jpa/search/SynchronousSearchSvcImpl.java | 2 +- .../jpa/search/builder/SearchBuilder.java | 36 +++++--- .../FhirResourceDaoR4SearchLastNAsyncIT.java | 9 +- .../r4/FhirResourceDaoR4SearchLastNIT.java | 4 +- .../fhir/jpa/dao/r4/FhirSearchDaoR4Test.java | 86 ++++++++++++++++++- .../jpa/term/ValueSetExpansionR4Test.java | 4 +- 8 files changed, 120 insertions(+), 30 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6372-lucene-and-jpa-search-combine-issues.yaml diff --git a/hapi-fhir-base/src/test/java/ca/uhn/fhir/util/TaskChunkerTest.java b/hapi-fhir-base/src/test/java/ca/uhn/fhir/util/TaskChunkerTest.java index aff2b3b9b89..38df69f017f 100644 --- a/hapi-fhir-base/src/test/java/ca/uhn/fhir/util/TaskChunkerTest.java +++ b/hapi-fhir-base/src/test/java/ca/uhn/fhir/util/TaskChunkerTest.java @@ -77,5 +77,4 @@ public class TaskChunkerTest { Arguments.of(List.of(1,2,3,4,5,6,7,8,9), List.of(List.of(1,2,3), List.of(4,5,6), List.of(7,8,9))) ); } - } diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6372-lucene-and-jpa-search-combine-issues.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6372-lucene-and-jpa-search-combine-issues.yaml new file mode 100644 index 00000000000..f119b524a75 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6372-lucene-and-jpa-search-combine-issues.yaml @@ -0,0 +1,8 @@ +--- +type: fix +issue: 6372 +title: "Searches that combined full-text searching (i.e. `_text` or `_content`) + with other search parameters could fail to return all results if we encountered + 1600 matches against the full-text index where none of them match the rest of the query. + This has now been fixed. +" diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SynchronousSearchSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SynchronousSearchSvcImpl.java index 478c60e2040..317b8fa2105 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SynchronousSearchSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SynchronousSearchSvcImpl.java @@ -91,6 +91,7 @@ public class SynchronousSearchSvcImpl implements ISynchronousSearchSvc { private int mySyncSize = 250; @Override + @SuppressWarnings({"rawtypes", "unchecked"}) public IBundleProvider executeQuery( SearchParameterMap theParams, RequestDetails theRequestDetails, @@ -113,7 +114,6 @@ public class SynchronousSearchSvcImpl implements ISynchronousSearchSvc { .withRequestPartitionId(theRequestPartitionId) .readOnly() .execute(() -> { - // Load the results synchronously List pids = new ArrayList<>(); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java index 8588733f5f8..9a28921f1ae 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java @@ -164,7 +164,6 @@ public class SearchBuilder implements ISearchBuilder { @Deprecated public static final int MAXIMUM_PAGE_SIZE = SearchConstants.MAX_PAGE_SIZE; - public static final int MAXIMUM_PAGE_SIZE_FOR_TESTING = 50; public static final String RESOURCE_ID_ALIAS = "resource_id"; public static final String RESOURCE_VERSION_ALIAS = "resource_version"; private static final Logger ourLog = LoggerFactory.getLogger(SearchBuilder.class); @@ -174,7 +173,7 @@ public class SearchBuilder implements ISearchBuilder { private static final String MY_TARGET_RESOURCE_TYPE = "myTargetResourceType"; private static final String MY_SOURCE_RESOURCE_TYPE = "mySourceResourceType"; private static final String MY_TARGET_RESOURCE_VERSION = "myTargetResourceVersion"; - public static boolean myUseMaxPageSize50ForTest = false; + public static Integer myMaxPageSizeForTests = null; protected final IInterceptorBroadcaster myInterceptorBroadcaster; protected final IResourceTagDao myResourceTagDao; private String myResourceName; @@ -467,6 +466,8 @@ public class SearchBuilder implements ISearchBuilder { .chunk( fulltextExecutor, SearchBuilder.getMaximumPageSize(), + // for each list of (SearchBuilder.getMaximumPageSize()) + // we create a chunked query and add it to 'queries' t -> doCreateChunkedQueries( theParams, t, theOffset, sort, theCountOnlyFlag, theRequest, queries)); } @@ -2376,15 +2377,23 @@ public class SearchBuilder implements ISearchBuilder { if (myNext == null) { // no next means we need a new query (if one is available) while (myResultsIterator.hasNext() || !myQueryList.isEmpty()) { - // Update iterator with next chunk if necessary. - if (!myResultsIterator.hasNext()) { + /* + * Because we combine our DB searches with Lucene + * sometimes we can have multiple results iterators + * (with only some having data in them to extract). + * + * We'll iterate our results iterators until we + * either run out of results iterators, or we + * have one that actually has data in it. + */ + while (!myResultsIterator.hasNext() && !myQueryList.isEmpty()) { retrieveNextIteratorQuery(); + } - // if our new results iterator is also empty + if (!myResultsIterator.hasNext()) { + // we couldn't find a results iterator; // we're done here - if (!myResultsIterator.hasNext()) { - break; - } + break; } Long nextLong = myResultsIterator.next(); @@ -2604,14 +2613,13 @@ public class SearchBuilder implements ISearchBuilder { } public static int getMaximumPageSize() { - if (myUseMaxPageSize50ForTest) { - return MAXIMUM_PAGE_SIZE_FOR_TESTING; - } else { - return MAXIMUM_PAGE_SIZE; + if (myMaxPageSizeForTests != null) { + return myMaxPageSizeForTests; } + return MAXIMUM_PAGE_SIZE; } - public static void setMaxPageSize50ForTest(boolean theIsTest) { - myUseMaxPageSize50ForTest = theIsTest; + public static void setMaxPageSizeForTest(Integer theTestSize) { + myMaxPageSizeForTests = theTestSize; } } diff --git a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNAsyncIT.java b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNAsyncIT.java index bd357c16053..05efac05a17 100644 --- a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNAsyncIT.java +++ b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNAsyncIT.java @@ -66,21 +66,19 @@ public class FhirResourceDaoR4SearchLastNAsyncIT extends BaseR4SearchLastN { mySmallerPreFetchThresholds.add(-1); myStorageSettings.setSearchPreFetchThresholds(mySmallerPreFetchThresholds); - SearchBuilder.setMaxPageSize50ForTest(true); + SearchBuilder.setMaxPageSizeForTest(50); myStorageSettings.setLastNEnabled(true); - } @AfterEach public void after() { myStorageSettings.setSearchPreFetchThresholds(originalPreFetchThresholds); - SearchBuilder.setMaxPageSize50ForTest(false); + SearchBuilder.setMaxPageSizeForTest(null); } @Test public void testLastNChunking() { - runInTransaction(() -> { Set all = mySearchDao.findAll().stream().map(Search::getId).collect(Collectors.toSet()); @@ -103,9 +101,6 @@ public class FhirResourceDaoR4SearchLastNAsyncIT extends BaseR4SearchLastN { Map requestParameters = new HashMap<>(); when(mySrd.getParameters()).thenReturn(requestParameters); - // Set chunk size to 50 - SearchBuilder.setMaxPageSize50ForTest(true); - // Expand default fetch sizes to ensure all observations are returned in first page: List myBiggerPreFetchThresholds = new ArrayList<>(); myBiggerPreFetchThresholds.add(100); diff --git a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNIT.java b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNIT.java index 50c269f56b5..c17a3aa742c 100644 --- a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNIT.java +++ b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNIT.java @@ -45,7 +45,7 @@ public class FhirResourceDaoR4SearchLastNIT extends BaseR4SearchLastN { @AfterEach public void reset() { - SearchBuilder.setMaxPageSize50ForTest(false); + SearchBuilder.setMaxPageSizeForTest(null); myStorageSettings.setStoreResourceInHSearchIndex(new JpaStorageSettings().isStoreResourceInHSearchIndex()); myStorageSettings.setAdvancedHSearchIndexing(new JpaStorageSettings().isAdvancedHSearchIndexing()); } @@ -77,7 +77,7 @@ public class FhirResourceDaoR4SearchLastNIT extends BaseR4SearchLastN { when(mySrd.getParameters()).thenReturn(requestParameters); // Set chunk size to 50 - SearchBuilder.setMaxPageSize50ForTest(true); + SearchBuilder.setMaxPageSizeForTest(50); myCaptureQueriesListener.clear(); List results = toUnqualifiedVersionlessIdValues(myObservationDao.observationsLastN(params, mockSrd(), null)); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSearchDaoR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSearchDaoR4Test.java index 70940f502fd..3c744b19aaa 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSearchDaoR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSearchDaoR4Test.java @@ -17,9 +17,10 @@ import ca.uhn.fhir.rest.param.StringParam; import ca.uhn.fhir.rest.param.TokenAndListParam; import ca.uhn.fhir.rest.param.TokenOrListParam; import ca.uhn.fhir.rest.param.TokenParam; -import ca.uhn.fhir.storage.test.BaseDateSearchDaoTests; import org.hl7.fhir.r4.model.Organization; import org.hl7.fhir.r4.model.Patient; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -33,6 +34,7 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndexTests { @@ -44,6 +46,17 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndex @Autowired private DataSource myDataSource; + @BeforeEach + public void before() throws Exception { + super.before(); + SearchBuilder.setMaxPageSizeForTest(10); + } + + @AfterEach + public void after() { + SearchBuilder.setMaxPageSizeForTest(null); + } + @Override public IInterceptorService getInterceptorService() { return myInterceptorRegistry; @@ -324,6 +337,75 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndex assertThat(resourceIdsFromSearchResult).containsExactlyInAnyOrderElementsOf(expectedActivePatientIds); } + @Test + public void searchLuceneAndJPA_withLuceneMatchingButJpaNot_returnsNothing() { + // setup + int numToCreate = 2 * SearchBuilder.getMaximumPageSize() + 10; + + // create resources + for (int i = 0; i < numToCreate; i++) { + Patient patient = new Patient(); + patient.setActive(true); + patient.addIdentifier() + .setSystem("http://fhir.com") + .setValue("ZYX"); + patient.getText().setDivAsString("
ABC
"); + myPatientDao.create(patient, mySrd); + } + + // test + SearchParameterMap map = new SearchParameterMap(); + map.setLoadSynchronous(true); + map.setSearchTotalMode(SearchTotalModeEnum.ACCURATE); + TokenAndListParam tokenAndListParam = new TokenAndListParam(); + tokenAndListParam.addAnd(new TokenOrListParam().addOr(new TokenParam().setValue("true"))); + map.add("active", tokenAndListParam); + map.add(Constants.PARAM_TEXT, new StringParam("ABC")); + map.add("identifier", new TokenParam(null, "not found")); + IBundleProvider provider = myPatientDao.search(map, mySrd); + + // verify + assertEquals(0, provider.getAllResources().size()); + } + + @Test + public void searchLuceneAndJPA_withLuceneBroadAndJPASearchNarrow_returnsFoundResults() { + // setup + int numToCreate = 2 * SearchBuilder.getMaximumPageSize() + 10; + String identifierToFind = "bcde"; + + // create patients + for (int i = 0; i < numToCreate; i++) { + Patient patient = new Patient(); + patient.setActive(true); + String identifierVal = i == numToCreate - 10 ? identifierToFind: + "abcd"; + patient.addIdentifier() + .setSystem("http://fhir.com") + .setValue(identifierVal); + + patient.getText().setDivAsString( + "
FINDME
" + ); + myPatientDao.create(patient, mySrd); + } + + // test + SearchParameterMap map = new SearchParameterMap(); + map.setLoadSynchronous(true); + map.setSearchTotalMode(SearchTotalModeEnum.ACCURATE); + TokenAndListParam tokenAndListParam = new TokenAndListParam(); + tokenAndListParam.addAnd(new TokenOrListParam().addOr(new TokenParam().setValue("true"))); + map.add("active", tokenAndListParam); + map.add(Constants.PARAM_TEXT, new StringParam("FINDME")); + map.add("identifier", new TokenParam(null, identifierToFind)); + IBundleProvider provider = myPatientDao.search(map, mySrd); + + // verify + List ids = provider.getAllResourceIds(); + assertEquals(1, ids.size()); + } + @Test public void testLuceneNarrativeSearchQueryIntersectingJpaQuery() { final int numberOfPatientsToCreate = SearchBuilder.getMaximumPageSize() + 10; @@ -378,10 +460,8 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndex } SearchParameterMap map = new SearchParameterMap().setLoadSynchronous(true); - TokenAndListParam tokenAndListParam = new TokenAndListParam(); tokenAndListParam.addAnd(new TokenOrListParam().addOr(new TokenParam().setValue("true"))); - map.add("active", tokenAndListParam); map.add(Constants.PARAM_CONTENT, new StringParam(patientFamilyName)); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/ValueSetExpansionR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/ValueSetExpansionR4Test.java index d6f5039d129..ec11b173a61 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/ValueSetExpansionR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/ValueSetExpansionR4Test.java @@ -87,7 +87,7 @@ public class ValueSetExpansionR4Test extends BaseTermR4Test implements IValueSet @AfterEach public void afterEach() { - SearchBuilder.setMaxPageSize50ForTest(false); + SearchBuilder.setMaxPageSizeForTest(null); } @Override @@ -235,7 +235,7 @@ public class ValueSetExpansionR4Test extends BaseTermR4Test implements IValueSet @Test public void testExpandHugeValueSet_FilterOnDisplay_LeftMatch_SelectAll() { - SearchBuilder.setMaxPageSize50ForTest(true); + SearchBuilder.setMaxPageSizeForTest(50); myStorageSettings.setPreExpandValueSets(true); IIdType vsId = createConceptsCodeSystemAndValueSet(1005);