From 42142299451076d425b50ff24e8ea702c75e2bb8 Mon Sep 17 00:00:00 2001 From: Aaron Clark <94654893+ac183@users.noreply.github.com> Date: Thu, 25 Nov 2021 23:02:37 +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 | 355 +++++++++--------- .../internal/checkers/FieldsChecker.java | 7 + .../fieldchecks/BigDecimalFieldCheck.java | 87 +++++ .../extended_contract/BigDecimalTest.java | 151 ++++++++ 6 files changed, 461 insertions(+), 170 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..e33e15112 100644 --- a/src/main/java/nl/jqno/equalsverifier/Warning.java +++ b/src/main/java/nl/jqno/equalsverifier/Warning.java @@ -1,170 +1,185 @@ -package nl.jqno.equalsverifier; - -/** - * Enum of warnings that can be suppressed in {@link nl.jqno.equalsverifier.EqualsVerifier}. - * - * @see nl.jqno.equalsverifier.api.EqualsVerifierApi#suppress(Warning...) - */ -public enum Warning { - /** - * Signals that not all fields are relevant in the {@code equals} contract. {@code - * EqualsVerifier} will not fail if one or more fields do not affect the outcome of {@code - * equals}. - * - *
Only applies to non-transient fields. - */ - ALL_FIELDS_SHOULD_BE_USED, - - /** - * Signals that non-final fields are not relevant in the {@code equals} contract. {@code - * EqualsVerifier} will not fail if one or more non-final fields do not affect the outcome of - * {@code equals}. - * - *
Only applies to non-transient fields. - */ - ALL_NONFINAL_FIELDS_SHOULD_BE_USED, - - /** - * Disables the check for reference equality on fields. - * - *
EqualsVerifier will check if the {@code equals} method calls equals on the object fields - * of the class under test, instead of the {@code ==} operator, since normally this signifies a - * mistake (e.g. comparing string fields with {@code ==}). - * - *
However, sometimes {@code ==} is used intentionally, or the field in question doesn't - * implement {@code equals} itself, so a call to the {@code equals} method of that field is - * essentially a reference equality check instead of a value equality check. In these cases, - * this warning can be suppressed. - */ - REFERENCE_EQUALITY, - - /** - * Disables the check, when the {@code equals} method is overridden in the class under test, - * that an instance of this class should be equal to an identical copy of itself. - * - *
Normally, it is important that an object be equal to an identical copy of itself: after - * all, this is the point of overriding {@code equals} in the first place. - * - *
However, when the class is part of a hierarchy, but should be (pseudo-)singleton, it can - * be useful to suppress this warning. This can happen in certain implementations of the Null - * Object Pattern, for example. - * - *
If this warning is suppressed, and it turns out that an instance of the class under test - * is equal to an identical copy of itself after all, {@link EqualsVerifier} will fail. - */ - IDENTICAL_COPY, - - /** - * Disables the check, when the {@code equals} method is overridden in the class under test, - * that an instance of this class should be equal to an identical copy of itself. - * - *
Normally, it is important that an object be equal to an identical copy of itself: after - * all, this is the point of overriding {@code equals} in the first place. - * - *
However, when the class is a kind of versioned entity and there is an {@code id} field - * that is zero when the object is new, it is often the case that two new objects are never - * equal to each other. In these cases, it can be useful to suppress this warning. - * - *
You cannot use {@link #IDENTICAL_COPY} in these cases, because when the {@code id}s are - * equal, the objects should be, too, and {@link EqualsVerifier} would fail in this case. - * - *
If this warning is suppressed, and it turns out that an instance of the class under test - * is equal to an identical copy of itself after all, {@link EqualsVerifier} will NOT fail. - */ - IDENTICAL_COPY_FOR_VERSIONED_ENTITY, - - /** - * Disables the check that verifies {@code equals} is actually overridden. - * - *
Can be used when a whole package of classes is automatically scanned and presented to - * EqualsVerifier, and one or more of them don't need to override {@code equals}. - */ - INHERITED_DIRECTLY_FROM_OBJECT, - - /** - * Disables the example check for cached {@code hashCode}. - * - *
The example check verifies that the cached {@code hashCode} is properly initialized. You - * can use this, if creating an example object is too cumbersome. In this case, null can be - * passed as an example. - * - *
Note that suppressing this warning can be dangerous and should only be done in unusual - * circumstances. - */ - NO_EXAMPLE_FOR_CACHED_HASHCODE, - - /** - * Disables checks for non-final fields on which {@code equals} and {@code hashCode} depend. - * - *
{@link EqualsVerifier}'s standard behaviour is to disallow non-final fields being used in - * {@code equals} and {@code hashCode} methods, since classes that depend on non-final fields in - * these methods cannot reliably be used in collections. - * - *
However, sometimes an external library requires that fields be non-final. An example of - * this are Java Beans. In such a case, suppress this warning to prevent {@link EqualsVerifier} - * from checking for non-final fields. - */ - NONFINAL_FIELDS, - - /** - * Disables checks for {@link NullPointerException} within {@code equals}, {@code hashCode} and - * {@code toString} methods. - * - *
Sometimes the constructor of a class makes sure no field can be null. If this is the case, - * and if the fields cannot be made null later in the lifecycle of the class by setters or other - * methods, suppress this warning to disable the check for {@link NullPointerException}. - */ - NULL_FIELDS, - - /** - * Disables the check that all fields used in {@code equals} must also be used in {@code - * hashCode}. - * - *
This is useful when bringing legacy systems under test, where you don't want to change the - * existing {@code hashCode} behaviour but you do want to use EqualsVerifier. - * - *
Note that {@code hashCode}s with higher distributions give better performance when used in - * collections such as {@link java.util.HashMap}. Therefore, if possible, you should use all - * fields that are used in {@code equals}, in {@code hashCode} as well. - */ - STRICT_HASHCODE, - - /** - * Disables some of the stricter inheritance tests. - * - *
{@link EqualsVerifier}'s standard behaviour, if T is not final and neither are its {@code - * equals} and {@code hashCode} methods, is to require a reference to a subclass of T for which - * no instance can be equal to any instance of T, to make sure that subclasses that can redefine - * {@code equals} or {@code hashCode} don't break the contract; or it asks to call the {@code - * usingGetClass} method if T uses {@code getClass()} instead of {@code instanceof} in its - * {@code equals} method. - * - *
Some may find that too strict for their liking; suppressing this warning disables that - * test. - * - * @see nl.jqno.equalsverifier.api.SingleTypeEqualsVerifierApi#withRedefinedSubclass(Class) - */ - STRICT_INHERITANCE, - - /** - * Disables the check that fields marked with the @Id or @EmbeddedId annotations in JPA entities - * may not be used in the {@code equals} contract. - * - *
When this warning is suppressed, the fields marked with @Id or @EmbeddedId will become the - * entity's surrogate key. Only these fields can now be part of the {@code equals} contract; all - * other fields may no longer be used in {@code equals}. - */ - SURROGATE_KEY, - - /** - * Disables the check that transient fields not be part of the {@code equals} contract. - * - *
{@link EqualsVerifier}'s standard behaviour is to disallow transient fields being used in - * {@code equals} and {@code hashCode} methods, since these fields may not be restored to their - * original state after deserialization, which would break {@code equals}. - * - *
If measures are taken that this will never happen, this warning can be suppressed to - * disable {@link EqualsVerifier}'s transience test. - */ - TRANSIENT_FIELDS -} +package nl.jqno.equalsverifier; + +/** + * Enum of warnings that can be suppressed in {@link nl.jqno.equalsverifier.EqualsVerifier}. + * + * @see nl.jqno.equalsverifier.api.EqualsVerifierApi#suppress(Warning...) + */ +public enum Warning { + /** + * Signals that not all fields are relevant in the {@code equals} contract. {@code + * EqualsVerifier} will not fail if one or more fields do not affect the outcome of {@code + * equals}. + * + *
Only applies to non-transient fields. + */ + ALL_FIELDS_SHOULD_BE_USED, + + /** + * Signals that non-final fields are not relevant in the {@code equals} contract. {@code + * EqualsVerifier} will not fail if one or more non-final fields do not affect the outcome of + * {@code equals}. + * + *
Only applies to non-transient fields. + */ + ALL_NONFINAL_FIELDS_SHOULD_BE_USED, + + /** + * Disables the check for reference equality on fields. + * + *
EqualsVerifier will check if the {@code equals} method calls equals on the object fields + * of the class under test, instead of the {@code ==} operator, since normally this signifies a + * mistake (e.g. comparing string fields with {@code ==}). + * + *
However, sometimes {@code ==} is used intentionally, or the field in question doesn't + * implement {@code equals} itself, so a call to the {@code equals} method of that field is + * essentially a reference equality check instead of a value equality check. In these cases, + * this warning can be suppressed. + */ + REFERENCE_EQUALITY, + + /** + * Disables the check, when the {@code equals} method is overridden in the class under test, + * that an instance of this class should be equal to an identical copy of itself. + * + *
Normally, it is important that an object be equal to an identical copy of itself: after + * all, this is the point of overriding {@code equals} in the first place. + * + *
However, when the class is part of a hierarchy, but should be (pseudo-)singleton, it can + * be useful to suppress this warning. This can happen in certain implementations of the Null + * Object Pattern, for example. + * + *
If this warning is suppressed, and it turns out that an instance of the class under test + * is equal to an identical copy of itself after all, {@link EqualsVerifier} will fail. + */ + IDENTICAL_COPY, + + /** + * Disables the check, when the {@code equals} method is overridden in the class under test, + * that an instance of this class should be equal to an identical copy of itself. + * + *
Normally, it is important that an object be equal to an identical copy of itself: after + * all, this is the point of overriding {@code equals} in the first place. + * + *
However, when the class is a kind of versioned entity and there is an {@code id} field + * that is zero when the object is new, it is often the case that two new objects are never + * equal to each other. In these cases, it can be useful to suppress this warning. + * + *
You cannot use {@link #IDENTICAL_COPY} in these cases, because when the {@code id}s are + * equal, the objects should be, too, and {@link EqualsVerifier} would fail in this case. + * + *
If this warning is suppressed, and it turns out that an instance of the class under test + * is equal to an identical copy of itself after all, {@link EqualsVerifier} will NOT fail. + */ + IDENTICAL_COPY_FOR_VERSIONED_ENTITY, + + /** + * Disables the check that verifies {@code equals} is actually overridden. + * + *
Can be used when a whole package of classes is automatically scanned and presented to + * EqualsVerifier, and one or more of them don't need to override {@code equals}. + */ + INHERITED_DIRECTLY_FROM_OBJECT, + + /** + * Disables the example check for cached {@code hashCode}. + * + *
The example check verifies that the cached {@code hashCode} is properly initialized. You + * can use this, if creating an example object is too cumbersome. In this case, null can be + * passed as an example. + * + *
Note that suppressing this warning can be dangerous and should only be done in unusual + * circumstances. + */ + NO_EXAMPLE_FOR_CACHED_HASHCODE, + + /** + * Disables checks for non-final fields on which {@code equals} and {@code hashCode} depend. + * + *
{@link EqualsVerifier}'s standard behaviour is to disallow non-final fields being used in + * {@code equals} and {@code hashCode} methods, since classes that depend on non-final fields in + * these methods cannot reliably be used in collections. + * + *
However, sometimes an external library requires that fields be non-final. An example of + * this are Java Beans. In such a case, suppress this warning to prevent {@link EqualsVerifier} + * from checking for non-final fields. + */ + NONFINAL_FIELDS, + + /** + * Disables checks for {@link NullPointerException} within {@code equals}, {@code hashCode} and + * {@code toString} methods. + * + *
Sometimes the constructor of a class makes sure no field can be null. If this is the case, + * and if the fields cannot be made null later in the lifecycle of the class by setters or other + * methods, suppress this warning to disable the check for {@link NullPointerException}. + */ + NULL_FIELDS, + + /** + * Disables the check that all fields used in {@code equals} must also be used in {@code + * hashCode}. + * + *
This is useful when bringing legacy systems under test, where you don't want to change the + * existing {@code hashCode} behaviour but you do want to use EqualsVerifier. + * + *
Note that {@code hashCode}s with higher distributions give better performance when used in + * collections such as {@link java.util.HashMap}. Therefore, if possible, you should use all + * fields that are used in {@code equals}, in {@code hashCode} as well. + */ + STRICT_HASHCODE, + + /** + * Disables some of the stricter inheritance tests. + * + *
{@link EqualsVerifier}'s standard behaviour, if T is not final and neither are its {@code + * equals} and {@code hashCode} methods, is to require a reference to a subclass of T for which + * no instance can be equal to any instance of T, to make sure that subclasses that can redefine + * {@code equals} or {@code hashCode} don't break the contract; or it asks to call the {@code + * usingGetClass} method if T uses {@code getClass()} instead of {@code instanceof} in its + * {@code equals} method. + * + *
Some may find that too strict for their liking; suppressing this warning disables that + * test. + * + * @see nl.jqno.equalsverifier.api.SingleTypeEqualsVerifierApi#withRedefinedSubclass(Class) + */ + STRICT_INHERITANCE, + + /** + * Disables the check that fields marked with the @Id or @EmbeddedId annotations in JPA entities + * may not be used in the {@code equals} contract. + * + *
When this warning is suppressed, the fields marked with @Id or @EmbeddedId will become the + * entity's surrogate key. Only these fields can now be part of the {@code equals} contract; all + * other fields may no longer be used in {@code equals}. + */ + SURROGATE_KEY, + + /** + * Disables the check that transient fields not be part of the {@code equals} contract. + * + *
{@link EqualsVerifier}'s standard behaviour is to disallow transient fields being used in + * {@code equals} and {@code hashCode} methods, since these fields may not be restored to their + * original state after deserialization, which would break {@code equals}. + * + *
If measures are taken that this will never happen, this warning can be suppressed to + * disable {@link EqualsVerifier}'s transience test. + */ + 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..dd06a8a1a 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