From 0bf387b7ff8bfe6a5d0cea09c3ec6a9f93f687b7 Mon Sep 17 00:00:00 2001 From: Aaron Clark <94654893+ac183@users.noreply.github.com> Date: Thu, 25 Nov 2021 23:23:28 +0000 Subject: [PATCH] #540 BigDecimal equality using compareTo --- docs/_manual/12-bigdecimal-equality.md | 30 ++++ ...legacy-systems.md => 13-legacy-systems.md} | 1 + .../java/nl/jqno/equalsverifier/Warning.java | 17 +- .../internal/checkers/FieldsChecker.java | 31 ++-- .../fieldchecks/BigDecimalFieldCheck.java | 87 ++++++++++ .../extended_contract/BigDecimalTest.java | 151 ++++++++++++++++++ 6 files changed, 304 insertions(+), 13 deletions(-) create mode 100644 docs/_manual/12-bigdecimal-equality.md rename docs/_manual/{12-legacy-systems.md => 13-legacy-systems.md} (91%) create mode 100644 src/main/java/nl/jqno/equalsverifier/internal/checkers/fieldchecks/BigDecimalFieldCheck.java create mode 100644 src/test/java/nl/jqno/equalsverifier/integration/extended_contract/BigDecimalTest.java diff --git a/docs/_manual/12-bigdecimal-equality.md b/docs/_manual/12-bigdecimal-equality.md new file mode 100644 index 000000000..46cb4fc2a --- /dev/null +++ b/docs/_manual/12-bigdecimal-equality.md @@ -0,0 +1,30 @@ +--- +title: "`BigDecimal` equality using `compareTo(BigDecimal val)`" +permalink: /manual/bigdecimal-equality/ +--- +The `Comparable` interface strongly recommends but does not require that implementations consider two objects equal using `compareTo` whenever they are equal using `equals` and vice versa. `BigDecimal` is a class where this is not applied. + +{% highlight java %} +BigDecimal zero = new BigDecimal("0"); +BigDecimal alsoZero = new BigDecimal("0.0"); + +// prints true - zero is the same as zero +System.out.println(zero.compareTo(alsoZero) == 0); +// prints false - zero is not the same as zero +System.out.println(zero.equals(alsoZero)); +{% endhighlight %} + +This is because `BigDecimal` can have multiple representations of the same value. It uses an unscaled value and a scale so, for example, the value of 1 can be represented as unscaled value 1 with scale of 0 (the number of places after the decimal point) or as unscaled value 10 with scale of 1 resolving to 1.0. Its `equals` and `hashCode` methods use both of these attributes in their calculation rather than the resolved value. + +If your class contains any `BigDecimal` fields, in order for comparably equal values to be considered the same, then the `equals` method must use `compareTo` for the check and the `hashCode` calculation must derive the same value for all `BigDecimal` instances that are equal using `compareTo` (taking care if it is a nullable field). + +EqualsVerifier will check this by default. If you do not have this requirement the check can be disabled by suppressing `Warning.BIGDECIMAL_EQUALITY`. + +{% highlight java %} +EqualsVerifier.forClass(FooWithComparablyEqualBigDecimalFields.class) + .suppress(Warning.BIGDECIMAL_EQUALITY) + .verify(); +{% endhighlight %} + +There is more information on `compareTo` and `equals` in the `Comparable` Javadoc and Effective Java's chapter on implementing `Comparable`. +There is more information on `BigDecimal` in its Javadoc (and its representation can be seen by printing `unscaledValue()` and `scale()`). diff --git a/docs/_manual/12-legacy-systems.md b/docs/_manual/13-legacy-systems.md similarity index 91% rename from docs/_manual/12-legacy-systems.md rename to docs/_manual/13-legacy-systems.md index b4c6285fd..ac48aa02d 100644 --- a/docs/_manual/12-legacy-systems.md +++ b/docs/_manual/13-legacy-systems.md @@ -17,6 +17,7 @@ For these situations, there are several types of warnings that you can suppress: * `Warning.STRICT_HASHCODE`: disables the check that all fields used in `equals` must also be used in `hashCode`. * `Warning.STRICT_INHERITANCE`: disables the check that classes or their `equals` methods be final, or that inheritance is properly accounted for. Read more about this topic on the [page about inheritance](/equalsverifier/manual/inheritance). * `Warning.TRANSIENT_FIELDS`: disables the check that transient fields do not participate in `equals`. This applies both to Java's `transient` keyword, which applies to serialization, and to JPA's `@Transient` annotation, which applies to, well, JPA. +* `Warning.BIGDECIMAL_EQUALITY`: disables the check that equality of `BigDecimal` fields is implemented using `compareTo` rather than `equals`. Read more about this topic on the [page about BigDecimal equality](/equalsverifier/manual/bigdecimal-equality). Of course, once you have sufficient test coverage, you _will_ come back and fix these issues, right? 😉 diff --git a/src/main/java/nl/jqno/equalsverifier/Warning.java b/src/main/java/nl/jqno/equalsverifier/Warning.java index 387695d0e..48fd2a1fc 100644 --- a/src/main/java/nl/jqno/equalsverifier/Warning.java +++ b/src/main/java/nl/jqno/equalsverifier/Warning.java @@ -166,5 +166,20 @@ public enum Warning { *

