Align FakeConnection constructor signature to base class

[wandering-tales]

As FakeConnection is a subclass of the Connection class in redis-py, in order to honor OOP substitutability principle, the signature of the constructor should be aligned with the one of the base class. That means it should contain at least all the parameters of the base class.

This automatically fixes a problem occurring when the connection object is dynamically instantiated within the context of the default connection pool class in redis-py, named ConnectionPool, and when, in turn, the latter is dynamically instantiated using its standard from_url factory method. In such scenario the instantiation of the connection object fails because the FakeConnection class misses both host and port parameters in its constructor signature.

That happens because the from_url factory method above, given a canonical Redis URL, guesses several connection parameters values, among which host, username, password, path, and db, which values override any other corresponding value passed to the factory method itself via the connection variadic arguments.

Aligning the FakeConnection constructor signature to the one of its base class, by including the host and port parameters, is helpful in contexts where there's no direct control over the instantiantiation of Redis clients, connections and connection pools objects (e.g. django-redis library): whilst preserving current behavior, this avoids to workaround the issue by defining either custom connection factories or custom connection pools.

Closes #234

[wandering-tales]

My actual workaround to use fakeredis to mock a Redis instance used as a cache backend via django-redis library in a Django project is the following:

import django.core.cache

from django.test import TestCase, override_settings

import fakeredis
import mock

_caches = {
    'default': {
        'BACKEND': 'django_redis.cache.RedisCache',
        'LOCATION': 'redis://fake-redis/0',
        'OPTIONS': {
            'CLIENT_CLASS': 'django_redis.client.DefaultClient',
        }
    }
}

@override_settings(CACHES=_caches)
@mock.patch("django_redis.pool.ConnectionFactory.get_connection",
            new=mock.MagicMock(return_value=fakeredis.FakeRedis()))
class TestCache(TestCase):
    def test_set_unicode_key_redis_cache(self):
        try:
            django.core.cache.caches['default'].set('unicode-key-€', 'fake_value')
        except UnicodeEncodeError:
            self.fail("`RedisCache.set` raised "
                      "UnicodeEncodeError unexpectedly")

[bmerry]

Thanks. This seems reasonable, and I think using the base class __init__ should help make it more robust.

Travis is showing a flake8 error that is preventing the rest of the unit tests from running. I think what is likely to happen is that it will fail on older versions of redis-py that didn't have all the arguments that the current Connection class takes. It may be possible to fix that by having the constructor take *args, **kwargs instead of named arguments.

[wandering-tales]

@bmerry Thanks to you for your quick reply.

I checked all the Travis CI builds and indeed the flake8 commands failed, but apparently for another reason:

$ flake8
./fakeredis/_server.py:2:1: F401 'os' imported but unused
The command "flake8" failed and exited with 1 during .

This is caused by the fact that I removed some code in the __init__ that was already included in the related redis-py baseclass.

I removed that unused import. Let's see.

[bmerry]

It looks like there are now some failures of the sort I expected, but also these:

E           assert WrappedException(ResponseError("wrong number of arguments for 'exec' command")) == WrappedException(ExecAbortError("Transaction discarded because of: wrong number of arguments for 'exec' command"))

That seems to be some bug on master that for some reason hasn't appeared before, so don't worry about those.

[wandering-tales]

You're right. Tests actually fails for redis-py versions lower than 3.3.11.

I followed your suggestion to use variadic arguments. I think it's a safer way to guarantee compatibility with multiple redis-py interfaces.

[wandering-tales]

@bmerry Excluding the test you mentioned, now only one the CI job using redis-py 2.10.6 fails. Precisely, only the following test fails:

TestInitArgs.test_can_allow_extra_args

At first glance, it's clear that the purpose of that test is to ensure the FakeStrictRedis object can be instantiated with any type of arguments. Of course, now that the constructor of the base class redis.Connection is called, that is not possible anymore, as the parent object does not support variadic arguments. Nonetheless, I'm missing which particular usage scenario the test is trying to cover.

If I use fakeredis in a project where I use redis-py 2.10.6, I should be aware I cannot instantiate a FakeStrictRedis object with any parameter I want, as its interface is bound to that particular version of redis-py. What is the reason for allowing extra arguments to the client that are not supported by the signature of the base class, which signature depends on the specific version of redis-py? The only reason I can see is that in that project I need to be able to run code that uses fakeredis against multiple versions of redis-py. Am I missing something else?

[bmerry]

I've fixed up master.

That test is intended to ensure that arguments that can be passed to redis can also be passed to fakeredis, even though it doesn't use them. I think the simplest thing is just to add the @redis3_only decorator to it.

[wandering-tales]

I think the simplest thing is just to add the @redis3_only decorator to it.

That makes sense.

[coveralls]

Coverage decreased (-0.2%) to 96.0% when pulling 3bcb8fc on wandering-tales:master into f8e3df1 on jamesls:master.

[bmerry]

In future please don't rewrite history on a PR. It makes it impossible to see what changed since my previous review.

[wandering-tales]

In future please don't rewrite history on a PR. It makes it impossible to see what changed since my previous review.

Sorry, I won't do it again. I did because sometimes I found out that Travis CI do not automatically run Pull Request builds.

[bmerry]

Thanks for the contribution! Since you mentioned that this is for use with django,-redis, which seems to be a popular usage of fakeredis but not one I'm familiar with, would you be interested in making another PR to add instructions to the README?

[wandering-tales]

I sure can.

…

On Mon, Mar 15, 2021, 8:46 AM Bruce Merry ***@***.***> wrote: Thanks for the contribution! Since you mentioned that this is for use with django,-redis, which seems to be a popular usage of fakeredis but not one I'm familiar with, would you be interested in making another PR to add instructions to the README? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#293 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF4JFLSMHNCHDK5RVME5NELTDW3N5ANCNFSM4XAWQGVA> .