Arraylist<LayoutBlock> GSON anonymous inner class initialization bug #1004

WilliamBergamin · 2022-06-21T15:44:14Z

GSON has a known issue where null is returned if it tries to serialize an object that was initialized using an anonymous inner class.

Example:

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;

import java.util.ArrayList;
import java.util.List;

List<String> strings = new ArrayList<>() {{
            add("hello");
            add("world");
        }};

GsonBuilder builder = new GsonBuilder();
Gson gson = builder.create();

String json = gson.toJson(strings);

assertEquals("{\"hello\", \"world\"}", json); // this should be true but json = "null"

The above assertion should be true but the json string generated by the GSON lib actually returns a string of value "null". This can lead to some head scratching for the users of this library.

These changes try to reinitialize & reserialize the list of blocks if this "null" output is obtained from the GSON library.

Category (place an `x` in each of the `[ ]`)

bolt (Bolt for Java)
bolt-{sub modules} (Bolt for Java - optional modules)
slack-api-client (Slack API Clients)
slack-api-model (Slack API Data Models)
slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to the those rules.

GSON has a known issue where null is returned if it tries to serialize a list of object that was initialized using an anonymous inner class. This commit tried to patch this.

codecov · 2022-06-21T16:10:15Z

Codecov Report

Merging #1004 (fdcdf10) into main (c79cdbb) will decrease coverage by 0.08%.
The diff coverage is 61.53%.

❗ Current head fdcdf10 differs from pull request most recent head f2aa8a4. Consider uploading reports for the commit f2aa8a4 to get more accurate results

@@             Coverage Diff              @@
##               main    #1004      +/-   ##
============================================
- Coverage     76.77%   76.69%   -0.09%     
- Complexity     3685     3688       +3     
============================================
  Files           403      403              
  Lines         11111    11120       +9     
  Branches       1108     1111       +3     
============================================
- Hits           8530     8528       -2     
- Misses         1907     1921      +14     
+ Partials        674      671       -3

