From bcdbb51fde50c5a29e458c8fac5a4f4bc221d831 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 27 Sep 2022 15:39:10 -0400 Subject: [PATCH] =?UTF-8?q?Fix=20hashCode()=20to=20reflect=20the=20equals(?= =?UTF-8?q?)=20contract=20by=20passing=20in=20getValu=E2=80=A6=20(#4086)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix hashCode() to reflect the equals() contract by passing in getValue() instead of getHashIdentity(). Fix bug with DaoSearchParamSynchronizer setting wrong count on AddRemoveCount.addToCount(). * Add hashIdentity to both equals and hashCode. Rename unit test to be more descriptive. * Add changelog and fix changelog for previous MR. * Add newline to changelog file. --- ...validation-exception-library-measure.yaml} | 2 +- ...ameter-resource-update-deletes-record.yaml | 6 ++ .../dao/index/DaoSearchParamSynchronizer.java | 3 +- .../index/DaoSearchParamSynchronizerTest.java | 77 +++++++++++++++++++ .../ResourceIndexedSearchParamNumber.java | 2 + .../ResourceIndexedSearchParamNumberTest.java | 46 +++++++++++ 6 files changed, 134 insertions(+), 2 deletions(-) rename hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/{3124-bundle-validation-exception-library-measure.yaml => 4058-bundle-validation-exception-library-measure.yaml} (97%) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4087-searchparameter-resource-update-deletes-record.yaml create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizerTest.java create mode 100644 hapi-fhir-jpaserver-model/src/test/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumberTest.java diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3124-bundle-validation-exception-library-measure.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4058-bundle-validation-exception-library-measure.yaml similarity index 97% rename from hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3124-bundle-validation-exception-library-measure.yaml rename to hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4058-bundle-validation-exception-library-measure.yaml index 58460cd3d46..34f3868e81e 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3124-bundle-validation-exception-library-measure.yaml +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4058-bundle-validation-exception-library-measure.yaml @@ -1,4 +1,4 @@ type: fix -issue: 3124 +issue: 4058 jira: SMILE-4345 title: "When enabling validation on a fhir_endpoint module, a bundle with a Measure resource referencing a Library resource or a MeasureReport resource referencing a Measure, validation will fail with an IllegalArgumentException complaining about the Library or Measure resource, respectively. The fix ensures that Library and Measure resources are handled correctly and all of validation can proceed with a report of all success and errors." diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4087-searchparameter-resource-update-deletes-record.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4087-searchparameter-resource-update-deletes-record.yaml new file mode 100644 index 00000000000..e65fdcc11d4 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4087-searchparameter-resource-update-deletes-record.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 4087 +jira: SMILE-4875 +title: "Filter by range for numeric extension does not work after update. hfj_spidx_number record deleted after PUT update on SearchParameter value for Resource" + diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizer.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizer.java index 4ade1ecb8a7..292f5feb158 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizer.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizer.java @@ -91,7 +91,8 @@ public class DaoSearchParamSynchronizer { myEntityManager.merge(next); } - theAddRemoveCount.addToAddCount(paramsToRemove.size()); + // TODO: are there any unintended consequences to fixing this bug? + theAddRemoveCount.addToAddCount(paramsToAdd.size()); theAddRemoveCount.addToRemoveCount(paramsToRemove.size()); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizerTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizerTest.java new file mode 100644 index 00000000000..672a023b65e --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/DaoSearchParamSynchronizerTest.java @@ -0,0 +1,77 @@ +package ca.uhn.fhir.jpa.dao.index; + +import ca.uhn.fhir.jpa.model.config.PartitionSettings; +import ca.uhn.fhir.jpa.model.entity.BaseResourceIndex; +import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamNumber; +import ca.uhn.fhir.jpa.model.entity.ResourceTable; +import ca.uhn.fhir.jpa.searchparam.extractor.ResourceIndexedSearchParams; +import ca.uhn.fhir.jpa.util.AddRemoveCount; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import javax.persistence.EntityManager; +import java.math.BigDecimal; +import java.util.List; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +@ExtendWith(MockitoExtension.class) +public class DaoSearchParamSynchronizerTest { + private static final String GRITTSCORE = "grittscore"; + + private static final ResourceIndexedSearchParamNumber EXISTING_SEARCH_PARAM_NUMBER = new ResourceIndexedSearchParamNumber(new PartitionSettings(), "Patient", GRITTSCORE, BigDecimal.valueOf(10)); + private static final ResourceIndexedSearchParamNumber THE_SEARCH_PARAM_NUMBER = new ResourceIndexedSearchParamNumber(new PartitionSettings(), "Patient", GRITTSCORE, BigDecimal.valueOf(12)); + + private final DaoSearchParamSynchronizer subject = new DaoSearchParamSynchronizer(); + + private ResourceIndexedSearchParams theParams; + + @Mock + private ResourceTable theEntity; + + @Mock + private ResourceTable existingEntity; + + @Mock + private EntityManager entityManager; + + private ResourceIndexedSearchParams existingParams; + + @BeforeEach + void setUp() { + when(theEntity.isParamsNumberPopulated()).thenReturn(true); + when(theEntity.getParamsNumber()).thenReturn(List.of(THE_SEARCH_PARAM_NUMBER)); + when(existingEntity.isParamsNumberPopulated()).thenReturn(true); + when(existingEntity.getParamsNumber()).thenReturn(List.of(EXISTING_SEARCH_PARAM_NUMBER)); + + theParams = new ResourceIndexedSearchParams(theEntity); + existingParams = new ResourceIndexedSearchParams(existingEntity); + + final ResourceTable resourceTable = new ResourceTable(); + resourceTable.setId(1L); + EXISTING_SEARCH_PARAM_NUMBER.setResource(resourceTable); + THE_SEARCH_PARAM_NUMBER.setResource(resourceTable); + + subject.setEntityManager(entityManager); + } + + @Test + void synchronizeSearchParamsNumberOnlyValuesDifferent() { + final AddRemoveCount addRemoveCount = subject.synchronizeSearchParamsToDatabase(theParams, theEntity, existingParams); + + assertEquals(0, addRemoveCount.getRemoveCount()); + assertEquals(1, addRemoveCount.getAddCount()); + + verify(entityManager, never()).remove(any(BaseResourceIndex.class)); + verify(entityManager, times(1)).merge(THE_SEARCH_PARAM_NUMBER); + } +} diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumber.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumber.java index 73a9bddb77d..1c5044ee793 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumber.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumber.java @@ -129,6 +129,7 @@ public class ResourceIndexedSearchParamNumber extends BaseResourceIndexedSearchP b.append(getResourceType(), obj.getResourceType()); b.append(getParamName(), obj.getParamName()); b.append(getHashIdentity(), obj.getHashIdentity()); + b.append(getValue(), obj.getValue()); b.append(isMissing(), obj.isMissing()); return b.isEquals(); } @@ -160,6 +161,7 @@ public class ResourceIndexedSearchParamNumber extends BaseResourceIndexedSearchP HashCodeBuilder b = new HashCodeBuilder(); b.append(getResourceType()); b.append(getParamName()); + b.append(getHashIdentity()); b.append(getValue()); b.append(isMissing()); return b.toHashCode(); diff --git a/hapi-fhir-jpaserver-model/src/test/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumberTest.java b/hapi-fhir-jpaserver-model/src/test/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumberTest.java new file mode 100644 index 00000000000..a099f9ebe58 --- /dev/null +++ b/hapi-fhir-jpaserver-model/src/test/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamNumberTest.java @@ -0,0 +1,46 @@ +package ca.uhn.fhir.jpa.model.entity; + +import ca.uhn.fhir.jpa.model.config.PartitionSettings; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.math.BigDecimal; + +import static org.junit.jupiter.api.Assertions.*; + +public class ResourceIndexedSearchParamNumberTest { + private static final String GRITTSCORE = "grittscore"; + + public static final ResourceIndexedSearchParamNumber PARAM_VALUE_10_FIRST = new ResourceIndexedSearchParamNumber(new PartitionSettings(), "Patient", GRITTSCORE, BigDecimal.valueOf(10)); + public static final ResourceIndexedSearchParamNumber PARAM_VALUE_10_SECOND = new ResourceIndexedSearchParamNumber(new PartitionSettings(), "Patient", GRITTSCORE, BigDecimal.valueOf(10)); + public static final ResourceIndexedSearchParamNumber PARAM_VALUE_12_FIRST = new ResourceIndexedSearchParamNumber(new PartitionSettings(), "Patient", GRITTSCORE, BigDecimal.valueOf(12)); + + @BeforeEach + void setUp() { + final ResourceTable resourceTable = new ResourceTable(); + resourceTable.setId(1L); + PARAM_VALUE_10_FIRST.setResource(resourceTable); + PARAM_VALUE_10_SECOND.setResource(resourceTable); + PARAM_VALUE_12_FIRST.setResource(resourceTable); + } + + @Test + void notEqual() { + assertNotEquals(PARAM_VALUE_10_FIRST, PARAM_VALUE_12_FIRST); + assertNotEquals(PARAM_VALUE_12_FIRST, PARAM_VALUE_10_FIRST); + assertNotEquals(PARAM_VALUE_10_FIRST.hashCode(), PARAM_VALUE_12_FIRST.hashCode()); + } + + @Test + void equalByReference() { + assertEquals(PARAM_VALUE_10_FIRST, PARAM_VALUE_10_FIRST); + assertEquals(PARAM_VALUE_10_FIRST.hashCode(), PARAM_VALUE_10_FIRST.hashCode()); + } + + @Test + void equalByContract() { + assertEquals(PARAM_VALUE_10_FIRST, PARAM_VALUE_10_SECOND); + assertEquals(PARAM_VALUE_10_SECOND, PARAM_VALUE_10_FIRST); + assertEquals(PARAM_VALUE_10_FIRST.hashCode(), PARAM_VALUE_10_SECOND.hashCode()); + } +}