yagpdb icon indicating copy to clipboard operation
yagpdb copied to clipboard

templates: add getAllMessageReactions

Open jo3-l opened this issue 2 years ago • 4 comments

Expose the MessageReactions endpoint, which yields the users that reacted with an emoji on a message. The MessageReactions endpoint is paginated (with before and after) parameters. This PR opts to abstract that out for the user by looping internally until all users are fetched.

To prevent abuse getAllMessageReactions is limited to one use per command, and is further limited to fetching 5000 users at maximum (which corresponds to 50 API calls.)

Tested to the best of my ability on a self-host, but I sadly don't have > 100 people to test with so it's possible the inner loop has bugs.

Some things to consider:

  • Should this function be named getAllMessageReactions? It aligns with the API endpoint but I'm afraid the name may suggest that it fetches all message reactions (i.e. emojis.) Should we be more clear and name it something like getAllReactedUsers, or is this not a concern?
  • Do the limits here make sense? Presently, getAllMessageReactions is limited to one use per command and counts as one API call. The problem is that -- being a paginated endpoint -- an unknown number of API requests is needed to fetch all reactions. The implementation here makes 50 API calls at max which are not counted toward the total API call counter, to keep the maximum number of reactions returned constant thereby allowing for more reliable error handling within custom commands. Is this behaviour acceptable? (Read the comments below for further context & discussion.)

jo3-l avatar Dec 24 '21 19:12 jo3-l

Shouldn't the function IncreaseCheckGenericAPICall() be called everytime an API call is made? Instead of only once at the beginning. Perhaps check that inside the loop and if it hits the limit return what has been fetched thus far and stop the calls?

phenpessoa avatar Dec 24 '21 19:12 phenpessoa

Re: calling IncreaseCheckGenericAPICall per-iteration, I believe the current approach is preferable as it provides a fixed limit on how many reactions can be fetched, whereas your suggestion would make it dynamic. Having the limit be fixed and not dependent on how many API calls have been made previously allows users to check whether calling getAllMessageReactions would hit the limit by simply checking the reaction count and output a user-friendly error in such a case, whereas a dynamic limit would make robust error detection much more difficult.

I'm willing to be convinced otherwise, though. What do you think?

jo3-l avatar Dec 24 '21 20:12 jo3-l

I see your point and I agree with it. But I think it is fair to bring this to the dev's attention, reason being API Calls on CC's are hard capped to 100. With this function users could potentially be doing 150 API Calls per CC, a 50% increase from the previous limit.

If this is ok for YAG or no, I'm not the one to tell. But, again, I agree with you here, and I think the way you handled IncreaseCheckGenericAPICall is the best for the function per se, but I don't know if it is the best for "CCs context" as a whole or not.

phenpessoa avatar Dec 24 '21 20:12 phenpessoa

I think your points make sense (but I'm still not convinced either way, unfortunately :P)

Gonna mark this as draft to make sure it isn't merged for now & wait for some other opinions.

jo3-l avatar Dec 24 '21 21:12 jo3-l