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.

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

Adding ping message and pong event to RTM client #459

Merged
merged 5 commits into from
May 21, 2020

Conversation

gaspardpetit
Copy link
Contributor

Signed-off-by: Gaspard Petit [email protected]

Summary

It seems like the ping and pong message and event are currently missing from the RTM java client. This change makes it possible to sendMessage a PingMessage and subscribe to PongEvent

In theory, the PingMessage could contain any field desired - I am not sure how that can easily be handled especially when reading back the PongEvent without special handling, but most people should be happy with just a time field to check how long it took for Slack to respond.

(Background on this change - I am currently investigating with the RTM client gets disconnected after a long period of idle time. I suspect sending periodic ping will either keep the websocket alive or will help the client detect when it was disconnected as it will fail to send the ping.

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #459 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #459      +/-   ##
============================================
+ Coverage     83.59%   83.65%   +0.06%     
- Complexity     2386     2387       +1     
============================================
  Files           250      250              
  Lines          6442     6442              
  Branches        590      590              
============================================
+ Hits           5385     5389       +4     
+ Misses          694      690       -4     
  Partials        363      363              
Impacted Files Coverage Δ Complexity Δ
...pi/methods/metrics/impl/RedisMetricsDatastore.java 89.50% <0.00%> (+2.46%) 38.00% <0.00%> (+1.00%)

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 408b1a2...b8c0cee. Read the comment docs.

@seratch
Copy link
Member

seratch commented May 19, 2020

👋 Thanks again! I will check this later when I have a chance. Can you add an integration test here to help me verify the behavior more easily (also for future maintenance)?
https://github.com/slackapi/java-slack-sdk/blob/v1.0.7/slack-api-client/src/test/java/test_with_remote_apis/methods/rtm_Test.java

@seratch
Copy link
Member

seratch commented May 19, 2020

You don't need to run any other tests under the package (they require some preparations).

@seratch seratch added this to the 1.0.x milestone May 20, 2020
@seratch seratch added enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client labels May 20, 2020
@gaspardpetit
Copy link
Contributor Author

Thank you for pointing me to the right place to add an integration test. It allowed me to fix the PongEvent which was expecting id instead of reply_to.

I am not very familiar with lombok (although finding it very convenient!); it generated getReply_to and setReply_to instead of getReplyTo and setReplyTo for this field. Do you know if there is a way to configure it so it handles snake_case correctly?

@gaspardpetit
Copy link
Contributor Author

Also, as a side note, I am >24h in and so far, pinging every 30 seconds has maintained my websocket connection opened with Slack with no other interaction. Generally, my connection drops after 6-10 hours. Perhaps periodic ping should be provided as a built-in feature of the RTM client?

public static final String TYPE_NAME = "pong";
private final String type = TYPE_NAME;

private final Long reply_to;
Copy link
Member

Choose a reason for hiding this comment

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

Can you define this field as private Long replyTo (camelCase + no need to have final for this) instead? Lombok generates Long getReplyTo() and void setReplyTo(Long) for you.

Regarding the conversion from snake_cased JSON keys, Gson in this project automatically translates snake_case to camelCase. https://github.com/slackapi/java-slack-sdk/blob/v1.0.7/slack-api-client/src/main/java/com/slack/api/rtm/RTMEventsDispatcherImpl.java#L53

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - good to know!

@seratch
Copy link
Member

seratch commented May 20, 2020

Also, as a side note, I am >24h in and so far, pinging every 30 seconds has maintained my websocket connection opened with Slack with no other interaction. Generally, my connection drops after 6-10 hours. Perhaps periodic ping should be provided as a built-in feature of the RTM client?

That's fair enough. Actually, the Python SDK does.
https://github.com/slackapi/python-slackclient/blob/v2.6.0rc2/slack/rtm/client.py#L51-L53

If you have time to work on it, it'd be greatly appreciated. Of course, I can work on it after merging this.

@seratch seratch self-assigned this May 20, 2020
@seratch seratch merged commit b8c0cee into slackapi:master May 21, 2020
@seratch
Copy link
Member

seratch commented May 21, 2020

I've applied the code formatter and tweaked the test. ac2a010

Thanks for your contribution again!

seratch added a commit that referenced this pull request May 28, 2020
* [slack-api-model] Add missing fields in objects (confirm.style in blocks, user.is_invited_user: boolean, message.hidden, Slack post related fields in file objects) - thanks @seratch
* [slack-api-client etc] #466 #462 Calls API support - thanks @seratch
* [slack-api-client] #475 #474 Make redirect_uri for oauth.access / oauth.v2.access optional - thanks @natevaughan @seratch
* [slack-api-client] #476 Bump dependencies (okhttp, micronaut, tyrus-standalone-client) - thanks @seratch
* [slack-api-client] #459 Adding ping message and pong event to RTM client - thanks @gaspardpetit
* [slack-api-client] #451 Add support and check for the event subtypes in RTM client - thanks @gaspardpetit
* [bolt] #455 Improve OAuth flow module to consider the cases where team is missing in oauth.v2.access responses - thanks @seratch
* [bolt] #476 Bump dependencies (aws-java-sdk-s3) - thanks @seratch
emanguy pushed a commit to emanguy/java-slack-sdk that referenced this pull request Jun 22, 2020
* [slack-api-model] Add missing fields in objects (confirm.style in blocks, user.is_invited_user: boolean, message.hidden, Slack post related fields in file objects) - thanks @seratch
* [slack-api-client etc] slackapi#466 slackapi#462 Calls API support - thanks @seratch
* [slack-api-client] slackapi#475 slackapi#474 Make redirect_uri for oauth.access / oauth.v2.access optional - thanks @natevaughan @seratch
* [slack-api-client] slackapi#476 Bump dependencies (okhttp, micronaut, tyrus-standalone-client) - thanks @seratch
* [slack-api-client] slackapi#459 Adding ping message and pong event to RTM client - thanks @gaspardpetit
* [slack-api-client] slackapi#451 Add support and check for the event subtypes in RTM client - thanks @gaspardpetit
* [bolt] slackapi#455 Improve OAuth flow module to consider the cases where team is missing in oauth.v2.access responses - thanks @seratch
* [bolt] slackapi#476 Bump dependencies (aws-java-sdk-s3) - thanks @seratch
@gaspardpetit gaspardpetit deleted the ping_pong_message_and_event branch September 17, 2020 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality project:slack-api-client/rtm project:slack-api-client project:slack-api-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants