Refactor the ratelimiting behavior out of the request handlers.

[shtlrs]

As I was browsing the code, I noticed that all get and post handlers had to check for ratelimits before handling the request.

This had been ported over to a ratelimit decorator that we use to wrap any http verb's callback function with the ratelimiting behavior.

I initially implemented this as a Mixin, but wookie suggested that a per-handler decorator would be better, which makes sense, so it has been changed to that.

Thanks @wookie184 for the alternative approach idea :)

[supakeen]

This one needs a bit of review; I'll read it when I have some spare time :)

[wookie184]

IMO a cleaner approach could be a per method decorator, as I think it makes more sense for ratelimits to be per method. Then you would have something like this:

class Lexer(Base):
    @ratelimit_endpoint(area="read")
    async def get(self) -> None:
        self.write(utility.list_languages())

I also think that if the change is made here it should be applied to website.py for consistency. Not sure about api_curl or api_deprecated, they could probably be left as is.

[shtlrs]

@wookie184 Good point, I'll make it a decorator instead of a mixin.

[shtlrs]

IMO a cleaner approach could be a per method decorator, as I think it makes more sense for ratelimits to be per method. Then you would have something like this:

class Lexer(Base):
    @ratelimit_endpoint(area="read")
    async def get(self) -> None:
        self.write(utility.list_languages())

Redid the commits to use a decorator instead of a mixin.

I also think that if the change is made here it should be applied to website.py for consistency. Not sure about api_curl or api_deprecated, they could probably be left as is.

I ended up applying it all over.

[wookie184]

Since it's guaranteed that the first argument will always be self which is the request handler, does this work?

Suggested change

	@wraps(func)
	def inner(*args, **kwargs):
	# We transform the args into a dict because ChainMap works differently starting python 3.9
	# In previous versions, it expects all maps to be dictionaries, whereas starting py3.9
	# It transforms list/tuples into a dict whose keys are the different elements.
	arguments = ChainMap(dict.fromkeys(args), kwargs.values())
	request_handler = None
	for arg in arguments:
	if isinstance(arg, RequestHandler):
	request_handler = arg
	break
	if should_be_ratelimited(request_handler.request, area):
	raise error.RatelimitError()
	return func(*args, **kwargs)
	@wraps(func)
	def inner(self, *args, **kwargs):
	if should_be_ratelimited(self.request, area):
	raise error.RatelimitError()
	return func(self, *args, **kwargs)

[shtlrs]

I first had it like that, by precising that the first argument is instance of RequestHandler, but then some tests failed because it got other unexpected arguments, so i just i thought I'd make it generic.

I prefer the explicitness of the parameter as well, so a combination like the one you're suggesting should work.

I'll try that out

[shtlrs]

Fixed in the forcepush 002b21f

[wookie184]

Looking good, one thing.

[wookie184]

Tested and works nicely. Seems like a good improvement to me

[supakeen]

Awesome stuff. Let's get the conflicts resolved and merge :)

[shtlrs]

Awesome stuff. Let's get the conflicts resolved and merge :)

Rebased the commits, so this is done :D

[supakeen]

@shtlrs Awesome!!