Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds support to BigDecimal in JsonPrimitive#equals #2364

Merged
merged 13 commits into from
Feb 9, 2024
Merged

Adds support to BigDecimal in JsonPrimitive#equals #2364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6b051a8
Adds support to `BigDecimal`
MaicolAntali Apr 1, 2023
fdc3ca4
Adds test
MaicolAntali Apr 1, 2023
3aee514
Implements review comments
MaicolAntali Apr 2, 2023
500535e
Changes to follow the google-style-guide
MaicolAntali Apr 2, 2023
e0788ec
implements review comment
MaicolAntali Apr 2, 2023
3c364b9
Merge branch 'master' into fix-904
MaicolAntali Apr 3, 2023
85cab67
Merge branch 'main' into fix-904
MaicolAntali Oct 26, 2023
d67e4b8
Fixes the `OperatorPrecedence` warn
MaicolAntali Oct 26, 2023
f91c2dc
Implements code improvements
MaicolAntali Oct 26, 2023
98bec86
Implements `BigDecimal` check in the `JsonPrimitive.equals()`
MaicolAntali Oct 29, 2023
01a9fa7
Merge branch 'main' into fix-904
MaicolAntali Nov 18, 2023
c15fb57
Formats the code with `spotless:apply`
MaicolAntali Nov 18, 2023
d59f69b
Merge branch 'main' into fix-904
MaicolAntali Nov 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions gson/src/main/java/com/google/gson/JsonPrimitive.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,11 +287,16 @@ public boolean equals(Object obj) {
: this.getAsNumber().longValue() == other.getAsNumber().longValue();
}
if (value instanceof Number && other.value instanceof Number) {
double a = getAsNumber().doubleValue();
// Java standard types other than double return true for two NaN. So, need
// special handling for double.
double b = other.getAsNumber().doubleValue();
return a == b || (Double.isNaN(a) && Double.isNaN(b));
if (value instanceof BigDecimal && other.value instanceof BigDecimal) {
// Uses compareTo to ignore scale of values, e.g. `0` and `0.00` should be considered equal
return this.getAsBigDecimal().compareTo(other.getAsBigDecimal()) == 0;
Comment on lines +291 to +292
Copy link
Collaborator

@Marcono1234 Marcono1234 Nov 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As side note / for future reference: I originally proposed using compareTo when this code was also comparing BigDecimal with non-BigDecimal, see #2364 (comment).

However, using compareTo instead of equals here has another advantage: It ensures JsonPrimitive.equals is transitive (as required by Object.equals), at least when wrapping a BigDecimal.

(x equals y and y equals z) implies x equals z

Now let

  • x = new JsonPrimitive(new BigDecimal("0"))
  • y = new JsonPrimitive(0.0d)
  • z = new JsonPrimitive(new BigDecimal("0.00"))

Using compareTo makes sure that equals is transitive. Whereas using BigDecimal.equals here would cause: x equals y and y equals z (because they fall back to double comparison), but x !equals z, which would not be transitive.

}

double thisAsDouble = this.getAsDouble();
double otherAsDouble = other.getAsDouble();
// Don't use Double.compare(double, double) because that considers -0.0 and +0.0 not equal
return (thisAsDouble == otherAsDouble)
|| (Double.isNaN(thisAsDouble) && Double.isNaN(otherAsDouble));
}
return value.equals(other.value);
}
Expand Down
31 changes: 31 additions & 0 deletions gson/src/test/java/com/google/gson/JsonPrimitiveTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,4 +321,35 @@ public void testDeepCopy() {
JsonPrimitive a = new JsonPrimitive("a");
assertThat(a).isSameInstanceAs(a.deepCopy()); // Primitives are immutable!
}

@Test
public void testBigDecimalEquals() {
JsonPrimitive small = new JsonPrimitive(1.0);
JsonPrimitive large = new JsonPrimitive(2.0);
assertThat(small.equals(large)).isFalse();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert probably is overkill because is already tested here:

gson/gson/src/test/java/com/google/gson/JsonPrimitiveTest.java

Lines 246 to 250 in 3adead6

public void testFloatEqualsBigDecimal() {
JsonPrimitive p1 = new JsonPrimitive(10.25F);
JsonPrimitive p2 = new JsonPrimitive(new BigDecimal("10.25"));
assertThat(p1).isEqualTo(p2);
assertThat(p1.hashCode()).isEqualTo(p2.hashCode());

It's here to be consistent with the code in the issue #904


BigDecimal doubleMax = BigDecimal.valueOf(Double.MAX_VALUE);
JsonPrimitive smallBD = new JsonPrimitive(doubleMax.add(new BigDecimal("100.0")));
JsonPrimitive largeBD = new JsonPrimitive(doubleMax.add(new BigDecimal("200.0")));
assertThat(smallBD.equals(largeBD)).isFalse();
}

@Test
public void testBigDecimalEqualsZero() {
assertThat(
new JsonPrimitive(new BigDecimal("0.0"))
.equals(new JsonPrimitive(new BigDecimal("0.00"))))
.isTrue();

assertThat(
new JsonPrimitive(new BigDecimal("0.00"))
.equals(new JsonPrimitive(Double.valueOf("0.00"))))
.isTrue();
}

@Test
public void testEqualsDoubleNaNAndBigDecimal() {
assertThat(new JsonPrimitive(Double.NaN).equals(new JsonPrimitive(new BigDecimal("1.0"))))
.isFalse();
}
}