Skip to content

Commit 57ee024

Browse files
epeartreepeartree
epeartree
and
peartree
authored
Fix to unnecessary resource version bump on update (hapifhir#6715)
* initial failing test. * solution with changelog * spotless --------- Co-authored-by: peartree <[email protected]>
1 parent 066c242 commit 57ee024

File tree

3 files changed

+86
-9
lines changed

3 files changed

+86
-9
lines changed

‎hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_2_0/6714-unnessary-resource-version-bump-on-update.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
type: fix
3+
issue: 6714
4+
jira: SMILE-9691
5+
title: "Previously, it was possible for a persisted resource to get a version bump due to unnecessary updates to its
6+
associated tags. This issue is fixed."

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

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@
109109
import org.apache.commons.lang3.NotImplementedException;
110110
import org.apache.commons.lang3.StringUtils;
111111
import org.apache.commons.lang3.Validate;
112+
import org.apache.commons.lang3.builder.EqualsBuilder;
112113
import org.hl7.fhir.instance.model.api.IAnyResource;
113114
import org.hl7.fhir.instance.model.api.IBase;
114115
import org.hl7.fhir.instance.model.api.IBaseCoding;
@@ -291,7 +292,7 @@ private void extractHapiTags(
291292
next.getVersion(),
292293
myCodingSpy.getBooleanObject(next));
293294
if (def != null) {
294-
ResourceTag tag = theEntity.addTag(def);
295+
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
295296
allDefs.add(tag);
296297
theEntity.setHasTags(true);
297298
}
@@ -310,7 +311,7 @@ private void extractHapiTags(
310311
next.getVersionElement().getValue(),
311312
next.getUserSelectedElement().getValue());
312313
if (def != null) {
313-
ResourceTag tag = theEntity.addTag(def);
314+
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
314315
allDefs.add(tag);
315316
theEntity.setHasTags(true);
316317
}
@@ -323,7 +324,7 @@ private void extractHapiTags(
323324
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
324325
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
325326
if (def != null) {
326-
ResourceTag tag = theEntity.addTag(def);
327+
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
327328
allDefs.add(tag);
328329
theEntity.setHasTags(true);
329330
}
@@ -348,7 +349,7 @@ private void extractRiTags(
348349
next.getVersion(),
349350
myCodingSpy.getBooleanObject(next));
350351
if (def != null) {
351-
ResourceTag tag = theEntity.addTag(def);
352+
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
352353
theAllTags.add(tag);
353354
theEntity.setHasTags(true);
354355
}
@@ -367,7 +368,7 @@ private void extractRiTags(
367368
next.getVersion(),
368369
myCodingSpy.getBooleanObject(next));
369370
if (def != null) {
370-
ResourceTag tag = theEntity.addTag(def);
371+
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
371372
theAllTags.add(tag);
372373
theEntity.setHasTags(true);
373374
}
@@ -380,7 +381,7 @@ private void extractRiTags(
380381
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
381382
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
382383
if (def != null) {
383-
ResourceTag tag = theEntity.addTag(def);
384+
ResourceTag tag = maybeAddTagToEntity(theEntity, def);
384385
theAllTags.add(tag);
385386
theEntity.setHasTags(true);
386387
}
@@ -400,13 +401,42 @@ private void extractProfileTags(
400401
TagDefinition profileDef = cacheTagDefinitionDao.getTagOrNull(
401402
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, profile, null, null, null);
402403

403-
ResourceTag tag = theEntity.addTag(profileDef);
404+
ResourceTag tag = maybeAddTagToEntity(theEntity, profileDef);
404405
theAllTags.add(tag);
405406
theEntity.setHasTags(true);
406407
}
407408
}
408409
}
409410

411+
/**
412+
* Utility method adding <code>theTagDefToAdd</code> to <code>theEntity</code>. Since the database may contain
413+
* duplicate tagDefinitions, we perform a logical comparison, i.e. we don't care about the tagDefiniton.id, and add
414+
* the tag to the entity only if the entity does not already have that tag.
415+
*
416+
* @param theEntity receiving the tagDefinition
417+
* @param theTagDefToAdd to theEntity
418+
* @return <code>theTagDefToAdd</code> wrapped in a resourceTag if it was added to <code>theEntity</code> or <code>theEntity</code>'s
419+
* resourceTag encapsulating a tagDefinition that is logically equal to theTagDefToAdd.
420+
*/
421+
private ResourceTag maybeAddTagToEntity(ResourceTable theEntity, TagDefinition theTagDefToAdd) {
422+
for (ResourceTag resourceTagFromEntity : theEntity.getTags()) {
423+
TagDefinition tag = resourceTagFromEntity.getTag();
424+
EqualsBuilder equalsBuilder = new EqualsBuilder();
425+
equalsBuilder.append(tag.getSystem(), theTagDefToAdd.getSystem());
426+
equalsBuilder.append(tag.getCode(), theTagDefToAdd.getCode());
427+
equalsBuilder.append(tag.getDisplay(), theTagDefToAdd.getDisplay());
428+
equalsBuilder.append(tag.getVersion(), theTagDefToAdd.getVersion());
429+
equalsBuilder.append(tag.getUserSelected(), theTagDefToAdd.getUserSelected());
430+
equalsBuilder.append(tag.getTagType(), theTagDefToAdd.getTagType());
431+
432+
if (equalsBuilder.isEquals()) {
433+
return resourceTagFromEntity;
434+
}
435+
}
436+
437+
return theEntity.addTag(theTagDefToAdd);
438+
}
439+
410440
private Set<ResourceTag> getAllTagDefinitions(ResourceTable theEntity) {
411441
HashSet<ResourceTag> retVal = Sets.newHashSet();
412442
if (theEntity.isHasTags()) {
@@ -643,7 +673,7 @@ protected EncodedResource populateResourceIntoEntity(
643673
return sourceExtension;
644674
}
645675

646-
private boolean updateTags(
676+
protected boolean updateTags(
647677
TransactionDetails theTransactionDetails,
648678
RequestDetails theRequest,
649679
IBaseResource theResource,

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

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@
2020
import ca.uhn.fhir.jpa.delete.DeleteConflictService;
2121
import ca.uhn.fhir.jpa.model.dao.JpaPid;
2222
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
23+
import ca.uhn.fhir.jpa.model.entity.ResourceTag;
24+
import ca.uhn.fhir.jpa.model.entity.TagDefinition;
25+
import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
2326
import ca.uhn.fhir.jpa.model.search.SearchRuntimeDetails;
2427
import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc;
2528
import ca.uhn.fhir.jpa.search.ResourceSearchUrlSvc;
@@ -74,7 +77,6 @@
7477
import static org.junit.jupiter.api.Assertions.assertNotNull;
7578
import static org.junit.jupiter.api.Assertions.fail;
7679
import static org.mockito.ArgumentMatchers.any;
77-
import static org.mockito.ArgumentMatchers.anyLong;
7880
import static org.mockito.ArgumentMatchers.anyString;
7981
import static org.mockito.ArgumentMatchers.eq;
8082
import static org.mockito.ArgumentMatchers.isNotNull;
@@ -138,6 +140,9 @@ class BaseHapiFhirResourceDaoTest {
138140
@Mock
139141
private ResourceSearchUrlSvc myResourceSearchUrlSvc;
140142

143+
@Mock
144+
private CacheTagDefinitionDao myCacheTagDefinitionDao;
145+
141146
@Captor
142147
private ArgumentCaptor<SearchParameterMap> mySearchParameterMapCaptor;
143148

@@ -385,6 +390,42 @@ static Stream<Arguments> searchParameterMapProvider() {
385390
);
386391
}
387392

393+
@Test
394+
public void testUpdateTags_withDuplicateTags_willNotUpdateEntityTagList(){
395+
RequestDetails sysRequest = new SystemRequestDetails();
396+
TransactionDetails transactionDetails = new TransactionDetails();
397+
TagDefinition duplicateTagDefinition = createTagDefinition(2);
398+
399+
// given a patient resource with tags stored in the database
400+
TagDefinition tagDefinition = createTagDefinition(1);
401+
ResourceTable entity = new ResourceTable();
402+
entity.setId(new JpaPid(null, 1l));
403+
entity.addTag(tagDefinition);
404+
entity.setHasTags(true);
405+
406+
// given a client update request on the patient resource that is stored in the database
407+
Patient patient = new Patient();
408+
patient.getMeta().addTag(tagDefinition.getSystem(), tagDefinition.getCode(), tagDefinition.getDisplay());
409+
410+
// when we search the database for existing tags, return a duplicate one.
411+
when(myCacheTagDefinitionDao.getTagOrNull(any(), any(), any(), any(), any(), any(), any())).thenReturn(duplicateTagDefinition);
412+
413+
boolean updated = mySpiedSvc.updateTags(transactionDetails, sysRequest, patient, entity);
414+
415+
// assert that the entity was not updated with the duplicate tag
416+
assertThat(updated).isFalse();
417+
Collection<ResourceTag> entityTags = entity.getTags();
418+
assertThat(entityTags).hasSize(1);
419+
assertThat(entityTags.iterator().next().getTag().getId()).isEqualTo(tagDefinition.getId());
420+
}
421+
422+
private TagDefinition createTagDefinition(int theId) {
423+
TagDefinition tagDefinition = new TagDefinition(TagTypeEnum.TAG, "sytem", "code", null);
424+
tagDefinition.setId((long) theId);
425+
426+
return tagDefinition;
427+
}
428+
388429
@Nested
389430
class DeleteThresholds {
390431
private static final String URL = "Patient?_lastUpdated=gt2024-01-01";

0 commit comments

Comments
 (0)