[GOBBLIN-2158] upgraded gson version with unit test

[Blazer-007]

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

• [✅] My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
  • https://issues.apache.org/jira/browse/GOBBLIN-2158

Description

• [✅] Here are some details about my PR, including screenshots (if applicable):
  • Upgraded Gson version to 2.8.9

Tests

• [✅] My PR adds the following unit tests OR does not need testing for this extremely good reason:
  • testObjectToIntegerDeserialize

Commits

• [✅] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

[pratapaditya04]

nit : a better way for naming exceptions in this scenario could be ignored 2/3, instead of var 2/3.

[Will-Lo]

+1 to this

[pratapaditya04]

nit: prefer using String.format to improve readability and performance, as String concatenation using + can create multiple intermediate String objects, which impacts performance/
For ex sample replacement :
throw new JsonParseException(String.format("Cannot parse %s; at path %s", value, in.getPath()), var1);

[pratapaditya04]

Test for null/infinites are missing

[pratapaditya04]

Nice work on the custom number parsing. But this includes a lot of nesting which could imapact readibility and maintainability , I sugges tbreaking down the nested try-catch blocks in readNumber into smaller methods for each number type (e.g., tryParseInteger, tryParseLong, tryParseDouble). This will improve readability and maintainability by reducing nesting.
Sample code

`private Number parseNumber(String value, JsonReader in) throws IOException {
    Number number = tryParseInteger(value);
    if (number != null) return number;

    number = tryParseLong(value);
    if (number != null) return number;

    return tryParseDouble(value, in);
}

private Number tryParseInteger(String value) { /* ... */ }
private Number tryParseLong(String value) { /* ... */ }
private Number tryParseDouble(String value, JsonReader in) throws IOException { /* ... */ }

`

[pratapaditya04]

nit : minor suggestion on naming :CustomToNumberPolicy sounds a bit generic, can be renamed to something like GsonNumberParsingPolicy to reflect its purpose more clearly
.

[Blazer-007]

The library implementation is
public enum ToNumberPolicy implements ToNumberStrategy

Since my code is wrapper on top of this that's the reason for keeping it generic

[Will-Lo]

It can be something like LongIntNumberPolicy

[Blazer-007]

@pratapaditya04 this is the default implementation of LONG_OR_DOUBLE enum policy present in the library

LONG_OR_DOUBLE {
  @Override public Number readNumber(JsonReader in) throws IOException, JsonParseException {
    String value = in.nextString();
    try {
      return Long.parseLong(value);
    } catch (NumberFormatException longE) {
      try {
        Double d = Double.valueOf(value);
        if ((d.isInfinite() || d.isNaN()) && !in.isLenient()) {
          throw new MalformedJsonException("JSON forbids NaN and infinities: " + d + "; at path " + in.getPath());
        }
        return d;
      } catch (NumberFormatException doubleE) {
        throw new JsonParseException("Cannot parse " + value + "; at path " + in.getPath(), doubleE);
      }
    }
  }
},

I have tried to just add one more layer on top to parse Int that's why exception and naming i have kept similar to library implementation

[pratapaditya04]

@pratapaditya04 this is the default implementation of LONG_OR_DOUBLE enum policy present in the library

LONG_OR_DOUBLE {
  @Override public Number readNumber(JsonReader in) throws IOException, JsonParseException {
    String value = in.nextString();
    try {
      return Long.parseLong(value);
    } catch (NumberFormatException longE) {
      try {
        Double d = Double.valueOf(value);
        if ((d.isInfinite() || d.isNaN()) && !in.isLenient()) {
          throw new MalformedJsonException("JSON forbids NaN and infinities: " + d + "; at path " + in.getPath());
        }
        return d;
      } catch (NumberFormatException doubleE) {
        throw new JsonParseException("Cannot parse " + value + "; at path " + in.getPath(), doubleE);
      }
    }
  }
},

I have tried to just add one more layer on top to parse Int that's why exception and naming i have kept similar to library implementation

Hmmm, but still 3 layers of nesting impacts readability a lot, so breaking down into smaller functions can be a possible alternative,but leaving it upto you on how to handle it. Also naming exception variables var3, var2, doesn't seem right.

[Will-Lo]

Can we update the PR description to add the motivation of the change around custom number parsing and the gson version bump?

[Blazer-007]

Closing this PR as writing custom NumberPolicy is leading to incorrect type conversion between INT & LONG in case value of object lies in range of int then there is no way to differentiate whether this object (currently in form of json string) is INT or LONG, so the required result is not achieved.