Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixing fulltext search wit h_lastUpdated filter #6411

Open
wants to merge 9 commits into
base: rel_7_6
Choose a base branch
from
+400 −198
Open

fixing fulltext search wit h_lastUpdated filter #6411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9d3b8c9
fixing fulltext search wit h_lastUpdated filter
Oct 28, 2024
c85ce2f
cleanup
Oct 29, 2024
e0bb4fb
review fixes
Oct 29, 2024
d91316b
spotless
Oct 29, 2024
5dc278f
Failing test
tadgh Oct 29, 2024
7dfe040
review fixes round 2
Oct 29, 2024
5722b75
spotless
Oct 29, 2024
54ae463
fixing tests
Oct 30, 2024
50e7ab0
Merge branch 'rel_7_6' into 6404-lucene-search-with-last-updated
Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ...main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6404-fulltext-search-with-lastupdate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
type: fix
issue: 6404
title: "Searches using fulltext search that combined `_lastUpdated` query parameter with
any other (supported) fulltext query parameter would find no matches, even
if matches existed.
This has been corrected.
"
TipzCM marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/upgrade.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,8 @@ If `true`, this will return summarized subject bundle with only detectedIssue.

The `subject` parameter of the `Questionnaire/$populate` operation has been changed to expect a `Reference` as specified
in the SDC IG.

# Fulltext Search with _lastUpdated Filter

Fulltext searches have been updated to support `_lastUpdated` search parameter. A reindexing of Search Parameters
is required to migrate old data to support the `_lastUpdated` search parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be expanded to clarify under what exact conditions it would be necessary to do this, since requiring a reindex of all of your data is a huge ask.. Presumably this only matters if XXX is set to YYY and you are intending to perform a search containing ZZZ

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda required if you're using lucene/fulltext search at all.

