Issue #1 wontfix

The selected database shouldn't be set by the adapter

Gustavo Narea
created an issue

Hi, Jonás.

I've been reviewing the repoze.what Redis plugin and the Python interface for Redis, and I believe the former shouldn't select the database.

What would happen if a user wants to reuse the Redis db object? It'd be pretty common if they use a custom host, port and/or timeout.

But if they use a custom object, and want to reuse it in the groups and permissions adapter, this will lead to a bug because both adapters will use the same database.

One solution could be not to select the database in the constructor, but select it explicitly right before a write operation (e.g., item addition, section removal). For this to work, you'd have to store the database name in the adapter, instead of select()'ing it with the db_connection.


Comments (4)

  1. Jonas mg repo owner

    Hi Gustavo,

    let's say that I have a Redis db object and an adapter as:

    import redis
    from repoze.what.plugins.redis import adapters
    r = redis.Redis(db=0)  # using host, port by default
    group = adapters.RedisGroupAdapter(r)

    Now, the adapter has selected a new database (1). So if I were to use the Redis object (r), I'd use the db 1, instead of 0 (as it was set initially).

    But this same thing will happen when 'select' been run before a write operation of the adapter.


  2. Jonas mg repo owner
    • changed status to open

    Last night I was thinking on this issue. And the way that Redis lets to change the database from a Redis object will lead to bugs on this adapter.

    So, I only see two possible solutions:

    1. The Redis db object must be unique for the adapter. So the user could not reuse it and so to avoid that he could to select another database.

    2. --This depends on Redis's author--. The idea would be that would be an option to locking a database so the user could not to select another one.

    group_db = redis.Redis(db=1, lock=True)

    Gustavo, I'd want to listen you opinion before of implement the solution 1 or post about 2 in the Redis group.

  3. Gustavo Narea reporter

    Hi, Jonás!

    1.- In some situations, reusing the connection object is best, so I think this constraint shouldn't exist. I wouldn't like to do:

    groups = RedisGroupAdapter(Redis(host="example.local", port=6380, timeout=60))
    permissions = RedisPermissionAdapter(Redis(host="example.local", port=6380, timeout=60))

    if I could do:

    conn  = Redis(host="example.local", port=6380, timeout=60)
    groups = RedisGroupAdapter(conn)
    permissions = RedisPermissionAdapter(conn)

    Also, what if I'm already using Redis for something else and thus I already have connection open; I'd prefer to reuse that object.

    Actually there's a good reason to reuse these objects: Each Redis object creates a new socket to the database server... So when people use this plugin, they'll have 2 sockets open just to retrieve the groups and permissions.

    2.- Still developers wouldn't be able to reuse the connection.

    I've just taken another look into the "redis" package and I think this is something we can't or shouldn't try to fix in this plugin... The problem is on their package, which has a serious design problem: Their Redis class is a horrible all-in-one monster:

    The Redis class should have been split into two classes, for example, RedisConnection and RedisDatabase:

    • RedisConnection: Manages the connection (i.e., sockets), nothing else. This will contain the methods that are currently known as: .connect(), .disconnect(), .flush(), etc.
    • RedisDatabase: Performs the actual operations in the database, as a wrapper for RedisConnection. This will contain the methods that are currently known as: .sadd(), .srem(), etc.

    So its usage will be as follows:

    conn = RedisConnection(host="example.local", ...)
    db1 = conn.use_db(1)
    db2 = conn.use_db(2)
    db1.sadd('foo', 'bar')
    db2.sadd('bar', 'foo')

    This is, they would've been defined as:

    class RedisConnection(object):
        def use_db(self, db):
            return RedisDatabase(self, db)
        # ...
    class RedisDatabase(object):
        # ...
        def __init__(self, connection, db):
            self.connection = connection
            self.db = db
        def sadd(self, name, value):
                value = value if isinstance(value, basestring) else str(value)
                self._write('SADD %s %s\r\n%s\r\n' % (
                    name, len(value), value
            except UnicodeEncodeError, e:
                raise InvalidData("Error encoding unicode value for element in set '%s': %s." % (name, e))
            return self.connection.get_response()
        # ...

    This way, developers would only have to focus on dealing with RedisDatabase objects, with whatever connection the user used. So your repoze.what adapters would look like this:

    class _RedisAdapter(BaseSourceAdapter):
        def __init__(self, db_connection, db_name, *args, **kwargs):
          super(_RedisAdapter, self).__init__(*args, **kwargs)
          self.db = db_connection.use_db(db_name)
    # OR:
    class _RedisAdapter(BaseSourceAdapter):
        def __init__(self, db, *args, **kwargs):
          super(_RedisAdapter, self).__init__(*args, **kwargs)
          self.db = db

    And we wouldn't have to care about what's the selected database.

    I'd try to get this bug fixed in Redis. Not only us, but also many people would benefit from this.


  4. Log in to comment