-
Notifications
You must be signed in to change notification settings - Fork 18
Add animals plugin, with cats and dogs command #91
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
base: main
Are you sure you want to change the base?
Conversation
plugins/animals.py
Outdated
async def animals_config(ctx: Context, cat_api_key: Optional[str], dog_api_key: Optional[str]) -> None: | ||
''' Configures the animals plugin ''' | ||
if cat_api_key: | ||
cat_api.configure(api_key=cat_api_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This .configure
call doesn't persist in any way, meaning if the bot is restarted it will lose the API key.
plugins/animals.py
Outdated
if dog_api_key: | ||
dog_api.configure(api_key=dog_api_key) | ||
|
||
await ctx.send(f'Animals plugin configuration:\n**Cat**\n{cat_api.get_configuration()}\n**Dog**\n{dog_api.get_configuration()}') No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will leak the API key, I would be more careful with it. E.g.
Lines 422 to 435 in 5cc74bd
@config.command("submit_token") | |
@privileged | |
async def config_submit_token(ctx: Context, submit_token: Optional[str]) -> None: | |
global conf | |
async with sessionmaker() as session: | |
c = await session.get(GlobalConfig, 0) | |
assert c | |
if submit_token is None: | |
await ctx.send("None" if c.submit_token is None else format("{!i}", conf.submit_token)) | |
else: | |
c.submit_token = None if submit_token == "None" else submit_token | |
await session.commit() | |
conf = c | |
await ctx.send("\u2705") |
.config phish submit_token
plugins/animals.py
Outdated
allowed, message = limiter.is_allowed(ctx) | ||
if not allowed: | ||
raise UserError(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very golang idiom, returning a tuple[bool, str]
. The problem here is that if the bool
is True
, then the str
part is unused, and you have to come up with a dummy value to put there (empty string in your case).
I would prefer if limiter
returned a Union[str, None]
indicating either a problem or a lack of problem. Or maybe even if limiter
raised UserError
itself.
plugins/animals.py
Outdated
limit: Optional[int] = None | ||
page: Optional[int] = None | ||
order: Literal['ASC', 'DESC', 'RAND', None] = None | ||
has_breeds: Optional[bool] = None | ||
breed_ids: Optional[list[str]] = None | ||
#sub_id: Optional[str] | ||
|
||
def __post_init__(self): | ||
try: | ||
assert self.limit is None or 1 <= self.limit <= 100 | ||
assert self.page is None or 0 <= self.page | ||
except AssertionError: | ||
raise ValueError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lotsa unused here. I only see this class initialized once as CatRequest()
. Also the API has some built-in defaults, e.g. limit
defaults to 1
if unspecified, which is something I was worried about. Would be good to know this information.
I'm guessing it's also untested. I don't think has_breeds
can be a bool
, see https://yarl.aio-libs.org/en/latest/#why-isn-t-boolean-supported-by-the-url-query-api . Also the API recommends using the values 1
and 0
.
Also per my reading of the docs, the cat API and the dog API are identical, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does actually convert the has_breeds
bool to an integer properly, see to_dict. I expected parameter passing to be simpler than it was (.cat --breed lynx
wants to give me breed="--breed lynx"
), which is why I built all of this infrastructure. Getting rid of it does seem reasonable, though.
Annoyingly, the cat and dog APIs are almost identical, except that the dog API does not support breed_ids
, and they have some slight differences on the returned data.
plugins/animals.py
Outdated
def _is_allowed(self, user_id: int) -> tuple[bool, str]: | ||
now = dt.datetime.now() | ||
timestamps = self.users[user_id] | ||
# Remove timestamps outside the period | ||
while timestamps and now - timestamps[0] > self.period: | ||
timestamps.pop(0) | ||
if len(timestamps) < self.calls: | ||
timestamps.append(now) | ||
return True, '' | ||
else: | ||
next_time = timestamps[0] + self.period | ||
return False, f'Rate limit exceeded: {self.calls} calls per {self.period}. Please try again <t:{int(next_time.timestamp())}:R>.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ratelimiting is a hard problem, and this implementation is a good example of why.
You're storing for each user that has ever interacted with these commands a list of timestamps of when they last interacted. The list for a specific user is only truncated when that specific user interacts again, but otherwise will remain fixed (and occupy memory) until the bot is restarted. The memory usage of this algorithm is thus essentially unbounded.
All while we actually know that at most 14400 requests could have been made in a day, and requests made more than a day ago have no effect.
So in a sense this ratelimiter is actually making the system worse than it would've been without it. Ratelimiters are supposed to make a system better in presence of adversaries, so they have to be very well designed in terms of their own resource usages.
We could discuss and design a better algorithm, but I think implementing a good ratelimiter is outside the scope of this module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so. To me that doesn't seem like that much data, but I haven't actually run any numbers on it. Mainly I'm trying to be a good citizen and not smoke the API in case someone starts scripting or whatever, as well provide some semblance of fairness and DoS protection.
I'd like to see a rate limiter be command independent, similar to the ACL system.
plugins/animals.py
Outdated
allowed, message = limiter.is_allowed(ctx) | ||
if not allowed: | ||
raise UserError(message) | ||
async with aiohttp.ClientSession() as session: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't create a ClientSession
on every request. E.g. phish
uses a module global with a finalizer:
Lines 97 to 98 in 5cc74bd
http: aiohttp.ClientSession = aiohttp.ClientSession() | |
plugins.finalizer(http.close) |
plugins/animals.py
Outdated
return out | ||
|
||
async def fetch_random_animal(self, ctx: Context, params: dict[str, Any]) -> AnimalResponse: | ||
logging.debug(f'Fetching random animal with params: {params}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a separate logger with logging.getLogger(__name__)
so that the logs would be attributed to this module specifically.
plugins/animals.py
Outdated
def with_suffix(obj: dict[str, Any], *path: str, suffix: str) -> Optional[str]: | ||
val = obj | ||
try: | ||
for p in path: | ||
val = val[p] | ||
except (KeyError, TypeError): | ||
return None | ||
if val is not None: | ||
return f'{val}{suffix}' | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes very little sense as a function. I understand you're trying to avoid manually handling None
s, but this isn't the way to do it.
plugins/animals.py
Outdated
out += f'Rate Limiter: {limiter}\n' | ||
return out | ||
|
||
async def fetch_random_animal(self, ctx: Context, params: dict[str, Any]) -> AnimalResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange separation of responsibilities here.
Ultimately we want to take the user input (which in our case is "I want a random cat" -- with no other parameters -- an empty tuple, zero bits of information), and turn it into a discord Embed for display (because that's the cutoff point where discord.py takes over). We do this by talking to a third party API, which accepts an HTTP request of a particular format, and returns an HTTP response of a particular format.
Conceptually I can identify 3 steps here: convert user input into HTTP request, perform HTTP, convert HTTP response into discord Embed. We "perform HTTP" using aiohttp
, which dictates what constitutes "HTTP request" and "HTTP response" -- aiohttp
's types. What we are left with are the conversion from user input to HTTP request, and the conversion from HTTP response to discord Embed.
Now these conversion procedures can be broken up in various ways because they consist of multiple steps. To name a few: convert to/from JSON, specify/extract correct field names, specify the correct server and API key. This function seems to include the responsibility of fully parsing the reponse JSON and extracting the correct field names from it (by the virtue of calling AnimalResponse(...)
), but does not include the responsiblity of specifying the correct query string field names (by the virtue of accepting params: dict[str, Any]
rather than specific parameters, or a dataclass). I find this very strange.
Considering how simple our use case is -- we only make one kind of request (random, limit=1, no filters), I would probably hardcode the request, or at most, have a simple function that returns params: dict[str, str]
. Converting JSON to Embed could probably also just be a simple function.
A CatRequest
dataclass makes sense, provided we understand what its single responsiblity is, be it simply "specify correct field names" or "always be convertible to a valid request". AnimalResponse
likewise, but yours for example seems to mix the responsibilities of validating correct field names (but only at the root), and extracting data in a format specific to the Embed we're formatting.
No database changes. Someone will need to generate an API key for the cats and dogs APIs. Adds commands
.cat
and.dog
, as well as.config animals
.