From cd316a94f1f410fa87664db6705bef3ccbe1f4e9 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Tue, 25 Sep 2012 23:46:46 +0100 Subject: [PATCH 1/3] Dropped quirks in connection arguments handling Now connect() raises an exception instead of swallowing keyword arguments when a connection string is specified as well Closes ticket #131. --- NEWS | 2 ++ lib/__init__.py | 52 ++++++++++++++++++++++---------------------- tests/test_module.py | 8 +++++++ 3 files changed, 36 insertions(+), 26 deletions(-) diff --git a/NEWS b/NEWS index 3753462c..2f3b8650 100644 --- a/NEWS +++ b/NEWS @@ -13,6 +13,8 @@ What's new in psycopg 2.4.6 Thanks to Manu Cupcic for the report (ticket #110). - 'register_hstore()', 'register_composite()', 'tpc_recover()' work with RealDictConnection and Cursor (ticket #114). + - connect() raises an exception instead of swallowing keyword arguments + when a connection string is specified as well (ticket #131). - 'errorcodes' map updated to PostgreSQL 9.2. diff --git a/lib/__init__.py b/lib/__init__.py index 0a8ed0f5..b84236c4 100644 --- a/lib/__init__.py +++ b/lib/__init__.py @@ -146,37 +146,37 @@ def connect(dsn=None, Using *async*=True an asynchronous connection will be created. Any other keyword parameter will be passed to the underlying client - library: the list of supported parameter depends on the library version. + library: the list of supported parameters depends on the library version. """ + items = [] + if database is not None: + items.append(('dbname', database)) + if user is not None: + items.append(('user', user)) + if password is not None: + items.append(('password', password)) + if host is not None: + items.append(('host', host)) + if port is not None: + items.append(('port', port)) + + items.extend([(k, v) for (k, v) in kwargs.iteritems() if v is not None]) + + if dsn is not None and items: + raise InterfaceError( + "you cannot specify keyword arguments (%s)" + " together with a connection string" + % ", ".join([k for k, v in items])) + if dsn is None: - # Note: reproducing the behaviour of the previous C implementation: - # keyword are silently swallowed if a DSN is specified. I would have - # raised an exception. File under "histerical raisins". - items = [] - if database is not None: - items.append(('dbname', database)) - if user is not None: - items.append(('user', user)) - if password is not None: - items.append(('password', password)) - if host is not None: - items.append(('host', host)) - # Reproducing the previous C implementation behaviour: swallow a - # negative port. The libpq would raise an exception for it. - if port is not None and int(port) > 0: - items.append(('port', port)) - - items.extend( - [(k, v) for (k, v) in kwargs.iteritems() if v is not None]) - dsn = " ".join(["%s=%s" % (k, _param_escape(str(v))) - for (k, v) in items]) - - if not dsn: + if not items: raise InterfaceError('missing dsn and no parameters') + else: + dsn = " ".join(["%s=%s" % (k, _param_escape(str(v))) + for (k, v) in items]) - return _connect(dsn, - connection_factory=connection_factory, async=async) + return _connect(dsn, connection_factory=connection_factory, async=async) __all__ = filter(lambda k: not k.startswith('_'), locals().keys()) diff --git a/tests/test_module.py b/tests/test_module.py index 870dbbc2..c2edeb24 100755 --- a/tests/test_module.py +++ b/tests/test_module.py @@ -127,6 +127,14 @@ class ConnectTestCase(unittest.TestCase): psycopg2.connect(database=r"\every thing'") self.assertEqual(self.args[0], r"dbname='\\every thing\''") + def test_no_kwargs_swallow(self): + self.assertRaises(psycopg2.InterfaceError, + psycopg2.connect, 'dbname=foo', database='foo') + self.assertRaises(psycopg2.InterfaceError, + psycopg2.connect, 'dbname=foo', user='postgres') + self.assertRaises(psycopg2.InterfaceError, + psycopg2.connect, 'dbname=foo', no_such_param='meh') + class ExceptionsTestCase(unittest.TestCase): def setUp(self): From cf3c6f86ffe436aa740aad6b45603c51d2a01e16 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Wed, 26 Sep 2012 11:47:06 +0100 Subject: [PATCH 2/3] Improved error message on connect when kwargs are passed together with dns --- lib/__init__.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/__init__.py b/lib/__init__.py index b84236c4..e49bcd77 100644 --- a/lib/__init__.py +++ b/lib/__init__.py @@ -165,9 +165,8 @@ def connect(dsn=None, if dsn is not None and items: raise InterfaceError( - "you cannot specify keyword arguments (%s)" - " together with a connection string" - % ", ".join([k for k, v in items])) + "'%s' is an invalid keyword argument when the dsn is specified" + % items[0][0]) if dsn is None: if not items: From 20d3d0f66d510686028b2fdb5bb2c9e2c306f87b Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Wed, 26 Sep 2012 11:55:21 +0100 Subject: [PATCH 3/3] Raise TypeError instead of InterfaceError on bad params on connect() TypeError is the standard Python error raised in this case: $ python -c "(lambda a: None)(b=10)" TypeError: () got an unexpected keyword argument 'b' We only used to raise InterfaceError when connect was used without any parameter at all, so it's hard to think a program depending on that design. Furthermore the function has always raised (and still does) OperationalError too, if the bad argument is detected by the libpq, and that cannot be changed because we can't tell the difference from a normal connection error. --- lib/__init__.py | 4 ++-- tests/test_module.py | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/__init__.py b/lib/__init__.py index e49bcd77..892d8038 100644 --- a/lib/__init__.py +++ b/lib/__init__.py @@ -164,13 +164,13 @@ def connect(dsn=None, items.extend([(k, v) for (k, v) in kwargs.iteritems() if v is not None]) if dsn is not None and items: - raise InterfaceError( + raise TypeError( "'%s' is an invalid keyword argument when the dsn is specified" % items[0][0]) if dsn is None: if not items: - raise InterfaceError('missing dsn and no parameters') + raise TypeError('missing dsn and no parameters') else: dsn = " ".join(["%s=%s" % (k, _param_escape(str(v))) for (k, v) in items]) diff --git a/tests/test_module.py b/tests/test_module.py index c2edeb24..4083c368 100755 --- a/tests/test_module.py +++ b/tests/test_module.py @@ -40,10 +40,10 @@ class ConnectTestCase(unittest.TestCase): psycopg2._connect = self._connect_orig def test_there_has_to_be_something(self): - self.assertRaises(psycopg2.InterfaceError, psycopg2.connect) - self.assertRaises(psycopg2.InterfaceError, psycopg2.connect, + self.assertRaises(TypeError, psycopg2.connect) + self.assertRaises(TypeError, psycopg2.connect, connection_factory=lambda dsn, async=False: None) - self.assertRaises(psycopg2.InterfaceError, psycopg2.connect, + self.assertRaises(TypeError, psycopg2.connect, async=True) def test_no_keywords(self): @@ -128,11 +128,11 @@ class ConnectTestCase(unittest.TestCase): self.assertEqual(self.args[0], r"dbname='\\every thing\''") def test_no_kwargs_swallow(self): - self.assertRaises(psycopg2.InterfaceError, + self.assertRaises(TypeError, psycopg2.connect, 'dbname=foo', database='foo') - self.assertRaises(psycopg2.InterfaceError, + self.assertRaises(TypeError, psycopg2.connect, 'dbname=foo', user='postgres') - self.assertRaises(psycopg2.InterfaceError, + self.assertRaises(TypeError, psycopg2.connect, 'dbname=foo', no_such_param='meh')