From fd399fbdcb5c9196a1872318ff1a93bbe011ac36 Mon Sep 17 00:00:00 2001 From: Changaco Date: Sat, 28 Feb 2015 12:12:43 +0100 Subject: [PATCH 01/11] failing test for #26 --- tests.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests.py b/tests.py index 1f628d7..4590b47 100644 --- a/tests.py +++ b/tests.py @@ -334,6 +334,11 @@ def test_unregister_unregisters_multiple(self): self.db.unregister_model(self.MyModel) assert self.db.model_registry == {} + def test_add_column_doesnt_break_anything(self): + self.db.run("ALTER TABLE foo ADD COLUMN boo text") + one = self.db.one("SELECT foo.*::foo FROM foo WHERE bar='baz'") + assert one.boo is None + # cursor_factory # ============== From d3a7090dd24db2fff0ce687e9877f30a4e1845d8 Mon Sep 17 00:00:00 2001 From: Changaco Date: Sat, 28 Feb 2015 17:03:00 +0100 Subject: [PATCH 02/11] fix #26 --- postgres/__init__.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/postgres/__init__.py b/postgres/__init__.py index d16bd7c..09a21b6 100644 --- a/postgres/__init__.py +++ b/postgres/__init__.py @@ -824,6 +824,7 @@ def make_DelegatingCaster(postgres): """ class DelegatingCaster(CompositeCaster): + def make(self, values): if self.name not in postgres.model_registry: @@ -838,6 +839,20 @@ def make(self, values): instance = ModelSubclass(record) return instance + def parse(self, s, curs): + if s is None: + return None + + tokens = self.tokenize(s) + if len(tokens) != len(self.atttypes): + # The type has changed, re-fetch it from the DB + self.__dict__.update(self._from_db(self.name, curs).__dict__) + + values = [ curs.cast(oid, token) + for oid, token in zip(self.atttypes, tokens) ] + + return self.make(values) + return DelegatingCaster From a2846e4d0fe3170866cf4df84aeca24002050d59 Mon Sep 17 00:00:00 2001 From: Changaco Date: Sat, 28 Feb 2015 17:21:35 +0100 Subject: [PATCH 03/11] add an xfail test for what we can't fix --- tests.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests.py b/tests.py index 4590b47..bd5a840 100644 --- a/tests.py +++ b/tests.py @@ -8,7 +8,7 @@ from postgres.cursors import TooFew, TooMany, SimpleDictCursor from postgres.orm import ReadOnly, Model from psycopg2 import InterfaceError, ProgrammingError -from pytest import raises +from pytest import mark, raises DATABASE_URL = os.environ['DATABASE_URL'] @@ -339,6 +339,14 @@ def test_add_column_doesnt_break_anything(self): one = self.db.one("SELECT foo.*::foo FROM foo WHERE bar='baz'") assert one.boo is None + @mark.xfail + def test_add_and_drop_columns(self): + self.db.run("ALTER TABLE foo ADD COLUMN biz int NOT NULL DEFAULT 0") + self.db.run("ALTER TABLE foo DROP COLUMN bar") + one = self.db.one("SELECT foo.*::foo FROM foo LIMIT 1") + assert one.biz == 0 + assert not hasattr(one, 'bar') + # cursor_factory # ============== From 3ebcc6b1b065582bcbe64f5107ba2477fd02adae Mon Sep 17 00:00:00 2001 From: Changaco Date: Sat, 28 Feb 2015 17:55:13 +0100 Subject: [PATCH 04/11] add another failing test --- tests.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests.py b/tests.py index bd5a840..1916835 100644 --- a/tests.py +++ b/tests.py @@ -339,8 +339,21 @@ def test_add_column_doesnt_break_anything(self): one = self.db.one("SELECT foo.*::foo FROM foo WHERE bar='baz'") assert one.boo is None + def test_replace_column_different_type(self): + self.db.run("CREATE TABLE grok (bar int)") + self.db.run("INSERT INTO grok VALUES (0)") + class EmptyModel(Model): pass + self.db.register_model(EmptyModel, 'grok') + # Add a new column then drop the original one + self.db.run("ALTER TABLE grok ADD COLUMN biz text NOT NULL DEFAULT 'x'") + self.db.run("ALTER TABLE grok DROP COLUMN bar") + # The number of columns hasn't changed but the names and types have + one = self.db.one("SELECT grok.*::grok FROM grok LIMIT 1") + assert one.biz == 'x' + assert not hasattr(one, 'bar') + @mark.xfail - def test_add_and_drop_columns(self): + def test_replace_column_same_type(self): self.db.run("ALTER TABLE foo ADD COLUMN biz int NOT NULL DEFAULT 0") self.db.run("ALTER TABLE foo DROP COLUMN bar") one = self.db.one("SELECT foo.*::foo FROM foo LIMIT 1") From fae67d8f4a5e8dfef1dc425cf0f43fa029c5a392 Mon Sep 17 00:00:00 2001 From: Changaco Date: Sat, 28 Feb 2015 17:55:32 +0100 Subject: [PATCH 05/11] catch type errors and retry parsing after re-fetching the type info --- postgres/__init__.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/postgres/__init__.py b/postgres/__init__.py index 09a21b6..558e474 100644 --- a/postgres/__init__.py +++ b/postgres/__init__.py @@ -839,17 +839,24 @@ def make(self, values): instance = ModelSubclass(record) return instance - def parse(self, s, curs): + def parse(self, s, curs, retry=False): if s is None: return None tokens = self.tokenize(s) if len(tokens) != len(self.atttypes): - # The type has changed, re-fetch it from the DB + # The number of columns has changed, re-fetch the type info self.__dict__.update(self._from_db(self.name, curs).__dict__) - values = [ curs.cast(oid, token) - for oid, token in zip(self.atttypes, tokens) ] + try: + values = [ curs.cast(oid, token) + for oid, token in zip(self.atttypes, tokens) ] + except ValueError: + # The type of a column has changed, re-fetch it and retry once + if retry: + raise + self.__dict__.update(self._from_db(self.name, curs).__dict__) + return self.parse(s, curs, True) return self.make(values) From 6414fe1304ab0b1262c72d4a2e975825bd5cb49c Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Wed, 4 Mar 2015 15:18:04 -0500 Subject: [PATCH 06/11] Add a little doc in comments --- postgres/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/postgres/__init__.py b/postgres/__init__.py index 558e474..058f666 100644 --- a/postgres/__init__.py +++ b/postgres/__init__.py @@ -826,6 +826,7 @@ def make_DelegatingCaster(postgres): class DelegatingCaster(CompositeCaster): def make(self, values): + # Override to delegate to the model registry. if self.name not in postgres.model_registry: # This is probably a bug, not a normal user error. It means @@ -840,6 +841,9 @@ def make(self, values): return instance def parse(self, s, curs, retry=False): + # Override to protect against race conditions: + # https://github.com/gratipay/postgres.py/issues/26 + if s is None: return None From b55b87dd7632f0c6d1232f539e8676928c208cef Mon Sep 17 00:00:00 2001 From: Changaco Date: Wed, 4 Mar 2015 21:50:15 +0100 Subject: [PATCH 07/11] specify the expected exception type of the xfail test --- tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests.py b/tests.py index 1916835..327be07 100644 --- a/tests.py +++ b/tests.py @@ -352,7 +352,7 @@ class EmptyModel(Model): pass assert one.biz == 'x' assert not hasattr(one, 'bar') - @mark.xfail + @mark.xfail(raises=AttributeError) def test_replace_column_same_type(self): self.db.run("ALTER TABLE foo ADD COLUMN biz int NOT NULL DEFAULT 0") self.db.run("ALTER TABLE foo DROP COLUMN bar") From 5d49f4ded98d880d4a3e64dab3ab96c509bf43da Mon Sep 17 00:00:00 2001 From: Changaco Date: Wed, 4 Mar 2015 22:17:18 +0100 Subject: [PATCH 08/11] improve test name and fix column type to match --- tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests.py b/tests.py index 327be07..fee7b4f 100644 --- a/tests.py +++ b/tests.py @@ -353,8 +353,8 @@ class EmptyModel(Model): pass assert not hasattr(one, 'bar') @mark.xfail(raises=AttributeError) - def test_replace_column_same_type(self): - self.db.run("ALTER TABLE foo ADD COLUMN biz int NOT NULL DEFAULT 0") + def test_replace_column_same_type_different_name(self): + self.db.run("ALTER TABLE foo ADD COLUMN biz text NOT NULL DEFAULT 0") self.db.run("ALTER TABLE foo DROP COLUMN bar") one = self.db.one("SELECT foo.*::foo FROM foo LIMIT 1") assert one.biz == 0 From 3431d24a764c3087ecbb6da43c58bff5c80e0744 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Tue, 10 Mar 2015 15:57:15 -0400 Subject: [PATCH 09/11] Match the method order in the base class --- postgres/__init__.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/postgres/__init__.py b/postgres/__init__.py index 058f666..5372841 100644 --- a/postgres/__init__.py +++ b/postgres/__init__.py @@ -825,21 +825,6 @@ def make_DelegatingCaster(postgres): """ class DelegatingCaster(CompositeCaster): - def make(self, values): - # Override to delegate to the model registry. - if self.name not in postgres.model_registry: - - # This is probably a bug, not a normal user error. It means - # we've called register_composite for this typname without also - # registering with model_registry. - - raise NotImplementedError - - ModelSubclass = postgres.model_registry[self.name] - record = dict(zip(self.attnames, values)) - instance = ModelSubclass(record) - return instance - def parse(self, s, curs, retry=False): # Override to protect against race conditions: # https://github.com/gratipay/postgres.py/issues/26 @@ -864,6 +849,21 @@ def parse(self, s, curs, retry=False): return self.make(values) + def make(self, values): + # Override to delegate to the model registry. + if self.name not in postgres.model_registry: + + # This is probably a bug, not a normal user error. It means + # we've called register_composite for this typname without also + # registering with model_registry. + + raise NotImplementedError + + ModelSubclass = postgres.model_registry[self.name] + record = dict(zip(self.attnames, values)) + instance = ModelSubclass(record) + return instance + return DelegatingCaster From 0f422eb76d93e5fdd2364926a909fcb9154e48a5 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Tue, 10 Mar 2015 16:11:26 -0400 Subject: [PATCH 10/11] Factor out a method to fetch type information --- postgres/__init__.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/postgres/__init__.py b/postgres/__init__.py index 5372841..45b0fc8 100644 --- a/postgres/__init__.py +++ b/postgres/__init__.py @@ -835,7 +835,7 @@ def parse(self, s, curs, retry=False): tokens = self.tokenize(s) if len(tokens) != len(self.atttypes): # The number of columns has changed, re-fetch the type info - self.__dict__.update(self._from_db(self.name, curs).__dict__) + self._refetch_type_info(curs) try: values = [ curs.cast(oid, token) @@ -844,7 +844,7 @@ def parse(self, s, curs, retry=False): # The type of a column has changed, re-fetch it and retry once if retry: raise - self.__dict__.update(self._from_db(self.name, curs).__dict__) + self._refetch_type_info(curs) return self.parse(s, curs, True) return self.make(values) @@ -864,6 +864,12 @@ def make(self, values): instance = ModelSubclass(record) return instance + def _refetch_type_info(self, curs): + """Given a cursor, update the current object with a fresh type definition. + """ + new_self = self._from_db(self.name, curs) + self.__dict__.update(new_self.__dict__) + return DelegatingCaster From 363123f6910cfd2e41b88d976bf2fb30a8937656 Mon Sep 17 00:00:00 2001 From: Changaco Date: Thu, 12 Mar 2015 13:04:07 +0100 Subject: [PATCH 11/11] call `CompositeCaster.parse()` instead of duplicating its code --- postgres/__init__.py | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/postgres/__init__.py b/postgres/__init__.py index 45b0fc8..9e46364 100644 --- a/postgres/__init__.py +++ b/postgres/__init__.py @@ -180,6 +180,7 @@ from postgres.cursors import SimpleTupleCursor, SimpleNamedTupleCursor from postgres.cursors import SimpleDictCursor, SimpleCursorBase from postgres.orm import Model +from psycopg2 import DataError from psycopg2.extras import register_composite, CompositeCaster from psycopg2.pool import ThreadedConnectionPool as ConnectionPool @@ -825,29 +826,18 @@ def make_DelegatingCaster(postgres): """ class DelegatingCaster(CompositeCaster): - def parse(self, s, curs, retry=False): + def parse(self, s, curs, retry=True): # Override to protect against race conditions: # https://github.com/gratipay/postgres.py/issues/26 - if s is None: - return None - - tokens = self.tokenize(s) - if len(tokens) != len(self.atttypes): - # The number of columns has changed, re-fetch the type info - self._refetch_type_info(curs) - try: - values = [ curs.cast(oid, token) - for oid, token in zip(self.atttypes, tokens) ] - except ValueError: - # The type of a column has changed, re-fetch it and retry once - if retry: + return super(DelegatingCaster, self).parse(s, curs) + except (DataError, ValueError): + if not retry: raise + # Re-fetch the type info and retry once self._refetch_type_info(curs) - return self.parse(s, curs, True) - - return self.make(values) + return self.parse(s, curs, False) def make(self, values): # Override to delegate to the model registry.