Start moving forced-id inline to ResourceTable (#4260)

New fhir_id column in HFJ_RESOURCE.

Migration, changelog, and insert hook for inline fhir_id column.

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_3_0/4260-inline-forcedid.yaml

@@ -0,0 +1,7 @@
+---
+type: add
+issue: 4260
+title: "The HFJ_RESOURCE table has a new column FHIR_ID which always contains the FHIR resource id,
+   both client-assigned and server-assigned.
+   This is parallel storage to allow gradual upgrade away from the current HFJ_FORCED_ID join table,
+   which should improve performance in a future release."

hapi-fhir-jpa/src/main/java/ca/uhn/fhir/jpa/config/HapiFhirLocalContainerEntityManagerFactoryBean.java

@@ -20,6 +20,7 @@ package ca.uhn.fhir.jpa.config;
  * #L%
  */
 
+import com.google.common.base.Strings;
 import org.hibernate.cfg.AvailableSettings;
 import org.hibernate.query.criteria.LiteralHandlingMode;
 import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode;
@@ -27,6 +28,9 @@ import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
 import org.springframework.orm.hibernate5.SpringBeanContainer;
 import org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -46,6 +50,7 @@ public class HapiFhirLocalContainerEntityManagerFactoryBean extends LocalContain
 	public Map<String, Object> getJpaPropertyMap() {
 		Map<String, Object> retVal = super.getJpaPropertyMap();
 
+		// SOMEDAY these defaults can be set in the constructor.  setJpaProperties does a merge.
 		if (!retVal.containsKey(AvailableSettings.CRITERIA_LITERAL_HANDLING_MODE)) {
 			retVal.put(AvailableSettings.CRITERIA_LITERAL_HANDLING_MODE, LiteralHandlingMode.BIND);
 		}
@@ -82,5 +87,28 @@ public class HapiFhirLocalContainerEntityManagerFactoryBean extends LocalContain
 		return retVal;
 	}
 
+	/**
+	 * Helper to add hook to property.
+	 *
+	 * Listener properties are comma-separated lists, so we can't just overwrite or default it.
+	 */
+	void addHibernateHook(String thePropertyName, String theHookFQCN) {
+		Map<String, Object> retVal = super.getJpaPropertyMap();
+		List<String> listeners = new ArrayList<>();
+
+		{
+			String currentListeners = (String) retVal.get(thePropertyName);
+			if (!Strings.isNullOrEmpty(currentListeners)) {
+				listeners.addAll(Arrays.asList(currentListeners.split(",")));
+			}
+		}
+
+		// add if missing
+		if (!listeners.contains(theHookFQCN)) {
+			listeners.add(theHookFQCN);
+			retVal.put(thePropertyName, String.join(",", listeners));
+		}
+	}
+
 
 }

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

@@ -321,6 +321,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		String resourceIdBeforeStorage = theResource.getIdElement().getIdPart();
 		boolean resourceHadIdBeforeStorage = isNotBlank(resourceIdBeforeStorage);
 		boolean resourceIdWasServerAssigned = theResource.getUserData(JpaConstants.RESOURCE_ID_SERVER_ASSIGNED) == Boolean.TRUE;
+		entity.setFhirId(resourceIdBeforeStorage);
 
 		HookParams hookParams;
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java

