Issue with combo index search parameter when using date (#6566)

* WIP: commenting out default value assignement for the dateParam prefix.  The purpose is to run the 'fix' through the pipelines to see what fails.

* spotless

* fixing spotless

* updated solution and tests

* fixing tests

* adding changelog

* fixing comment for clarity.

* fixing changelog filename

* spotless

* addressing comments from code review.

* addressing comments from second code review.

* providing new solution where = and =eq should yield the same behavior.

* spotless happiness.

---------

Co-authored-by: peartree <etienne.poirier@smilecdr.com>
This commit is contained in:
Etienne Poirier 2025-01-16 08:22:05 -05:00 committed by GitHub
parent 6f8ba9ab47
commit 69cb38f005
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 172 additions and 111 deletions

View File

@ -28,6 +28,7 @@ import ca.uhn.fhir.rest.api.QualifiedParamList;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.util.DateUtils;
import jakarta.annotation.Nonnull;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.Validate;
import org.hl7.fhir.instance.model.api.IPrimitiveType;
@ -182,11 +183,9 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
}
private void addParam(DateParam theParsed) throws InvalidRequestException {
if (theParsed.getPrefix() == null) {
theParsed.setPrefix(EQUAL);
}
ParamPrefixEnum prefix = getPrefixOrDefault(theParsed);
switch (theParsed.getPrefix()) {
switch (prefix) {
case NOT_EQUAL:
case EQUAL:
if (myLowerBound != null || myUpperBound != null) {
@ -312,8 +311,9 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
}
int retVal = DateUtils.convertDateToDayInteger(myLowerBound.getValue());
if (myLowerBound.getPrefix() != null) {
switch (myLowerBound.getPrefix()) {
ParamPrefixEnum prefix = getPrefixOrDefault(myLowerBound);
switch (prefix) {
case GREATERTHAN:
case STARTS_AFTER:
retVal += 1;
@ -326,10 +326,9 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
case LESSTHAN_OR_EQUALS:
case APPROXIMATE:
case ENDS_BEFORE:
throw new IllegalStateException(
Msg.code(1926) + "Invalid lower bound comparator: " + myLowerBound.getPrefix());
}
throw new IllegalStateException(Msg.code(1926) + "Invalid lower bound comparator: " + prefix);
}
return retVal;
}
@ -343,8 +342,10 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
return null;
}
int retVal = DateUtils.convertDateToDayInteger(myUpperBound.getValue());
if (myUpperBound.getPrefix() != null) {
switch (myUpperBound.getPrefix()) {
ParamPrefixEnum prefix = getPrefixOrDefault(myUpperBound);
switch (prefix) {
case LESSTHAN:
case ENDS_BEFORE:
retVal -= 1;
@ -357,10 +358,9 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
case GREATERTHAN:
case APPROXIMATE:
case STARTS_AFTER:
throw new IllegalStateException(
Msg.code(1927) + "Invalid upper bound comparator: " + myUpperBound.getPrefix());
}
throw new IllegalStateException(Msg.code(1927) + "Invalid upper bound comparator: " + prefix);
}
return retVal;
}
@ -374,12 +374,14 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
@Nonnull
private static Date getLowerBoundAsInstant(@Nonnull DateParam theLowerBound) {
Date retVal = theLowerBound.getValue();
if (theLowerBound.getPrecision().ordinal() <= TemporalPrecisionEnum.DAY.ordinal()) {
retVal = DateUtils.getLowestInstantFromDate(retVal);
}
if (theLowerBound.getPrefix() != null) {
switch (theLowerBound.getPrefix()) {
ParamPrefixEnum prefix = getPrefixOrDefault(theLowerBound);
switch (prefix) {
case GREATERTHAN:
case STARTS_AFTER:
retVal = theLowerBound.getPrecision().add(retVal, 1);
@ -392,10 +394,9 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
case LESSTHAN:
case APPROXIMATE:
case ENDS_BEFORE:
throw new IllegalStateException(
Msg.code(1928) + "Invalid lower bound comparator: " + theLowerBound.getPrefix());
}
throw new IllegalStateException(Msg.code(1928) + "Invalid lower bound comparator: " + prefix);
}
return retVal;
}
@ -446,8 +447,9 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
retVal = DateUtils.getHighestInstantFromDate(retVal);
}
if (theUpperBound.getPrefix() != null) {
switch (theUpperBound.getPrefix()) {
ParamPrefixEnum prefix = getPrefixOrDefault(theUpperBound);
switch (prefix) {
case LESSTHAN:
case ENDS_BEFORE:
retVal = new Date(retVal.getTime() - 1L);
@ -462,10 +464,9 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
case GREATERTHAN:
case APPROXIMATE:
case STARTS_AFTER:
throw new IllegalStateException(
Msg.code(1929) + "Invalid upper bound comparator: " + theUpperBound.getPrefix());
}
throw new IllegalStateException(Msg.code(1929) + "Invalid upper bound comparator: " + prefix);
}
return retVal;
}
@ -586,6 +587,16 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
FhirContext theContext, String theParamName, List<QualifiedParamList> theParameters)
throws InvalidRequestException {
// When we create and populate a DateRangeParam from a query parameter (?birthdate=2024-12-02 or
// ?birthdate=eq2024-12-02), we
// set the prefix only if it was specifically provided by the client as it is mandatory to retain the capability
// to make the differentiation. See {@link SearchBuilder#validateParamValuesAreValidForComboParam}.
//
// Since the FHIR specification says that "If no prefix is present, the prefix <code>eq</code> is assumed",
// we will do so by invoking method {@link DateRangeParam#getPrefixOrDefault} everytime computation is
// conditional on the
// prefix value.
boolean haveHadUnqualifiedParameter = false;
for (QualifiedParamList paramList : theParameters) {
if (paramList.size() == 0) {
@ -701,4 +712,13 @@ public class DateRangeParam implements IQueryParameterAnd<DateParam> {
myLowerBound = lowerBound;
myUpperBound = upperBound;
}
/**
*
* This method should be used when performing computation conditional on the prefix value to ensure that a dateParam
* without prefix is treated as if it has one set to 'eq'.
*/
private static ParamPrefixEnum getPrefixOrDefault(DateParam theDateParam) {
return ObjectUtils.defaultIfNull(theDateParam.getPrefix(), EQUAL);
}
}

View File

@ -0,0 +1,8 @@
---
type: fix
issue: 6603
jira: SMILE-8048
title: "Previously, searches where processing was optimised with a combo index search parameters would skip searching
the optimised indexes if a date query string was prefixed with 'eq'. Since date=2025-01-01 and date=eq2025-01-01 are
equivalent search queries, we have harmonized the search behavior allowing optimised indexes inspection when the prefix
is present. This issue is fixed."

View File

@ -93,6 +93,7 @@ import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.param.BaseParamWithPrefix;
import ca.uhn.fhir.rest.param.DateParam;
import ca.uhn.fhir.rest.param.DateRangeParam;
import ca.uhn.fhir.rest.param.ParamPrefixEnum;
import ca.uhn.fhir.rest.param.ParameterUtil;
import ca.uhn.fhir.rest.param.ReferenceParam;
import ca.uhn.fhir.rest.param.StringParam;
@ -152,11 +153,13 @@ import static ca.uhn.fhir.jpa.model.util.JpaConstants.UNDESIRED_RESOURCE_LINKAGE
import static ca.uhn.fhir.jpa.search.builder.QueryStack.LOCATION_POSITION;
import static ca.uhn.fhir.jpa.search.builder.QueryStack.SearchForIdsParams.with;
import static ca.uhn.fhir.jpa.util.InClauseNormalizer.normalizeIdListForInClause;
import static ca.uhn.fhir.rest.param.ParamPrefixEnum.EQUAL;
import static java.util.Objects.requireNonNull;
import static org.apache.commons.collections4.CollectionUtils.isNotEmpty;
import static org.apache.commons.lang3.StringUtils.defaultString;
import static org.apache.commons.lang3.StringUtils.isBlank;
import static org.apache.commons.lang3.StringUtils.isNotBlank;
import static org.apache.commons.lang3.StringUtils.stripStart;
/**
* The SearchBuilder is responsible for actually forming the SQL query that handles
@ -2354,7 +2357,9 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
String nextParamName = theComboParamNames.get(paramIndex);
IQueryParameterType nextOr = nextPermutation.get(paramIndex);
String nextOrValue = nextOr.getValueAsQueryToken(myContext);
// The only prefix accepted when combo searching is 'eq' (see validateParamValuesAreValidForComboParam).
// As a result, we strip the prefix if present.
String nextOrValue = stripStart(nextOr.getValueAsQueryToken(myContext), EQUAL.getValue());
RuntimeSearchParam nextParamDef = mySearchParamRegistry.getActiveSearchParam(
myResourceName, nextParamName, ISearchParamRegistry.SearchParamLookupContextEnum.SEARCH);
@ -2453,7 +2458,10 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
}
if (nextOrValue instanceof BaseParamWithPrefix) {
BaseParamWithPrefix<?> paramWithPrefix = (BaseParamWithPrefix<?>) nextOrValue;
if (paramWithPrefix.getPrefix() != null) {
ParamPrefixEnum prefix = paramWithPrefix.getPrefix();
// A parameter with the 'eq' prefix is the only accepted prefix when combo searching since
// birthdate=2025-01-01 and birthdate=eq2025-01-01 are equivalent searches.
if (prefix != null && prefix != EQUAL) {
String message = "Search with params " + theComboParamNames
+ " is not a candidate for combo searching - Parameter '" + nextParamName
+ "' has prefix: '"

View File

@ -826,10 +826,7 @@ public class SearchQueryBuilder {
DateParam lb = theDateRange.getLowerBound();
DateParam ub = theDateRange.getUpperBound();
return lb != null
&& ub != null
&& lb.getPrefix().equals(NOT_EQUAL)
&& ub.getPrefix().equals(NOT_EQUAL);
return lb != null && ub != null && NOT_EQUAL.equals(lb.getPrefix()) && NOT_EQUAL.equals(ub.getPrefix());
}
return false;
}

View File

@ -567,8 +567,8 @@ public class SearchParameterMap implements Serializable {
private boolean isNotEqualsComparator(DateParam theLowerBound, DateParam theUpperBound) {
return theLowerBound != null
&& theUpperBound != null
&& theLowerBound.getPrefix().equals(NOT_EQUAL)
&& theUpperBound.getPrefix().equals(NOT_EQUAL);
&& NOT_EQUAL.equals(theLowerBound.getPrefix())
&& NOT_EQUAL.equals(theUpperBound.getPrefix());
}
/**

View File

@ -7,6 +7,7 @@ import ca.uhn.fhir.jpa.model.search.StorageProcessingMessage;
import ca.uhn.fhir.jpa.search.DatabaseBackedPagingProvider;
import ca.uhn.fhir.jpa.search.reindex.ResourceReindexingSvcImpl;
import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
import ca.uhn.fhir.jpa.test.util.ComboSearchParameterTestHelper;
import ca.uhn.fhir.jpa.util.SpringObjectCaster;
import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
import ca.uhn.fhir.test.utilities.MockInvoker;
@ -30,6 +31,7 @@ public abstract class BaseComboParamsR4Test extends BaseJpaR4Test {
protected ISearchParamRegistry mySearchParamRegistry;
protected List<String> myMessages = new ArrayList<>();
private IInterceptorBroadcaster myInterceptorBroadcaster;
protected ComboSearchParameterTestHelper myComboSearchParameterTestHelper;
@Override
@BeforeEach
@ -62,6 +64,7 @@ public abstract class BaseComboParamsR4Test extends BaseJpaR4Test {
// allow searches to use cached results
when(myInterceptorBroadcaster.getInvokersForPointcut(eq(Pointcut.STORAGE_PRECHECK_FOR_CACHED_SEARCH))).thenReturn(MockInvoker.list(params->true));
myComboSearchParameterTestHelper = new ComboSearchParameterTestHelper(mySearchParameterDao, mySearchParamRegistry);
}
@AfterEach
@ -80,5 +83,9 @@ public abstract class BaseComboParamsR4Test extends BaseJpaR4Test {
ourLog.info("Messages:\n {}", String.join("\n ", myMessages));
}
protected void createBirthdateAndGenderSps(boolean theUnique) {
myComboSearchParameterTestHelper.createBirthdateAndGenderSps(theUnique);
myMessages.clear();
}
}

View File

@ -216,7 +216,7 @@ public class FhirResourceDaoR4ComboNonUniqueParamTest extends BaseComboParamsR4T
IIdType id1 = createPatient1(null);
assertNotNull(id1);
assertThat(myCaptureQueriesListener.countSelectQueries()).as(String.join(",", "\n" + myCaptureQueriesListener.getSelectQueries().stream().map(SqlQuery::getThreadName).toList())).isEqualTo(0);
assertThat(myCaptureQueriesListener.countSelectQueries()).as(String.join(",", "\n" + myCaptureQueriesListener.getSelectQueries().stream().map(SqlQuery::getThreadName).toList())).isZero();
assertEquals(12, myCaptureQueriesListener.countInsertQueries());
assertEquals(0, myCaptureQueriesListener.countUpdateQueries());
assertEquals(0, myCaptureQueriesListener.countDeleteQueries());
@ -422,7 +422,6 @@ public class FhirResourceDaoR4ComboNonUniqueParamTest extends BaseComboParamsR4T
assertThat(actual).contains(id1.toUnqualifiedVersionless().getValue());
myCaptureQueriesListener.logSelectQueries();
String expected;
assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false)).contains("where (rt1_0.FHIR_ID='my-org')");
String sql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(1).getSql(true, false);
assertThat(sql).contains("SP_VALUE_NORMALIZED LIKE 'FAMILY1%'");
@ -907,6 +906,38 @@ public class FhirResourceDaoR4ComboNonUniqueParamTest extends BaseComboParamsR4T
}
/**
* This test asserts that a date query string prefixed with 'eq' still qualifies the query for combo indexes
* inspection. This approach is preferred over disqualifying the query as it would be the case with any other
* prefixes (gt, le, lt, etc) since date=2025-01-01 and date=eq2025-01-01 are equivalent searches.
*/
@ParameterizedTest
@ValueSource(strings = {
"2025-01-01",
"eq2025-01-01"})
public void testSearchWithDateQueryString_whenModifierEqualIsPresent_willUseComboSearchIndexes(String theDateQueryParameter){
// given
IIdType id1 = createObservation("2025-01-01");
SearchParameterMap params = SearchParameterMap
.newSynchronous()
.add("note-text", new StringParam("Hello"))
.add("date", new DateParam(theDateQueryParameter));
logAllNonUniqueIndexes();
logAllDateIndexes();
myCaptureQueriesListener.clear();
// when
IBundleProvider results = myObservationDao.search(params, mySrd);
// then
assertComboIndexUsed();
List<String> actual = toUnqualifiedVersionlessIdValues(results);
assertThat(actual).contains(id1.toUnqualifiedVersionless().getValue());
}
@ParameterizedTest
@ValueSource(booleans = {true, false})
public void testStringModifier(boolean theUseExact) {
@ -982,6 +1013,4 @@ public class FhirResourceDaoR4ComboNonUniqueParamTest extends BaseComboParamsR4T
}
}
}

View File

@ -13,7 +13,6 @@ import ca.uhn.fhir.jpa.model.entity.ResourceIndexedComboStringUnique;
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import ca.uhn.fhir.jpa.searchparam.util.JpaParamUtil;
import ca.uhn.fhir.jpa.test.util.ComboSearchParameterTestHelper;
import ca.uhn.fhir.rest.api.server.IBundleProvider;
import ca.uhn.fhir.rest.param.DateParam;
import ca.uhn.fhir.rest.param.ReferenceParam;
@ -45,7 +44,6 @@ import org.hl7.fhir.r4.model.Reference;
import org.hl7.fhir.r4.model.SearchParameter;
import org.hl7.fhir.r4.model.ServiceRequest;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.transaction.TransactionStatus;
@ -72,23 +70,12 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Autowired
private IJobCoordinator myJobCoordinator;
private ComboSearchParameterTestHelper myComboSearchParameterTestHelper;
@BeforeEach
public void beforeEach() {
myComboSearchParameterTestHelper = new ComboSearchParameterTestHelper(mySearchParameterDao, mySearchParamRegistry);
}
@AfterEach
public void purgeUniqueIndexes() {
myResourceIndexedComboStringUniqueDao.deleteAll();
}
private void createUniqueBirthdateAndGenderSps() {
myComboSearchParameterTestHelper.createBirthdateAndGenderSps(true);
myMessages.clear();
}
private void createUniqueGenderFamilyComboSp() {
SearchParameter sp = new SearchParameter();
sp.setId("SearchParameter/patient-gender");
@ -896,7 +883,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
public void testDuplicateUniqueValuesAreRejectedWithChecking_TestingDisabled() {
myStorageSettings.setUniqueIndexesCheckedBeforeSave(false);
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
Patient pt1 = new Patient();
pt1.setGender(Enumerations.AdministrativeGender.MALE);
@ -913,7 +900,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Test
public void testDuplicateUniqueValuesAreRejectedWithChecking_TestingEnabled() {
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
Patient pt1 = new Patient();
pt1.setGender(Enumerations.AdministrativeGender.MALE);
@ -1167,7 +1154,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Test
public void testNonTransaction() {
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
Patient p = new Patient();
p.setGender(Enumerations.AdministrativeGender.MALE);
@ -1358,7 +1345,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Test
public void testUniqueValuesAreIndexed_DateAndToken() {
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
Patient pt1 = new Patient();
pt1.setGender(Enumerations.AdministrativeGender.MALE);
@ -1604,7 +1591,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Test
public void testUniqueValuesAreNotIndexedIfNotAllParamsAreFound_DateAndToken() {
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
Patient pt;
@ -1716,7 +1703,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Test
public void testDetectUniqueSearchParams() {
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
List<RuntimeSearchParam> params = mySearchParamRegistry.getActiveComboSearchParams("Patient", null);
assertThat(params).hasSize(1);
@ -1731,7 +1718,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Test
public void testDuplicateUniqueValuesWithDateAreRejected() {
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
Patient pt1 = new Patient();
pt1.setGender(Enumerations.AdministrativeGender.MALE);
@ -1782,7 +1769,7 @@ public class FhirResourceDaoR4ComboUniqueParamTest extends BaseComboParamsR4Test
@Test
public void testUniqueComboSearchWithDateNotUsingUniqueIndex() {
createUniqueBirthdateAndGenderSps();
createBirthdateAndGenderSps(true);
Patient pt1 = new Patient();
pt1.setGender(Enumerations.AdministrativeGender.MALE);

View File

@ -103,7 +103,7 @@ public class DateRangeParamR4Test {
}
@Test
public void testSearchForOneUnqualifiedDate() throws Exception {
public void testSearchWithUnqualifiedDate_shouldRemainUnqualified() throws Exception {
HttpGet httpGet = new HttpGet(ourServer.getBaseUrl() + "/Patient?birthdate=2012-01-01");
CloseableHttpResponse status = ourClient.execute(httpGet);
consumeResponse(status);
@ -114,8 +114,13 @@ public class DateRangeParamR4Test {
assertEquals(parseLowerForDatePrecision("2012-01-01 00:00:00.0000"), ourLastDateRange.getLowerBoundAsInstant());
assertEquals(parseUpperForDatePrecision("2012-01-03 00:00:00.0000"), ourLastDateRange.getUpperBoundAsInstant());
assertEquals(ParamPrefixEnum.EQUAL, ourLastDateRange.getLowerBound().getPrefix());
assertEquals(ParamPrefixEnum.EQUAL, ourLastDateRange.getUpperBound().getPrefix());
// In order for combo search indexes to be searched, it is required that date query parameters not include a date
// modifier (eq, gt,le, etc). Subsequently, parsing a date query parameter should result in Lower/upper bound prefixes
// having values only if specifically provided as part of the date query string (birthdate=eq2012-01-01) and not be given the default
// value of 'EQUAL'.
assertNull(ourLastDateRange.getLowerBound().getPrefix());
assertNull(ourLastDateRange.getUpperBound().getPrefix());
}
@Test
@ -467,7 +472,7 @@ public class DateRangeParamR4Test {
}
@AfterAll
public static void afterClassClearContext() throws Exception {
public static void afterClassClearContext() {
TestUtil.randomizeLocaleAndTimezone();
}