Set RedisModel.Meta.database at runtime, not import time

[dansan]

Often the software I write does not know the configuration at import time. It has to load it from multiple sources, as it is executed in different environments and with different (CLI) options.

Generally, It's best practice to not open database or network connections (or do any I/O) at import time.

Having to set the database connection at import time also makes testing harder. Things have to be patched before importing any dependency that might import the module with the RedisModel.

It would be more convenient if the database connection could be set at runtime... e.g. MyModel.Meta.database = ... when my software has finished collecting the configuration and other setup steps.

This does currently not work, because the creation of the class through the metaclass stores the value somewhere I cannot change it later.

Alternatively (and maybe less work to implement with the current code - not that I have looked at it) would be to allow the value of RedisModel.Meta.database to be a callable (type Callable[[], Redis]). That could be provided at import time without having to know the database connection yet, and would not even need patching in tests (as the test can just create a testing configuration) - it's dependency injection.

The built-in callable() can be used to test, if the value in MyModel.Meta.database is a callable. Luckily a Redis connection pool object is not a callable.

[belyak]

It seems that the problem you described causes this issue: #527
And in the case described in the issue, I was able to overwrite the database using MyModel._meta.database = <newvalue>

[dansan]

Thanks. Your solution works for me. I am using now:

@classmethod
def db(cls):
    if not cls._meta.database:
        cls._meta.database = get_redis_connection(url=db_url)
    return super().db()

Update: It does not work. It just seemed to work on my notebook, as Redis is running there at localhost. But when used in production, it still tries to connect to localhost, instead of db_url.

This happens, because Migrator().run() calls detect_migrations() and that already has conn = Redis<ConnectionPool<Connection<host=localhost,port=6379,db=0>>>.
I don't understand where it gets that from.
...oh I see: the metaclass sets _meta.database´ to get_redis_connection()at import time. So my code never gets into thenot cls._meta.database` branch...

I wanted to use cls._meta.database to make sure the connection is not opened multiple times. I modified this now using a separate var and it works:

class RedisUser(JsonModel):
    fields...

    _conn: Optional[Redis] = None

    @classmethod
    def db(cls):
        if not cls._conn:
            cls._conn = get_redis_connection(url=db_url)
        cls._meta.database = cls._conn
        return super().db()

[svabra]

@belyak @dansan

You can set the REDIS_OM_URL = os.environ.get("REDIS_OM_URL") url

and then use it in the Meta class of your model.

class Meta:
        model_key_prefix="RedisUser"
        database = get_redis_connection(url=REDIS_OM_URL, decode_responses=True)

By default, not supplying the REDIS_OM_URL and not setting the database, will fallback to 6379 and localhost. That is where it has the connection string from. Of course, your workaround works too.

However, the point you are making is still valid. It should not try to connect before one needs the connection. There should be a lazy connection by default and eager as an option one needs to set deliberately. If redis is not available, the application will die during the import of all the models. I suggest this requires an architectural and code change.

[dansan]

@svabra That is not what's happening.
The connection is opened lazily. It's just not configured lazily:

$ docker stop redis-stack
$ python
>>> from myapp import RedisUser
>>> # no connection attempt
>>> RedisUser.find().first()
redis.exceptions.ConnectionError: Error 111 connecting to 127.0.0.1:6379. Connection refused.

>>> from myapp import RedisUser
>>> RedisUser.Meta.database = get_redis_connection(url="redis://10.20.30.40:6379")
>>> RedisUser._meta.database = get_redis_connection(url="redis://10.20.30.40:6379")
>>> RedisUser.find().first()
redis.exceptions.ConnectionError: Error 111 connecting to 127.0.0.1:6379. Connection refused.

The "redis://10.20.30.40:6379" cannot be set after the class RedisUser has been created by the metaclass.

[svabra]

@dansan Well, it is clearly attempting to open when importing a JsonModel in one's code. It requires no instantiation to that. As you pointed out, it becomes significantly quirkier to tested. Or what do you mean by it is lazy "opening"? I ask differently: has anyone written a test that proves it is NOT attempting to connect while importing? I gladly see the results to that. Thanks for sharing.

(Yes, I do understand that the connection does not seem to be modifiable after being set once. Another issue.). Also, I really would love to hear the reasoning of the author of that. There must have been a reason to tightly-couple model and connection/server.

[XChikuX]

It's an experimental library. The reason might be that, getting the library to work was of higher importance than getting all the nitty gritty details right.

Lazy connections seem like a better option. Hopefully this will be considered in future updates.

[dansan]

I added a PR that implements lazy DB connection configuration: When MyRedisModel.Meta.database is a function object, it is executed and its return value is returned.

def my_connection():
    return Redis(**get_db_config())


class RedisUser(JsonModel):
    fields...

    class Meta:
        database = my_connection

my_connection() will be executed only when the connection is needed (e.g. obj.save() or Migrator.run()).

I added a few tests. They might be in the wrong file or their style is not how you would do it. Feel free to fix it. The PR is meant as a start to find a solution. It's probably not perfect at all :)

[XChikuX]

@dansan I recommend pushing for review on the discord channel. @chayim Seems really busy and will need extra pokes from someone internally.

[skewty]

Did the discussion go anywhere? It has been over a year and async support is more-or-less still broken because of this.

[zidokobik]

Been bugging me for days. Hopefully get fixed soon.