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

[BREAKING CHANGE] Replace rtm.start with rtm.connect #184

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@acconrad
Copy link
Contributor

commented Mar 18, 2019

Replace rtm.start with rtm.connect and bump to a major version along with a migration guide.

@acconrad acconrad referenced this pull request Mar 18, 2019

Open

Crash upon start #181

groups: rtm_list_to_map(rtm.groups),
users: rtm_list_to_map(rtm.users),
ims: rtm_list_to_map(rtm.ims)
bots: rtm_list_to_map(Slack.Web.Bots.info(%{token: token}) |> Map.get("bot")),

This comment has been minimized.

Copy link
@BlakeWilliams

BlakeWilliams Mar 18, 2019

Owner

This would kick off 5 different requests at once which I don't think is ideal. Is there no equivalent API to rtm.start to bootstrap the client?

This comment has been minimized.

Copy link
@BlakeWilliams

BlakeWilliams Mar 18, 2019

Owner

I can't link to it directly, but https://api.slack.com/rtm has rtm.connect vs rtm.start. It looks like both are still valid.

I'd be okay with reverting the change to use rtm.connect meaning the library will continue to only support rtm.start until someone takes the time to add rtm.connect support (assuming anyone needs it over rtm.start).

That being said, it does look like this approach of making multiple requests is how Slack recommends you use rtm.connect.

This comment has been minimized.

Copy link
@acconrad

acconrad Mar 18, 2019

Author Contributor

That being said, it does look like this approach of making multiple requests is how Slack recommends you use rtm.connect.

Correct, it will fire off 5 more requests and this is the recommended way to do this. That, and anyone using the Enterprise Grid with rtm.start will not work as expected if we stick with the old solution.

The other option would be to not populate the slack state object you have in the first place. Save that for when people optimistically decide to pull channels, users, whatever (if ever). What do you think about that?

This comment has been minimized.

Copy link
@BlakeWilliams

BlakeWilliams Mar 18, 2019

Owner

That is true. I'm not super concerned about supporting it but if we want to move forward with this and we get support at the same time I'm all for it 👍

@BlakeWilliams

This comment has been minimized.

Copy link
Owner

commented Mar 18, 2019

👋 thanks for tackling this. In terms of tests I've setup a basic fake API. The initial entry-point is here: https://github.com/BlakeWilliams/Elixir-Slack/blob/master/test/support/fake_slack.ex#L22 which then uses the Router defined here: https://github.com/BlakeWilliams/Elixir-Slack/blob/master/test/support/fake_slack/router.ex

It's a simple Plug based router/app so we could likely extend that to return fake results for each of those API Calls.

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@BlakeWilliams cool I'll check that out tonight.

Another option I considered is having 2 private functions within the init call:

1 that we currently have that supports the current functionality of the 5 requests going in tandem to legacy support rtm.start.

The other function would not have that flag on and would only return the standard rtm.connect base objects. That way we can send a deprecation message in the old function saying something like "pulling all objects will be retired in the next version, update accordingly" - and then when you update a major version of this plugin, all you have to do is delete the old function and rely only on the new function.

Does that make sense?

@BlakeWilliams

This comment has been minimized.

Copy link
Owner

commented Mar 18, 2019

That makes sense to me. As long as we expose a way to bootstrap each of the possible fields individually when using the 2nd option (as an argument or a function, I kinda lean towards a function) I think that'd work API wise.

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@BlakeWilliams so I looked into the call and the problem is that the init function is never called in your library. It's only called via websocket_client (the Erlang dependency), so there is no way for me to force in that argument (or alternative function).

Here is what I would recommend: we push this up right now so we can fix everyone's issue and bump the version so 0.16 -> 0.17. Yes, it requires a bunch more calls, but 5 HTTP calls is honestly a drop in the bucket and does solve the bug. At the next version 0.2 (or 1.0, whatever the next breaking bump) you just need to change the README to state that the slack state no longer returns everything that it originally did and probably include a migration guide (happy to provide that for you).

