Event handling refactor and fix handling of pusher_internal:subscription_count event #336

proggen-com · 2022-08-23T10:30:44Z

Description of the pull request

pass the pusher_internal:subscription_count event on to the handlers. Also create the public
pusher:subscription_count event

Why is the change necessary?

The event was not send and the data was not parsed correctly.

CC @pusher/mobile

but pass it on to the handlers. Also create the public pusher:subscription_count event

sonologico · 2022-08-30T12:43:40Z

Looking at CI history, the test failures seem to be new to this PR.

- formatting - fix subscriptioncount

codecov-commenter · 2022-08-31T16:23:44Z

Codecov Report

Base: 74.79% // Head: 74.50% // Decreases project coverage by -0.29% ⚠️

Coverage data is based on head (c23ebee) compared to base (5c484c4).
Patch coverage: 80.27% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #336      +/-   ##
============================================
- Coverage     74.79%   74.50%   -0.30%     
- Complexity      346      361      +15     
============================================
  Files            51       48       -3     
  Lines          1964     1957       -7     
  Branches        150      147       -3     
============================================
- Hits           1469     1458      -11     
- Misses          430      433       +3     
- Partials         65       66       +1

Impacted Files	Coverage Δ
...sher/client/channel/SubscriptionEventListener.java	`0.00% <ø> (ø)`
src/main/java/com/pusher/client/channel/User.java	`50.00% <0.00%> (ø)`
...sher/client/channel/impl/message/AuthResponse.java	`100.00% <ø> (ø)`
...usher/client/channel/impl/message/ChannelData.java	`100.00% <ø> (ø)`
...nt/channel/impl/message/EncryptedReceivedData.java	`100.00% <ø> (ø)`
...lient/channel/impl/message/PresenceMemberData.java	`100.00% <ø> (ø)`
...nt/channel/impl/message/SubscriptionCountData.java	`0.00% <ø> (ø)`
...t/connection/websocket/WebSocketClientWrapper.java	`46.87% <0.00%> (ø)`
...com/pusher/client/crypto/nacl/SecretBoxOpener.java	`76.47% <0.00%> (ø)`
...her/client/crypto/nacl/SecretBoxOpenerFactory.java	`0.00% <ø> (ø)`
... and 39 more

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.

proggen-com · 2022-09-06T12:07:06Z

I did a change in the failing test, I think it's more stable now

sonologico

I'm going through the changes, but there are a lot of formatting changes and some other improvements (like adding final). They are all welcome, but Ideally would have been a separate PR.

sonologico · 2022-09-06T13:44:19Z

src/main/java/com/pusher/client/channel/PusherEvent.java

+
+    @SerializedName("user_id")
+    private final String userId;
+
+    private final String data;
+    private final String channel;
+    private final String event;
+
+    public String getUserId() {
+        return userId;
+    }
+
+    public String getChannelName() {
+        return channel;
+    }
+
+    public String getEventName() {
+        return event;
+    }
+
+    public String getData() {
+        return data;
+    }


Are the changes in this class needed for the fix? This is a breaking change in the public interface.

I've added the getProperty method back, so now the public API is the same?

Yes. That's better. I would also remove the automatic conversion from object to string on the data field.

All our SDKs should be serializing event data as strings, but that's not a breaking change of course.

I would also remove the automatic conversion from object to string on the data field.

I'm not sure what you mean here, it was a string and it's still a string?

I'm referring to the fromJson method.

Ah, I see. Test data is incorrect then. Let's fix that and make PusherEvent simpler

Well the data can be anything (even not json).

It should always be a string.

that is true, I'll fix the unit tests and always handle it as string

sonologico · 2022-09-08T07:42:10Z

src/main/java/com/pusher/client/Pusher.java

+    public void handleEvent(PusherEvent event) {
+        user.handleEvent(event);
+        channelManager.handleEvent(event);
    }


This should remain private. I don't think there's a benefit to exposing it.

It's implementing the PusherEventHandler interface now, so it has to be public

But what's the benefit of implementing that interface if we don't want to expose this method?

Previously it used a BiConsumer, which is kind of magic. Now it's a nice interface implementation, passed to factory.getConnection

On the call to getConnection, we could keep it as it was with this::handleEvent then Pusher doesn't have to implement anything if you make getConnection take a Consumer<PusherEvent> or if you annotate PusherEventHandler as a FunctionalInterface

ok, like this?

sonologico · 2022-09-09T07:45:06Z

src/main/java/com/pusher/client/channel/impl/BaseChannel.java

+            if (event.getEventName().equals(SUBSCRIPTION_COUNT_EVENT)) {
+                handleSubscriptionCountEvent(event);
            }
+            emit(event);


Processing the internal subscription count event further is a change in behaviour that is not required to fix the reported issue if I understand correctly. Do we need this?

It's used to update the internal subscriptionCount value, which provides for the getCount() method

That is done in the handleSubscriptionCountEvent.

What I'm pointing out is

Before this PR, handleEvent stopped after calling handleSubscriptionCountEvent. The only event processed further would be the public subscription count event.

With this PR, we call emit on the internal event after calling handleSubscriptionCountEvent. The equivalent behaviour wasn't there before. I'm not sure it is needed

Ok changed it

sonologico

Looks good to me. Thanks!

handle pusher_internal:subscription_count event internally,

80a9244

but pass it on to the handlers. Also create the public pusher:subscription_count event

- refactor pusher event handling

6bb5671

- formatting - fix subscriptioncount

proggen-com changed the title ~~handle pusher_internal:subscription_count event~~ Event handling refactor and fix handling of pusher_internal:subscription_count event Aug 31, 2022

formatting

b71edcf

try to let test wait for task to finish

2aa036a

sonologico reviewed Sep 6, 2022

View reviewed changes

proggen-com added 2 commits September 6, 2022 16:16

add legacy getProperty

cb18690

make sure the PusherEvent data member is always a string

5c0ac52

sonologico reviewed Sep 8, 2022

View reviewed changes

Use Consumer<PusherEvent> instead of PusherEventHandler interface

d8c108b

sonologico reviewed Sep 9, 2022

View reviewed changes

internally handle pusher_internal:subscription_count event only

c23ebee

sonologico approved these changes Sep 13, 2022

View reviewed changes

update changelog and readme

0c672e2

sonologico merged commit a8e9594 into master Sep 13, 2022

sonologico deleted the fix-subscription-count-event branch September 13, 2022 10:00

Event handling refactor and fix handling of pusher_internal:subscription_count event #336

Event handling refactor and fix handling of pusher_internal:subscription_count event #336

Uh oh!

Conversation

proggen-com commented Aug 23, 2022

Description of the pull request

Why is the change necessary?

The event was not send and the data was not parsed correctly.

Uh oh!

sonologico commented Aug 30, 2022

Uh oh!

codecov-commenter commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

proggen-com commented Sep 6, 2022

Uh oh!

sonologico 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!

proggen-com Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sonologico Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

sonologico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Aug 31, 2022 •

edited

Loading

proggen-com Sep 6, 2022 •

edited

Loading

sonologico Sep 7, 2022 •

edited

Loading