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

[GOBBLIN-2158] upgraded gson version with unit test #4057

Closed
wants to merge 2 commits into from
Closed

[GOBBLIN-2158] upgraded gson version with unit test #4057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2d921a
upgraded gson version with unit test
Blazer-007 Sep 17, 2024
8ccdba4
addressed review comments
Blazer-007 Sep 18, 2024
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
36 changes: 35 additions & 1 deletion gobblin-api/src/main/java/org/apache/gobblin/util/io/GsonInterfaceAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
import com.google.gson.reflect.TypeToken;
import com.google.gson.stream.JsonReader;
import com.google.gson.stream.JsonWriter;
import com.google.gson.JsonParseException;
import com.google.gson.ToNumberStrategy;
import com.google.gson.stream.MalformedJsonException;


/**
Expand Down Expand Up @@ -189,7 +192,38 @@ private <S> S readValue(JsonObject jsonObject, TypeToken<S> defaultTypeToken) th
}

public static <T> Gson getGson(Class<T> clazz) {
Gson gson = new GsonBuilder().registerTypeAdapterFactory(new GsonInterfaceAdapter(clazz)).create();
Gson gson = new GsonBuilder()
.setObjectToNumberStrategy(CustomToNumberPolicy.INTEGER_OR_LONG_OR_DOUBLE)
.registerTypeAdapterFactory(new GsonInterfaceAdapter(clazz))
.create();
return gson;
}

public enum CustomToNumberPolicy implements ToNumberStrategy {
Copy link
Contributor

@pratapaditya04 pratapaditya04 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be something like LongIntNumberPolicy

INTEGER_OR_LONG_OR_DOUBLE {
@Override
public Number readNumber(JsonReader in) throws IOException {
String value = in.nextString();
try {
return Integer.parseInt(value);
} catch (NumberFormatException var3) {
Copy link
Contributor

@pratapaditya04 pratapaditya04 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this

try {
return Long.parseLong(value);
} catch (NumberFormatException var2) {
Copy link
Contributor

@pratapaditya04 pratapaditya04 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 { /* ... */ }

`

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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

} else {
return d;
}
} catch (NumberFormatException var1) {
throw new JsonParseException("Cannot parse " + value + "; at path " + in.getPath(), var1);
}
}
}
}
}
}

}
55 changes: 55 additions & 0 deletions gobblin-api/src/test/java/org/apache/gobblin/util/io/GsonInterfaceAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import com.google.common.base.Optional;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;

import org.apache.gobblin.util.test.BaseClass;
import org.apache.gobblin.util.test.TestClass;
Expand All @@ -43,4 +44,58 @@ public void test() {

}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Test for null/infinites are missing

public void testObjectToIntegerDeserialize() {
Gson gson = new GsonBuilder().create();
Integer test = 5;
Blazer-007 marked this conversation as resolved.
Show resolved Hide resolved
String ser = gson.toJson(test);
Object deser = gson.fromJson(ser, Object.class);
Assert.assertNotEquals(test, deser);
Blazer-007 marked this conversation as resolved.
Show resolved Hide resolved
Assert.assertTrue(deser instanceof Double);

Gson customGson = new GsonBuilder()
.setObjectToNumberStrategy(GsonInterfaceAdapter.CustomToNumberPolicy.INTEGER_OR_LONG_OR_DOUBLE)
.create();

Object deser2 = customGson.fromJson(ser, Object.class);
Assert.assertEquals(test, deser2);
Assert.assertTrue(deser2 instanceof Integer);
}

@Test
public void testObjectToLongDeserialize() {
Gson gson = new GsonBuilder().create();
Long test = 1234567890123456789L;
String ser = gson.toJson(test);
Object deser = gson.fromJson(ser, Object.class);
Assert.assertNotEquals(test, deser);
Assert.assertTrue(deser instanceof Double);

Gson customGson = new GsonBuilder()
.setObjectToNumberStrategy(GsonInterfaceAdapter.CustomToNumberPolicy.INTEGER_OR_LONG_OR_DOUBLE)
.create();

Object deser2 = customGson.fromJson(ser, Object.class);
Assert.assertEquals(test, deser2);
Assert.assertTrue(deser2 instanceof Long);
}

@Test
public void testObjectToDoubleDeserialize() {
Gson gson = new GsonBuilder().create();
Double test = 5.0;
String ser = gson.toJson(test);
Object deser = gson.fromJson(ser, Object.class);
Assert.assertEquals(test, deser);
Assert.assertTrue(deser instanceof Double);

Gson customGson = new GsonBuilder()
.setObjectToNumberStrategy(GsonInterfaceAdapter.CustomToNumberPolicy.INTEGER_OR_LONG_OR_DOUBLE)
.create();

Object deser2 = customGson.fromJson(ser, Object.class);
Assert.assertEquals(test, deser2);
Assert.assertTrue(deser2 instanceof Double);
}

}
2 changes: 1 addition & 1 deletion gradle/scripts/dependencyDefinitions.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ ext.externalDependency = [
"eventhub": "com.microsoft.azure:azure-eventhubs:0.9.0",
"guava": "com.google.guava:guava:21.0",
"groovy": "org.codehaus.groovy:groovy:2.4.8",
"gson": "com.google.code.gson:gson:2.6.2",
"gson": "com.google.code.gson:gson:2.8.9",
"gsonJavatimeSerialisers": "com.fatboyindustrial.gson-javatime-serialisers:gson-javatime-serialisers:1.1.1",
"findBugsAnnotations": "com.google.code.findbugs:jsr305:" + findBugsVersion,
"hadoopCommon": "org.apache.hadoop:hadoop-common:" + hadoopVersion,
Expand Down
Loading