From a01170997cc530eeb463bf80a18a384167a2e9c3 Mon Sep 17 00:00:00 2001 From: peartree Date: Wed, 18 Dec 2024 13:31:36 -0500 Subject: [PATCH 01/11] WIP: commenting out default value assignement for the dateParam prefix. The purpose is to run the 'fix' through the pipelines to see what fails. --- .../java/ca/uhn/fhir/rest/param/DateRangeParam.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java index a2fbe76e1ad1..70ffeed82fd2 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java @@ -28,6 +28,7 @@ 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,13 @@ public DateRangeParam(String theLowerBound, String theUpperBound) { } private void addParam(DateParam theParsed) throws InvalidRequestException { - if (theParsed.getPrefix() == null) { - theParsed.setPrefix(EQUAL); - } +// if (theParsed.getPrefix() == null) { +// theParsed.setPrefix(EQUAL); +// } + + ParamPrefixEnum prefix = ObjectUtils.defaultIfNull(theParsed.getPrefix(), EQUAL); - switch (theParsed.getPrefix()) { + switch (prefix) { case NOT_EQUAL: case EQUAL: if (myLowerBound != null || myUpperBound != null) { From cfdb381b9a1f66ebcf71a7b5672961aa03d11a91 Mon Sep 17 00:00:00 2001 From: peartree Date: Wed, 18 Dec 2024 13:47:17 -0500 Subject: [PATCH 02/11] spotless --- .../java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java index 018ba9331e95..7b3eb9599b9b 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java @@ -112,8 +112,8 @@ public void testSearchForOneUnqualifiedDate() throws Exception { assertEquals("2012-01-01", ourLastDateRange.getLowerBound().getValueAsString()); assertEquals("2012-01-01", ourLastDateRange.getUpperBound().getValueAsString()); - assertEquals(parseLowerForDatePrecision("2012-01-01 00:00:00.0000"), ourLastDateRange.getLowerBoundAsInstant()); - assertEquals(parseUpperForDatePrecision("2012-01-03 00:00:00.0000"), ourLastDateRange.getUpperBoundAsInstant()); +// 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()); } From fc9df9ae0357db0427e2c3c9f1dad95a3053cafd Mon Sep 17 00:00:00 2001 From: peartree Date: Tue, 7 Jan 2025 10:01:04 -0500 Subject: [PATCH 03/11] fixing spotless --- .../main/java/ca/uhn/fhir/rest/param/DateRangeParam.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java index 70ffeed82fd2..bd44ebce780e 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java @@ -183,9 +183,9 @@ public DateRangeParam(String theLowerBound, String theUpperBound) { } private void addParam(DateParam theParsed) throws InvalidRequestException { -// if (theParsed.getPrefix() == null) { -// theParsed.setPrefix(EQUAL); -// } + // if (theParsed.getPrefix() == null) { + // theParsed.setPrefix(EQUAL); + // } ParamPrefixEnum prefix = ObjectUtils.defaultIfNull(theParsed.getPrefix(), EQUAL); From 3c5227df046785b2ac472875b38ec0a9b4a4d266 Mon Sep 17 00:00:00 2001 From: peartree Date: Wed, 8 Jan 2025 13:50:43 -0500 Subject: [PATCH 04/11] updated solution and tests --- .../uhn/fhir/rest/param/DateRangeParam.java | 166 ++++++++++-------- .../fhir/rest/param/DateRangeParamR4Test.java | 8 +- 2 files changed, 95 insertions(+), 79 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java index bd44ebce780e..c3317b57f1b9 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java @@ -183,11 +183,7 @@ public DateRangeParam(String theLowerBound, String theUpperBound) { } private void addParam(DateParam theParsed) throws InvalidRequestException { - // if (theParsed.getPrefix() == null) { - // theParsed.setPrefix(EQUAL); - // } - - ParamPrefixEnum prefix = ObjectUtils.defaultIfNull(theParsed.getPrefix(), EQUAL); + ParamPrefixEnum prefix = getPrefixOrDefault(theParsed); switch (prefix) { case NOT_EQUAL: @@ -315,24 +311,24 @@ public Integer getLowerBoundAsDateInteger() { } int retVal = DateUtils.convertDateToDayInteger(myLowerBound.getValue()); - if (myLowerBound.getPrefix() != null) { - switch (myLowerBound.getPrefix()) { - case GREATERTHAN: - case STARTS_AFTER: - retVal += 1; - break; - case EQUAL: - case GREATERTHAN_OR_EQUALS: - case NOT_EQUAL: - break; - case LESSTHAN: - case LESSTHAN_OR_EQUALS: - case APPROXIMATE: - case ENDS_BEFORE: - throw new IllegalStateException( - Msg.code(1926) + "Invalid lower bound comparator: " + myLowerBound.getPrefix()); - } + ParamPrefixEnum prefix = getPrefixOrDefault(myLowerBound); + + switch (prefix) { + case GREATERTHAN: + case STARTS_AFTER: + retVal += 1; + break; + case EQUAL: + case GREATERTHAN_OR_EQUALS: + case NOT_EQUAL: + break; + case LESSTHAN: + case LESSTHAN_OR_EQUALS: + case APPROXIMATE: + case ENDS_BEFORE: + throw new IllegalStateException(Msg.code(1926) + "Invalid lower bound comparator: " + prefix); } + return retVal; } @@ -346,24 +342,25 @@ public Integer getUpperBoundAsDateInteger() { return null; } int retVal = DateUtils.convertDateToDayInteger(myUpperBound.getValue()); - if (myUpperBound.getPrefix() != null) { - switch (myUpperBound.getPrefix()) { - case LESSTHAN: - case ENDS_BEFORE: - retVal -= 1; - break; - case EQUAL: - case LESSTHAN_OR_EQUALS: - case NOT_EQUAL: - break; - case GREATERTHAN_OR_EQUALS: - case GREATERTHAN: - case APPROXIMATE: - case STARTS_AFTER: - throw new IllegalStateException( - Msg.code(1927) + "Invalid upper bound comparator: " + myUpperBound.getPrefix()); - } + + ParamPrefixEnum prefix = getPrefixOrDefault(myUpperBound); + + switch (prefix) { + case LESSTHAN: + case ENDS_BEFORE: + retVal -= 1; + break; + case EQUAL: + case LESSTHAN_OR_EQUALS: + case NOT_EQUAL: + break; + case GREATERTHAN_OR_EQUALS: + case GREATERTHAN: + case APPROXIMATE: + case STARTS_AFTER: + throw new IllegalStateException(Msg.code(1927) + "Invalid upper bound comparator: " + prefix); } + return retVal; } @@ -377,28 +374,29 @@ public Date getLowerBoundAsInstant() { @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()) { - case GREATERTHAN: - case STARTS_AFTER: - retVal = theLowerBound.getPrecision().add(retVal, 1); - break; - case EQUAL: - case NOT_EQUAL: - case GREATERTHAN_OR_EQUALS: - break; - case LESSTHAN_OR_EQUALS: - case LESSTHAN: - case APPROXIMATE: - case ENDS_BEFORE: - throw new IllegalStateException( - Msg.code(1928) + "Invalid lower bound comparator: " + theLowerBound.getPrefix()); - } + ParamPrefixEnum prefix = getPrefixOrDefault(theLowerBound); + + switch (prefix) { + case GREATERTHAN: + case STARTS_AFTER: + retVal = theLowerBound.getPrecision().add(retVal, 1); + break; + case EQUAL: + case NOT_EQUAL: + case GREATERTHAN_OR_EQUALS: + break; + case LESSTHAN_OR_EQUALS: + case LESSTHAN: + case APPROXIMATE: + case ENDS_BEFORE: + throw new IllegalStateException(Msg.code(1928) + "Invalid lower bound comparator: " + prefix); } + return retVal; } @@ -449,26 +447,26 @@ private static Date getUpperBoundAsInstant(@Nonnull DateParam theUpperBound) { retVal = DateUtils.getHighestInstantFromDate(retVal); } - if (theUpperBound.getPrefix() != null) { - switch (theUpperBound.getPrefix()) { - case LESSTHAN: - case ENDS_BEFORE: - retVal = new Date(retVal.getTime() - 1L); - break; - case EQUAL: - case NOT_EQUAL: - case LESSTHAN_OR_EQUALS: - retVal = theUpperBound.getPrecision().add(retVal, 1); - retVal = new Date(retVal.getTime() - 1L); - break; - case GREATERTHAN_OR_EQUALS: - case GREATERTHAN: - case APPROXIMATE: - case STARTS_AFTER: - throw new IllegalStateException( - Msg.code(1929) + "Invalid upper bound comparator: " + theUpperBound.getPrefix()); - } + ParamPrefixEnum prefix = getPrefixOrDefault(theUpperBound); + + switch (prefix) { + case LESSTHAN: + case ENDS_BEFORE: + retVal = new Date(retVal.getTime() - 1L); + break; + case EQUAL: + case NOT_EQUAL: + case LESSTHAN_OR_EQUALS: + retVal = theUpperBound.getPrecision().add(retVal, 1); + retVal = new Date(retVal.getTime() - 1L); + break; + case GREATERTHAN_OR_EQUALS: + case GREATERTHAN: + case APPROXIMATE: + case STARTS_AFTER: + throw new IllegalStateException(Msg.code(1929) + "Invalid upper bound comparator: " + prefix); } + return retVal; } @@ -704,4 +702,22 @@ private void validateAndSet(DateParam lowerBound, DateParam upperBound) { myLowerBound = lowerBound; myUpperBound = upperBound; } + + /** + * As per FHIR specification "If no prefix is present, the prefix eq is assumed". + * + * All constructors and setters of this class will set the 'prefix' on myLowerBound and myUpperBound + * properties except when a DateRangeParam is created from query parameter (?birthdate=2024-12-02 or ?birthdate=eq2024-12-02). See + * {@link DateRangeParam#setValuesAsQueryTokens}. + * + * In such scenario, it is mandatory to retain the capability to determine whether the 'eq' modifier is provided or + * not by the client as searching against a combo search index parameter can only be invoked when date query parameter + * have no modifier. + * + * 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); + } } diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java index 7b3eb9599b9b..93d3fc956e98 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java @@ -112,10 +112,10 @@ public void testSearchForOneUnqualifiedDate() throws Exception { assertEquals("2012-01-01", ourLastDateRange.getLowerBound().getValueAsString()); assertEquals("2012-01-01", ourLastDateRange.getUpperBound().getValueAsString()); -// 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()); + assertEquals(parseLowerForDatePrecision("2012-01-01 00:00:00.0000"), ourLastDateRange.getLowerBoundAsInstant()); + assertEquals(parseUpperForDatePrecision("2012-01-03 00:00:00.0000"), ourLastDateRange.getUpperBoundAsInstant()); + assertNull(ourLastDateRange.getLowerBound().getPrefix()); + assertNull(ourLastDateRange.getUpperBound().getPrefix()); } @Test From eb9bd3cc47837a85cb87a93d9dadfb3513d60d1c Mon Sep 17 00:00:00 2001 From: peartree Date: Thu, 9 Jan 2025 08:28:32 -0500 Subject: [PATCH 05/11] fixing tests --- .../uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java | 5 +---- .../java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java index 7b04dc93f258..e57c03ab037c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java @@ -826,10 +826,7 @@ private boolean isNotEqualsComparator(DateRangeParam theDateRange) { 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; } diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java index 3f11fe3449d8..86c7c1662223 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java @@ -567,8 +567,8 @@ public String toNormalizedQueryString(FhirContext theCtx) { 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()); } /** From d65922c7242a8709e0898169ca7dce8070e25dd4 Mon Sep 17 00:00:00 2001 From: peartree Date: Thu, 9 Jan 2025 09:44:00 -0500 Subject: [PATCH 06/11] adding changelog --- .../6603_combo_index_sp_not_invoked_with_dateparam.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603_combo_index_sp_not_invoked_with_dateparam.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603_combo_index_sp_not_invoked_with_dateparam.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603_combo_index_sp_not_invoked_with_dateparam.yaml new file mode 100644 index 000000000000..f0b8afa9250f --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603_combo_index_sp_not_invoked_with_dateparam.yaml @@ -0,0 +1,6 @@ +--- +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 when the query string included date parameters. This issue is fixed." From f869d5cb99702a48f4b446735a6c7aed4f95a1b5 Mon Sep 17 00:00:00 2001 From: peartree Date: Thu, 9 Jan 2025 10:29:25 -0500 Subject: [PATCH 07/11] fixing comment for clarity. --- .../uhn/fhir/rest/param/DateRangeParam.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java index 5c9d2772ede3..460bb16e1cba 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java @@ -582,12 +582,20 @@ public void setRangeFromDatesInclusive(String theLowerBound, String theUpperBoun validateAndSet(lowerBound, upperBound); } - @Override + @Override public void setValuesAsQueryTokens( FhirContext theContext, String theParamName, List theParameters) throws InvalidRequestException { - boolean haveHadUnqualifiedParameter = false; + // 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 eq 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) { continue; @@ -704,15 +712,6 @@ private void validateAndSet(DateParam lowerBound, DateParam upperBound) { } /** - * As per FHIR specification "If no prefix is present, the prefix eq is assumed". - * - * All constructors and setters of this class will set the 'prefix' on myLowerBound and myUpperBound - * properties except when a DateRangeParam is created from query parameter (?birthdate=2024-12-02 or ?birthdate=eq2024-12-02). See - * {@link DateRangeParam#setValuesAsQueryTokens}. - * - * In such scenario, it is mandatory to retain the capability to determine whether the 'eq' modifier is provided or - * not by the client as searching against a combo search index parameter can only be invoked when date query parameter - * have no modifier. * * 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'. From 4b800eff918591af2089616f447bc86f9d75fd99 Mon Sep 17 00:00:00 2001 From: peartree Date: Thu, 9 Jan 2025 10:42:15 -0500 Subject: [PATCH 08/11] fixing changelog filename --- ...m.yaml => 6603-combo-index-sp-not-invoked-with-dateparam.yaml} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/{6603_combo_index_sp_not_invoked_with_dateparam.yaml => 6603-combo-index-sp-not-invoked-with-dateparam.yaml} (100%) diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603_combo_index_sp_not_invoked_with_dateparam.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml similarity index 100% rename from hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603_combo_index_sp_not_invoked_with_dateparam.yaml rename to hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml From 98a108164007caaad2e235fc03dfaf1d4ba7ee84 Mon Sep 17 00:00:00 2001 From: peartree Date: Thu, 9 Jan 2025 10:52:30 -0500 Subject: [PATCH 09/11] spotless --- .../java/ca/uhn/fhir/rest/param/DateRangeParam.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java index 460bb16e1cba..2df6979f9940 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/DateRangeParam.java @@ -582,20 +582,22 @@ public void setRangeFromDatesInclusive(String theLowerBound, String theUpperBoun validateAndSet(lowerBound, upperBound); } - @Override + @Override public void setValuesAsQueryTokens( FhirContext theContext, String theParamName, List theParameters) throws InvalidRequestException { - // When we create and populate a DateRangeParam from a query parameter (?birthdate=2024-12-02 or ?birthdate=eq2024-12-02), we + // 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 eq is assumed", - // we will do so by invoking method {@link DateRangeParam#getPrefixOrDefault} everytime computation is conditional on the + // we will do so by invoking method {@link DateRangeParam#getPrefixOrDefault} everytime computation is + // conditional on the // prefix value. - boolean haveHadUnqualifiedParameter = false; + boolean haveHadUnqualifiedParameter = false; for (QualifiedParamList paramList : theParameters) { if (paramList.size() == 0) { continue; From 3baa480a08d3862fc7e7e2fdeb7baaa9712cb1bc Mon Sep 17 00:00:00 2001 From: peartree Date: Mon, 13 Jan 2025 11:37:17 -0500 Subject: [PATCH 10/11] addressing comments from code review. --- ...o-index-sp-not-invoked-with-dateparam.yaml | 3 +- .../jpa/dao/r4/BaseComboParamsR4Test.java | 7 +++ ...rResourceDaoR4ComboNonUniqueParamTest.java | 44 ++++++++++++++++++- ...FhirResourceDaoR4ComboUniqueParamTest.java | 29 ++++-------- .../fhir/rest/param/DateRangeParamR4Test.java | 9 +++- 5 files changed, 66 insertions(+), 26 deletions(-) diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml index f0b8afa9250f..0b8b7a8f8861 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml @@ -3,4 +3,5 @@ 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 when the query string included date parameters. This issue is fixed." +optimised indexes when the query string included a date ragne (i.e. multiple instances of the same date parameter). This +issue is fixed." diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java index 5b50aa855fc0..84b699e242cb 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java @@ -7,6 +7,7 @@ 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 myMessages = new ArrayList<>(); private IInterceptorBroadcaster myInterceptorBroadcaster; + protected ComboSearchParameterTestHelper myComboSearchParameterTestHelper; @Override @BeforeEach @@ -62,6 +64,7 @@ public void before() throws Exception { // 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 @@ protected void logCapturedMessages() { ourLog.info("Messages:\n {}", String.join("\n ", myMessages)); } + protected void createBirthdateAndGenderSps(boolean theUnique) { + myComboSearchParameterTestHelper.createBirthdateAndGenderSps(theUnique); + myMessages.clear(); + } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java index 4a6f3e49091b..8407548ce6ee 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java @@ -6,9 +6,11 @@ import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.searchparam.submit.interceptor.SearchParamValidatingInterceptor; import ca.uhn.fhir.jpa.util.SqlQuery; +import ca.uhn.fhir.rest.api.QualifiedParamList; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.param.DateOrListParam; 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.ReferenceParam; import ca.uhn.fhir.rest.param.StringAndListParam; @@ -37,6 +39,7 @@ import org.junit.jupiter.params.provider.ValueSource; import org.springframework.beans.factory.annotation.Autowired; +import java.util.ArrayList; import java.util.Comparator; import java.util.List; @@ -216,7 +219,7 @@ public void testStringAndToken_CreateAndUpdate() { 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 +425,6 @@ public void testStringAndReference_SearchByUnqualifiedReference() { 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 +909,44 @@ public void testDateModifier(boolean theUseComparator) { } + @ParameterizedTest + @CsvSource(value = { + "2025-01-01, true", + "eq2025-01-01, false" + }) + public void testSearchWithDateQueryString_whenModifierEqualIsMissing_willUseComboIndexes(String theDateQueryParameter, boolean theShouldUseComboIndex){ + // given + createBirthdateAndGenderSps(false); + IIdType patientId = createPatient(withBirthdate("2025-01-01"), withGender("male")); + + logAllNonUniqueIndexes(); + logAllDateIndexes(); + + List tokens = new ArrayList<>(); + tokens.add(QualifiedParamList.singleton(null, theDateQueryParameter)); + DateRangeParam dateRangeParam = new DateRangeParam(); + dateRangeParam.setValuesAsQueryTokens(myFhirContext, Patient.SP_BIRTHDATE, tokens); + + myCaptureQueriesListener.clear(); + SearchParameterMap sp = new SearchParameterMap(); + sp.setLoadSynchronous(true); + sp.add(Patient.SP_GENDER, new TokenParam(null, "male")); + sp.add(Patient.SP_BIRTHDATE, dateRangeParam); + + // when + IBundleProvider results = myPatientDao.search(sp, mySrd); + + // then + if(theShouldUseComboIndex){ + assertComboIndexUsed(); + } else { + assertComboIndexNotUsed(); + } + + List actual = toUnqualifiedVersionlessIdValues(results); + assertThat(actual).contains(patientId.toUnqualifiedVersionless().getValue()); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) public void testStringModifier(boolean theUseExact) { diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboUniqueParamTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboUniqueParamTest.java index 5606bf0ad8e2..89f1f90da48a 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboUniqueParamTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboUniqueParamTest.java @@ -13,7 +13,6 @@ 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.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 void testDuplicateUniqueValuesAreReIndexed() { public void testDuplicateUniqueValuesAreRejectedWithChecking_TestingDisabled() { myStorageSettings.setUniqueIndexesCheckedBeforeSave(false); - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); Patient pt1 = new Patient(); pt1.setGender(Enumerations.AdministrativeGender.MALE); @@ -913,7 +900,7 @@ public void testDuplicateUniqueValuesAreRejectedWithChecking_TestingDisabled() { @Test public void testDuplicateUniqueValuesAreRejectedWithChecking_TestingEnabled() { - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); Patient pt1 = new Patient(); pt1.setGender(Enumerations.AdministrativeGender.MALE); @@ -1167,7 +1154,7 @@ public void testIndexTransactionWithMatchUrl2() { @Test public void testNonTransaction() { - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); Patient p = new Patient(); p.setGender(Enumerations.AdministrativeGender.MALE); @@ -1358,7 +1345,7 @@ public void testSearchUsingUniqueComposite() { @Test public void testUniqueValuesAreIndexed_DateAndToken() { - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); Patient pt1 = new Patient(); pt1.setGender(Enumerations.AdministrativeGender.MALE); @@ -1604,7 +1591,7 @@ public void testUniqueValuesAreIndexed_StringAndReference_UsingConditionalInTran @Test public void testUniqueValuesAreNotIndexedIfNotAllParamsAreFound_DateAndToken() { - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); Patient pt; @@ -1716,7 +1703,7 @@ public void testUniqueValuesAreReIndexed() { @Test public void testDetectUniqueSearchParams() { - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); List params = mySearchParamRegistry.getActiveComboSearchParams("Patient", null); assertThat(params).hasSize(1); @@ -1731,7 +1718,7 @@ public void testDetectUniqueSearchParams() { @Test public void testDuplicateUniqueValuesWithDateAreRejected() { - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); Patient pt1 = new Patient(); pt1.setGender(Enumerations.AdministrativeGender.MALE); @@ -1782,7 +1769,7 @@ public void testDuplicateUniqueValuesWithDateTimeAreRejected() { @Test public void testUniqueComboSearchWithDateNotUsingUniqueIndex() { - createUniqueBirthdateAndGenderSps(); + createBirthdateAndGenderSps(true); Patient pt1 = new Patient(); pt1.setGender(Enumerations.AdministrativeGender.MALE); diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java index 93d3fc956e98..a2bf85f82c45 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamR4Test.java @@ -103,7 +103,7 @@ private void consumeResponse(CloseableHttpResponse theStatus) throws IOException } @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,6 +114,11 @@ public void testSearchForOneUnqualifiedDate() throws Exception { assertEquals(parseLowerForDatePrecision("2012-01-01 00:00:00.0000"), ourLastDateRange.getLowerBoundAsInstant()); assertEquals(parseUpperForDatePrecision("2012-01-03 00:00:00.0000"), ourLastDateRange.getUpperBoundAsInstant()); + + // 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()); } @@ -467,7 +472,7 @@ private static Date parseUpperForTimePrecision(String theString) throws ParseExc } @AfterAll - public static void afterClassClearContext() throws Exception { + public static void afterClassClearContext() { TestUtil.randomizeLocaleAndTimezone(); } From 81f349038e3036c200de1a233fba8f9661ba1c04 Mon Sep 17 00:00:00 2001 From: peartree Date: Tue, 14 Jan 2025 09:36:42 -0500 Subject: [PATCH 11/11] addressing comments from second code review. --- ...o-index-sp-not-invoked-with-dateparam.yaml | 2 +- ...rResourceDaoR4ComboNonUniqueParamTest.java | 24 ++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml index 0b8b7a8f8861..ceefa142de5d 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6603-combo-index-sp-not-invoked-with-dateparam.yaml @@ -3,5 +3,5 @@ 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 when the query string included a date ragne (i.e. multiple instances of the same date parameter). This +optimised indexes when the query string included a date range (i.e. multiple instances of the same date parameter). This issue is fixed." diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java index 8407548ce6ee..ede890e2a8f2 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java @@ -909,12 +909,32 @@ public void testDateModifier(boolean theUseComparator) { } + /** + * Given: + * - a combo search index parameters combining Patient.gender and Patient.birthdate; + * - two search invocations with query parameters: + * + * Patient?birthdate=2025-01-01&gender=male + * Patient?birthdate=eq2025-01-01&gender=male + * + * The above searches will provide the same result bundle with the execution time of the second one being + * significantly longer when invoked against a large dataset. The difference is in the 'eq' prefix or modifier + * added in the second invocation causing the search layer to skip inspection of combo search indexes. + * + * To retain the optimized searching functionalities provided by combo search index parameters, it is crucial + * that the un-modified aspect of a date query string (i.e. birthdate=2025-01-01) gets preserved when marshalled + * into objects suitable for submission to the search layer. + * + * The purpose of this test is to ensure the necessity described above. It marshals a date query string into a + * DateRangeParam object, submits the param object to the search layer and asserts that combo search indexes + * are used to generate the result bundle when the date query string is un-modified. + */ @ParameterizedTest @CsvSource(value = { "2025-01-01, true", "eq2025-01-01, false" }) - public void testSearchWithDateQueryString_whenModifierEqualIsMissing_willUseComboIndexes(String theDateQueryParameter, boolean theShouldUseComboIndex){ + public void testSearchWithDateQueryString_whenModifierEqualIsMissing_willUseComboSearchIndexes(String theDateQueryParameter, boolean theShouldUseComboIndex){ // given createBirthdateAndGenderSps(false); IIdType patientId = createPatient(withBirthdate("2025-01-01"), withGender("male")); @@ -1022,6 +1042,4 @@ private IIdType createObservation(String theDateValue) { } } - - }