Adds support to `BigDecimal` in `JsonPrimitive#equals` #2364

MaicolAntali · 2023-04-01T16:10:29Z

Purpose

Fixes #904

Description

Currently a BigDeciamal is cast to a double and this can give issue if the number doesn't fit in it.

double a = getAsNumber().doubleValue();
double b = other.getAsNumber().doubleValue();
return a == b || (Double.isNaN(a) && Double.isNaN(b));

I have implemented a ternary operator to check if a number is a BigDecimal and in that case use the proper equals method.

Checklist

New code follows the Google Java Style Guide
If necessary, new public API validates arguments, for example rejects null
New public API has Javadoc
- Javadoc uses @since $next-version$
  ( $next-version$ is a special placeholder which is automatically replaced during release)
If necessary, new unit tests have been added
- Assertions in unit tests use Truth, see existing tests
- No JUnit 3 features are used (such as extending class TestCase)
- If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
mvn clean verify javadoc:jar passes without errors

Adds to the JsonPrimitive#equals the possibility to support BigDecimal

Adds test to check if the equals work with BigDecimals. Code snippet from issue google#904

MaicolAntali · 2023-04-01T16:20:50Z

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

+  public void testBigDecimalEquals() {
+    JsonPrimitive small = new JsonPrimitive(1.0);
+    JsonPrimitive large = new JsonPrimitive(2.0);
+    assertThat(small.equals(large)).isFalse();


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

gson/src/main/java/com/google/gson/JsonPrimitive.java

Replaces the `.equals` method with the `compareTo` in the `JsonPrimitive#equals` Change the ternary operator from `||` to `&&` so we are sure that both are `BigDecimal` Implements tests

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

Co-authored-by: Marcono1234 <[email protected]>

Marcono1234

@MaicolAntali, could you please merge main into your branch and fix the new Error Prone warning:

[WARNING] /gson/src/main/java/com/google/gson/JsonPrimitive.java:[294,47] [OperatorPrecedence] Use grouping parenthesis to make the operator precedence explicit
    (see https://errorprone.info/bugpattern/OperatorPrecedence)
  Did you mean 'return (this.value instanceof BigDecimal && other.value instanceof BigDecimal)'?

To me these changes look reasonable, and they should solve #904. I assume there might be some other cases where comparing BigDecimal with primitive values such as double or long might still cause precision loss, but if necessary that can be addressed later.

@eamonnmcmanus, what do you think?

gson/src/main/java/com/google/gson/JsonPrimitive.java

- Extracts `thisAsDouble` & `otherAsDouble` variables to avoid double functions calls. - Adds a comment to improve the code readability.

Marcono1234 · 2023-11-18T19:21:57Z

@MaicolAntali, could you please merge main into the branch of this PR, mainly to be sure it passes the formatting checks? Sorry for the trouble.

MaicolAntali · 2023-11-18T19:24:57Z

Don't worry, help it's a pleasure!

Marcono1234 · 2023-11-26T15:26:24Z

gson/src/main/java/com/google/gson/JsonPrimitive.java

+        // Uses compareTo to ignore scale of values, e.g. `0` and `0.00` should be considered equal
+        return this.getAsBigDecimal().compareTo(other.getAsBigDecimal()) == 0;


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.

eamonnmcmanus

Thanks for doing this! I ran it against all of Google's internal tests and everything passed.

* Adds support to `BigDecimal` Adds to the JsonPrimitive#equals the possibility to support BigDecimal * Adds test Adds test to check if the equals work with BigDecimals. Code snippet from issue google#904 * Implements review comments Replaces the `.equals` method with the `compareTo` in the `JsonPrimitive#equals` Change the ternary operator from `||` to `&&` so we are sure that both are `BigDecimal` Implements tests * Changes to follow the google-style-guide * implements review comment Co-authored-by: Marcono1234 <[email protected]> * Fixes the `OperatorPrecedence` warn * Implements code improvements - Extracts `thisAsDouble` & `otherAsDouble` variables to avoid double functions calls. - Adds a comment to improve the code readability. * Implements `BigDecimal` check in the `JsonPrimitive.equals()` * Formats the code with `spotless:apply` --------- Co-authored-by: Marcono1234 <[email protected]>

MaicolAntali added 2 commits April 1, 2023 17:55

Adds support to BigDecimal

6b051a8

Adds to the JsonPrimitive#equals the possibility to support BigDecimal

Adds test

fdc3ca4

Adds test to check if the equals work with BigDecimals. Code snippet from issue google#904

MaicolAntali commented Apr 1, 2023

View reviewed changes

Marcono1234 reviewed Apr 1, 2023

View reviewed changes

gson/src/main/java/com/google/gson/JsonPrimitive.java Outdated Show resolved Hide resolved

Marcono1234 reviewed Apr 1, 2023

View reviewed changes

gson/src/main/java/com/google/gson/JsonPrimitive.java Outdated Show resolved Hide resolved

MaicolAntali added 2 commits April 2, 2023 10:30

Implements review comments

3aee514

Replaces the `.equals` method with the `compareTo` in the `JsonPrimitive#equals` Change the ternary operator from `||` to `&&` so we are sure that both are `BigDecimal` Implements tests

Changes to follow the google-style-guide

500535e

Marcono1234 reviewed Apr 2, 2023

View reviewed changes

gson/src/test/java/com/google/gson/JsonPrimitiveTest.java Outdated Show resolved Hide resolved

MaicolAntali and others added 2 commits April 2, 2023 17:59

implements review comment

e0788ec

Co-authored-by: Marcono1234 <[email protected]>

Merge branch 'master' into fix-904

3c364b9

Marcono1234 reviewed Oct 26, 2023

View reviewed changes

gson/src/main/java/com/google/gson/JsonPrimitive.java Outdated Show resolved Hide resolved

gson/src/main/java/com/google/gson/JsonPrimitive.java Outdated Show resolved Hide resolved

MaicolAntali and others added 4 commits October 26, 2023 17:59

Merge branch 'main' into fix-904

85cab67

Fixes the OperatorPrecedence warn

d67e4b8

Implements code improvements

f91c2dc

- Extracts `thisAsDouble` & `otherAsDouble` variables to avoid double functions calls. - Adds a comment to improve the code readability.

Implements BigDecimal check in the JsonPrimitive.equals()

98bec86

Merge branch 'main' into fix-904

01a9fa7

Marcono1234 mentioned this pull request Nov 18, 2023

Make Maven build reproducible #2543

Merged

9 tasks

MaicolAntali and others added 2 commits November 18, 2023 20:26

Formats the code with spotless:apply

c15fb57

Merge branch 'main' into fix-904

d59f69b

Marcono1234 reviewed Nov 26, 2023

View reviewed changes

Marcono1234 requested a review from eamonnmcmanus February 7, 2024 18:21

eamonnmcmanus approved these changes Feb 9, 2024

View reviewed changes

eamonnmcmanus merged commit db61bb0 into google:main Feb 9, 2024
10 checks passed

MaicolAntali deleted the fix-904 branch February 9, 2024 17:22

Marcono1234 mentioned this pull request Feb 9, 2024

Fix compilation warnings & add test #2614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support to `BigDecimal` in `JsonPrimitive#equals` #2364

Adds support to `BigDecimal` in `JsonPrimitive#equals` #2364

MaicolAntali commented Apr 1, 2023 •

edited

Loading

MaicolAntali Apr 1, 2023

Marcono1234 left a comment •

edited

Loading

Marcono1234 commented Nov 18, 2023

MaicolAntali commented Nov 18, 2023

Marcono1234 Nov 26, 2023 •

edited

Loading

eamonnmcmanus left a comment

	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());

		// Uses compareTo to ignore scale of values, e.g. `0` and `0.00` should be considered equal
		return this.getAsBigDecimal().compareTo(other.getAsBigDecimal()) == 0;

Adds support to BigDecimal in JsonPrimitive#equals #2364

Adds support to BigDecimal in JsonPrimitive#equals #2364

Conversation

MaicolAntali commented Apr 1, 2023 • edited Loading

Purpose

Description

Checklist

MaicolAntali Apr 1, 2023

Choose a reason for hiding this comment

Marcono1234 left a comment • edited Loading

Choose a reason for hiding this comment

Marcono1234 commented Nov 18, 2023

MaicolAntali commented Nov 18, 2023

Marcono1234 Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Adds support to `BigDecimal` in `JsonPrimitive#equals` #2364

Adds support to `BigDecimal` in `JsonPrimitive#equals` #2364

MaicolAntali commented Apr 1, 2023 •

edited

Loading

Marcono1234 left a comment •

edited

Loading

Marcono1234 Nov 26, 2023 •

edited

Loading