os / slacker

Full-featured Python interface for the Slack API
Apache License 2.0
1.6k stars 245 forks source link

429 Client Error Too Many Requests for users.list #133

Closed jaraco closed 5 years ago

jaraco commented 6 years ago

We're using slacker.get_user_id in this code, which runs for every message transmitted by our bot. Anytime a @ appears in the output, it searches for a user id for that value. This causes many invocations to get_user_id which in turn hits the rate limiting in Slack:

Error resolving slack reference
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/pmxbot/slack.py", line 99, in _expand
    ref = resolvers[match_type](match_name)
  File "/usr/local/lib/python3.6/site-packages/slacker/__init__.py", line 206, in get_user_id
    members = self.list().body['members']
  File "/usr/local/lib/python3.6/site-packages/slacker/__init__.py", line 191, in list
    return self.get('users.list', params={'presence': int(presence)})
  File "/usr/local/lib/python3.6/site-packages/slacker/__init__.py", line 117, in get
    api, **kwargs
  File "/usr/local/lib/python3.6/site-packages/slacker/__init__.py", line 95, in _request
    response.raise_for_status()
  File "/usr/local/lib/python3.6/site-packages/requests/models.py", line 935, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 429 Client Error: Too Many Requests for url: https://slack.com/api/users.list?presence=0&token=xoxb-just-kidding-no-secrets-here

My instinct is that get_user_id should probably cache responses with a method cache, such that get_user_id for a given ID doesn't need to re-query the list of users. That should drastically reduce the number of requests in a busy system.

Is this behavior something this project might consider, or should the client (us) add the caching themselves?

mdove commented 6 years ago

I've just started running into this issue, has anyone made a change/PR to attempt to address this?

os commented 5 years ago

@jaraco @mdove Sorry for my late response. I think it's better to be cached on the client side as some users may not want this caching behaviour or need something entirely different. For instance you may just want to cache the IDs where others may want to cache the whole response. I don't see caching functionality to become a part of the library.

os commented 5 years ago

For other people who might be interested in implementing their own caching solution, here's a quick example:

from slacker import Users

class CachedUsers(Users):
    def __init__(self, *args, **kwargs):
        super(Users, self).__init__(*args, **kwargs)
        self.user_ids = {}

    def get_user_id(self, user_name):
        user_id = self.user_ids.get(user_name)

        if not user_id:
            user_id = super().get_user_id(user_name)
            self.user_ids[user_name] = user_id

        return user_id