Fixed an issue with the _lastUpdated search param not being recognised as a valid sort spec with Elastic Search enabled in CDR #6670

AbayomiE-T · 2025-01-31T19:38:36Z

In CDR, with HSearch is enabled, it is observed that a lot of search parameters are missing from the SearchParamRegistry which results in the error 500 HAPI-0389: Failed to call access method: org.springframework.dao.InvalidDataAccessApiUsageException: HAPI-2523: Invalid sort specification: _lastUpdated when using the $apply-codesystem-delta-add operation.

A fall back was added in HSearchSortHelperImpl.getParamType() to account for these missing search params.

github-actions · 2025-01-31T19:39:44Z

Formatting check succeeded!

…d as a valid sort spec with Hsearch enabled in CDR

codecov · 2025-02-01T02:39:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (rel_8_0@637a79f). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff             @@
##             rel_8_0    #6670   +/-   ##
==========================================
  Coverage           ?   83.56%           
  Complexity         ?    28625           
==========================================
  Files              ?     1798           
  Lines              ?   111309           
  Branches           ?    13978           
==========================================
  Hits               ?    93011           
  Misses             ?    12297           
  Partials           ?     6001

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

epeartree · 2025-02-03T18:34:00Z

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImpl.java

@@ -155,12 +156,23 @@ Optional<RestSearchParameterTypeEnum> getParamType(String theResourceTypeName, S
 				theResourceTypeName, ISearchParamRegistry.SearchParamLookupContextEnum.SEARCH);
 		RuntimeSearchParam searchParam = activeSearchParams.get(theParamName);
 		if (searchParam == null) {
-			return Optional.empty();
+			RestSearchParameterTypeEnum paramType = this.getParamTypeOfSortingParams(theParamName);


no need to use this

Corrected 👍

epeartree · 2025-02-03T18:35:05Z

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImpl.java

@@ -155,12 +156,23 @@ Optional<RestSearchParameterTypeEnum> getParamType(String theResourceTypeName, S
 				theResourceTypeName, ISearchParamRegistry.SearchParamLookupContextEnum.SEARCH);
 		RuntimeSearchParam searchParam = activeSearchParams.get(theParamName);
 		if (searchParam == null) {
-			return Optional.empty();
+			RestSearchParameterTypeEnum paramType = this.getParamTypeOfSortingParams(theParamName);
+			return paramType != null ? Optional.of(paramType) : Optional.empty();


Optional.ofNullable(paramType);

Updated. Thanks for this.

epeartree · 2025-02-03T18:35:57Z

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImpl.java

 		}

 		return Optional.of(searchParam.getParamType());
 	}

+	private RestSearchParameterTypeEnum getParamTypeOfSortingParams(String paramName) {


make the map static so u don't create/throw away with each method invocation

epeartree · 2025-02-03T18:41:30Z

...-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImplTest.java

+	 * Validates that _id, _lastUpdated, _tag, _security and _source returns a type when absent from
+	 * the search param registry.
+	 */
+	@Test


try to use the test name concept test[WhatUrTesting]_when[TheCondition]_will[WhatIsExpected]

epeartree · 2025-02-03T18:44:52Z

...-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImplTest.java

+	void testGetParamTypeWhenParamNameIsNotInSearchParamRegistry() {
+		//Given that we have params absent from the SearchParamsRegistry
+		String resourceType = "CodeSystem";
+		SortSpec [] sortSpecs = {


i would recommend using a @ParameterizedTest with @MethodSource("theNameOfTheMethod"). do a search on the code base for example.

with a @ParameterizedTest, u will have test function parameters (SortSpec theSortSpec, Optional theOptionalSPTypeEnum). that will be much cleaner.

Used @ParameterizedTest and it does look much, much cleaner. Thank you!

epeartree · 2025-02-03T18:45:40Z

...-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImplTest.java

+			RestSearchParameterTypeEnum.TOKEN
+		};
+		when(mockSearchParamRegistry.getActiveSearchParams(eq(resourceType), any())).thenReturn(mockResourceSearchParams);
+		//Execute


not too sure about what's ur testing. i'll have to take a look once u clean it all up with @ParameterizedTest

Should be a bit clearer now after your suggested changes.

ok, now i can see better and here's my two cents on it:
Class HSearchSortHelperImpl declares a map of default mapping between SortSpec's and ParameterTypeEnum. The test methodSource function does the same thing but stores the mappings in a stream instead of a map. so u have 2 declarations of pretty much the same stuff minus the optional. if someone adds to the map in the class, ur test suddenly does not account for the new value. someone would have to know that u have implemented the test and go look for it. that makes it a brittle test.

let's fix this.

u could make that map visible (public) and non-modifiable, there's no arm in exposing something that no one can mess with.

then build ur parameterized argument out of that map.

since u've spent already quite a bit of time on this, i 'll give u the starting pointers:

private static Stream<Arguments> provideArgumentsForGetParamType() { Stream.Builder<Arguments> retVal = Stream.builder(); HSearchSortHelperImpl.ourSortingParamNameToParamType.forEach((theSortSpecName, theRestSearchParameterTypeEnum) -> { // ur code goes here }); return retVal.build(); }

epeartree · 2025-02-03T18:46:53Z

...ver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/TerminologyUploaderProviderR4Test.java

+		myStorageSettings.setHibernateSearchIndexFullText(true);
+		myStorageSettings.setHibernateSearchIndexSearchParams(true);
+		myStorageSettings.setStoreResourceInHSearchIndex(true);
+		//Given: We have a non-existent code system


add breathing spaces in between stages of ur test. good construction though, i can see what's going on.

Added some whitespaces.

epeartree · 2025-02-07T15:31:35Z

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImpl.java

@@ -52,6 +53,12 @@
 */
 public class HSearchSortHelperImpl implements IHSearchSortHelper {
 	private static final Logger ourLog = LoggerFactory.getLogger(HSearchSortHelperImpl.class);
+	private static final Map<String, RestSearchParameterTypeEnum> sortingParamNameToParamType = Map.of(


as per code convention, any class static variable name is 'our'. please change to ourSortingParamNameToParamType

Updated. Thank you.

epeartree · 2025-02-07T15:34:16Z

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImpl.java

@@ -155,9 +162,9 @@ Optional<RestSearchParameterTypeEnum> getParamType(String theResourceTypeName, S
 				theResourceTypeName, ISearchParamRegistry.SearchParamLookupContextEnum.SEARCH);
 		RuntimeSearchParam searchParam = activeSearchParams.get(theParamName);
 		if (searchParam == null) {
-			return Optional.empty();


what would happen if theParamName is null? does the get returns null (which i hope) or blows up?

i'll see if u have a test covering that.

epeartree · 2025-02-07T16:10:52Z

...-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/search/HSearchSortHelperImplTest.java

+			RestSearchParameterTypeEnum.TOKEN
+		};
+		when(mockSearchParamRegistry.getActiveSearchParams(eq(resourceType), any())).thenReturn(mockResourceSearchParams);
+		//Execute


ok, now i can see better and here's my two cents on it:
Class HSearchSortHelperImpl declares a map of default mapping between SortSpec's and ParameterTypeEnum. The test methodSource function does the same thing but stores the mappings in a stream instead of a map. so u have 2 declarations of pretty much the same stuff minus the optional. if someone adds to the map in the class, ur test suddenly does not account for the new value. someone would have to know that u have implemented the test and go look for it. that makes it a brittle test.

let's fix this.

u could make that map visible (public) and non-modifiable, there's no arm in exposing something that no one can mess with.

then build ur parameterized argument out of that map.

since u've spent already quite a bit of time on this, i 'll give u the starting pointers:

private static Stream<Arguments> provideArgumentsForGetParamType() { Stream.Builder<Arguments> retVal = Stream.builder(); HSearchSortHelperImpl.ourSortingParamNameToParamType.forEach((theSortSpecName, theRestSearchParameterTypeEnum) -> { // ur code goes here }); return retVal.build(); }

epeartree

also, u'll need a changelog. ping me when u get to this.

AbayomiE-T · 2025-02-07T19:37:43Z

also, u'll need a changelog. ping me when u get to this.

Right so I do have one on the CDR side but wasn't sure on how to go about the HAPI side since the bug was reported on CDR. Do I use the same ticket number for the .yaml file? Should it have the same content?

added test for '-delt-add' in hapi

6e0b6d2

AbayomiE-T self-assigned this Jan 31, 2025

fixed an issue with the _lastUpdated search param not being recognize…

5802286

…d as a valid sort spec with Hsearch enabled in CDR

AbayomiE-T changed the title ~~Fixed an issue with the _lastUpdated search param not being recognised as a valid sort spec with Elastic Search enabled~~ Fixed an issue with the _lastUpdated search param not being recognised as a valid sort spec with Elastic Search enabled in CDR Jan 31, 2025

Abayomi Adebowale added 2 commits January 31, 2025 20:14

added null check

dba6493

Merge branch 'rel_8_0' into missing-search-params-with-hsearch-enabled

13b66d9

Abayomi Adebowale added 3 commits February 1, 2025 11:37

added tests

1986239

added comments

211a2a9

Merge branch 'rel_8_0' into missing-search-params-with-hsearch-enabled

58df9f1

epeartree reviewed Feb 3, 2025

View reviewed changes

Abayomi Adebowale added 4 commits February 4, 2025 12:25

addressed comments

da6fa3a

Merge branch 'rel_8_0' into missing-search-params-with-hsearch-enabled

c185cc4

Merge branch 'rel_8_0' into missing-search-params-with-hsearch-enabled

acbce24

Merge branch 'rel_8_0' into missing-search-params-with-hsearch-enabled

f6f5659

epeartree reviewed Feb 7, 2025

View reviewed changes

updated static class property name

00b9038

epeartree reviewed Feb 7, 2025

View reviewed changes

modified test

546004a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed an issue with the _lastUpdated search param not being recognised as a valid sort spec with Elastic Search enabled in CDR #6670

Fixed an issue with the _lastUpdated search param not being recognised as a valid sort spec with Elastic Search enabled in CDR #6670

AbayomiE-T commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

codecov bot commented Feb 1, 2025 •

edited

Loading

epeartree Feb 3, 2025

AbayomiE-T Feb 4, 2025

epeartree Feb 3, 2025

AbayomiE-T Feb 4, 2025

epeartree Feb 3, 2025

AbayomiE-T Feb 4, 2025

epeartree Feb 3, 2025

AbayomiE-T Feb 4, 2025

epeartree Feb 3, 2025

AbayomiE-T Feb 4, 2025

epeartree Feb 3, 2025

AbayomiE-T Feb 4, 2025

epeartree Feb 7, 2025

epeartree Feb 3, 2025

AbayomiE-T Feb 4, 2025

epeartree Feb 7, 2025

AbayomiE-T Feb 7, 2025

epeartree Feb 7, 2025

epeartree Feb 7, 2025

epeartree left a comment

AbayomiE-T commented Feb 7, 2025

Fixed an issue with the _lastUpdated search param not being recognised as a valid sort spec with Elastic Search enabled in CDR #6670

Are you sure you want to change the base?

Fixed an issue with the _lastUpdated search param not being recognised as a valid sort spec with Elastic Search enabled in CDR #6670

Conversation

AbayomiE-T commented Jan 31, 2025 • edited Loading

github-actions bot commented Jan 31, 2025 • edited Loading

codecov bot commented Feb 1, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epeartree left a comment

Choose a reason for hiding this comment

AbayomiE-T commented Feb 7, 2025

AbayomiE-T commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

codecov bot commented Feb 1, 2025 •

edited

Loading