fixing bug with fulltext and jpa search filters (#6374)

This commit is contained in:
TipzCM 2024-10-17 10:46:21 -04:00 committed by GitHub
parent 161a077f2a
commit 5b23e1bbec
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 120 additions and 30 deletions

View File

@ -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))) 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)))
); );
} }
} }

View File

@ -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.
"

View File

@ -91,6 +91,7 @@ public class SynchronousSearchSvcImpl implements ISynchronousSearchSvc {
private int mySyncSize = 250; private int mySyncSize = 250;
@Override @Override
@SuppressWarnings({"rawtypes", "unchecked"})
public IBundleProvider executeQuery( public IBundleProvider executeQuery(
SearchParameterMap theParams, SearchParameterMap theParams,
RequestDetails theRequestDetails, RequestDetails theRequestDetails,
@ -113,7 +114,6 @@ public class SynchronousSearchSvcImpl implements ISynchronousSearchSvc {
.withRequestPartitionId(theRequestPartitionId) .withRequestPartitionId(theRequestPartitionId)
.readOnly() .readOnly()
.execute(() -> { .execute(() -> {
// Load the results synchronously // Load the results synchronously
List<JpaPid> pids = new ArrayList<>(); List<JpaPid> pids = new ArrayList<>();

View File

@ -164,7 +164,6 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
@Deprecated @Deprecated
public static final int MAXIMUM_PAGE_SIZE = SearchConstants.MAX_PAGE_SIZE; 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_ID_ALIAS = "resource_id";
public static final String RESOURCE_VERSION_ALIAS = "resource_version"; public static final String RESOURCE_VERSION_ALIAS = "resource_version";
private static final Logger ourLog = LoggerFactory.getLogger(SearchBuilder.class); private static final Logger ourLog = LoggerFactory.getLogger(SearchBuilder.class);
@ -174,7 +173,7 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
private static final String MY_TARGET_RESOURCE_TYPE = "myTargetResourceType"; private static final String MY_TARGET_RESOURCE_TYPE = "myTargetResourceType";
private static final String MY_SOURCE_RESOURCE_TYPE = "mySourceResourceType"; private static final String MY_SOURCE_RESOURCE_TYPE = "mySourceResourceType";
private static final String MY_TARGET_RESOURCE_VERSION = "myTargetResourceVersion"; 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 IInterceptorBroadcaster myInterceptorBroadcaster;
protected final IResourceTagDao myResourceTagDao; protected final IResourceTagDao myResourceTagDao;
private String myResourceName; private String myResourceName;
@ -467,6 +466,8 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
.chunk( .chunk(
fulltextExecutor, fulltextExecutor,
SearchBuilder.getMaximumPageSize(), SearchBuilder.getMaximumPageSize(),
// for each list of (SearchBuilder.getMaximumPageSize())
// we create a chunked query and add it to 'queries'
t -> doCreateChunkedQueries( t -> doCreateChunkedQueries(
theParams, t, theOffset, sort, theCountOnlyFlag, theRequest, queries)); theParams, t, theOffset, sort, theCountOnlyFlag, theRequest, queries));
} }
@ -2376,15 +2377,23 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
if (myNext == null) { if (myNext == null) {
// no next means we need a new query (if one is available) // no next means we need a new query (if one is available)
while (myResultsIterator.hasNext() || !myQueryList.isEmpty()) { 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(); retrieveNextIteratorQuery();
}
// if our new results iterator is also empty if (!myResultsIterator.hasNext()) {
// we couldn't find a results iterator;
// we're done here // we're done here
if (!myResultsIterator.hasNext()) { break;
break;
}
} }
Long nextLong = myResultsIterator.next(); Long nextLong = myResultsIterator.next();
@ -2604,14 +2613,13 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
} }
public static int getMaximumPageSize() { public static int getMaximumPageSize() {
if (myUseMaxPageSize50ForTest) { if (myMaxPageSizeForTests != null) {
return MAXIMUM_PAGE_SIZE_FOR_TESTING; return myMaxPageSizeForTests;
} else {
return MAXIMUM_PAGE_SIZE;
} }
return MAXIMUM_PAGE_SIZE;
} }
public static void setMaxPageSize50ForTest(boolean theIsTest) { public static void setMaxPageSizeForTest(Integer theTestSize) {
myUseMaxPageSize50ForTest = theIsTest; myMaxPageSizeForTests = theTestSize;
} }
} }

View File

@ -66,21 +66,19 @@ public class FhirResourceDaoR4SearchLastNAsyncIT extends BaseR4SearchLastN {
mySmallerPreFetchThresholds.add(-1); mySmallerPreFetchThresholds.add(-1);
myStorageSettings.setSearchPreFetchThresholds(mySmallerPreFetchThresholds); myStorageSettings.setSearchPreFetchThresholds(mySmallerPreFetchThresholds);
SearchBuilder.setMaxPageSize50ForTest(true); SearchBuilder.setMaxPageSizeForTest(50);
myStorageSettings.setLastNEnabled(true); myStorageSettings.setLastNEnabled(true);
} }
@AfterEach @AfterEach
public void after() { public void after() {
myStorageSettings.setSearchPreFetchThresholds(originalPreFetchThresholds); myStorageSettings.setSearchPreFetchThresholds(originalPreFetchThresholds);
SearchBuilder.setMaxPageSize50ForTest(false); SearchBuilder.setMaxPageSizeForTest(null);
} }
@Test @Test
public void testLastNChunking() { public void testLastNChunking() {
runInTransaction(() -> { runInTransaction(() -> {
Set<Long> all = mySearchDao.findAll().stream().map(Search::getId).collect(Collectors.toSet()); Set<Long> all = mySearchDao.findAll().stream().map(Search::getId).collect(Collectors.toSet());
@ -103,9 +101,6 @@ public class FhirResourceDaoR4SearchLastNAsyncIT extends BaseR4SearchLastN {
Map<String, String[]> requestParameters = new HashMap<>(); Map<String, String[]> requestParameters = new HashMap<>();
when(mySrd.getParameters()).thenReturn(requestParameters); 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: // Expand default fetch sizes to ensure all observations are returned in first page:
List<Integer> myBiggerPreFetchThresholds = new ArrayList<>(); List<Integer> myBiggerPreFetchThresholds = new ArrayList<>();
myBiggerPreFetchThresholds.add(100); myBiggerPreFetchThresholds.add(100);

View File

@ -45,7 +45,7 @@ public class FhirResourceDaoR4SearchLastNIT extends BaseR4SearchLastN {
@AfterEach @AfterEach
public void reset() { public void reset() {
SearchBuilder.setMaxPageSize50ForTest(false); SearchBuilder.setMaxPageSizeForTest(null);
myStorageSettings.setStoreResourceInHSearchIndex(new JpaStorageSettings().isStoreResourceInHSearchIndex()); myStorageSettings.setStoreResourceInHSearchIndex(new JpaStorageSettings().isStoreResourceInHSearchIndex());
myStorageSettings.setAdvancedHSearchIndexing(new JpaStorageSettings().isAdvancedHSearchIndexing()); myStorageSettings.setAdvancedHSearchIndexing(new JpaStorageSettings().isAdvancedHSearchIndexing());
} }
@ -77,7 +77,7 @@ public class FhirResourceDaoR4SearchLastNIT extends BaseR4SearchLastN {
when(mySrd.getParameters()).thenReturn(requestParameters); when(mySrd.getParameters()).thenReturn(requestParameters);
// Set chunk size to 50 // Set chunk size to 50
SearchBuilder.setMaxPageSize50ForTest(true); SearchBuilder.setMaxPageSizeForTest(50);
myCaptureQueriesListener.clear(); myCaptureQueriesListener.clear();
List<String> results = toUnqualifiedVersionlessIdValues(myObservationDao.observationsLastN(params, mockSrd(), null)); List<String> results = toUnqualifiedVersionlessIdValues(myObservationDao.observationsLastN(params, mockSrd(), null));

View File

@ -17,9 +17,10 @@ import ca.uhn.fhir.rest.param.StringParam;
import ca.uhn.fhir.rest.param.TokenAndListParam; import ca.uhn.fhir.rest.param.TokenAndListParam;
import ca.uhn.fhir.rest.param.TokenOrListParam; import ca.uhn.fhir.rest.param.TokenOrListParam;
import ca.uhn.fhir.rest.param.TokenParam; 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.Organization;
import org.hl7.fhir.r4.model.Patient; 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.junit.jupiter.api.Test;
import org.slf4j.Logger; import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
@ -33,6 +34,7 @@ import java.util.List;
import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndexTests { public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndexTests {
@ -44,6 +46,17 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndex
@Autowired @Autowired
private DataSource myDataSource; private DataSource myDataSource;
@BeforeEach
public void before() throws Exception {
super.before();
SearchBuilder.setMaxPageSizeForTest(10);
}
@AfterEach
public void after() {
SearchBuilder.setMaxPageSizeForTest(null);
}
@Override @Override
public IInterceptorService getInterceptorService() { public IInterceptorService getInterceptorService() {
return myInterceptorRegistry; return myInterceptorRegistry;
@ -324,6 +337,75 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndex
assertThat(resourceIdsFromSearchResult).containsExactlyInAnyOrderElementsOf(expectedActivePatientIds); 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("<div>ABC</div>");
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(
"<div>FINDME</div>"
);
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<String> ids = provider.getAllResourceIds();
assertEquals(1, ids.size());
}
@Test @Test
public void testLuceneNarrativeSearchQueryIntersectingJpaQuery() { public void testLuceneNarrativeSearchQueryIntersectingJpaQuery() {
final int numberOfPatientsToCreate = SearchBuilder.getMaximumPageSize() + 10; final int numberOfPatientsToCreate = SearchBuilder.getMaximumPageSize() + 10;
@ -378,10 +460,8 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test implements IR4SearchIndex
} }
SearchParameterMap map = new SearchParameterMap().setLoadSynchronous(true); SearchParameterMap map = new SearchParameterMap().setLoadSynchronous(true);
TokenAndListParam tokenAndListParam = new TokenAndListParam(); TokenAndListParam tokenAndListParam = new TokenAndListParam();
tokenAndListParam.addAnd(new TokenOrListParam().addOr(new TokenParam().setValue("true"))); tokenAndListParam.addAnd(new TokenOrListParam().addOr(new TokenParam().setValue("true")));
map.add("active", tokenAndListParam); map.add("active", tokenAndListParam);
map.add(Constants.PARAM_CONTENT, new StringParam(patientFamilyName)); map.add(Constants.PARAM_CONTENT, new StringParam(patientFamilyName));

View File

@ -87,7 +87,7 @@ public class ValueSetExpansionR4Test extends BaseTermR4Test implements IValueSet
@AfterEach @AfterEach
public void afterEach() { public void afterEach() {
SearchBuilder.setMaxPageSize50ForTest(false); SearchBuilder.setMaxPageSizeForTest(null);
} }
@Override @Override
@ -235,7 +235,7 @@ public class ValueSetExpansionR4Test extends BaseTermR4Test implements IValueSet
@Test @Test
public void testExpandHugeValueSet_FilterOnDisplay_LeftMatch_SelectAll() { public void testExpandHugeValueSet_FilterOnDisplay_LeftMatch_SelectAll() {
SearchBuilder.setMaxPageSize50ForTest(true); SearchBuilder.setMaxPageSizeForTest(50);
myStorageSettings.setPreExpandValueSets(true); myStorageSettings.setPreExpandValueSets(true);
IIdType vsId = createConceptsCodeSystemAndValueSet(1005); IIdType vsId = createConceptsCodeSystemAndValueSet(1005);