Add non-standard __pid SP for breaking ties cheaply during sorts. (#5428)

Add a non-standard __pid SP.

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java

@@ -199,6 +199,8 @@ public class Constants {
 	public static final String PARAM_PRETTY_VALUE_FALSE = "false";
 	public static final String PARAM_PRETTY_VALUE_TRUE = "true";
 	public static final String PARAM_PROFILE = "_profile";
+	public static final String PARAM__PID = "__pid";
+
 	public static final String PARAM_QUERY = "_query";
 	public static final String PARAM_RESPONSE_URL = "response-url"; // Used in messaging
 	public static final String PARAM_REVINCLUDE = "_revinclude";

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_10_0/5428-pid-sp.yaml

@@ -0,0 +1,5 @@
+---
+type: add
+issue: 5428
+title: "Add support for non-standard __pid SearchParameter to the the JPA engine.
+  This new SP provides an efficient tie-breaking sort key."

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa/search.md

@@ -22,6 +22,10 @@ Searching on Location.Position using `near` currently uses a box search, not a r
 
 The special `_filter` is only partially implemented.
 
+### __pid
+
+The JPA server implements a non-standard special `__pid` which matches/sorts on the raw internal database id.
+This sort is useful for imposing tie-breaking sort order in an efficient way.
 
 <a name="uplifted-refchains"/>
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java

@@ -285,6 +285,12 @@ public class QueryStack {
 		mySqlBuilder.addSortString(resourceTablePredicateBuilder.getColumnFhirId(), theAscending, myUseAggregate);
 	}
 
+	/** Sort on RES_ID -- used to break ties for reliable sort */
+	public void addSortOnResourcePID(boolean theAscending) {
+		BaseJoiningPredicateBuilder predicateBuilder = mySqlBuilder.getOrCreateFirstPredicateBuilder();
+		mySqlBuilder.addSortString(predicateBuilder.getResourceIdColumn(), theAscending);
+	}
+
 	public void addSortOnResourceLink(
 			String theResourceName,
 			String theReferenceTargetType,
@@ -2287,6 +2293,10 @@ public class QueryStack {
 						null,
 						theSearchForIdsParams.myRequestPartitionId);
 
+			case Constants.PARAM__PID:
+				return createPredicateResourcePID(
+						theSearchForIdsParams.mySourceJoinColumn, theSearchForIdsParams.myAndOrParams);
+
 			case PARAM_HAS:
 				return createPredicateHas(
 						theSearchForIdsParams.mySourceJoinColumn,
@@ -2337,6 +2347,36 @@ public class QueryStack {
 		}
 	}
 
+	/**
+	 * Raw match on RES_ID
+	 */
+	private Condition createPredicateResourcePID(
+			DbColumn theSourceJoinColumn, List<List<IQueryParameterType>> theAndOrParams) {
+
+		DbColumn pidColumn = theSourceJoinColumn;
+
+		if (pidColumn == null) {
+			BaseJoiningPredicateBuilder predicateBuilder = mySqlBuilder.getOrCreateFirstPredicateBuilder();
+			pidColumn = predicateBuilder.getResourceIdColumn();
+		}
+
+		// we don't support any modifiers for now
+		Set<Long> pids = theAndOrParams.stream()
+				.map(orList -> orList.stream()
+						.map(v -> v.getValueAsQueryToken(myFhirContext))
+						.map(Long::valueOf)
+						.collect(Collectors.toSet()))
+				.reduce(Sets::intersection)
+				.orElse(Set.of());
+
+		if (pids.isEmpty()) {
+			mySqlBuilder.setMatchNothing();
+			return null;
+		}
+
+		return toEqualToOrInPredicate(pidColumn, mySqlBuilder.generatePlaceholders(pids));
+	}
+
 	private Condition createReverseSearchPredicateLastUpdated(
 			List<List<IQueryParameterType>> theAndOrParams, DbColumn theSourceColumn) {
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java

@@ -838,6 +838,10 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 
 			theQueryStack.addSortOnResourceId(ascending);
 
+		} else if (Constants.PARAM__PID.equals(theSort.getParamName())) {
+
+			theQueryStack.addSortOnResourcePID(ascending);
+
 		} else if (Constants.PARAM_LASTUPDATED.equals(theSort.getParamName())) {
 
 			theQueryStack.addSortOnLastUpdated(ascending);

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

@@ -51,6 +51,8 @@ public class ResourceMetaParams {
 		Map<String, Class<? extends IQueryParameterAnd<?>>> resourceMetaAndParams = new HashMap<>();
 		resourceMetaParams.put(IAnyResource.SP_RES_ID, StringParam.class);
 		resourceMetaAndParams.put(IAnyResource.SP_RES_ID, StringAndListParam.class);
+		resourceMetaParams.put(Constants.PARAM__PID, TokenParam.class);
+		resourceMetaAndParams.put(Constants.PARAM__PID, TokenAndListParam.class);
 		resourceMetaParams.put(Constants.PARAM_TAG, TokenParam.class);
 		resourceMetaAndParams.put(Constants.PARAM_TAG, TokenAndListParam.class);
 		resourceMetaParams.put(Constants.PARAM_PROFILE, UriParam.class);

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

@@ -0,0 +1,150 @@
+package ca.uhn.fhir.jpa.dao.r4;
+
+import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.interceptor.api.Hook;
+import ca.uhn.fhir.interceptor.api.Pointcut;
+import ca.uhn.fhir.jpa.dao.TestDaoSearch;
+import ca.uhn.fhir.jpa.test.BaseJpaTest;
+import ca.uhn.fhir.jpa.test.config.TestHSearchAddInConfig;
+import ca.uhn.fhir.jpa.test.config.TestR4Config;
+import ca.uhn.fhir.jpa.util.SqlQuery;
+import ca.uhn.fhir.jpa.util.SqlQueryList;
+import ca.uhn.fhir.rest.api.server.RequestDetails;
+import ca.uhn.fhir.storage.test.DaoTestDataBuilder;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.test.context.ContextConfiguration;
+import org.springframework.test.context.TestContext;
+import org.springframework.test.context.TestExecutionListeners;
+import org.springframework.test.context.junit.jupiter.SpringExtension;
+import org.springframework.test.context.support.DependencyInjectionTestExecutionListener;
+import org.springframework.test.context.support.DirtiesContextTestExecutionListener;
+import org.springframework.transaction.PlatformTransactionManager;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.not;
+
+/**
+ * Sandbox for implementing queries.
+ * This will NOT run during the build - use this class as a convenient
+ * place to explore, debug, profile, and optimize.
+ */
+@ExtendWith(SpringExtension.class)
+@ContextConfiguration(classes = {
+	TestR4Config.class,
+	TestHSearchAddInConfig.NoFT.class,
+	DaoTestDataBuilder.Config.class,
+	TestDaoSearch.Config.class
+})
+@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS)
+@TestExecutionListeners(listeners = {
+	DependencyInjectionTestExecutionListener.class
+	, FhirResourceDaoR4QuerySandbox.TestDirtiesContextTestExecutionListener.class
+})
+public class FhirResourceDaoR4QuerySandbox extends BaseJpaTest {
+	private static final Logger ourLog = LoggerFactory.getLogger(FhirResourceDaoR4QuerySandbox.class);
+
+	@Autowired
+	PlatformTransactionManager myTxManager;
+	@Autowired
+	FhirContext myFhirCtx;
+	@RegisterExtension
+	@Autowired
+	DaoTestDataBuilder myDataBuilder;
+	@Autowired
+	TestDaoSearch myTestDaoSearch;
+
+	@Override
+	protected PlatformTransactionManager getTxManager() {
+		return myTxManager;
+	}
+
+	@Override
+	protected FhirContext getFhirContext() {
+		return myFhirCtx;
+	}
+
+	List<String> myCapturedQueries = new ArrayList<>();
+	@BeforeEach
+	void registerLoggingInterceptor() {
+		registerInterceptor(new Object(){
+			@Hook(Pointcut.JPA_PERFTRACE_RAW_SQL)
+			public void captureSql(RequestDetails theRequestDetails, SqlQueryList theQueries) {
+				for (SqlQuery next : theQueries) {
+					String output = next.getSql(true, true, true);
+					ourLog.info("Query: {}", output);
+					myCapturedQueries.add(output);
+				}
+			}
+		});
+
+	}
+
+	@Test
+	public void testSearches_logQueries() {
+		myDataBuilder.createPatient();
+
+		myTestDaoSearch.searchForIds("Patient?name=smith");
+
+		assertThat(myCapturedQueries, not(empty()));
+	}
+
+	@Test
+	void testQueryByPid() {
+
+		// sentinel for over-match
+		myDataBuilder.createPatient();
+
+		String id = myDataBuilder.createPatient(
+			myDataBuilder.withBirthdate("1971-01-01"),
+			myDataBuilder.withActiveTrue(),
+			myDataBuilder.withFamily("Smith")).getIdPart();
+
+		myTestDaoSearch.assertSearchFindsOnly("search by server assigned id", "Patient?__pid=" + id, id);
+	}
+
+	@Test
+	void testQueryByPid_withOtherSPAvoidsResourceTable() {
+		// sentinel for over-match
+		myDataBuilder.createPatient();
+
+		String id = myDataBuilder.createPatient(
+			myDataBuilder.withBirthdate("1971-01-01"),
+			myDataBuilder.withActiveTrue(),
+			myDataBuilder.withFamily("Smith")).getIdPart();
+
+		myTestDaoSearch.assertSearchFindsOnly("search by server assigned id", "Patient?name=smith&__pid=" + id, id);
+	}
+
+	@Test
+	void testSortByPid() {
+
+		String id1 = myDataBuilder.createPatient(myDataBuilder.withFamily("Smithy")).getIdPart();
+		String id2 = myDataBuilder.createPatient(myDataBuilder.withFamily("Smithwick")).getIdPart();
+		String id3 = myDataBuilder.createPatient(myDataBuilder.withFamily("Smith")).getIdPart();
+
+		myTestDaoSearch.assertSearchFindsInOrder("sort by server assigned id", "Patient?family=smith&_sort=__pid", id1,id2,id3);
+		myTestDaoSearch.assertSearchFindsInOrder("reverse sort by server assigned id", "Patient?family=smith&_sort=-__pid", id3,id2,id1);
+	}
+
+	public static final class TestDirtiesContextTestExecutionListener extends DirtiesContextTestExecutionListener {
+
+		@Override
+		protected void beforeOrAfterTestClass(TestContext testContext, DirtiesContext.ClassMode requiredClassMode) throws Exception {
+			if (!testContext.getTestClass().getName().contains("$")) {
+				super.beforeOrAfterTestClass(testContext, requiredClassMode);
+			}
+		}
+	}
+
+}

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

@@ -4,10 +4,10 @@ import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.dao.TestDaoSearch;
+import ca.uhn.fhir.jpa.search.BaseSourceSearchParameterTestCases;
 import ca.uhn.fhir.jpa.search.CompositeSearchParameterTestCases;
 import ca.uhn.fhir.jpa.search.IIdSearchTestTemplate;
 import ca.uhn.fhir.jpa.search.QuantitySearchParameterTestCases;
-import ca.uhn.fhir.jpa.search.BaseSourceSearchParameterTestCases;
 import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
 import ca.uhn.fhir.jpa.test.BaseJpaTest;
 import ca.uhn.fhir.jpa.test.config.TestHSearchAddInConfig;
@@ -38,9 +38,14 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
-import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.not;
 
+/**
+ * Verify that our query behaviour matches the spec.
+ * Note: we do not extend BaseJpaR4Test here.
+ * That does a full purge in @AfterEach which is a bit slow.
+ * Instead, this test tracks all created resources in DaoTestDataBuilder, and deletes them in teardown.
+ */
 @ExtendWith(SpringExtension.class)
 @ContextConfiguration(classes = {
 	TestR4Config.class,
@@ -499,6 +504,32 @@ public class FhirResourceDaoR4StandardQueriesNoFTTest extends BaseJpaTest {
 
 	}
 
+	@Test
+	void testQueryByPid() {
+
+		// sentinel for over-match
+		myDataBuilder.createPatient();
+
+		String id = myDataBuilder.createPatient(
+			myDataBuilder.withBirthdate("1971-01-01"),
+			myDataBuilder.withActiveTrue(),
+			myDataBuilder.withFamily("Smith")).getIdPart();
+
+		myTestDaoSearch.assertSearchFindsOnly("search by server assigned id", "Patient?__pid=" + id, id);
+		myTestDaoSearch.assertSearchFindsOnly("search by server assigned id", "Patient?family=smith&__pid=" + id, id);
+	}
+
+	@Test
+	void testSortByPid() {
+
+		String id1 = myDataBuilder.createPatient(myDataBuilder.withFamily("Smithy")).getIdPart();
+		String id2 = myDataBuilder.createPatient(myDataBuilder.withFamily("Smithwick")).getIdPart();
+		String id3 = myDataBuilder.createPatient(myDataBuilder.withFamily("Smith")).getIdPart();
+
+		myTestDaoSearch.assertSearchFindsInOrder("sort by server assigned id", "Patient?family=smith&_sort=__pid", id1,id2,id3);
+		myTestDaoSearch.assertSearchFindsInOrder("reverse sort by server assigned id", "Patient?family=smith&_sort=-__pid", id3,id2,id1);
+	}
+
 	@Nested
 	public class IdSearch implements IIdSearchTestTemplate {
 		@Override

hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java

@@ -48,6 +48,7 @@ import javax.annotation.Nonnull;
 
 import static org.apache.commons.lang3.ArrayUtils.EMPTY_STRING_ARRAY;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.everyItem;
 import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.in;
@@ -133,6 +134,17 @@ public class TestDaoSearch {
 		assertSearchFindsInOrder(theReason, theQueryUrl, theIds.toArray(EMPTY_STRING_ARRAY));
 	}
 
+	public void assertSearchFindsOnly(String theReason, String theQueryUrl, String... theIds) {
+		assertSearchIdsMatch(theReason, theQueryUrl, containsInAnyOrder(theIds));
+	}
+
+	public void assertSearchIdsMatch(
+			String theReason, String theQueryUrl, Matcher<? super Iterable<String>> theMatchers) {
+		List<String> ids = searchForIds(theQueryUrl);
+
+		MatcherAssert.assertThat(theReason, ids, theMatchers);
+	}
+
 	public void assertSearchResultIds(String theQueryUrl, String theReason, Matcher<Iterable<String>> matcher) {
 		List<String> ids = searchForIds(theQueryUrl);
 

hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/BaseJpaTest.java

@@ -431,6 +431,11 @@ public abstract class BaseJpaTest extends BaseTest {
 		return deliveryLatch;
 	}
 
+	protected void registerInterceptor(Object theInterceptor) {
+		myRegisteredInterceptors.add(theInterceptor);
+		myInterceptorRegistry.registerInterceptor(theInterceptor);
+	}
+
 	protected void purgeHibernateSearch(EntityManager theEntityManager) {
 		runInTransaction(() -> {
 			if (myFulltestSearchSvc != null && !myFulltestSearchSvc.isDisabled()) {

hapi-fhir-storage-test-utilities/src/main/java/ca/uhn/fhir/storage/test/DaoTestDataBuilder.java

@@ -39,7 +39,8 @@ import org.springframework.context.annotation.Configuration;
 
 /**
  * Implements ITestDataBuilder via a live DaoRegistry.
- *
+ * Note: this implements {@link AfterEachCallback} and will delete any resources created when registered
+ * via {@link org.junit.jupiter.api.extension.RegisterExtension}.
  * Add the inner {@link Config} to your spring context to inject this.
  * For convenience, you can still implement ITestDataBuilder on your test class, and delegate the missing methods to this bean.
  */