Cache translation from channel and user name to ID (translation prone to failure)

[jmanian]

When a channel name is given for a channel parameter, rather than a channel ID, we are using channels.list to look up the ID of the channel and then using the ID in the request instead of the name.

However, channels.list is a legacy method and is no longer guaranteed to return every channel. This leads to a channel_not_found error if you pass a channel name for a channel that is not supported by channels.list.

A couple suggested solutions:

1. Update channels_id to use conversations.list instead of channels.list. The tricky thing about this is that conversations.list only works with pagination, so it's not a simple drop-in replacement. It also means that if you do something like chat_postMessage(channel: 'general', text: 'foo') then for a team with many channels you might end up making hundreds of paginated calls to conversations.list before actually hitting chat.postMessage, which is not ideal. If we use this solution I think we should change the readme to discourage this approach and encourage using channel IDs directly.
2. Remove the translation completely, which would be a breaking change (but not the worst breaking change, since it's already basically broken). Since using conversations.list is not ideal for making the translation (see above) we could remove support for this altogether, forcing people to confront the issue directly.
3. Leave it as it is, and update the readme to note that it does not always work and discourage its use.

See #270 for more background.

[alexagranov]

I would vote for 2) or 3) as I'm not sure how people are getting a channel name to begin with?

Any payload that Slack sends that I've seen - either triggered by slash command, event update, RTM message, dialog submission - where a channel name was mentioned is always delivered converted to a channel_id.

Is there some other use case - other than manually hardcoding a channel name - that needs support for channel names when sending off API calls?

[mortee]

in all fairness, chat_postMessage does work given a channel name, but omitting the leading hash mark. in this case no automatic mapping occurs, and Slack does find the channel by name. however, it doesn't do the same for all the other chat methods (e.g. chat_update). pretty weird inconsistency.

[dblock]

I will take PRs into any direction.

My 0.02c is that pagination is not hard and maintaining the feature would continue helping people not have to think about whether they supply a channel name or ID. I personally think I use it all the time :)

[mortee]

Well, maybe that channel list, once acquired, should be cached, so that the client wouldn't reload it for every single chat operation. I kind of guess that following channel creations/deletions dynamically from a single client instance should be quite a rarely occuring requirement, in which case I'd suggest a method to invalidate/force redownload the channel list.

[dblock]

Well, maybe that channel list, once acquired, should be cached, so that the client wouldn't reload it for every single chat operation. I kind of guess that following channel creations/deletions dynamically from a single client instance should be quite a rarely occuring requirement, in which case I'd suggest a method to invalidate/force redownload the channel list.

Personally, I feel that caching would be beyond the scope of the library. A client can do it if they want. Similarly I do see how doing the lookup could also be beyond the scope of the library ;)

[mortee]

If I'm not mistaken, that lookup functionality is currently actually implemented, just it got (partially) broken eventually. On the other hand, if it gets fixed using that paginating conversations API, then in case of the presence of many channels, the whole paginating process (with as many API calls) will be run over and over again on every single chat operation, which needs this channel lookup. Kind of seems like suboptimal performance (:

[dblock]

Yes, it's suboptimal.

[ioquatix]

Is there no api for "Give me the metadata for this specific channel named X"?

[chrisbloom7]

We are doing the channel name -> ID lookup in almost every call because we're passing in a channel name from a chatop command, as in (contrived, but illustrative) .chatops check_members #roomname. It's unfortunate that Slack doesn't provide a native endpoint to just look that up, so in lieu of that I appreciate that this functionality is baked in even if it can lead to excessive paging. I agree that caching should not be something this library provides, but what about a simple memoization feature? That would at least allow multiple calls to methods referencing the same channel (or user, etc.) from the same client instance without doing the conversations.list lookup each time.

[chrisbloom7]

If that's of interest, I'd be happy to submit a PR

[dblock]

If that's of interest, I'd be happy to submit a PR

We have a concept of a store for RTM connections that persist information, so I’d be open for something similar. I also wonder whether we can achieve storing lookups with subclassing the client and overriding some methods? Open to PRs either way.

[chrisbloom7]

storing lookups with subclassing the client and overriding some methods

I'm not sure I follow. Can you expand on this idea and help me understand what you're suggesting here?

In the meantime I'll take a closer look at what is happening under the covers of the RTM connections.

[dblock]

storing lookups with subclassing the client and overriding some methods

I'm not sure I follow. Can you expand on this idea and help me understand what you're suggesting here?

The entire implementation of looking up IDs is in a mixin, so you could create a new client class that overrides id_for, or monkey patch the mixin and implement whatever you wanted in there.

I do think this could be valuable in the library itself, but it's obviously a higher bar. Something that works is always a good start!

In the meantime I'll take a closer look at what is happening under the covers of the RTM connections.

For storage, the RealTime client supports stores like so:

Thinking you would pass a similar store class that would start by storing lookup values for channel IDs, but could do more in the future.

[chrisbloom7]

Thanks for providing the extra context. It sounds as if it would be trivial to do this 1-off as needed in consuming applications through subclassing and then overriding the mixin. I can certainly start there and see if that helps improve performance.

[merefield]

Hmmm, caching is not possible? How about something like:

  def self.channel_cache_key(channel)
    "slack_channel_#{channel}"
  end

  match /.*/ do |client, data, match|

    mychannel = Rails.cache.fetch(channel_cache_key(data.channel), expires_in: 1.hour) do
      web_client = Slack::Web::Client.new
      web_client.conversations_info(channel: "#{data.channel}")
    end

etc.

Otherwise, I agree, this is very wasteful.