honeymoose/hapi-fhir
1
0
Fork 0
mirror of https://github.com/hapifhir/hapi-fhir.git synced 2025-02-19 19:35:42 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix to unnecessary resource version bump on update (#6715)

Browse Source 
* initial failing test.

* solution with changelog

* spotless

---------

Co-authored-by: peartree <etienne.poirier@smilecdr.com>
This commit is contained in:
Etienne Poirier 2025-02-14 10:50:03 -05:00 committed by GitHub
parent 066c242619
commit 57ee0240d4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 86 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_2_0/6714-unnessary-resource-version-bump-on-update.yaml Normal file
View File

@ -0,0 +1,6 @@
---
type: fix
issue: 6714
jira: SMILE-9691
title: "Previously, it was possible for a persisted resource to get a version bump due to unnecessary updates to its
associated tags. This issue is fixed."

46
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java
View File

@ -109,6 +109,7 @@ import jakarta.persistence.PersistenceContextType;
import org.apache.commons.lang3.NotImplementedException;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.Validate;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.hl7.fhir.instance.model.api.IAnyResource;
import org.hl7.fhir.instance.model.api.IBase;
import org.hl7.fhir.instance.model.api.IBaseCoding;
@ -291,7 +292,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
next.getVersion(),
myCodingSpy.getBooleanObject(next));
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
allDefs.add(tag);
theEntity.setHasTags(true);
}
@ -310,7 +311,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
next.getVersionElement().getValue(),
next.getUserSelectedElement().getValue());
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
allDefs.add(tag);
theEntity.setHasTags(true);
}
@ -323,7 +324,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
allDefs.add(tag);
theEntity.setHasTags(true);
}
@ -348,7 +349,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
next.getVersion(),
myCodingSpy.getBooleanObject(next));
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
theAllTags.add(tag);
theEntity.setHasTags(true);
}
@ -367,7 +368,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
next.getVersion(),
myCodingSpy.getBooleanObject(next));
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
theAllTags.add(tag);
theEntity.setHasTags(true);
}
@ -380,7 +381,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
theAllTags.add(tag);
theEntity.setHasTags(true);
}
@ -400,13 +401,42 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
TagDefinition profileDef = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, profile, null, null, null);
ResourceTag tag = theEntity.addTag(profileDef);
ResourceTag tag = maybeAddTagToEntity(theEntity, profileDef);
theAllTags.add(tag);
theEntity.setHasTags(true);
}
}
}
/**
* Utility method adding <code>theTagDefToAdd</code> to <code>theEntity</code>. Since the database may contain
* duplicate tagDefinitions, we perform a logical comparison, i.e. we don't care about the tagDefiniton.id, and add
* the tag to the entity only if the entity does not already have that tag.
*
* @param theEntity receiving the tagDefinition
* @param theTagDefToAdd to theEntity
* @return <code>theTagDefToAdd</code> wrapped in a resourceTag if it was added to <code>theEntity</code> or <code>theEntity</code>'s
* resourceTag encapsulating a tagDefinition that is logically equal to theTagDefToAdd.
*/
private ResourceTag maybeAddTagToEntity(ResourceTable theEntity, TagDefinition theTagDefToAdd) {
for (ResourceTag resourceTagFromEntity : theEntity.getTags()) {
TagDefinition tag = resourceTagFromEntity.getTag();
EqualsBuilder equalsBuilder = new EqualsBuilder();
equalsBuilder.append(tag.getSystem(), theTagDefToAdd.getSystem());
equalsBuilder.append(tag.getCode(), theTagDefToAdd.getCode());
equalsBuilder.append(tag.getDisplay(), theTagDefToAdd.getDisplay());
equalsBuilder.append(tag.getVersion(), theTagDefToAdd.getVersion());
equalsBuilder.append(tag.getUserSelected(), theTagDefToAdd.getUserSelected());
equalsBuilder.append(tag.getTagType(), theTagDefToAdd.getTagType());
if (equalsBuilder.isEquals()) {
return resourceTagFromEntity;
}
}
return theEntity.addTag(theTagDefToAdd);
}
private Set<ResourceTag> getAllTagDefinitions(ResourceTable theEntity) {
HashSet<ResourceTag> retVal = Sets.newHashSet();
if (theEntity.isHasTags()) {
@ -643,7 +673,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
return sourceExtension;
}
private boolean updateTags(
protected boolean updateTags(
TransactionDetails theTransactionDetails,
RequestDetails theRequest,
IBaseResource theResource,

43
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDaoTest.java
View File

@ -20,6 +20,9 @@ import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService;
import ca.uhn.fhir.jpa.delete.DeleteConflictService;
import ca.uhn.fhir.jpa.model.dao.JpaPid;
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
import ca.uhn.fhir.jpa.model.entity.ResourceTag;
import ca.uhn.fhir.jpa.model.entity.TagDefinition;
import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
import ca.uhn.fhir.jpa.model.search.SearchRuntimeDetails;
import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc;
import ca.uhn.fhir.jpa.search.ResourceSearchUrlSvc;
@ -74,7 +77,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.isNotNull;
@ -138,6 +140,9 @@ class BaseHapiFhirResourceDaoTest {
@Mock
private ResourceSearchUrlSvc myResourceSearchUrlSvc;
@Mock
private CacheTagDefinitionDao myCacheTagDefinitionDao;
@Captor
private ArgumentCaptor<SearchParameterMap> mySearchParameterMapCaptor;
@ -385,6 +390,42 @@ class BaseHapiFhirResourceDaoTest {
);
}
@Test
public void testUpdateTags_withDuplicateTags_willNotUpdateEntityTagList(){
RequestDetails sysRequest = new SystemRequestDetails();
TransactionDetails transactionDetails = new TransactionDetails();
TagDefinition duplicateTagDefinition = createTagDefinition(2);
// given a patient resource with tags stored in the database
TagDefinition tagDefinition = createTagDefinition(1);
ResourceTable entity = new ResourceTable();
entity.setId(new JpaPid(null, 1l));
entity.addTag(tagDefinition);
entity.setHasTags(true);
// given a client update request on the patient resource that is stored in the database
Patient patient = new Patient();
patient.getMeta().addTag(tagDefinition.getSystem(), tagDefinition.getCode(), tagDefinition.getDisplay());
// when we search the database for existing tags, return a duplicate one.
when(myCacheTagDefinitionDao.getTagOrNull(any(), any(), any(), any(), any(), any(), any())).thenReturn(duplicateTagDefinition);
boolean updated = mySpiedSvc.updateTags(transactionDetails, sysRequest, patient, entity);
// assert that the entity was not updated with the duplicate tag
assertThat(updated).isFalse();
Collection<ResourceTag> entityTags = entity.getTags();
assertThat(entityTags).hasSize(1);
assertThat(entityTags.iterator().next().getTag().getId()).isEqualTo(tagDefinition.getId());
}
private TagDefinition createTagDefinition(int theId) {
TagDefinition tagDefinition = new TagDefinition(TagTypeEnum.TAG, "sytem", "code", null);
tagDefinition.setId((long) theId);
return tagDefinition;
}
@Nested
class DeleteThresholds {
private static final String URL = "Patient?_lastUpdated=gt2024-01-01";