redis.connection.ConnectionPool.from_url should raise an exception when passed an obviously bad URL value

[lddubeau]

I'm using version 2.10.5 of redis-py.

The documentation of from_url() is pretty clear:

    Three URL schemes are supported:
        redis:// creates a normal TCP socket connection
        rediss:// creates a SSL wrapped TCP socket connection
        unix:// creates a Unix Domain Socket connection

There are only 3 schemes supported, so anything falling outside of these schemes should result in an immediate error. However, as of 2.10.5, if passed schemes that it does not understand, it will silently misinterpret them. For instance, passing "/dev/null" will result in a client connecting to the redis instance running on localhost at port 6379. Same with passing "uni:/dev/null" (this is a case simulating a typo in the scheme). If I pass "unix:/dev/null", then I will get a connection error because the URL will be interpreted correctly. Here's a session illustrating what I'm taking about:

>>> from redis.client import StrictRedis
>>> c = StrictRedis.from_url("/dev/null")
>>> c
StrictRedis<ConnectionPool<Connection<host=None,port=6379,db=0>>>
>>> c.echo("foo")
'foo'
>>> c = StrictRedis.from_url("uni:/dev/null")
>>> c.echo("foo")
'foo'
>>> c = StrictRedis.from_url("unix:/dev/null")
>>> c.echo("foo")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ldd/Documents/mangalam/btw/software/btw_env/local/lib/python2.7/site-packages/redis/client.py", line 644, in echo
    return self.execute_command('ECHO', value)
  File "/home/ldd/Documents/mangalam/btw/software/btw_env/local/lib/python2.7/site-packages/redis/client.py", line 578, in execute_command
    connection.send_command(*args)
  File "/home/ldd/Documents/mangalam/btw/software/btw_env/local/lib/python2.7/site-packages/redis/connection.py", line 563, in send_command
    self.send_packed_command(self.pack_command(*args))
  File "/home/ldd/Documents/mangalam/btw/software/btw_env/local/lib/python2.7/site-packages/redis/connection.py", line 538, in send_packed_command
    self.connect()
  File "/home/ldd/Documents/mangalam/btw/software/btw_env/local/lib/python2.7/site-packages/redis/connection.py", line 442, in connect
    raise ConnectionError(self._error_message(e))
redis.exceptions.ConnectionError: Error 111 connecting to unix socket: /dev/null. Connection refused.

Note that amending the URLs in the illustration above to add the double forward slash that should appear under the scheme (so that they become "uni:///dev/null" and "unix:///dev/null" ) changes nothing except what socket path is reported in the exception in the last case.

[obi1kenobi]

Possibly related:

In [2]: StrictRedis.from_url('redis://localhost:637912')
Out[2]: StrictRedis<ConnectionPool<Connection<host=localhost,port=6379,db=0>>>

Note that the port is set to the (likely) invalid value 637912, and is silently ignored when parsing.

[depaolim]

@obi1kenobi that is an urlparse issue

Python 2.7.6 (default, Jun 22 2015, 17:58:13)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from urlparse import urlparse
>>> u = urlparse('redis://localhost:6380')
>>> u.port
6380
>>> u = urlparse('redis://localhost:63801')
>>> u.port
63801
>>> u = urlparse('redis://localhost:638012')
>>> u.port
>>> u.port is None
True

[depaolim]

@lddubeau at the moment any scheme different from "unix" is interpreted as if it was "redis" (or "rediss")
What should we do?
Personally I would prefer a mandatory scheme, as stated by the documentation: "unix", "redis" and "rediss" (or any other available in the future)
But if we implement it, this will result in a backward incompatible change for any client out there which assumes this undocumented feature...

[depaolim]

some test cases to snapshot of current behavior, they can be modified to reflect the new behavior

[obi1kenobi]

@depaolim I believe that both of these things, while they may be underlying issues of urlparse or the control flow structure, are bugs. redis-py can always be written to verify the result it receives from urlparse, and I think that errors should be raised in both cases.

Since the two issues would create backward incompatibility as you said, I think they should be fixed in a new minor version, perhaps 2.11.0

[depaolim]

I have some news on issue regarding the very big port number

In the future releases Python will raise exception

https://github.com/python/cpython/blob/master/Lib/urllib/parse.py#L159

I think this is really better than the current situation:

https://github.com/python/cpython/blob/288becf5563483ae9da14c6fb5deb4f32e4cd3d5/Lib/urllib/parse.py#L160

Here the reasons of the choice
https://bugs.python.org/issue14036

[depaolim]

an implementation proposal

I decided to throw ValueError in case of invalid or missing schema

I considered also: RedisError, ConnectionError, AuthenticationError but I think that the ValueError definition ("Raised when a built-in operation or function receives an argument that has the right type but an inappropriate value") fits the case

I think that the port number problem is not a redis-py business, but a python business (as the https://bugs.python.org/issue14036 states)

[pawelad]

Yup, just bumped into this:

In[2]: from redis import Redis
In[3]: Redis.from_url('redis://127.0.0.1:6379/0/foobar')
Out[3]: Redis<ConnectionPool<Connection<host=127.0.0.1,port=6379,db=0>>>
In[4]: Redis.from_url('redis://127.0.0.1:6379/0')
Out[4]: Redis<ConnectionPool<Connection<host=127.0.0.1,port=6379,db=0>>>
In[5]: # Both `Redis` and `ConnectionPool` classes don't implement `__eq__`...
In[6]: _3.connection_pool.connection_kwargs
Out[6]: {'db': 0, 'host': '127.0.0.1', 'password': None, 'port': 6379}
In[7]: _4.connection_pool.connection_kwargs
Out[7]: {'db': 0, 'host': '127.0.0.1', 'password': None, 'port': 6379}
In[8]: _6 == _7
Out[8]: True
In[9]: # And even worse... (notice the DB number)
In[10]: Redis.from_url('redis://127.0.0.1:6379/1/foobar')
Out[10]: Redis<ConnectionPool<Connection<host=127.0.0.1,port=6379,db=0>>>
In[11]: _10.connection_pool.connection_kwargs
Out[11]: {'db': 0, 'host': '127.0.0.1', 'password': None, 'port': 6379}
In[12]: _6 == _11
Out[12]: True

So long story short - both redis://127.0.0.1:6379/0/foobar and redis://127.0.0.1:6379/1/foobar point to the same DB, which is... non ideal.

I'd be happy to look into it and possibly do a pull request if there's a consensus of how to handle this.