Impacted Files	Coverage Δ
...java/com/slack/api/methods/RequestFormBuilder.java	`88.39% <61.53%> (+0.13%)`	⬆️
...va/com/slack/api/socket_mode/SocketModeClient.java	`64.33% <0.00%> (-3.83%)`	⬇️
...i/socket_mode/impl/SocketModeClientJavaWSImpl.java	`63.81% <0.00%> (-2.64%)`	⬇️
...imits/metrics/impl/BaseMemoryMetricsDatastore.java	`79.22% <0.00%> (-0.87%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 716a14d...f2aa8a4. Read the comment docs.

filmaj

LGTM, but will defer to @seratch for approval

filmaj · 2022-06-21T16:16:06Z

slack-api-client/src/main/java/com/slack/api/methods/RequestFormBuilder.java

        }
    }

+    private static String getJsonWithGsonAnonymInnerClassHandling(List<LayoutBlock> blocks){


I'd add a comment here that explains why we need this method, just as a hint to a maintainer reading through this code in the future (I have a bad memory so I probably won't remember the details of this problem in 6 months!). Maybe just a quick link to the GSON issue and a single sentence explanation.

Sounds good. Adding a link to the gson project issue would be easier.

Agreed, I added a comment with the link to the original issue

seratch · 2022-06-21T20:43:03Z

slack-api-client/src/test/java/test_locally/api/methods/RequestFormBuilderTest.java

+
+    @Test
+    public void testGetJsonWithGsonAnonymInnerClassHandling() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {
+        // SETUP


I like the test structure. If you want to have these comments, probably Given-When-Then style would be more common.

seratch · 2022-06-21T20:43:37Z

slack-api-client/src/main/java/com/slack/api/methods/RequestFormBuilder.java

        }
    }

+    private static String getJsonWithGsonAnonymInnerClassHandling(List<LayoutBlock> blocks){


Sounds good. Adding a link to the gson project issue would be easier.

seratch · 2022-06-21T20:45:25Z

slack-api-client/src/test/java/test_locally/api/methods/RequestFormBuilderTest.java

+    @Test
+    public void testGetJsonWithGsonAnonymInnerClassHandling() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {
+        // SETUP
+        Method method = RequestFormBuilder.class.getDeclaredMethod("getJsonWithGsonAnonymInnerClassHandling", List.class);


You can test one of the public methods that internally calls getJsonWithGsonAnonymInnerClassHandling instead of trying to run the private method using reflection APIs.

seratch · 2022-06-21T20:46:28Z

slack-api-client/src/main/java/com/slack/api/methods/RequestFormBuilder.java

            form.add("blocks", req.getBlocksAsString());
        } else if (req.getBlocks() != null) {
-            String json = GSON.toJson(req.getBlocks());
+            String json = getJsonWithGsonAnonymInnerClassHandling(req.getBlocks());


req.getUsers(): List<String> patterns also need the same workaround.

Makes sense, I also noticed that the Map objects also suffered from this GSON issue, I added additional code to handle these use cases.

Good catch! Thanks

Took feedback from PR and added support for Map object which suffer from the same GSON issue

seratch

Great improvement 💯 Thanks for fixing this!

seratch · 2022-06-22T21:08:18Z

slack-api-client/src/main/java/com/slack/api/methods/RequestFormBuilder.java

            form.add("blocks", req.getBlocksAsString());
        } else if (req.getBlocks() != null) {
-            String json = GSON.toJson(req.getBlocks());
+            String json = getJsonWithGsonAnonymInnerClassHandling(req.getBlocks());


Good catch! Thanks

seratch · 2022-06-22T21:10:49Z

slack-api-client/src/test/java/test_locally/api/methods/RequestFormBuilderTest.java

+
+    @Test
+    public void testEmptyListJsonSerialization() {
+        // GIVEN


As far as I know, using all capital letters for the Given/When/Then labels is not a common style but it's a minor thing. I will merge this as-is this time. See also: https://dzone.com/articles/a-new-approach-to-given-when-then

I was using this pattern based on an article I read a long time ago, I did not know this was well known pattern. Thanks!

WilliamBergamin added 2 commits June 20, 2022 20:14

Added fix for GSON issue with anonymous inner class initialization

7e0fd66

GSON has a known issue where null is returned if it tries to serialize a list of object that was initialized using an anonymous inner class. This commit tried to patch this.

Improved code clarity

4bf8a68

WilliamBergamin added the bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented label Jun 21, 2022

WilliamBergamin requested review from filmaj and seratch June 21, 2022 15:44

filmaj approved these changes Jun 21, 2022

View reviewed changes

seratch added this to the 1.23.0 milestone Jun 21, 2022

seratch added the project:slack-api-client project:slack-api-client label Jun 21, 2022

seratch assigned WilliamBergamin Jun 21, 2022

seratch suggested changes Jun 21, 2022

View reviewed changes

WilliamBergamin and others added 2 commits June 22, 2022 15:31

Added handling for Map

fdcdf10

Took feedback from PR and added support for Map object which suffer from the same GSON issue

Merge branch 'main' into WB-arraylist-gson-anonymous-inner-class-bug

f2aa8a4

seratch approved these changes Jun 22, 2022

View reviewed changes

seratch merged commit 6184bb6 into main Jun 22, 2022

seratch deleted the WB-arraylist-gson-anonymous-inner-class-bug branch June 22, 2022 21:20

Arraylist<LayoutBlock> GSON anonymous inner class initialization bug #1004

Arraylist<LayoutBlock> GSON anonymous inner class initialization bug #1004

Uh oh!

Conversation

WilliamBergamin commented Jun 21, 2022

Category (place an x in each of the [ ])

Requirements

Uh oh!

codecov bot commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

filmaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Category (place an `x` in each of the `[ ]`)

codecov bot commented Jun 21, 2022 •

edited

Loading