@@ -86,6 +86,20 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks<VersionEnum> {
 		init600(); // 20211102 -
 		init610();
 		init620();
+		init630();
+	}
+
+	private void init630() {
+		Builder version = forVersion(VersionEnum.V6_3_0);
+
+		// start forced_id inline migration
+		version
+			.onTable("HFJ_RESOURCE")
+			.addColumn("20221108.1", "FHIR_ID")
+			.nullable()
+			// FHIR ids contain a subset of ascii, limited to 64 chars.
+			.type(ColumnTypeEnum.STRING, 64);
+
 	}
 
 	private void init620() {

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTable.java

@@ -31,6 +31,9 @@ import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
 import org.apache.commons.lang3.builder.ToStringBuilder;
 import org.apache.commons.lang3.builder.ToStringStyle;
+import org.hibernate.Session;
+import org.hibernate.annotations.GenerationTime;
+import org.hibernate.annotations.GeneratorType;
 import org.hibernate.annotations.OptimisticLock;
 import org.hibernate.search.engine.backend.types.Projectable;
 import org.hibernate.search.engine.backend.types.Searchable;
@@ -43,6 +46,7 @@ import org.hibernate.search.mapper.pojo.mapping.definition.annotation.IndexingDe
 import org.hibernate.search.mapper.pojo.mapping.definition.annotation.ObjectPath;
 import org.hibernate.search.mapper.pojo.mapping.definition.annotation.PropertyBinding;
 import org.hibernate.search.mapper.pojo.mapping.definition.annotation.PropertyValue;
+import org.hibernate.tuple.ValueGenerator;
 import org.hl7.fhir.instance.model.api.IIdType;
 
 import javax.persistence.CascadeType;
@@ -273,6 +277,38 @@ public class ResourceTable extends BaseHasResource implements Serializable, IBas
 	@Transient
 	private transient boolean myUnchangedInCurrentOperation;
 
+
+	/**
+	 * The id of the Resource.
+	 * Will contain either the client-assigned id, or the sequence value.
+	 * Will be null during insert time until the first read.
+	 *
+	 */
+	@Column(name= "FHIR_ID",
+		// [A-Za-z0-9\-\.]{1,64} - https://www.hl7.org/fhir/datatypes.html#id
+		length = 64,
+		// we never update this after insert, and the Generator will otherwise "dirty" the object.
+		updatable = false)
+	// inject the pk for server-assigned sequence ids.
+	@GeneratorType(when = GenerationTime.INSERT, type = FhirIdGenerator.class)
+	// Make sure the generator doesn't bump the history version.
+	@OptimisticLock(excluded = true)
+	private String myFhirId;
+
+	/**
+	 * Populate myFhirId with server-assigned sequence id when no client-id provided.
+	 * We eat this complexity during insert to simplify query time with a uniform column.
+	 * Server-assigned sequence ids aren't available until just before insertion.
+	 * Hibernate calls insert Generators after the pk has been assigned, so we can use myId safely here.
+	 */
+	public static final class FhirIdGenerator implements ValueGenerator<String> {
+		@Override
+		public String generateValue(Session session, Object owner) {
+			ResourceTable that = (ResourceTable) owner;
+			return that.myFhirId != null ? that.myFhirId : that.myId.toString();
+		}
+	}
+
 	@Version
 	@Column(name = "RES_VER")
 	private long myVersion;
@@ -782,4 +818,18 @@ public class ResourceTable extends BaseHasResource implements Serializable, IBas
 		}
 		return mySearchParamPresents;
 	}
+
+	/**
+	 * Get the FHIR resource id.
+	 *
+	 * @return the resource id, or null if the resource doesn't have a client-assigned id,
+	 * and hasn't been saved to the db to get a server-assigned id yet.
+	 */
+	public String getFhirId() {
+		return myFhirId;
+	}
+
+	public void setFhirId(String theFhirId) {
+		myFhirId = theFhirId;
+	}
 }

hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3Test.java

@@ -2,11 +2,17 @@ package ca.uhn.fhir.jpa.dao.dstu3;
 
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
+import ca.uhn.fhir.jpa.api.config.DaoConfig.ClientIdStrategyEnum;
+import ca.uhn.fhir.jpa.api.config.DaoConfig.IdStrategyEnum;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
+import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.jpa.api.model.HistoryCountModeEnum;
 import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao;
 import ca.uhn.fhir.jpa.dao.DaoTestUtils;
+import ca.uhn.fhir.jpa.entity.ResourceSearchView;
+import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString;
+import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
 import ca.uhn.fhir.jpa.searchparam.SearchParamConstants;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
@@ -87,7 +93,10 @@ import org.hl7.fhir.instance.model.api.IIdType;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.springframework.transaction.TransactionStatus;
 import org.springframework.transaction.support.TransactionCallbackWithoutResult;
 import org.springframework.transaction.support.TransactionTemplate;
@@ -544,6 +553,115 @@ public class FhirResourceDaoDstu3Test extends BaseJpaDstu3Test {
 		obs = myObservationDao.read(obsId.toUnqualifiedVersionless(), mySrd);
 	}
 
