Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to handle changes to type definitions #43

Merged
merged 11 commits into from
Sep 12, 2015
22 changes: 22 additions & 0 deletions postgres/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,7 @@ def make_DelegatingCaster(postgres):

"""
class DelegatingCaster(CompositeCaster):

def make(self, values):
if self.name not in postgres.model_registry:

Expand All @@ -838,6 +839,27 @@ def make(self, values):
instance = ModelSubclass(record)
return instance

def parse(self, s, curs, retry=False):
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.__dict__.update(self._from_db(self.name, curs).__dict__)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did we get this line from, and why do we not call it in the normal course of operation? Why are we doing something in this case that we don't otherwise do? Don't we need to configure the class when it's first instantiated? Why am I not seeing that in our code? Is it in psycopg2?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, we're calling _from_db, which is a constructor classmethod, and then we're overwriting all possible attributes on ourself with the corresponding attributes from the new instance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified in 0f422eb.


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)

return DelegatingCaster


Expand Down
28 changes: 27 additions & 1 deletion tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Expand Down Expand Up @@ -334,6 +334,32 @@ 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

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'")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't biz the same type as bar? Why is this test "different_type"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is grok here, not foo.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using an xfail here instead of a raises?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because ideally it shouldn't raise, but it's a problem we can't fix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But xfail will silently swallow all exceptions, no? Seems like we should test for the specific exception we're expecting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the appropriate raises parameter. From pytest's documentation:

If you want to be more specific as to why the test is failing, you can specify a single exception, or a list of exceptions, in the raises argument. Then the test will be reported as a regular failure if it fails with an exception not mentioned in raises.

def test_replace_column_same_type(self):
self.db.run("ALTER TABLE foo ADD COLUMN biz int NOT NULL DEFAULT 0")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's going on here, then? foo.bar is a text column, right? So we're adding a biz int column and dropping bar?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks a lot like adding a biz text column and dropping bar in the previous test. Why does that test pass and this one fails?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

biz should be text here to fit with the name of the test, sorry.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'm more confused. It seems like there's some interaction between the number and types of the columns we're adding and dropping here, but I'm not getting it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails because the number of columns is the same (so the length test doesn't detect the change) and the types of the old and new columns are compatible (so there is no type error to catch).

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
# ==============
Expand Down