If measures are taken that this will never happen, this warning can be suppressed to * disable {@link EqualsVerifier}'s transience test. */ - TRANSIENT_FIELDS + TRANSIENT_FIELDS, + + /** + * Disables the check that equality of {@code BigDecimal} fields is implemented using + * {@code compareTo} rather than {@code equals}. + * + *

{@code BigDecimal} objects that are equal using {@code compareTo} are not necessarily + * equal using {@code equals}, for example the values of {@literal 1} and {@literal 1.0}. + * For variants of the same value to be considered equal, classes with {@code BigDecimal} + * fields should use {@code compareTo} to check equality of non-null {@code BigDecimal} + * references and produce the same hashcode for all equal variants. + * + *

{@code EqualsVerifier} checks for this by default but it can be disabled by suppressing + * this warning. + */ + BIGDECIMAL_EQUALITY } diff --git a/src/main/java/nl/jqno/equalsverifier/internal/checkers/FieldsChecker.java b/src/main/java/nl/jqno/equalsverifier/internal/checkers/FieldsChecker.java index b6a89d7f1..0b2be67e1 100644 --- a/src/main/java/nl/jqno/equalsverifier/internal/checkers/FieldsChecker.java +++ b/src/main/java/nl/jqno/equalsverifier/internal/checkers/FieldsChecker.java @@ -23,6 +23,7 @@ public class FieldsChecker implements Checker { private final SymmetryFieldCheck symmetryFieldCheck; private final TransientFieldsCheck transientFieldsCheck; private final TransitivityFieldCheck transitivityFieldCheck; + private final BigDecimalFieldCheck bigDecimalFieldCheck; public FieldsChecker(Configuration config) { this.config = config; @@ -31,23 +32,25 @@ public FieldsChecker(Configuration config) { TypeTag typeTag = config.getTypeTag(); String cachedHashCodeFieldName = config - .getCachedHashCodeInitializer() - .getCachedHashCodeFieldName(); + .getCachedHashCodeInitializer() + .getCachedHashCodeFieldName(); Predicate isCachedHashCodeField = a -> - a.getFieldName().equals(cachedHashCodeFieldName); + a.getFieldName().equals(cachedHashCodeFieldName); this.arrayFieldCheck = new ArrayFieldCheck<>(config.getCachedHashCodeInitializer()); this.floatAndDoubleFieldCheck = new FloatAndDoubleFieldCheck<>(); this.mutableStateFieldCheck = - new MutableStateFieldCheck<>(prefabValues, typeTag, isCachedHashCodeField); + new MutableStateFieldCheck<>(prefabValues, typeTag, isCachedHashCodeField); this.reflexivityFieldCheck = new ReflexivityFieldCheck<>(config); this.significantFieldCheck = - new SignificantFieldCheck<>(config, isCachedHashCodeField, false); + new SignificantFieldCheck<>(config, isCachedHashCodeField, false); this.skippingSignificantFieldCheck = - new SignificantFieldCheck<>(config, isCachedHashCodeField, true); + new SignificantFieldCheck<>(config, isCachedHashCodeField, true); this.symmetryFieldCheck = new SymmetryFieldCheck<>(prefabValues, typeTag); this.transientFieldsCheck = new TransientFieldsCheck<>(config); this.transitivityFieldCheck = new TransitivityFieldCheck<>(prefabValues, typeTag); + this.bigDecimalFieldCheck = + new BigDecimalFieldCheck<>(config.getCachedHashCodeInitializer()); } @Override @@ -75,19 +78,23 @@ public void check() { if (!config.getWarningsToSuppress().contains(Warning.NULL_FIELDS)) { inspector.checkWithNull( - config.getNonnullFields(), - config.getAnnotationCache(), - skippingSignificantFieldCheck + config.getNonnullFields(), + config.getAnnotationCache(), + skippingSignificantFieldCheck ); } + + if (!config.getWarningsToSuppress().contains(Warning.BIGDECIMAL_EQUALITY)) { + inspector.check(bigDecimalFieldCheck); + } } private boolean ignoreMutability(Class type) { AnnotationCache cache = config.getAnnotationCache(); return ( - config.getWarningsToSuppress().contains(Warning.NONFINAL_FIELDS) || - cache.hasClassAnnotation(type, SupportedAnnotations.IMMUTABLE) || - cache.hasClassAnnotation(type, SupportedAnnotations.ENTITY) + config.getWarningsToSuppress().contains(Warning.NONFINAL_FIELDS) || + cache.hasClassAnnotation(type, SupportedAnnotations.IMMUTABLE) || + cache.hasClassAnnotation(type, SupportedAnnotations.ENTITY) ); } } diff --git a/src/main/java/nl/jqno/equalsverifier/internal/checkers/fieldchecks/BigDecimalFieldCheck.java b/src/main/java/nl/jqno/equalsverifier/internal/checkers/fieldchecks/BigDecimalFieldCheck.java new file mode 100644 index 000000000..008696188 --- /dev/null +++ b/src/main/java/nl/jqno/equalsverifier/internal/checkers/fieldchecks/BigDecimalFieldCheck.java @@ -0,0 +1,87 @@ +package nl.jqno.equalsverifier.internal.checkers.fieldchecks; + +import static nl.jqno.equalsverifier.internal.util.Assert.assertEquals; + +import java.lang.reflect.Field; +import java.math.BigDecimal; +import java.math.RoundingMode; +import nl.jqno.equalsverifier.Warning; +import nl.jqno.equalsverifier.internal.reflection.FieldAccessor; +import nl.jqno.equalsverifier.internal.reflection.ObjectAccessor; +import nl.jqno.equalsverifier.internal.util.CachedHashCodeInitializer; +import nl.jqno.equalsverifier.internal.util.Formatter; + +public class BigDecimalFieldCheck implements FieldCheck { + + private final CachedHashCodeInitializer cachedHashCodeInitializer; + + public BigDecimalFieldCheck(CachedHashCodeInitializer cachedHashCodeInitializer) { + this.cachedHashCodeInitializer = cachedHashCodeInitializer; + } + + @Override + public void execute( + ObjectAccessor referenceAccessor, + ObjectAccessor copyAccessor, + FieldAccessor fieldAccessor + ) { + if (BigDecimal.class.equals(fieldAccessor.getFieldType())) { + Field field = fieldAccessor.getField(); + BigDecimal referenceField = (BigDecimal) referenceAccessor.getField(field); + BigDecimal changedField = referenceField.setScale( + referenceField.scale() + 1, + RoundingMode.UNNECESSARY + ); + ObjectAccessor changed = copyAccessor.withFieldSetTo(field, changedField); + + T left = referenceAccessor.get(); + T right = changed.get(); + + checkEquals(field, referenceField, changedField, left, right); + checkHashCode(field, referenceField, changedField, left, right); + } + } + + private void checkEquals( + Field field, + BigDecimal referenceField, + BigDecimal changedField, + T left, + T right + ) { + Formatter f = Formatter.of( + "BigDecimal equality by comparison: object does not equal a copy of itself where BigDecimal field %%" + + " has a value that is equal using compareTo: %% compared to %%" + + "\nIf these values should be considered equal then use compareTo rather than equals for this field." + + "\nIf these values should not be considered equal, suppress Warning.%% to disable this check.", + field.getName(), + referenceField, + changedField, + Warning.BIGDECIMAL_EQUALITY + ); + assertEquals(f, left, right); + } + + private void checkHashCode( + Field field, + BigDecimal referenceField, + BigDecimal changedField, + T left, + T right + ) { + Formatter f = Formatter.of( + "BigDecimal equality by comparison: hashCode of object does not equal hashCode of a copy of itself" + + " where BigDecimal field %%" + + " has a value that is equal using compareTo: %% compared to %%" + + "\nIf these values should be considered equal then make sure to derive the same constituent hashCode from this field." + + "\nIf these values should not be considered equal, suppress Warning.%% to disable this check.", + field.getName(), + referenceField, + changedField, + Warning.BIGDECIMAL_EQUALITY + ); + int leftHashCode = cachedHashCodeInitializer.getInitializedHashCode(left); + int rightHashCode = cachedHashCodeInitializer.getInitializedHashCode(right); + assertEquals(f, leftHashCode, rightHashCode); + } +} diff --git a/src/test/java/nl/jqno/equalsverifier/integration/extended_contract/BigDecimalTest.java b/src/test/java/nl/jqno/equalsverifier/integration/extended_contract/BigDecimalTest.java new file mode 100644 index 000000000..06b3a219d --- /dev/null +++ b/src/test/java/nl/jqno/equalsverifier/integration/extended_contract/BigDecimalTest.java @@ -0,0 +1,151 @@ +package nl.jqno.equalsverifier.integration.extended_contract; + +import static nl.jqno.equalsverifier.testhelpers.Util.defaultHashCode; + +import java.math.BigDecimal; +import java.util.Objects; +import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; +import nl.jqno.equalsverifier.testhelpers.ExpectedException; +import org.junit.jupiter.api.Test; + +public class BigDecimalTest { + + @Test + public void fail_whenBigDecimalsComparedUsingEquals() { + ExpectedException + .when(() -> EqualsVerifier.forClass(BigDecimalEquals.class).verify()) + .assertFailure() + .assertMessageContains( + "BigDecimal", + "equals", + "compareTo", + "bd", + Warning.BIGDECIMAL_EQUALITY.toString() + ); + } + + @Test + public void succeed_whenBigDecimalsComparedUsingEquals_givenBigDecimalEqualsWarningIsSuppressed() { + EqualsVerifier + .forClass(BigDecimalEquals.class) + .suppress(Warning.BIGDECIMAL_EQUALITY) + .verify(); + } + + @Test + public void succeed_whenBigDecimalsComparedUsingCompareTo() { + EqualsVerifier.forClass(BigDecimalCompareTo.class).verify(); + } + + @Test + public void fail_whenBigDecimalsComparedUsingCompareToWithInconsistentHashCode() { + ExpectedException + .when(() -> EqualsVerifier.forClass(BigDecimalInconsistentHashCode.class).verify()) + .assertFailure() + .assertMessageContains( + "BigDecimal", + "hashCode", + "compareTo", + "bd", + Warning.BIGDECIMAL_EQUALITY.toString() + ); + } + + /** + * Uses standard equals and hashCode for objects. + * 0 and 0.0 are not equal. + */ + private static final class BigDecimalEquals { + + private final BigDecimal bd; + + public BigDecimalEquals(BigDecimal bd) { + this.bd = bd; + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof BigDecimalEquals)) { + return false; + } + BigDecimalEquals other = (BigDecimalEquals) obj; + return Objects.equals(bd, other.bd); + } + + @Override + public int hashCode() { + return defaultHashCode(this); + } + } + + /** + * Uses compareTo for BigDecimal equality and ensures hashCode is equal for equal BigDecimal instances. + * 0 and 0.0 are equal and produce the same hashCode. + */ + private static final class BigDecimalCompareTo { + + private final BigDecimal bd; + + public BigDecimalCompareTo(BigDecimal bd) { + this.bd = bd; + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof BigDecimalCompareTo)) { + return false; + } + BigDecimalCompareTo other = (BigDecimalCompareTo) obj; + return comparablyEquals(bd, other.bd); + } + + @Override + public int hashCode() { + return Objects.hashCode(comparablyHashed(bd)); + } + } + + /** + * Uses compareTo for BigDecimal equality but has hashCode that may be inconsistent. + * 0 and 0.0 are equal but may produce a different hashCode. + */ + private static final class BigDecimalInconsistentHashCode { + + private final BigDecimal bd; + + public BigDecimalInconsistentHashCode(BigDecimal bd) { + this.bd = bd; + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof BigDecimalInconsistentHashCode)) { + return false; + } + BigDecimalInconsistentHashCode other = (BigDecimalInconsistentHashCode) obj; + return comparablyEquals(bd, other.bd); + } + + @Override + public int hashCode() { + return defaultHashCode(this); + } + } + + /** + * Checks equality using compareTo rather than equals. + */ + private static boolean comparablyEquals(BigDecimal left, BigDecimal right) { + boolean bothNull = left == null && right == null; + boolean bothNonNullAndEqual = left != null && right != null && left.compareTo(right) == 0; + return bothNull || bothNonNullAndEqual; + } + + /** + * Returns a instance (or null) that produces the same hashCode as any other instance that is equal using compareTo. + */ + private static BigDecimal comparablyHashed(BigDecimal bd) { + return bd != null ? bd.stripTrailingZeros() : null; + } +}