+	/**
+	 * We are starting a migration to inline forced_id into RESOURCE_TABLE.
+	 * Verify we are populating the new field AND the old join table for a couple of releases to allow smooth upgrades.
+	 */
+	@Nested
+	class ForcedIdFieldPopulation {
+		DaoMethodOutcome myMethodOutcome;
+		String myExpectedId;
+
+		@AfterEach
+		void tearDown() {
+			myDaoConfig.setResourceClientIdStrategy(new DaoConfig().getResourceClientIdStrategy());
+			myDaoConfig.setResourceServerIdStrategy(new DaoConfig().getResourceServerIdStrategy());
+		}
+
+		@ParameterizedTest
+		@CsvSource({
+			"SEQUENTIAL_NUMERIC,NOT_ALLOWED,",
+			"SEQUENTIAL_NUMERIC,ANY,forcedId",
+			"SEQUENTIAL_NUMERIC,ANY,123",
+			"SEQUENTIAL_NUMERIC,ANY,",
+			"SEQUENTIAL_NUMERIC,ALPHANUMERIC,forcedId",
+			// illegal "SEQUENTIAL_NUMERIC,ALPHANUMERIC,123",
+			"SEQUENTIAL_NUMERIC,ALPHANUMERIC,",
+			"UUID,NOT_ALLOWED,",
+			"UUID,ANY,forcedId",
+			"UUID,ANY,123",
+			"UUID,ANY,", // uuid server-ids still use  hfj_forced_id
+			"UUID,ALPHANUMERIC,forcedId",
+			// illegal "UUID,ALPHANUMERIC,123",
+			"UUID,ALPHANUMERIC,",
+		})
+		public void testCreateResource_populatesResourceTableFhirIdField(
+			IdStrategyEnum theServerIdStrategy,
+			ClientIdStrategyEnum theClientIdStrategy,
+			String theClientId
+		) {
+			// given id configuration settings
+			myDaoConfig.setResourceClientIdStrategy(theClientIdStrategy);
+			myDaoConfig.setResourceServerIdStrategy(theServerIdStrategy);
+
+			// create the resource with POST or PUT
+			Patient pat = new Patient();
+			runInTransaction(() -> {
+				if (theClientId != null) {
+					// PUT with id
+					pat.setId(theClientId);
+					myMethodOutcome = myPatientDao.update(pat, mySrd);
+					myExpectedId = theClientId;
+				} else {
+					// POST with server-assigned id
+					myMethodOutcome = myPatientDao.create(pat, mySrd);
+					myExpectedId = myMethodOutcome.getId().getIdPart();
+				}
+				assertEquals("Patient/" + myExpectedId,
+					myMethodOutcome.getId().toUnqualifiedVersionless().getValue(),
+					"the method returns the id");
+
+				// saving another resource in the same tx was causing a version bump.
+				// leaving it here to make sure that bug doesn't come back.
+				Condition condition = new Condition();
+				condition.getSubject().setReferenceElement(myMethodOutcome.getId());
+				myConditionDao.create(condition);
+
+				ourLog.info("about to flush");
+			});
+
+			// then verify the database contents
+			myEntityManager.clear();
+
+			// fetch the resource from the db and verify
+			ResourceTable readBackResource = myEntityManager
+				.find(ResourceTable.class, myMethodOutcome.getPersistentId().getId());
+			assertNotNull(readBackResource, "found entity");
+			assertEquals(1, readBackResource.getVersion(), "first version");
+			assertEquals(myExpectedId, readBackResource.getFhirId(), "inline column populated on readback");
+
+			ResourceHistoryTable readBackHistory = myEntityManager
+				.createQuery("select h from ResourceHistoryTable h where h.myResourceId = :resId and h.myResourceVersion = 1", ResourceHistoryTable.class)
+				.setParameter("resId", myMethodOutcome.getPersistentId().getId())
+				.getSingleResult();
+			assertNotNull(readBackHistory, "found history");
+
+			// no extra history
+			long historyCount = myEntityManager
+				.createQuery("select count(h) from ResourceHistoryTable h where h.myResourceId = :resId", Long.class)
+				.setParameter("resId", myMethodOutcome.getPersistentId().getId())
+				.getSingleResult();
+			assertEquals(1, historyCount, "only create one history version");
+
+			// make sure the search view works too
+			ResourceSearchView readBackView = myEntityManager
+				.createQuery("select v from ResourceSearchView v where v.myResourceId = :resId", ResourceSearchView.class)
+				.setParameter("resId", myMethodOutcome.getPersistentId().getId())
+				.getSingleResult();
+			assertNotNull(readBackView, "found search view");
+
+			// verify the forced id join still works
+			if (readBackResource.getForcedId() != null) {
+				assertEquals(myExpectedId, readBackResource.getForcedId().getForcedId(),
+					"legacy join populated");
+				assertEquals(myExpectedId, readBackView.getForcedId(),
+					"legacy join populated");
+			} else {
+				assertEquals(IdStrategyEnum.SEQUENTIAL_NUMERIC, theServerIdStrategy,
+					"hfj_forced_id join column is only empty when using server-assigned ids");
+			}		}
+	}
+
 	@Test
 	public void testCreateDuplicateTagsDoesNotCauseDuplicates() {
 		Patient p = new Patient();

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/config/HapiFhirLocalContainerEntityManagerFactoryBeanTest.java

@@ -0,0 +1,30 @@
+package ca.uhn.fhir.jpa.config;
+
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.support.DefaultListableBeanFactory;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class HapiFhirLocalContainerEntityManagerFactoryBeanTest {
+	HapiFhirLocalContainerEntityManagerFactoryBean myBean = new HapiFhirLocalContainerEntityManagerFactoryBean(new DefaultListableBeanFactory());
+
+	@Test
+	void testAddHibernateHookToEmptyProperty() {
+		// start empty
+
+		myBean.addHibernateHook("theKey", "theHookClass");
+
+		assertEquals("theHookClass", myBean.getJpaPropertyMap().get("theKey"));
+	}
+
+	@Test
+	void testAddHibernateHookToSomeRegisteredHooks() {
+		// start with some hooks already registered
+		myBean.getJpaPropertyMap().put("theKey", "hook1,hook2");
+
+		myBean.addHibernateHook("theKey", "theHookClass");
+
+		assertEquals("hook1,hook2,theHookClass", myBean.getJpaPropertyMap().get("theKey"));
+	}
+
+}