@BlakeWilliams

This comment has been minimized.

Copy link
Owner

commented Mar 30, 2019

@acconrad That sounds like a solid plan. I'm in favor of shipping with the extra HTTP calls now then removing them later as a breaking change (+ appropriate version bump).

@BlakeWilliams

This comment has been minimized.

Copy link
Owner

commented Mar 30, 2019

@acconrad I'd also really like to see a deprecation warning letting folks know that a future release will no longer auto-populate those fields with a link to the migration guide once we get that in. I think that'd be good UX for folks using the package.

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

@BlakeWilliams all set! I also fixed the tests, added the deprecation, and bumped it by 1 minor version and provided the appropriate documentation

@acconrad acconrad force-pushed the acconrad:acc-fix-rtm-connect branch from 8314874 to bdf2f1b Mar 30, 2019

Show resolved Hide resolved lib/slack/bot.ex Outdated
Show resolved Hide resolved lib/slack/bot.ex Outdated
assert slack.groups == %{"123" => %{id: "123"}}
assert slack.users == %{"123" => %{id: "123"}}
assert slack.ims == %{"123" => %{id: "123"}}
with_mocks([

This comment has been minimized.

Copy link
@BlakeWilliams

BlakeWilliams Apr 2, 2019

Owner

I've kept it as somewhat of a personal goal to keep mock out of the project. I'm happy to take it on myself, but it'd be great to see the fake http client return these instead of mocking.

This comment has been minimized.

Copy link
@acconrad

acconrad Apr 2, 2019

Author Contributor

I tried for a few hours on Saturday and this was the only way I could think of to mock out the external Web calls

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@BlakeWilliams ready to merge when you are

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@BlakeWilliams just checking in - fixed conflicts, ready when you are

@jameskbride jameskbride referenced this pull request Apr 14, 2019

Open

Outstanding PRs #189

{
"desc": "Gets information about a bot user.",
"args": {
"token": {

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 15, 2019

No other doc file lists token. Listing it results in the following error:

    (hackney) .../deps/hackney/src/hackney_bstr.erl:35: :hackney_bstr.to_binary/1
    (hackney) .../deps/hackney/src/hackney_url.erl:313: :hackney_url.urlencode/2
    (hackney) .../deps/hackney/src/hackney_url.erl:373: :hackney_url.qs/3
    (hackney) .../deps/hackney/src/hackney_request.erl:312: :hackney_request.encode_form/1
    (hackney) .../deps/hackney/src/hackney_request.erl:320: :hackney_request.handle_body/4
    (hackney) .../deps/hackney/src/hackney_request.erl:83: :hackney_request.perform/2
    (hackney) .../deps/hackney/src/hackney.erl:373: :hackney.send_request/2
    (httpoison) lib/httpoison/base.ex:787: HTTPoison.Base.request/6
    (httpoison) lib/httpoison.ex:128: HTTPoison.request!/5
    (slack) lib/slack/web/default_client.ex:23: Slack.Web.DefaultClient.post!/2
groups: rtm_list_to_map(rtm.groups),
users: rtm_list_to_map(rtm.users),
ims: rtm_list_to_map(rtm.ims)
bots: bot_to_map(Bots.info(%{token: token}) |> Map.get("bot")),

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 15, 2019

Currently bots.info returns an empty {ok: true} response if bot argument is absent. As a result, bot_to_map fails.

This comment has been minimized.

Copy link
@acconrad

acconrad Apr 16, 2019

Author Contributor

Is this a permissions problem because Bots.info requires you have the bot permission enabled for your app?

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 16, 2019

I was testing with a bot token on an app that doesn't have access to this scope via generic one, just in case. It could be a temporary API bug, but I don't have good way to check for that.

@Slotos
Copy link

left a comment

There are still issues not resolved by this PR that leave the library non-functional. I've tried leaving suggestions whenever I could make some.

I'll keep trying to make this run, but as I'm working on a bot prototype, I am unlikely to have a release quality code in the next few days, so I decided to at least share results on my struggles.

If possible, please revert rtm.start commit and release a minor version bump, as it keeps all released versions above 0.15.0 fail on RTM use with error mentioned in #181

@@ -176,6 +194,10 @@ defmodule Slack.Bot do

def websocket_handle(_, _conn, state), do: {:ok, state}

defp bot_to_map(bot) do
%{"#{bot.id}" => bot}

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 15, 2019

keys: :atoms is not used for Web API. This function will always fail.

Suggested change
%{"#{bot.id}" => bot}
%{Map.get("id") => bot}

This comment has been minimized.

Copy link
@acconrad

acconrad Apr 16, 2019

Author Contributor

The tests pass for my original code and fail for your suggestion. Using what I think you want to use, Map.get(bot, "id") will result in nil => %{id: "123"} for the tests instead of the appropriate "123" => %{id: "123"} - what version of Elixir are you using? If you're on the supported version these tests should pass for you

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 16, 2019

Tests are passing, it's use against real API that's failing. #114 describes the reason for it.

Basically, RTM and Bot modules decode JSON with keys: :atoms option. Whereas Slack.Web.DefaultClient doesn't.

Generally speaking, it might be a good idea to get rid of keys to atoms conversion as a part of major version release.

This comment has been minimized.

Copy link
@BlakeWilliams

BlakeWilliams Apr 16, 2019

Owner

Generally speaking, it might be a good idea to get rid of keys to atoms conversion as a part of major version release.

Agreed, that was a design decision made before the library was a library and it kinda snuck in. That'd be a great issue if there isn't one already.

defp bot_to_map(bot) do
%{"#{bot.id}" => bot}
end

defp rtm_list_to_map(list) do
Enum.reduce(list, %{}, fn item, map ->
Map.put(map, item.id, item)

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 15, 2019

keys: :atoms is not used for Web API. This function will always fail.

Suggested change
Map.put(map, item.id, item)
Map.put(map, Map.get(item, "id"), item)
channels: rtm_list_to_map(Channels.list(%{token: token}) |> Map.get("channels")),
groups: rtm_list_to_map(Groups.list(%{token: token}) |> Map.get("groups")),
users: rtm_list_to_map(Users.list(%{token: token}) |> Map.get("members")),
ims: rtm_list_to_map(Im.list(%{token: token}) |> Map.get("ims"))

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 15, 2019

All these lists are likely partial responses. It's better to not include this info at all instead of trying to preserve a semblance of backwards compatibility. Especially considering that old rtm.start is truncating members lists to 500 records for almost a year already.

This comment has been minimized.

Copy link
@acconrad

acconrad Apr 16, 2019

Author Contributor

All these lists are likely partial responses.

So if it's at parity with rtm.start then are we losing any more functionality than we already had?

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 16, 2019

Good point. I would still make it optional personally. The data in these keys is definitely not immutable on Slack side and it's best to fetch, cache, and expire it as specific bot requires.

@@ -8,32 +10,35 @@ defmodule Slack.BotTest do
@rtm %{

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 15, 2019

It's best to use a fake test web server here and instead of mocks later on. It's relatively safe to assume that unless Slack API misbehaves, the data will arrive as a well formed JSON. The tests will then catch issues with assuming keys to atoms conversion and others.

Mocking results of processing outside data leads to green tests that are likely to lie.

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

If possible, please revert rtm.start commit and release a minor version bump

@Slotos after reading all of these comments I'm just going to close this PR and re-open it with a major version bump since it has breaking changes. I don't want to be the one who is releasing the actual V1 of this but I think it's necessary to expose the gravity of the upgrade required. Revert PR incoming.

@acconrad acconrad closed this Apr 16, 2019

@BlakeWilliams

This comment has been minimized.

Copy link
Owner

commented Apr 16, 2019

@acconrad Thanks a ton for all your work on this. I definitely think this is the way to go but I agree the path forward is a major release. 👍

@Slotos

This comment has been minimized.

Copy link

commented Apr 16, 2019

@acconrad thank you. I've left some responses in threads to share more info, I hope it comes in handy.

@acconrad acconrad reopened this Apr 16, 2019

@acconrad acconrad force-pushed the acconrad:acc-fix-rtm-connect branch from f54d9d8 to 933bca0 Apr 16, 2019

@acconrad acconrad changed the title Fix rtm.connect startup issue Replace rtm.start with rtm.connect Apr 16, 2019

@acconrad acconrad changed the title Replace rtm.start with rtm.connect [DO NOT MERGE] Replace rtm.start with rtm.connect Apr 16, 2019

@acconrad acconrad changed the title [DO NOT MERGE] Replace rtm.start with rtm.connect [BREAKING CHANGE] Replace rtm.start with rtm.connect Apr 16, 2019

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@Slotos @BlakeWilliams @chulkilee

OKAY!

This should go in after #190 - first, we add in #190 to fix everyone's problems with rtm.connect. Then we introduce this fix to properly deprecate the Slack API while also providing a deprecation guide on how to upgrade and maintain current functionality (if you so desire all of those lists).

All tests are passing, there is no longer a dependency on the mock library since those tests are no longer necessary, and the code is just simpler overall.

Let me know what you think!

acconrad added some commits Apr 16, 2019

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

#190 just went in so I'll fix the conflicts tonight and this can be RTM

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@BlakeWilliams alright conflicts resolved - this should be good to go!

Wherever you grab the passed in `slack` state, add in additional calls to populate these lists:

```elixir
slack

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 23, 2019

websocket_handle in Slack.Bot is calling Slack.State.update_slack, which attempts to keep these maps up to date.

This creates a race condition where an update might arrive before the maps are created resulting in:

[error] ** Websocket client Slack.Bot terminating in :websocket_handle/3
   for the reason :error:{:badmap, nil}

Either the autoupdate routines should be purged or the system needs to be initialized with empty maps in place.

This comment has been minimized.

Copy link
@acconrad

acconrad Apr 23, 2019

Author Contributor

oh good point, I would recommend purging them since those maps are no longer needed. Will save connections and also improve performance.

This comment has been minimized.

Copy link
@BlakeWilliams

BlakeWilliams Apr 23, 2019

Owner

Sorry if this was already discussed, but why wouldn't we grab this data for the user? Having to manually manage these maps isn't a great experience imo.

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 23, 2019

Agreed. Existing code can be transformed into a battle tested example documentation.

I'll work on a cherry-pickable commit for this in a few hours.

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 23, 2019

@BlakeWilliams We can do that, but IMO it shouldn't be done in a structure that gets passed around. What do you think about an agent or ETS backed store with a singular access interface?

This comment has been minimized.

Copy link
@Slotos

Slotos Apr 28, 2019

@acconrad please take a look at Slotos/Elixir-Slack@703bf40

It introduces non-nil defaults to Slack.State and updates update routines to not fail when they encounter those. Tests pass, although I would (and will try to) spend more time to increase test coverage, as some cases aren't covered properly. There's likely to be a better way to handle some of the updates, but being an Elixir newbie I'm kinda blind to them.

Next step is to remove updates handling from Slack.Bot and transform them into documentation examples for handle_ callbacks in specific bots.

@BlakeWilliams There is an axolotl-sized dragon lying in Slack.State structure. While the data is being fetched an update can arrive with fresher state, which will then be promptly overridden by fetched bulk-update with older data. Slack records and events carry timestamps to indicate freshness of the data, but it's getting lost. In larger organizations this is a likely race condition with potentially visible, if not too significant, outcomes. Still, I'd personally leave implementation of a properly synchronized storage to whoever writes the bot, at least for the 1.0.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.