Force a flush of the SearchParameter cache sooner when SPs change (#2030)

* Force a flush of the SearchParameter cache sooner when SPs change

* Add changelog

* Test fixes

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/NumberParam.java

@@ -56,7 +56,7 @@ public class NumberParam extends BaseParamWithPrefix<NumberParam> implements IQu
 	 * Constructor
 	 * 
 	 * @param theValue
-	 *            A string value, e.g. "&gt;5.0"
+	 *            A string value, e.g. "gt5.0"
 	 */
 	public NumberParam(String theValue) {
 		setValueAsQueryToken(null, null, null, theValue);

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_1_0/2023-force-sp-cache-flush-sooner.yaml

@@ -0,0 +1,7 @@
+---
+type: add
+issue: 2023
+title: "The JPA server maintains a cache of active SearchParameeter resources that can cause misleading results
+  if a SearchParameter is changed and other resources that would be indexed by the changed SearchParameter are updated
+  before the cache refreshes. A new interceptor has been added that should force a refresh sooner, especially on 
+  non-clustered systems."

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

@@ -51,6 +51,7 @@ public class FhirResourceDaoSearchParameterDstu2 extends BaseHapiFhirResourceDao
 	protected void postPersist(ResourceTable theEntity, SearchParameter theResource) {
 		super.postPersist(theEntity, theResource);
 		markAffectedResources(theResource);
+
 	}
 
 	@Override

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2SearchCustomSearchParamTest.java

@@ -1,5 +1,6 @@
 package ca.uhn.fhir.jpa.dao.dstu2;
 
+import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.jpa.model.entity.ModelConfig;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
@@ -28,6 +29,8 @@ import ca.uhn.fhir.model.primitive.DecimalDt;
 import ca.uhn.fhir.model.primitive.IntegerDt;
 import ca.uhn.fhir.model.primitive.StringDt;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
+import ca.uhn.fhir.rest.client.api.IGenericClient;
+import ca.uhn.fhir.rest.client.interceptor.LoggingInterceptor;
 import ca.uhn.fhir.rest.param.DateParam;
 import ca.uhn.fhir.rest.param.NumberParam;
 import ca.uhn.fhir.rest.param.ReferenceParam;
@@ -232,6 +235,47 @@ public class FhirResourceDaoDstu2SearchCustomSearchParamTest extends BaseJpaDstu
 
 	}
 
+	/**
+	 * See #2023
+	 */
+	@Test
+	public void testNumberSearchParam() {
+		SearchParameter numberParameter = new ca.uhn.fhir.model.dstu2.resource.SearchParameter();
+		numberParameter.setId("future-appointment-count");
+		numberParameter.setName("Future Appointment Count");
+		numberParameter.setCode("future-appointment-count");
+		numberParameter.setDescription("Count of future appointments for the patient");
+		numberParameter.setUrl("http://integer");
+		numberParameter.setStatus(ca.uhn.fhir.model.dstu2.valueset.ConformanceResourceStatusEnum.ACTIVE);
+		numberParameter.setBase(ca.uhn.fhir.model.dstu2.valueset.ResourceTypeEnum.PATIENT);
+		numberParameter.setType(ca.uhn.fhir.model.dstu2.valueset.SearchParamTypeEnum.NUMBER);
+		numberParameter.setXpathUsage(XPathUsageTypeEnum.NORMAL);
+		numberParameter.setXpath("Patient.extension('http://integer')");
+		mySearchParameterDao.update(numberParameter);
+
+		// This fires every 10 seconds
+		mySearchParamRegistry.refreshCacheIfNecessary();
+
+		Patient patient = new Patient();
+		patient.setId("future-appointment-count-pt");
+		patient.setActive(true);
+		patient.addUndeclaredExtension(false, "http://integer", new IntegerDt(1));
+		myPatientDao.update(patient);
+
+		IBundleProvider search;
+
+		search = myPatientDao.search(SearchParameterMap.newSynchronous("future-appointment-count", new NumberParam(1)));
+		assertEquals(1, search.size());
+
+		search = myPatientDao.search(SearchParameterMap.newSynchronous("future-appointment-count", new NumberParam("gt0")));
+		assertEquals(1, search.size());
+
+		search = myPatientDao.search(SearchParameterMap.newSynchronous("future-appointment-count", new NumberParam("lt0")));
+		assertEquals(0, search.size());
+
+	}
+
+
 	@Test
 	public void testIncludeExtensionReferenceAsRecurse() {
 		SearchParameter attendingSp = new SearchParameter();

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchCustomSearchParamTest.java

@@ -1,5 +1,6 @@
 package ca.uhn.fhir.jpa.dao.r4;
 
+import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.interceptor.api.HookParams;
 import ca.uhn.fhir.interceptor.api.IAnonymousInterceptor;
 import ca.uhn.fhir.interceptor.api.Pointcut;
@@ -9,7 +10,11 @@ import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
 import ca.uhn.fhir.jpa.model.search.StorageProcessingMessage;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.model.api.Include;
+import ca.uhn.fhir.model.dstu2.valueset.XPathUsageTypeEnum;
+import ca.uhn.fhir.model.primitive.IntegerDt;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
+import ca.uhn.fhir.rest.client.api.IGenericClient;
+import ca.uhn.fhir.rest.client.interceptor.LoggingInterceptor;
 import ca.uhn.fhir.rest.param.DateParam;
 import ca.uhn.fhir.rest.param.NumberParam;
 import ca.uhn.fhir.rest.param.ReferenceOrListParam;
@@ -112,6 +117,46 @@ public class FhirResourceDaoR4SearchCustomSearchParamTest extends BaseJpaR4Test
 		mySearchParamRegistry.forceRefresh();
 	}
 
+	/**
+	 * See #2023
+	 */
+	@Test
+	public void testNumberSearchParam() {
+		SearchParameter numberParameter = new SearchParameter();
+		numberParameter.setId("future-appointment-count");
+		numberParameter.setName("Future Appointment Count");
+		numberParameter.setCode("future-appointment-count");
+		numberParameter.setDescription("Count of future appointments for the patient");
+		numberParameter.setUrl("http://integer");
+		numberParameter.setStatus(Enumerations.PublicationStatus.ACTIVE);
+		numberParameter.addBase("Patient");
+		numberParameter.setType(Enumerations.SearchParamType.NUMBER);
+		numberParameter.setExpression("Patient.extension('http://integer')");
+		mySearchParameterDao.update(numberParameter);
+
+		// This fires every 10 seconds
+		mySearchParamRegistry.refreshCacheIfNecessary();
+
+		Patient patient = new Patient();
+		patient.setId("future-appointment-count-pt");
+		patient.setActive(true);
+		patient.addExtension( "http://integer", new IntegerType(1));
+		myPatientDao.update(patient);
+
+		IBundleProvider search;
+
+		search = myPatientDao.search(SearchParameterMap.newSynchronous("future-appointment-count", new NumberParam(1)));
+		assertEquals(1, search.size());
+
+		search = myPatientDao.search(SearchParameterMap.newSynchronous("future-appointment-count", new NumberParam("gt0")));
+		assertEquals(1, search.size());
+
+		search = myPatientDao.search(SearchParameterMap.newSynchronous("future-appointment-count", new NumberParam("lt0")));
+		assertEquals(0, search.size());
+
+	}
+
+
 	/**
 	 * Draft search parameters should be ok even if they aren't completely valid
 	 */

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java

@@ -24,8 +24,10 @@ import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.context.RuntimeResourceDefinition;
 import ca.uhn.fhir.context.RuntimeSearchParam;
 import ca.uhn.fhir.context.phonetic.IPhoneticEncoder;
+import ca.uhn.fhir.interceptor.api.Hook;
 import ca.uhn.fhir.interceptor.api.HookParams;
-import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster;
+import ca.uhn.fhir.interceptor.api.IInterceptorService;
+import ca.uhn.fhir.interceptor.api.Interceptor;
 import ca.uhn.fhir.interceptor.api.Pointcut;
 import ca.uhn.fhir.jpa.model.entity.ModelConfig;
 import ca.uhn.fhir.jpa.model.sched.HapiJob;
@@ -38,8 +40,6 @@ import ca.uhn.fhir.jpa.searchparam.retry.Retrier;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
-import ca.uhn.fhir.util.DatatypeUtil;
-import ca.uhn.fhir.util.HapiExtensions;
 import ca.uhn.fhir.util.SearchParameterUtil;
 import ca.uhn.fhir.util.StopWatch;
 import org.apache.commons.lang3.StringUtils;
@@ -51,6 +51,7 @@ import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 
 import javax.annotation.PostConstruct;
+import javax.annotation.PreDestroy;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -89,7 +90,8 @@ public class SearchParamRegistryImpl implements ISearchParamRegistry {
 	private volatile long myLastRefresh;
 
 	@Autowired
-	private IInterceptorBroadcaster myInterceptorBroadcaster;
+	private IInterceptorService myInterceptorBroadcaster;
+	private RefreshSearchParameterCacheOnUpdate myInterceptor;
 
 	@Override
 	public RuntimeSearchParam getActiveSearchParam(String theResourceName, String theParamName) {
@@ -236,8 +238,16 @@ public class SearchParamRegistryImpl implements ISearchParamRegistry {
 	}
 
 	@PostConstruct
-	public void postConstruct() {
+	public void start() {
 		myBuiltInSearchParams = createBuiltInSearchParamMap(myFhirContext);
+
+		myInterceptor = new RefreshSearchParameterCacheOnUpdate();
+		myInterceptorBroadcaster.registerInterceptor(myInterceptor);
+	}
+
+	@PreDestroy
+	public void stop() {
+		myInterceptorBroadcaster.unregisterInterceptor(myInterceptor);
 	}
 
 	public int doRefresh(long theRefreshInterval) {
@@ -376,16 +386,6 @@ public class SearchParamRegistryImpl implements ISearchParamRegistry {
 		mySchedulerService.scheduleLocalJob(10 * DateUtils.MILLIS_PER_SECOND, jobDetail);
 	}
 
-	public static class Job implements HapiJob {
-		@Autowired
-		private ISearchParamRegistry myTarget;
-
-		@Override
-		public void execute(JobExecutionContext theContext) {
-			myTarget.refreshCacheIfNecessary();
-		}
-	}
-
 	@Override
 	public boolean refreshCacheIfNecessary() {
 		if (myActiveSearchParams == null || System.currentTimeMillis() - REFRESH_INTERVAL > myLastRefresh) {
@@ -402,30 +402,12 @@ public class SearchParamRegistryImpl implements ISearchParamRegistry {
 		return Collections.unmodifiableMap(myActiveSearchParams);
 	}
 
-	public static Map<String, Map<String, RuntimeSearchParam>> createBuiltInSearchParamMap(FhirContext theFhirContext) {
-		Map<String, Map<String, RuntimeSearchParam>> resourceNameToSearchParams = new HashMap<>();
-
-		Set<String> resourceNames = theFhirContext.getResourceTypes();
-
-		for (String resourceName : resourceNames) {
-			RuntimeResourceDefinition nextResDef = theFhirContext.getResourceDefinition(resourceName);
-			String nextResourceName = nextResDef.getName();
-			HashMap<String, RuntimeSearchParam> nameToParam = new HashMap<>();
-			resourceNameToSearchParams.put(nextResourceName, nameToParam);
-
-			for (RuntimeSearchParam nextSp : nextResDef.getSearchParams()) {
-				nameToParam.put(nextSp.getName(), nextSp);
-			}
-		}
-		return Collections.unmodifiableMap(resourceNameToSearchParams);
-	}
-
 	/**
 	 * All SearchParameters with the name "phonetic" encode the normalized index value using this phonetic encoder.
 	 *
 	 * @since 5.1.0
 	 */
-
+	@Override
 	public void setPhoneticEncoder(IPhoneticEncoder thePhoneticEncoder) {
 		myPhoneticEncoder = thePhoneticEncoder;
 
@@ -446,4 +428,58 @@ public class SearchParamRegistryImpl implements ISearchParamRegistry {
 			searchParam.setPhoneticEncoder(myPhoneticEncoder);
 		}
 	}
+
+	@Interceptor
+	public class RefreshSearchParameterCacheOnUpdate {
+
+		@Hook(Pointcut.STORAGE_PRECOMMIT_RESOURCE_CREATED)
+		public void created(IBaseResource theResource) {
+			handle(theResource);
+		}
+
+		@Hook(Pointcut.STORAGE_PRECOMMIT_RESOURCE_DELETED)
+		public void deleted(IBaseResource theResource) {
+			handle(theResource);
+		}
+
+		@Hook(Pointcut.STORAGE_PRECOMMIT_RESOURCE_UPDATED)
+		public void updated(IBaseResource theResource) {
+			handle(theResource);
+		}
+
+		private void handle(IBaseResource theResource) {
+			if (theResource != null && myFhirContext.getResourceType(theResource).equals("SearchParameter")) {
+				requestRefresh();
+			}
+		}
+
+	}
+
+	public static class Job implements HapiJob {
+		@Autowired
+		private ISearchParamRegistry myTarget;
+
+		@Override
+		public void execute(JobExecutionContext theContext) {
+			myTarget.refreshCacheIfNecessary();
+		}
+	}
+
+	public static Map<String, Map<String, RuntimeSearchParam>> createBuiltInSearchParamMap(FhirContext theFhirContext) {
+		Map<String, Map<String, RuntimeSearchParam>> resourceNameToSearchParams = new HashMap<>();
+
+		Set<String> resourceNames = theFhirContext.getResourceTypes();
+
+		for (String resourceName : resourceNames) {
+			RuntimeResourceDefinition nextResDef = theFhirContext.getResourceDefinition(resourceName);
+			String nextResourceName = nextResDef.getName();
+			HashMap<String, RuntimeSearchParam> nameToParam = new HashMap<>();
+			resourceNameToSearchParams.put(nextResourceName, nameToParam);
+
+			for (RuntimeSearchParam nextSp : nextResDef.getSearchParams()) {
+				nameToParam.put(nextSp.getName(), nextSp);
+			}
+		}
+		return Collections.unmodifiableMap(resourceNameToSearchParams);
+	}
 }

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParameterCanonicalizer.java

@@ -96,6 +96,7 @@ public class SearchParameterCanonicalizer {
 		String path = theNextSp.getXpath();
 		RestSearchParameterTypeEnum paramType = null;
 		RuntimeSearchParam.RuntimeSearchParamStatusEnum status = null;
+		if (theNextSp.getTypeElement().getValueAsEnum() != null) {
 			switch (theNextSp.getTypeElement().getValueAsEnum()) {
 				case COMPOSITE:
 					paramType = RestSearchParameterTypeEnum.COMPOSITE;
@@ -122,6 +123,7 @@ public class SearchParameterCanonicalizer {
 					paramType = RestSearchParameterTypeEnum.URI;
 					break;
 			}
+		}
 		if (theNextSp.getStatus() != null) {
 			switch (theNextSp.getStatusElement().getValueAsEnum()) {
 				case ACTIVE:

hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImplTest.java

@@ -3,6 +3,7 @@ package ca.uhn.fhir.jpa.searchparam.registry;
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.context.RuntimeSearchParam;
 import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster;
+import ca.uhn.fhir.interceptor.api.IInterceptorService;
 import ca.uhn.fhir.jpa.model.entity.ModelConfig;
 import ca.uhn.fhir.jpa.model.sched.ISchedulerService;
 import ca.uhn.fhir.rest.server.SimpleBundleProvider;
@@ -46,7 +47,7 @@ public class SearchParamRegistryImplTest {
 	@MockBean
 	private ModelConfig myModelConfig;
 	@MockBean
-	private IInterceptorBroadcaster myInterceptorBroadcaster;
+	private IInterceptorService myInterceptorBroadcaster;
 
 	@Configuration
 	static class SpringConfig {