Skip to content

Conversation

@proggen-com
Copy link
Contributor

…member.

  • update to com.google.code.gson:gson:2.9.1
  • update to org.java-websocket:Java-WebSocket:1.5.3

CC @pusher/mobile

…member.

* update to com.google.code.gson:gson:2.9.1
* update to org.java-websocket:Java-WebSocket:1.5.3
return new Gson().toJson(this);
}

public static PusherEvent fromJson(String json) {

Choose a reason for hiding this comment

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

How about adding tests to it? Is there a way to reproduce the problem in a test case?

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Base: 74.50% // Head: 74.63% // Increases project coverage by +0.12% 🎉

Coverage data is based on head (fcbcbbd) compared to base (efed957).
Patch coverage: 87.50% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #339      +/-   ##
============================================
+ Coverage     74.50%   74.63%   +0.12%     
- Complexity      361      362       +1     
============================================
  Files            48       48              
  Lines          1957     1963       +6     
  Branches        147      144       -3     
============================================
+ Hits           1458     1465       +7     
  Misses          433      433              
+ Partials         66       65       -1     
Impacted Files Coverage Δ
...in/java/com/pusher/client/channel/PusherEvent.java 74.19% <86.36%> (+8.80%) ⬆️
...pusher/client/channel/impl/PrivateChannelImpl.java 90.62% <100.00%> (+0.30%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@sonologico sonologico left a comment

Choose a reason for hiding this comment

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

This solution won't fix the problem for all users.

The situation:
There are users receiving JSON objects in place of strings. This is not standard. The other example for this that I have seen was someone using this SDK with another mostly pusher compatible server. Regardless, these users are calling getProperty("data") and retrieving an Object.

  • On 2.40, this call would return an arbitrary Object.
  • With 2.41, this was broken, because we now explicitly extract a String upfront, so it crashes with arbitrary objects in the data field.
  • With this patch, we accept other types (besides String) and convert them to a String. However, this pushes the problem further down. Calling getProperty("data") will return a String (under the type Object), which is not what 2.40 is doing.

In my opinion, we should either

  • leave it as is and investigate why plain objects are being passed. If this is coming from one of our SDKs, it should be fixed. This leaves it broken for people who are purposefully or not using non-standard behaviour.
  • revert to the previous code in this class where we didn't actively extract the field upfront

@sonologico sonologico merged commit 698aaa4 into master Sep 29, 2022
@sonologico sonologico deleted the fix-data-crash branch September 29, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants