Replace 'org.json:json' lib with 'com.google.code.gson' lib #1437

yongyan-gh · 2021-02-19T11:19:19Z

This is for issue #1279

Some of Gson classes/methods doesn't support fluent syntax now, has to refactor a little bit.
org.json:json has append() / putOpt() can handle empty/null object/array, which need to manually handle in Gson.

yongyan-gh · 2021-02-19T11:44:08Z

Attached the Sarif log generated with this change (using com.google.code.gson)
also Sarif log generated using current SpotBugs (org.json:json) for a comparison reference.
GsonSarifLog.zip

@KengoTODA pls review.
cc @michaelcfanning @eddynaka

KengoTODA

Thanks for your contribution! Please check my comments for maintainability.

Also, please put https://github.com/google/gson/blob/gson-parent-2.8.6/LICENSE into spotbugs/licenses directory to distribute.

KengoTODA · 2021-02-20T07:05:38Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/Extension.java

+            extensionJson.addProperty("informationUri", informationUri.toString());
+        }
+        if (organization != null) {
+            extensionJson.addProperty("organization", String.valueOf(organization));


Is are any reason to prefer String.valueOf( ... ) over organization.toString()? If so, I want to make the intention clear.

Fixed.
organization was declared as Object it should be String

KengoTODA · 2021-02-20T07:14:10Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/Level.java

-    public String toJSONString() {
-        return String.format("\"%s\"", name().toLowerCase());
+    public String toJsonString() {
+        return name().toLowerCase();


Do we need this method? The old one was provided to configure the org.json's serializer.
So I assume what we need is not this method but similar way to configure the serialier, like SerializedName.

Added SerializedName annotation to the enum. Its useful if we use Gson toJson/fromJson methods to serialize/deserialize objects.

Current we create/setup JsonObject instead serializing object. e.g.

JsonObject result = new JsonObject(); result.addProperty("ruleId", ruleId); result.addProperty("ruleIndex", ruleIndex);; result.addProperty("level", level.toJsonString());

I think after POJO classes are introduced we should be able to use object serialization/deserialization and not need this method anymore

Also tried
return new Gson().toJson(this);
but it returned string with double quotes "\"warning"\", but we need string without double quotes "warning"

KengoTODA · 2021-02-20T07:17:57Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

                    "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json");
            processRuns(jsonWriter);
            jsonWriter.endObject();
            getBugCollection().bugsPopulated();
+        } catch (IOException e) {
+            e.printStackTrace(System.err);


I assume it's better to throw UncheckedIOException instead?
At least, we cannot ignore this exception like this, because we surely have problem in generated SARIF report file.

Here we do not need to invoke e.printStackTrace(System.err);. Who catches the error will print it.

And when we need to log, we use SLF4J as a logging facade so please do not use System.err directly.

Removed printStackTrace to let caller to log the error.

KengoTODA · 2021-02-20T07:19:42Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

+        try {
+            Gson gson = new Gson();
+            jsonWriter.name("runs").beginArray().beginObject();
+            BugCollectionAnalyser analyser = new BugCollectionAnalyser(getBugCollection());
+            processTool(jsonWriter, analyser.getRules());
+            processInvocations(jsonWriter, analyser.getBaseToId());
+
+            jsonWriter.name("results").beginArray();
+            analyser.getResults().forEach((result) -> gson.toJson(result, jsonWriter));
+            jsonWriter.endArray();
+
+            jsonWriter.name("originalUriBaseIds");
+            gson.toJson(analyser.getOriginalUriBaseIds(), jsonWriter);
+
+            jsonWriter.endObject().endArray(); // end "runs", end "runs" array
+        } catch (IOException e) {
+            throw e;
+        }


This try-block seems needless.

Suggested change

try {

Gson gson = new Gson();

jsonWriter.name("runs").beginArray().beginObject();

BugCollectionAnalyser analyser = new BugCollectionAnalyser(getBugCollection());

processTool(jsonWriter, analyser.getRules());

processInvocations(jsonWriter, analyser.getBaseToId());

jsonWriter.name("results").beginArray();

analyser.getResults().forEach((result) -> gson.toJson(result, jsonWriter));

jsonWriter.endArray();

jsonWriter.name("originalUriBaseIds");

gson.toJson(analyser.getOriginalUriBaseIds(), jsonWriter);

jsonWriter.endObject().endArray(); // end "runs", end "runs" array

} catch (IOException e) {

throw e;

}

Gson gson = new Gson();

jsonWriter.name("runs").beginArray().beginObject();

BugCollectionAnalyser analyser = new BugCollectionAnalyser(getBugCollection());

processTool(jsonWriter, analyser.getRules());

processInvocations(jsonWriter, analyser.getBaseToId());

jsonWriter.name("results").beginArray();

analyser.getResults().forEach((result) -> gson.toJson(result, jsonWriter));

jsonWriter.endArray();

jsonWriter.name("originalUriBaseIds");

gson.toJson(analyser.getOriginalUriBaseIds(), jsonWriter);

jsonWriter.endObject().endArray(); // end "runs", end "runs" array

Edit: fixed.
Most methods of Gson JsonWriter like name/value/begin.../end... throw IOException. That's why I have to catch it in try catch block.

KengoTODA · 2021-02-20T07:20:39Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

+        try {
+            Gson gson = new Gson();
+            jsonWriter.name("invocations").beginArray();
+            gson.toJson(invocation.toJsonObject(), jsonWriter);
+            jsonWriter.endArray();
+        } catch (IOException e) {
+            throw e;
+        }


This try-block seems needless.

Suggested change

try {

Gson gson = new Gson();

jsonWriter.name("invocations").beginArray();

gson.toJson(invocation.toJsonObject(), jsonWriter);

jsonWriter.endArray();

} catch (IOException e) {

throw e;

}

Gson gson = new Gson();

jsonWriter.name("invocations").beginArray();

gson.toJson(invocation.toJsonObject(), jsonWriter);

jsonWriter.endArray();

KengoTODA · 2021-02-20T07:21:32Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

-        jsonWriter.key("rules").value(rules);
-        jsonWriter.endObject().endObject();
+    private void processTool(@NonNull JsonWriter jsonWriter, @NonNull JsonArray rules) throws IOException {
+        try {


This try-block is also needless. Simply throw IOException to the caller side.

KengoTODA · 2021-02-20T07:22:57Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

-        jsonWriter.endObject().endObject();
+    private void processTool(@NonNull JsonWriter jsonWriter, @NonNull JsonArray rules) throws IOException {
+        try {
+            Gson gson = new Gson();


Is it OK to generate Gson instance on demand? Not sure its cost, but if it takes much resources (CPU or Java heap), consider to create it once and reuse by providing it via method parameter.

Added a Gson member instead creating new instance for each place.
According to Gson user guide:

The Gson instance does not maintain any state while invoking Json operations. So, you are free to reuse the same object for multiple Json serialization and deserialization operations.

KengoTODA · 2021-02-20T07:23:29Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

+            rules.forEach((rule) -> gson.toJson(rule, jsonWriter));
+            jsonWriter.endArray();
+
+            jsonWriter.endObject().endObject(); // end "driver", end "tool"


Good comment to keep the code readable! 👍

KengoTODA · 2021-02-20T07:23:46Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

-        DetectorFactoryCollection.instance().plugins().stream().map(Extension::fromPlugin).map(Extension::toJSONObject).forEach(jsonWriter::value);
-        jsonWriter.endArray();
+    private void processExtensions(@NonNull JsonWriter jsonWriter) throws IOException {
+        try {


this try-block seems needless.

KengoTODA

Cool. Just one change is necessary, please check.

KengoTODA · 2021-02-23T00:57:50Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

                    "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json");
            processRuns(jsonWriter);
            jsonWriter.endObject();
            getBugCollection().bugsPopulated();
+        } catch (IOException e) {
+            e.printStackTrace(System.err);


Here we do not need to invoke e.printStackTrace(System.err);. Who catches the error will print it.

And when we need to log, we use SLF4J as a logging facade so please do not use System.err directly.

KengoTODA

LGTM. Thanks for your contribution!

KengoTODA · 2021-02-23T01:18:04Z

@spotbugs/everyone if possible, please double-check this PR. I'm recently little bit careless to make a high quality review. 🙇‍♂️

yongyan-gh

LGTM. Thanks for your contribution!

Thank you for reviewing!

yongyan-gh · 2021-02-22T03:27:49Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

+        try {
+            Gson gson = new Gson();
+            jsonWriter.name("runs").beginArray().beginObject();
+            BugCollectionAnalyser analyser = new BugCollectionAnalyser(getBugCollection());
+            processTool(jsonWriter, analyser.getRules());
+            processInvocations(jsonWriter, analyser.getBaseToId());
+
+            jsonWriter.name("results").beginArray();
+            analyser.getResults().forEach((result) -> gson.toJson(result, jsonWriter));
+            jsonWriter.endArray();
+
+            jsonWriter.name("originalUriBaseIds");
+            gson.toJson(analyser.getOriginalUriBaseIds(), jsonWriter);
+
+            jsonWriter.endObject().endArray(); // end "runs", end "runs" array
+        } catch (IOException e) {
+            throw e;
+        }


Edit: fixed.
Most methods of Gson JsonWriter like name/value/begin.../end... throw IOException. That's why I have to catch it in try catch block.

yongyan-gh · 2021-02-22T06:27:22Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/Extension.java

+            extensionJson.addProperty("informationUri", informationUri.toString());
+        }
+        if (organization != null) {
+            extensionJson.addProperty("organization", String.valueOf(organization));


Fixed.
organization was declared as Object it should be String

yongyan-gh · 2021-02-22T06:28:08Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

                    "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json");
            processRuns(jsonWriter);
            jsonWriter.endObject();
            getBugCollection().bugsPopulated();
+        } catch (IOException e) {
+            e.printStackTrace(System.err);


yongyan-gh · 2021-02-22T06:35:28Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

-        jsonWriter.endObject().endObject();
+    private void processTool(@NonNull JsonWriter jsonWriter, @NonNull JsonArray rules) throws IOException {
+        try {
+            Gson gson = new Gson();


Added a Gson member instead creating new instance for each place.
According to Gson user guide:

The Gson instance does not maintain any state while invoking Json operations. So, you are free to reuse the same object for multiple Json serialization and deserialization operations.

yongyan-gh · 2021-02-22T06:53:55Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/Level.java

-    public String toJSONString() {
-        return String.format("\"%s\"", name().toLowerCase());
+    public String toJsonString() {
+        return name().toLowerCase();


Added SerializedName annotation to the enum. Its useful if we use Gson toJson/fromJson methods to serialize/deserialize objects.

Current we create/setup JsonObject instead serializing object. e.g.

JsonObject result = new JsonObject(); result.addProperty("ruleId", ruleId); result.addProperty("ruleIndex", ruleIndex);; result.addProperty("level", level.toJsonString());

I think after POJO classes are introduced we should be able to use object serialization/deserialization and not need this method anymore

yongyan-gh · 2021-02-22T06:56:44Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/Level.java

-    public String toJSONString() {
-        return String.format("\"%s\"", name().toLowerCase());
+    public String toJsonString() {
+        return name().toLowerCase();


Also tried
return new Gson().toJson(this);
but it returned string with double quotes "\"warning"\", but we need string without double quotes "warning"

yongyan-gh · 2021-02-22T18:50:29Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

+        try {
+            Gson gson = new Gson();
+            jsonWriter.name("invocations").beginArray();
+            gson.toJson(invocation.toJsonObject(), jsonWriter);
+            jsonWriter.endArray();
+        } catch (IOException e) {
+            throw e;
+        }


yongyan-gh · 2021-02-22T18:50:43Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

-        jsonWriter.key("rules").value(rules);
-        jsonWriter.endObject().endObject();
+    private void processTool(@NonNull JsonWriter jsonWriter, @NonNull JsonArray rules) throws IOException {
+        try {


yongyan-gh · 2021-02-22T18:51:43Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

-        DetectorFactoryCollection.instance().plugins().stream().map(Extension::fromPlugin).map(Extension::toJSONObject).forEach(jsonWriter::value);
-        jsonWriter.endArray();
+    private void processExtensions(@NonNull JsonWriter jsonWriter) throws IOException {
+        try {


yongyan-gh · 2021-02-23T01:19:01Z

spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java

                    "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json");
            processRuns(jsonWriter);
            jsonWriter.endObject();
            getBugCollection().bugsPopulated();
+        } catch (IOException e) {
+            e.printStackTrace(System.err);


Removed printStackTrace to let caller to log the error.

yongyan-gh · 2021-02-23T02:20:54Z

Attached the Sarif log generated by SpotBugs with the Gson changes.
Except some name/value pairs are in different order, its identical to original Sarif log file generated using org.json
gsonResult.zip

yongyan-gh · 2021-02-28T22:41:36Z

@KengoTODA
I ran all tests all passed and adhoc tests it works as expected. Can you pls merge this change if no more issue?
Also pls review #1434.

Thanks!

michaelcfanning · 2021-03-03T00:43:18Z

Fantastic!

yongyan-gh added 6 commits February 19, 2021 02:16

Replace 'org.json:json' lib with 'com.google.code.gson' lib

77aa961

Merge branch 'master' into users/yongyan-gh/jsonLib

70116ab

Only add to json if array is not empty

35eacab

Fix LogicalLocation Properties null case

31c5edf

Update changelog

8e888d4

Fix format violations

ce58c45

KengoTODA requested changes Feb 20, 2021

View reviewed changes

yongyan-gh added 2 commits February 21, 2021 22:22

Fix issues from PR review

b923c88

Add Gson license file.

540b592

yongyan-gh marked this pull request as ready for review February 22, 2021 18:24

Remove unnecessary try catch blocks

c2b3405

KengoTODA requested changes Feb 23, 2021

View reviewed changes

Let caller to handle logging error message

e41d5e6

KengoTODA approved these changes Feb 23, 2021

View reviewed changes

yongyan-gh commented Feb 23, 2021

View reviewed changes

KengoTODA merged commit 41485ae into spotbugs:master Mar 3, 2021

aajisaka mentioned this pull request Mar 11, 2021

Please consider org.json:json alternative #1279

Closed

This was referenced Mar 12, 2021

Chore(deps): Bump spotbugs.version from 4.2.1 to 4.2.2 bitwiseman/github-api#43

Closed

Bump spotbugs from 4.2.1 to 4.2.2 nhojpatrick/apache_commons-io#52

Closed

Replace 'org.json:json' lib with 'com.google.code.gson' lib #1437

Replace 'org.json:json' lib with 'com.google.code.gson' lib #1437

Conversation

yongyan-gh commented Feb 19, 2021

yongyan-gh commented Feb 19, 2021

KengoTODA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA left a comment

Choose a reason for hiding this comment

KengoTODA commented Feb 23, 2021

yongyan-gh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongyan-gh commented Feb 23, 2021

yongyan-gh commented Feb 28, 2021

michaelcfanning commented Mar 3, 2021