Fix CI and Update Tested Versions

[Andrew-Chen-Wang]

Fixes #241 as well

• CI testing Django 3.2 would actually only test Django 3.2 because GitHub actions converted 3.0 to an integer and tax didn't read it correctly (testing on Django 3.2a according to the logs)
• Updated Python and Django versions
• Tested all 3 memcache backends

[mahyard]

hi @jsocol
you may know that this interesting plugin is being used by the Open edX platform for rate-limiting user requests.
since the platform is about to be upgraded to utilize Django 3.2, I volunteered to make sure that django-ratelimit supports that version of django. I'm ready to contribute either to test or debug the code.
edx/upgrades#41

[Andrew-Chen-Wang]

@mahyard I will finish updating this PR sometime in the next few hours for CI configuration; any bad tests will then be up to you and I'll add you as a contributor to the fork. (Sorry, just completely forgot about this PR 😅 )

[Andrew-Chen-Wang]

@mahyard Django 3.2 is fully supported; no tests failed: https://github.com/Andrew-Chen-Wang/django-ratelimit/actions/runs/1163763411

[jsocol]

Thank you @mahyard for opening this and @Andrew-Chen-Wang for doing the polishing! Not the first time I've been caught by "x.0 gets parsed as the number x" 🤦

[mahyard]

Thank you @Andrew-Chen-Wang for your efforts on this commit
I think django-redis needs to be upgraded too. 4.10.0 doesn't support Django:: 3.
I believe that 5.0.0 could be your first option. I've already run tox successfully omitting python 3.9 and pypy36.

  py36-django22: commands succeeded
  py36-django31: commands succeeded
  py36-django32: commands succeeded
  py37-django22: commands succeeded
  py37-django31: commands succeeded
  py37-django32: commands succeeded
  py38-django22: commands succeeded
  py38-django31: commands succeeded
  py38-django32: commands succeeded
  py38-djangomain: commands succeeded
  congratulations :)

[mahyard]

I just found that CI tests pick the latest version of django-redis.
it should be the option in the tox.ini as well.

[Andrew-Chen-Wang]

👍 One more thing to note. Django has deprecated python-memcached in favor of a the pylibmc. @jsocol do you want me to test both packages as seen here https://github.com/noripyt/django-cachalot or would the replacement of python-memcached with pylibmc suffice? Imo, replacing is good enough to me (the linked repo tests several possible cache and db backend, but this package prob doesn't need it).

ref:

• test settings if to support both memcached backends https://github.com/noripyt/django-cachalot/blob/c8d6af575ad27d34f801105d192181f4072d9740/settings.py#L48-L53
• testing two packages is possible, but imo also unnecessary. Using PyLibMC backend suffices.

[jsocol]

👍 One more thing to note. Django has deprecated python-memcached in favor of a the pylibmc. @jsocol do you want me to test both packages as seen here https://github.com/noripyt/django-cachalot or would the replacement of python-memcached with pylibmc suffice? Imo, replacing is good enough to me (the linked repo tests several possible cache and db backend, but this package prob doesn't need it).

Good to know! It's been a minute but is pylibmc the one that's a C-based extension, or am I confusing that with something else?

Basically, if there's a compelling reason people might have to continue running python-memcached (e.g. they are in a deployment where pylibmc is unavailable for a reason like C extensions aren't present) then I'd say both. But if there's no reason people would need to keep using python-memcached, then I think it's fine to just go with the new and shiny.

[Andrew-Chen-Wang]

You can take a look at a comparison made by Pinterest here: https://github.com/pinterest/pymemcache#comparison-with-other-libraries

In other words, we'll be testing on two backends: pylibmc (yes you're guess was right; it's the C bindings one) and pymemcache, the true replacement library for python-memcached. pymemcache is a suitable replacement since it's written entirely in Python. Django has it natively supported via a cache backend but only in Django 3.2+.

So to me, the reason to keep testing python-memcached is for supporting Django 2.2 and 3.1. Deprecation timeline: https://www.djangoproject.com/download/ I guess I'll test on all three? There will prob be a lot of jobs being queued since we're tripling everything.

[Andrew-Chen-Wang]

All memcache backends are now tested. pymemcache having some issues. Seems like it only doubled due to the replacement of a backend rather than an addition.

Not sure what to make of this. Using the real uri fails python-memcached but fake one doesn't. It seems like pymemcache and pylibmc expect valid connections? Idk how to resolve tbh:

• https://github.com/Andrew-Chen-Wang/django-ratelimit/actions/runs/1164577891
• https://github.com/Andrew-Chen-Wang/django-ratelimit/actions/runs/1164588724

[Andrew-Chen-Wang]

@jsocol Memcached is failing on pymemcache (python-memcached's replacement) and pylibmc. A solution for now is that memcached support for the CacheFail tests are simply ignored if not using python-memcached since we don't know how users will configure their cache server failover strategy.

[Andrew-Chen-Wang]

This was intentionally set to 11211 so that the connection would fail by setting a LOCATION of a random free client port. Please revert the last two commits.

[mahyard]

my bad! all of yesterday I was doing nothing. I thought... never mind. I will revert it right now.

[mahyard]

@Andrew-Chen-Wang
both pylibmc and pymemcache throw an error on this line:

added = cache.add(cache_key, initial_value, period + EXPIRATION_FUDGE)

what if we put it in a try-except block to check the connection and decide whether/what we need to return?
I mean sth like this:

>>> from pylibmc import ServerDown, ConnectionError
>>> try:
...   added = cache.add(cache_key, initial_value, period + EXPIRATION_FUDGE)
... except (ServerDown, ConnectionError):
...   #here we can handle connection error of pylibmc

[mahyard]

Hi @Andrew-Chen-Wang
How are you doing? Did you see my suggestion?

[Andrew-Chen-Wang]

@mahyard apologies. Beginning of school. Will have much less time.

Based on my past PRs, having a direct dependency was a no-no. Though we could do a broad exception and then have the variable set in the except clause. I'm not sure when else the Django cache system will raise an exception besides configuration problems.

[jsocol]

Yeah we can't assume pylibmc is always installed, and I don't like implicitly changing the behavior based on the presence of a library (you could easily configure ratelimit to use a cache based on a different django cache backend). However, I also suspect that we could probably do something around the added = cache.add similar to what we do around count = cache.incr a few lines below.

Taking a step back here, though, I think we might get further by breaking apart a few steps here.

Currently we're splitting breaking out tests based on the memcached client, but all of them are still running the redis tests—that's a sign that maybe we should rethink this a bit. Maybe a first step is to think about how to parametrize the tests across "cache backend" instead of across "memcached client". Then add pylibmc, since that has been around for a while. Then add pymemcache, since that's new. And keep python-memcached for now, both because it's deprecated but not removed yet in 3.2, and because it's the closest thing to a reference implementation here.

So before we go too far down this road, I'm going to look at step 1 and rethink how we parametrize cache backend. I'd like to know for sure that the current tests that pass with python-memcached also, for example, all pass with django-redis—I'm not 100% sure they actually do because they don't run that way.

[jsocol]

I did two parts of this separately in #248 and #249 so we could get to a good starting point. I'm leaving some more commentary on #241, but I need to think on it a bit. Input would be super welcome.