Deprecate redundant BaseResourceModifiedMessage myId (#3116)

* Deprecate redundant BaseResourceModifiedMessage myId

* Add changelog

* Test fix

* Test fix

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3116-deprecate-getid.yaml

@@ -0,0 +1,5 @@
+---
+type: remove
+issue: 3116
+title: "The BaseResourceModifiedJson class had a redundant field called myId containing the
+  payload ID. This has been removed."

hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/broker/MdmMessageHandler.java

@@ -92,7 +92,7 @@ public class MdmMessageHandler implements MessageHandler {
 	}
 
 	private void matchMdmAndUpdateLinks(ResourceModifiedMessage theMsg) {
-		String resourceType = theMsg.getId(myFhirContext).getResourceType();
+		String resourceType = theMsg.getPayloadId(myFhirContext).getResourceType();
 		validateResourceType(resourceType);
 		MdmTransactionContext mdmContext = createMdmContext(theMsg, resourceType);
 		try {

hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/match/matcher/matching/DaoSubscriptionMatcher.java

@@ -50,13 +50,11 @@ public class DaoSubscriptionMatcher implements ISubscriptionMatcher {
 
 	@Override
 	public InMemoryMatchResult match(CanonicalSubscription theSubscription, ResourceModifiedMessage theMsg) {
-		IIdType id = theMsg.getId(myCtx);
+		IIdType id = theMsg.getPayloadId(myCtx);
-		String resourceType = id.getResourceType();
-		String resourceId = id.getIdPart();
 		String criteria = theSubscription.getCriteriaString();
 
-		// run the subscriptions query and look for matches, add the id as part of the criteria to avoid getting matches of previous resources rather than the recent resource
+		// Run the subscriptions query and look for matches, add the id as part of the criteria to avoid getting matches of previous resources rather than the recent resource
-		criteria += "&_id=" + resourceType + "/" + resourceId;
+		criteria += "&_id=" + id.toUnqualifiedVersionless().getValue();
 
 		IBundleProvider results = performSearch(criteria);
 

hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/match/matcher/matching/InMemorySubscriptionMatcher.java

@@ -44,7 +44,7 @@ public class InMemorySubscriptionMatcher implements ISubscriptionMatcher {
 			return mySearchParamMatcher.match(theSubscription.getCriteriaString(), theMsg.getNewPayload(myContext), null);
 		} catch (Exception e) {
 			ourLog.error("Failure in in-memory matcher", e);
-			throw new InternalErrorException("Failure performing memory-match for resource ID[" + theMsg.getId(myContext) + "] for subscription ID[" + theSubscription.getIdElementString() + "]: " + e.getMessage(), e);
+			throw new InternalErrorException("Failure performing memory-match for resource ID[" + theMsg.getPayloadId(myContext) + "] for subscription ID[" + theSubscription.getIdElementString() + "]: " + e.getMessage(), e);
 		}
 	}
 

hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/match/matcher/subscriber/BaseSubscriberForSubscriptionResources.java

@@ -37,7 +37,7 @@ public abstract class BaseSubscriberForSubscriptionResources implements MessageH
 
 	protected boolean isSubscription(ResourceModifiedMessage theNewResource) {
 		String payloadIdType = null;
-		IIdType payloadId = theNewResource.getId(myFhirContext);
+		IIdType payloadId = theNewResource.getPayloadId(myFhirContext);
 		if (payloadId != null) {
 			payloadIdType = payloadId.getResourceType();
 		}

hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/match/matcher/subscriber/SubscriptionMatchingSubscriber.java

@@ -116,7 +116,7 @@ public class SubscriptionMatchingSubscriber implements MessageHandler {
 	}
 
 	private void doMatchActiveSubscriptionsAndDeliver(ResourceModifiedMessage theMsg) {
-		IIdType resourceId = theMsg.getId(myFhirContext);
+		IIdType resourceId = theMsg.getPayloadId(myFhirContext);
 
 		Collection<ActiveSubscription> subscriptions = mySubscriptionRegistry.getAll();
 

hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/match/matcher/subscriber/SubscriptionRegisteringSubscriber.java

@@ -72,7 +72,7 @@ public class SubscriptionRegisteringSubscriber extends BaseSubscriberForSubscrip
 
 		switch (payload.getOperationType()) {
 			case DELETE:
-				mySubscriptionRegistry.unregisterSubscriptionIfRegistered(payload.getId(myFhirContext).getIdPart());
+				mySubscriptionRegistry.unregisterSubscriptionIfRegistered(payload.getPayloadId(myFhirContext).getIdPart());
 				break;
 			case CREATE:
 			case UPDATE:
@@ -81,7 +81,7 @@ public class SubscriptionRegisteringSubscriber extends BaseSubscriberForSubscrip
 				if ("active".equals(statusString)) {
 					mySubscriptionRegistry.registerSubscriptionUnlessAlreadyRegistered(payload.getNewPayload(myFhirContext));
 				} else {
-					mySubscriptionRegistry.unregisterSubscriptionIfRegistered(payload.getId(myFhirContext).getIdPart());
+					mySubscriptionRegistry.unregisterSubscriptionIfRegistered(payload.getPayloadId(myFhirContext).getIdPart());
 				}
 				break;
 			case MANUALLY_TRIGGERED:

hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/ResourceModifiedTest.java

@@ -17,7 +17,7 @@ public class ResourceModifiedTest {
 		org.setName("testOrgName");
 		org.setId("Organization/testOrgId");
 		ResourceModifiedMessage msg = new ResourceModifiedMessage(myFhirContext, org, ResourceModifiedMessage.OperationTypeEnum.CREATE);
-		assertEquals(org.getIdElement(), msg.getId(myFhirContext));
+		assertEquals(org.getIdElement(), msg.getPayloadId(myFhirContext));
 		assertEquals(ResourceModifiedMessage.OperationTypeEnum.CREATE, msg.getOperationType());
 		Organization decodedOrg = (Organization) msg.getNewPayload(myFhirContext);
 		assertEquals(org.getId(), decodedOrg.getId());
@@ -30,7 +30,7 @@ public class ResourceModifiedTest {
 		org.setName("testOrgName");
 		org.setId("Organization/testOrgId");
 		ResourceModifiedMessage msg = new ResourceModifiedMessage(myFhirContext, org, ResourceModifiedMessage.OperationTypeEnum.UPDATE);
-		assertEquals(org.getIdElement(), msg.getId(myFhirContext));
+		assertEquals(org.getIdElement(), msg.getPayloadId(myFhirContext));
 		assertEquals(ResourceModifiedMessage.OperationTypeEnum.UPDATE, msg.getOperationType());
 		Organization decodedOrg = (Organization) msg.getNewPayload(myFhirContext);
 		assertEquals(org.getId(), decodedOrg.getId());
@@ -43,7 +43,7 @@ public class ResourceModifiedTest {
 		org.setName("testOrgName");
 		org.setId("testOrgId");
 		ResourceModifiedMessage msg = new ResourceModifiedMessage(myFhirContext, org, ResourceModifiedMessage.OperationTypeEnum.DELETE);
-		assertEquals(org.getIdElement(), msg.getId(myFhirContext));
+		assertEquals("Organization/testOrgId", msg.getPayloadId(myFhirContext).getValue());
 		assertEquals(ResourceModifiedMessage.OperationTypeEnum.DELETE, msg.getOperationType());
 		assertNull(msg.getNewPayload(myFhirContext));
 	}

hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/subscriber/SubscriptionMatchingSubscriberTest.java

@@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.subscription.module.subscriber;
 
 import ca.uhn.fhir.jpa.subscription.module.standalone.BaseBlockingQueueSubscribableChannelDstu3Test;
 import ca.uhn.fhir.rest.api.Constants;
+import org.hl7.fhir.dstu3.model.Observation;
 import org.junit.jupiter.api.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -56,6 +57,24 @@ public class SubscriptionMatchingSubscriberTest extends BaseBlockingQueueSubscri
 		assertEquals(Constants.CT_FHIR_XML_NEW, ourContentTypes.get(0));
 	}
 
+	@Test
+	public void testRestHookSubscription_NoResourceTypeInPayloadId() throws Exception {
+		sendSubscription("Observation?", "application/fhir+xml", ourListenerServerBase);
+
+		assertEquals(1, mySubscriptionRegistry.size());
+		ourObservationListener.setExpectedCount(1);
+
+		Observation observation = new Observation();
+		observation.setId("OBS");
+		observation.setStatus(Observation.ObservationStatus.CORRECTED);
+		sendResource(observation);
+
+		ourObservationListener.awaitExpected();
+
+		assertEquals(1, ourContentTypes.size());
+		assertEquals(Constants.CT_FHIR_XML_NEW, ourContentTypes.get(0));
+	}
+
 	@Test
 	public void testRestHookSubscriptionWithoutPayload() throws Exception {
 		String payload = "";

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/messaging/BaseResourceModifiedMessage.java

@@ -39,8 +39,6 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public abstract class BaseResourceModifiedMessage extends BaseResourceMessage implements IResourceMessage, IModelJson {
 
-	@JsonProperty("resourceId")
-	protected String myId;
 	@JsonProperty("payload")
 	protected String myPayload;
 	@JsonProperty("payloadId")
@@ -57,10 +55,11 @@ public abstract class BaseResourceModifiedMessage extends BaseResourceMessage im
 
 	public BaseResourceModifiedMessage(FhirContext theFhirContext, IBaseResource theResource, OperationTypeEnum theOperationType) {
 		this();
-		setId(theResource.getIdElement());
 		setOperationType(theOperationType);
 		if (theOperationType != OperationTypeEnum.DELETE) {
 			setNewPayload(theFhirContext, theResource);
+		} else {
+			setPayloadIdFromPayload(theFhirContext, theResource);
 		}
 	}
 
@@ -76,14 +75,45 @@ public abstract class BaseResourceModifiedMessage extends BaseResourceMessage im
 		return myPayloadId;
 	}
 
-	public String getId() {
+	/**
-		return myId;
+	 * @since 5.6.0
+	 */
+	public void setPayloadId(IIdType thePayloadId) {
+		myPayloadId = null;
+		if (thePayloadId != null) {
+			myPayloadId = thePayloadId.getValue();
+		}
 	}
 
+	/**
+	 * @deprecated Use {@link #getPayloadId()} instead. Deprecated in 5.6.0 / 2021-10-27
+	 */
+	public String getId() {
+		return myPayloadId;
+	}
+
+	/**
+	 * @deprecated Use {@link #setPayloadId(IIdType)}. Deprecated in 5.6.0 / 2021-10-27
+	 */
+	@Deprecated
+	public void setId(IIdType theId) {
+		setPayloadId(theId);
+	}
+
+	/**
+	 * @deprecated Use {@link #getPayloadId(FhirContext)}. Deprecated in 5.6.0 / 2021-10-27
+	 */
 	public IIdType getId(FhirContext theCtx) {
+		return getPayloadId(theCtx);
+	}
+
+	/**
+	 * @since 5.6.0
+	 */
+	public IIdType getPayloadId(FhirContext theCtx) {
 		IIdType retVal = null;
-		if (myId != null) {
+		if (myPayloadId != null) {
-			retVal = theCtx.getVersion().newIdType().setValue(myId);
+			retVal = theCtx.getVersion().newIdType().setValue(myPayloadId);
 		}
 		return retVal;
 	}
@@ -113,14 +143,7 @@ public abstract class BaseResourceModifiedMessage extends BaseResourceMessage im
 		return "";
 	}
 
-	public void setId(IIdType theId) {
+	protected void setNewPayload(FhirContext theCtx, IBaseResource thePayload) {
-		myId = null;
-		if (theId != null) {
-			myId = theId.getValue();
-		}
-	}
-
-	protected void setNewPayload(FhirContext theCtx, IBaseResource theNewPayload) {
 		/*
 		 * References with placeholders would be invalid by the time we get here, and
 		 * would be caught before we even get here. This check is basically a last-ditch
@@ -128,7 +151,7 @@ public abstract class BaseResourceModifiedMessage extends BaseResourceMessage im
 		 * should prevent this from happening (hence it only being an assert as
 		 * opposed to something executed all the time).
 		 */
-		assert payloadContainsNoPlaceholderReferences(theCtx, theNewPayload);
+		assert payloadContainsNoPlaceholderReferences(theCtx, thePayload);
 
 		/*
 		 * Note: Don't set myPayloadDecoded in here- This is a false optimization since
@@ -137,8 +160,27 @@ public abstract class BaseResourceModifiedMessage extends BaseResourceMessage im
 		 * as we would in a queue engine can mask bugs.
 		 * -JA
 		 */
-		myPayload = theCtx.newJsonParser().encodeResourceToString(theNewPayload);
+		myPayload = theCtx.newJsonParser().encodeResourceToString(thePayload);
-		myPayloadId = theNewPayload.getIdElement().toUnqualified().getValue();
+
+		setPayloadIdFromPayload(theCtx, thePayload);
+	}
+
+	private void setPayloadIdFromPayload(FhirContext theCtx, IBaseResource thePayload) {
+		IIdType payloadIdType = thePayload.getIdElement().toUnqualified();
+		if (!payloadIdType.hasResourceType()) {
+			String resourceType = theCtx.getResourceType(thePayload);
+			payloadIdType = payloadIdType.withResourceType(resourceType);
+		}
+
+		setPayloadId(payloadIdType);
+	}
+
+	@Override
+	public String toString() {
+		return new ToStringBuilder(this)
+			.append("operationType", myOperationType)
+			.append("payloadId", myPayloadId)
+			.toString();
 	}
 
 	protected static boolean payloadContainsNoPlaceholderReferences(FhirContext theCtx, IBaseResource theNewPayload) {
@@ -162,16 +204,5 @@ public abstract class BaseResourceModifiedMessage extends BaseResourceMessage im
 		}
 		return true;
 	}
-
-	@Override
-	public String toString() {
-		return new ToStringBuilder(this)
-			.append("myId", myId)
-			.append("myOperationType", myOperationType)
-//			.append("myPayload", myPayload)
-			.append("myPayloadId", myPayloadId)
-//			.append("myPayloadDecoded", myPayloadDecoded)
-			.toString();
-	}
 }
 

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/subscription/model/ResourceModifiedMessage.java

@@ -67,12 +67,9 @@ public class ResourceModifiedMessage extends BaseResourceModifiedMessage {
 	@Override
 	public String toString() {
 		return new ToStringBuilder(this)
-			.append("myId", myId)
+			.append("operationType", myOperationType)
-			.append("myOperationType", myOperationType)
+			.append("subscriptionId", mySubscriptionId)
-			.append("mySubscriptionId", mySubscriptionId)
+			.append("payloadId", myPayloadId)
-//			.append("myPayload", myPayload)
-			.append("myPayloadId", myPayloadId)
-//			.append("myPayloadDecoded", myPayloadDecoded)
 			.toString();
 	}
 }