Skip to content

7152 - Add resource type to HFJ_RESOURCE_MODIFIED's PK to avoid conflicts #7154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

7152 - Add resource type to HFJ_RESOURCE_MODIFIED's PK to avoid conflicts #7154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
01d5ed0
7152 - Add resource type to HFJ_RESOURCE_MODIFIED's PK to avoid confl…
another-luis Jul 23, 2025
f9a034e
7495 Fix integration tests affected by changes
another-luis Jul 23, 2025
12fba4c
7495 Add unit tests plus some refinements
another-luis Jul 24, 2025
b24da1d
Merge remote-tracking branch 'origin/master' into 7495-incorporate-re…
another-luis Jul 29, 2025
5a54edb
7495: incorporate feedback from code review
another-luis Jul 29, 2025
abe6d17
7495 - Add a test to verify updates work as expected when resources s…
another-luis Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...i/fhir/changelog/8_4_0/7152-add-resource-type-to-primary-key-to-guarantee-uniqueness.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
type: fix
issue: 7152
jira: SMILE-10226
title: "Modified resources could potentially be removed from the subscription list of they shared ID and Version with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: rewrite this changelog. There's a typo in it, and it should be written from a user failure scenario point of view. Something like:

Previously, subscriptions could occasionally fail to be delivered if a resource had the same ID as a resource of another type, (e.g. `Patient/abc-123` and `Observation/abc-123`), as well as the same version number. This has been corrected, and now the resource type is part of the primary key, ensuring uniqueness during clearing of the table.

others. This has been fixed."
9 changes: 9 additions & 0 deletions ...jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,15 @@ public HapiFhirJpaMigrationTasks(Set<String> theFlags) {
init780();
init820();
init840();
init860();
}

protected void init860() {
Builder version = forVersion(VersionEnum.V8_6_0);
{
version.onTable("HFJ_RESOURCE_MODIFIED").dropPrimaryKey("20250729.1");
version.onTable("HFJ_RESOURCE_MODIFIED").addPrimaryKey("20250729.2", "RES_ID", "RES_VER", "RESOURCE_TYPE");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is this not making it into 8.4.0? Check with the support person/the ticket to ensure that this is known to them, and it isn't expected for 8.4.0

}

protected void init840() {
Expand Down
4 changes: 2 additions & 2 deletions ...src/main/java/ca/uhn/fhir/jpa/subscription/ResourceModifiedMessagePersistenceSvcImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,11 @@ ResourceModifiedEntity createEntityFrom(ResourceModifiedMessage theMsg) {
IIdType theMsgId = theMsg.getPayloadId(myFhirContext);

ResourceModifiedEntity resourceModifiedEntity = new ResourceModifiedEntity();
resourceModifiedEntity.setResourceModifiedEntityPK(with(theMsgId.getIdPart(), theMsgId.getVersionIdPart()));
resourceModifiedEntity.setResourceModifiedEntityPK(
with(theMsgId.getIdPart(), theMsgId.getVersionIdPart(), theMsgId.getResourceType()));

String partialModifiedMessage = getPayloadLessMessageAsString(theMsg);
resourceModifiedEntity.setSummaryResourceModifiedMessage(partialModifiedMessage);
resourceModifiedEntity.setResourceType(theMsgId.getResourceType());
resourceModifiedEntity.setCreatedTime(new Date());

return resourceModifiedEntity;
Expand Down
26 changes: 21 additions & 5 deletions .../src/main/java/ca/uhn/fhir/jpa/model/entity/PersistedResourceModifiedMessageEntityPK.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ public class PersistedResourceModifiedMessageEntityPK implements IPersistedResou
@Column(name = "RES_VER", length = 8, nullable = false)
private String myResourceVersion;

@Column(name = "RESOURCE_TYPE", length = ResourceTable.RESTYPE_LEN, nullable = false)
private String myResourceType;

public String getResourcePid() {
return myResourcePid;
}
Expand All @@ -53,27 +56,40 @@ public PersistedResourceModifiedMessageEntityPK setResourceVersion(String theRes
return this;
}

public static PersistedResourceModifiedMessageEntityPK with(String theResourcePid, String theResourceVersion) {
public String getResourceType() {
return myResourceType;
}

public PersistedResourceModifiedMessageEntityPK setResourceType(String theResourceType) {
myResourceType = theResourceType;
return this;
}

public static PersistedResourceModifiedMessageEntityPK with(
String theResourcePid, String theResourceVersion, String theResourceType) {
return new PersistedResourceModifiedMessageEntityPK()
.setResourcePid(theResourcePid)
.setResourceVersion(theResourceVersion);
.setResourceVersion(theResourceVersion)
.setResourceType(theResourceType);
}

@Override
public boolean equals(Object theO) {
if (this == theO) return true;
if (theO == null || getClass() != theO.getClass()) return false;
PersistedResourceModifiedMessageEntityPK that = (PersistedResourceModifiedMessageEntityPK) theO;
return myResourcePid.equals(that.myResourcePid) && myResourceVersion.equals(that.myResourceVersion);
return myResourcePid.equals(that.myResourcePid)
&& myResourceVersion.equals(that.myResourceVersion)
&& myResourceType.equals(that.myResourceType);
}

@Override
public int hashCode() {
return Objects.hash(myResourcePid, myResourceVersion);
return Objects.hash(myResourcePid, myResourceVersion, myResourceType);
}

@Override
public String toString() {
return myResourcePid + "/" + myResourceVersion;
return myResourceType + "/" + myResourcePid + "/" + myResourceVersion;
}
}
10 changes: 1 addition & 9 deletions ...ir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceModifiedEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ public class ResourceModifiedEntity implements IPersistedResourceModifiedMessage
@Temporal(TemporalType.TIMESTAMP)
private Date myCreatedTime;

@Column(name = "RESOURCE_TYPE", length = ResourceTable.RESTYPE_LEN, nullable = false)
private String myResourceType;

public PersistedResourceModifiedMessageEntityPK getResourceModifiedEntityPK() {
return myResourceModifiedEntityPK;
}
Expand All @@ -68,12 +65,7 @@ public ResourceModifiedEntity setResourceModifiedEntityPK(

@Override
public String getResourceType() {
return myResourceType;
}

public ResourceModifiedEntity setResourceType(String theResourceType) {
myResourceType = theResourceType;
return this;
return myResourceModifiedEntityPK == null ? null : myResourceModifiedEntityPK.getResourceType();
}

@Override
Expand Down
37 changes: 31 additions & 6 deletions ...jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionsR4Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import ca.uhn.fhir.util.BundleUtil;
import ca.uhn.test.concurrency.PointcutLatch;
import com.apicatalog.jsonld.StringUtils;
import com.google.common.base.Strings;
import jakarta.annotation.Nonnull;
import jakarta.annotation.PostConstruct;
import net.ttddyy.dsproxy.QueryCount;
Expand Down Expand Up @@ -224,21 +225,45 @@ protected Observation buildBaseObservation(String theCode, String theSystem) {
}

protected Patient sendPatient() {
return sendPatient(null);
}

protected Patient sendPatient(String thePatientId) {
Patient patient = new Patient();
patient.setActive(true);
if(!Strings.isNullOrEmpty(thePatientId)) {
patient.setId(thePatientId);
}

IIdType id = myPatientDao.create(patient).getId();
patient.setId(id);
DaoMethodOutcome outcome;
if(Strings.isNullOrEmpty(thePatientId)) {
outcome = myPatientDao.create(patient);
patient.setId( outcome.getId());
} else {
myPatientDao.update(patient);
}

patient.setActive(true);
return patient;
}

protected Organization sendOrganization() {
return sendOrganization(null);
}

protected Organization sendOrganization( String theOrgId) {
Organization org = new Organization();
org.setName("ORG");
org.setName("ORG" + (theOrgId == null ? "" : theOrgId));
if(!Strings.isNullOrEmpty(theOrgId)) {
org.setId(theOrgId);
}

IIdType id = myOrganizationDao.create(org).getId();
org.setId(id);
DaoMethodOutcome outcome;
if(Strings.isNullOrEmpty(theOrgId)) {
outcome = myOrganizationDao.create(org);
org.setId(outcome.getId());
} else {
myOrganizationDao.update(org);
}

return org;
}
Expand Down
2 changes: 1 addition & 1 deletion ...rc/test/java/ca/uhn/fhir/jpa/subscription/async/AsyncSubscriptionMessageSubmissionIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,9 @@ public void runDeliveryPass_withManyResources_isBatchedAndKeepsResourceUsageDown
int numberOfResourcesToCreate = factor * AsyncResourceModifiedSubmitterSvc.MAX_LIMIT;

ResourceModifiedEntity entity = new ResourceModifiedEntity();
entity.setResourceType(resourceType);
PersistedResourceModifiedMessageEntityPK rpm = new PersistedResourceModifiedMessageEntityPK();
rpm.setResourceVersion("1");
rpm.setResourceType(resourceType);
entity.setResourceModifiedEntityPK(rpm);

// we reuse the same exact msg content to avoid
Expand Down
51 changes: 50 additions & 1 deletion ...test-r4/src/test/java/ca/uhn/fhir/jpa/subscription/message/MessageSubscriptionR4Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.hl7.fhir.r4.model.Observation;
import org.hl7.fhir.r4.model.Organization;
import org.hl7.fhir.r4.model.Patient;
import org.hl7.fhir.r4.model.ResourceType;
import org.hl7.fhir.r4.model.Subscription;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -238,13 +239,61 @@ public void testMethodDeleteByPK_whenEntityExists_willDeleteTheEntityAndReturnTr
.hasSize(0);
}

@Test
public void testMethodDeleteByPK_whenEntitiesExistWithRepeatedId_willDeleteTheCorrectEntityAndReturnTrue(){
String commonId = "common-id";

mySubscriptionTestUtil.unregisterSubscriptionInterceptor();

// given
TransactionTemplate transactionTemplate = new TransactionTemplate(myTxManager);

Patient patient = sendPatient(commonId);
ResourceModifiedMessage patientResourceModifiedMessage = new ResourceModifiedMessage(myFhirContext, patient, BaseResourceMessage.OperationTypeEnum.CREATE);
IPersistedResourceModifiedMessage persistedPatientResourceModifiedMessage = myResourceModifiedMessagePersistenceSvc.persist(patientResourceModifiedMessage);

Organization org = sendOrganization(commonId);
ResourceModifiedMessage orgResourceModifiedMessage = new ResourceModifiedMessage(myFhirContext, org, BaseResourceMessage.OperationTypeEnum.CREATE);
myResourceModifiedMessagePersistenceSvc.persist(orgResourceModifiedMessage);

// when
boolean wasDeleted = transactionTemplate.execute(tx -> myResourceModifiedMessagePersistenceSvc.deleteByPK(persistedPatientResourceModifiedMessage.getPersistedResourceModifiedMessagePk()));

// then
assertTrue(wasDeleted);
Page<IPersistedResourceModifiedMessage> messages = myResourceModifiedMessagePersistenceSvc.findAllOrderedByCreatedTime(Pageable.unpaged());
assertThat(messages).hasSize(1);
assertEquals(ResourceType.Organization.name(), messages.stream().toList().get(0).getResourceType());

}

@Test
public void testMethodPersist_AddEntriesWithSameExternalIdAndVersion_expectSuccess(){
mySubscriptionTestUtil.unregisterSubscriptionInterceptor();

// given
String commonId = "common-id";
Patient patient = sendPatient(commonId);
Organization organization = sendOrganization(commonId);

ResourceModifiedMessage patientResourceModifiedMessage = new ResourceModifiedMessage(myFhirContext, patient, BaseResourceMessage.OperationTypeEnum.CREATE);
ResourceModifiedMessage organizationResourceModifiedMessage = new ResourceModifiedMessage(myFhirContext, organization, BaseResourceMessage.OperationTypeEnum.CREATE);

// when
myResourceModifiedMessagePersistenceSvc.persist(patientResourceModifiedMessage);
myResourceModifiedMessagePersistenceSvc.persist(organizationResourceModifiedMessage);

// then
assertEquals(2, myResourceModifiedMessagePersistenceSvc.getMessagePersistedCount());
}

@Test
public void testMethodDeleteByPK_whenEntityDoesNotExist_willReturnFalse(){
mySubscriptionTestUtil.unregisterSubscriptionInterceptor();

// given
TransactionTemplate transactionTemplate = new TransactionTemplate(myTxManager);
IPersistedResourceModifiedMessagePK nonExistentResourceWithPk = PersistedResourceModifiedMessageEntityPK.with("one", "one");
IPersistedResourceModifiedMessagePK nonExistentResourceWithPk = PersistedResourceModifiedMessageEntityPK.with("one", "one", ResourceType.Patient.toString());

// when
boolean wasDeleted = transactionTemplate.execute(tx -> myResourceModifiedMessagePersistenceSvc.deleteByPK(nonExistentResourceWithPk));
Expand Down
Loading