Currently, any searches with _lastIndex will not use lucene (if that's the only parameter).

If _lastIndex is provided with any other parameter, it will not work.

This includes in our $mdm-clear job (which was the original bug reported)

6 changes: 3 additions & 3 deletions hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,12 +1161,12 @@ public ResourceTable updateEntity(
entity.setIndexStatus(INDEX_STATUS_INDEXED);
}

if (myFulltextSearchSvc != null && !myFulltextSearchSvc.isDisabled()) {
if (myFulltextSearchSvc != null && !myFulltextSearchSvc.isDisabled() && changed.isChanged()) {
// set the lastUpdated dates so we can use them to search in lucene
theResource.getMeta().setLastUpdated(theTransactionDetails.getTransactionDate());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a super weird thing to be doing here. Why is it necessary to modify the resource passed in? We index the entity, not the FHIR resource model class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields to index are determined by what's in the resource (not the entity) since the resource contains the changed fields.

ExtendedHSearchIndexExtractor is where we create the ExtendedHSearchIndexData and SearchParamTextPropertyBinder is what consumes it to wrie the indexes

populateFullTextFields(myContext, theResource, entity, newParams);
}

} else {

entity.setUpdated(theTransactionDetails.getTransactionDate());
entity.setIndexStatus(null);

Expand Down
2 changes: 1 addition & 1 deletion ...jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchClauseBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ public void addReferenceUnchainedSearch(

/**
* Create date clause from date params. The date lower and upper bounds are taken
* into considertion when generating date query ranges
* into consideration when generating date query ranges
*
* <p>Example 1 ('eq' prefix/empty): <code>http://fhirserver/Observation?date=eq2020</code>
* would generate the following search clause
Expand Down
16 changes: 13 additions & 3 deletions ...jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,18 +89,28 @@ public boolean illegalForHibernateSearch(String theParamName, ResourceSearchPara
* be inaccurate and wrong.
*/
public boolean canUseHibernateSearch(
String theResourceType, SearchParameterMap myParams, ISearchParamRegistry theSearchParamRegistry) {
String theResourceType, SearchParameterMap theParams, ISearchParamRegistry theSearchParamRegistry) {
boolean canUseHibernate = false;

ResourceSearchParams resourceActiveSearchParams = theSearchParamRegistry.getActiveSearchParams(theResourceType);
for (String paramName : myParams.keySet()) {

// special SearchParam handling:
// _lastUpdated
if (theParams.getLastUpdated() != null) {
canUseHibernate = !illegalForHibernateSearch(Constants.PARAM_LASTUPDATED, resourceActiveSearchParams);
if (!canUseHibernate) {
return false;
}
}
TipzCM marked this conversation as resolved.
Show resolved Hide resolved

for (String paramName : theParams.keySet()) {
// is this parameter supported?
if (illegalForHibernateSearch(paramName, resourceActiveSearchParams)) {
canUseHibernate = false;
} else {
// are the parameter values supported?
canUseHibernate =
myParams.get(paramName).stream()
theParams.get(paramName).stream()
TipzCM marked this conversation as resolved.
Show resolved Hide resolved
TipzCM marked this conversation as resolved.
Show resolved Hide resolved
.flatMap(Collection::stream)
.collect(Collectors.toList())
.stream()
Expand Down
17 changes: 14 additions & 3 deletions ...ties/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@
import java.net.URLEncoder;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Date;
import java.util.HashSet;
import java.util.List;
Expand All @@ -130,13 +129,11 @@

import static ca.uhn.fhir.jpa.model.util.UcumServiceUtil.UCUM_CODESYSTEM_URL;
import static ca.uhn.fhir.rest.api.Constants.CHARSET_UTF8;
import static ca.uhn.fhir.rest.api.Constants.HEADER_CACHE_CONTROL;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.when;

@ExtendWith(SpringExtension.class)
@ExtendWith(MockitoExtension.class)
Expand Down Expand Up @@ -294,6 +291,20 @@ public List<StorageProcessingMessage> getMessages() {
}
}

@Test //TODO LS : This test fails, and did not before.
public void testNoOpUpdateDoesNotModifyLastUpdated() throws InterruptedException {
myStorageSettings.setAdvancedHSearchIndexing(true);
Patient patient = new Patient();
patient.getNameFirstRep().setFamily("graham").addGiven("gary");

patient = (Patient) myPatientDao.create(patient).getResource();
Date originalLastUpdated = patient.getMeta().getLastUpdated();

patient = (Patient) myPatientDao.update(patient).getResource();
Date newLastUpdated = patient.getMeta().getLastUpdated();

assertThat(originalLastUpdated).isEqualTo(newLastUpdated);
}

@Test
public void testFullTextSearchesArePerformanceLogged() {
Expand Down
18 changes: 9 additions & 9 deletions hapi-fhir-jpaserver-elastic-test-utilities/src/test/resources/logback-test.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@
</root>
<!--Uncomment the below if you are doing Elasticsearch debugging -->

<!-- <logger name="org.hibernate.search.elasticsearch.request" additivity="false" level="trace">-->
<!-- <appender-ref ref="STDOUT" />-->
<!-- </logger>-->
<!-- <logger name="org.elasticsearch.client" level="debug" additivity="false">-->
<!-- <appender-ref ref="STDOUT" />-->
<!-- </logger>-->
<!-- <logger name="tracer" level="TRACE" additivity="false">-->
<!-- <appender-ref ref="STDOUT" />-->
<!-- </logger>-->
<logger name="org.hibernate.search.elasticsearch.request" additivity="false" level="trace">
<appender-ref ref="STDOUT" />
</logger>
<logger name="org.elasticsearch.client" level="debug" additivity="false">
<appender-ref ref="STDOUT" />
</logger>
<logger name="tracer" level="TRACE" additivity="false">
<appender-ref ref="STDOUT" />
</logger>


</configuration>
169 changes: 0 additions & 169 deletions hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSearchDaoR4Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@
import ca.uhn.fhir.rest.param.StringAndListParam;
import ca.uhn.fhir.rest.param.StringOrListParam;
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 org.hl7.fhir.r4.model.Organization;
import org.hl7.fhir.r4.model.Patient;
import org.junit.jupiter.api.AfterEach;
Expand All @@ -29,7 +26,6 @@
import org.springframework.transaction.support.TransactionSynchronizationManager;

import javax.sql.DataSource;
import java.util.ArrayList;
import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -304,169 +300,4 @@ public void testNarrativeSearch() {
assertThat(JpaPid.toLongList(found)).isEmpty();
}
}

@Test
public void testSearchNarrativeWithLuceneSearch() {
final int numberOfPatientsToCreate = SearchBuilder.getMaximumPageSize() + 10;
List<String> expectedActivePatientIds = new ArrayList<>(numberOfPatientsToCreate);

for (int i = 0; i < numberOfPatientsToCreate; i++) {
Patient patient = new Patient();
patient.getText().setDivAsString("<div>AAAS<p>FOO</p> CCC </div>");
expectedActivePatientIds.add(myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless().getIdPart());
}

{
Patient patient = new Patient();
patient.getText().setDivAsString("<div>AAAB<p>FOO</p> CCC </div>");
myPatientDao.create(patient, mySrd);
}
{
Patient patient = new Patient();
patient.getText().setDivAsString("<div>ZZYZXY</div>");
myPatientDao.create(patient, mySrd);
}

SearchParameterMap map = new SearchParameterMap().setLoadSynchronous(true);
map.add(Constants.PARAM_TEXT, new StringParam("AAAS"));

IBundleProvider searchResultBundle = myPatientDao.search(map, mySrd);
List<String> resourceIdsFromSearchResult = searchResultBundle.getAllResourceIds();

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
public void testLuceneNarrativeSearchQueryIntersectingJpaQuery() {
final int numberOfPatientsToCreate = SearchBuilder.getMaximumPageSize() + 10;
List<String> expectedActivePatientIds = new ArrayList<>(numberOfPatientsToCreate);

// create active and non-active patients with the same narrative
for (int i = 0; i < numberOfPatientsToCreate; i++) {
Patient activePatient = new Patient();
activePatient.getText().setDivAsString("<div>AAAS<p>FOO</p> CCC </div>");
activePatient.setActive(true);
String patientId = myPatientDao.create(activePatient, mySrd).getId().toUnqualifiedVersionless().getIdPart();
expectedActivePatientIds.add(patientId);

Patient nonActivePatient = new Patient();
nonActivePatient.getText().setDivAsString("<div>AAAS<p>FOO</p> CCC </div>");
nonActivePatient.setActive(false);
myPatientDao.create(nonActivePatient, mySrd);
}

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_TEXT, new StringParam("AAAS"));

IBundleProvider searchResultBundle = myPatientDao.search(map, mySrd);
List<String> resourceIdsFromSearchResult = searchResultBundle.getAllResourceIds();

assertThat(resourceIdsFromSearchResult).containsExactlyInAnyOrderElementsOf(expectedActivePatientIds);
}

@Test
public void testLuceneContentSearchQueryIntersectingJpaQuery() {
final int numberOfPatientsToCreate = SearchBuilder.getMaximumPageSize() + 10;
final String patientFamilyName = "Flanders";
List<String> expectedActivePatientIds = new ArrayList<>(numberOfPatientsToCreate);

// create active and non-active patients with the same narrative
for (int i = 0; i < numberOfPatientsToCreate; i++) {
Patient activePatient = new Patient();
activePatient.addName().setFamily(patientFamilyName);
activePatient.setActive(true);
String patientId = myPatientDao.create(activePatient, mySrd).getId().toUnqualifiedVersionless().getIdPart();
expectedActivePatientIds.add(patientId);

Patient nonActivePatient = new Patient();
nonActivePatient.addName().setFamily(patientFamilyName);
nonActivePatient.setActive(false);
myPatientDao.create(nonActivePatient, mySrd);
}

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

IBundleProvider searchResultBundle = myPatientDao.search(map, mySrd);
List<String> resourceIdsFromSearchResult = searchResultBundle.getAllResourceIds();

assertThat(resourceIdsFromSearchResult).containsExactlyInAnyOrderElementsOf(expectedActivePatientIds);
}
}
6 changes: 1 addition & 5 deletions ...r-test-r4/src/test/java/ca/uhn/fhir/jpa/search/ExtendedHSearchResourceProjectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package ca.uhn.fhir.jpa.search;

import static org.junit.jupiter.api.Assertions.assertEquals;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.jpa.dao.search.ExtendedHSearchResourceProjection;
Expand All @@ -12,6 +11,7 @@

import static ca.uhn.fhir.jpa.dao.search.ExtendedHSearchResourceProjection.RESOURCE_NOT_STORED_ERROR;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

class ExtendedHSearchResourceProjectionTest {
Expand Down Expand Up @@ -57,8 +57,4 @@ public void emptyResourceStringThrows() {
() -> new ExtendedHSearchResourceProjection(22, null, ""));
assertEquals(Msg.code(2130) + RESOURCE_NOT_STORED_ERROR + "22", ex.getMessage());
}




}
4 changes: 2 additions & 2 deletions hapi-fhir-jpaserver-test-r4/src/test/resources/logback-test.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@
<logger name="ca.uhn.fhir.jpa.bulk" level="info"/>

<!-- more debugging -->
<!--
<!-- &lt;!&ndash;-->
<logger name="org.elasticsearch.client" level="trace"/>
<logger name="org.hibernate.search.elasticsearch.request" level="TRACE"/>
<logger name="ca.uhn.fhir.jpa.model.search" level="debug"/>
<logger name="org.elasticsearch.client" level="trace"/>
<logger name="org.hibernate.search" level="debug"/>
<logger name="org.hibernate.search.query" level="TRACE"/>
<logger name="org.hibernate.search.elasticsearch.request" level="TRACE"/>
-->
<!-- &ndash;&gt;-->
<!-- See https://docs.jboss.org/hibernate/stable/search/reference/en-US/html_single/#backend-lucene-io-writer-infostream for lucene logging
<logger name="org.hibernate.search.backend.lucene.infostream" level="TRACE"/> -->

Expand Down
Loading
Loading