From: Mike Bayer Date: Thu, 29 May 2014 00:01:21 +0000 (-0400) Subject: - Fixed a few edge cases which arise in the so-called "row switch" X-Git-Tag: rel_1_0_0b1~417^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=bc08ee9029258b23171bb67e191452e6c739c597;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - Fixed a few edge cases which arise in the so-called "row switch" scenario, where an INSERT/DELETE can be turned into an UPDATE. In this situation, a many-to-one relationship set to None, or in some cases a scalar attribute set to None, may not be detected as a net change in value, and therefore the UPDATE would not reset what was on the previous row. This is due to some as-yet unresovled side effects of the way attribute history works in terms of implicitly assuming None isn't really a "change" for a previously un-set attribute. See also :ticket:`3061`. fixes #3060 --- diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst index ecebfeab59..512dce0915 100644 --- a/doc/build/changelog/changelog_09.rst +++ b/doc/build/changelog/changelog_09.rst @@ -14,6 +14,21 @@ .. changelog:: :version: 0.9.5 + .. change:: + :tags: bug, orm + :tickets: 3060 + :versions: 1.0.0 + + Fixed a few edge cases which arise in the so-called "row switch" + scenario, where an INSERT/DELETE can be turned into an UPDATE. + In this situation, a many-to-one relationship set to None, or + in some cases a scalar attribute set to None, may not be detected + as a net change in value, and therefore the UPDATE would not reset + what was on the previous row. This is due to some as-yet + unresovled side effects of the way attribute history works in terms + of implicitly assuming None isn't really a "change" for a previously + un-set attribute. See also :ticket:`3061`. + .. change:: :tags: bug, orm :tickets: 3057 diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index bf7dab4e7f..09d6e988d4 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -568,7 +568,7 @@ class AttributeImpl(object): # if history present, don't load key = self.key if key not in state.committed_state or \ - state.committed_state[key] is NEVER_SET: + state.committed_state[key] is NEVER_SET: if not passive & CALLABLES_OK: return PASSIVE_NO_RESULT @@ -763,6 +763,13 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): if self.dispatch._active_history: old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT | NO_AUTOFLUSH) else: + # would like to call with PASSIVE_NO_FETCH ^ INIT_OK. However, + # we have a long-standing behavior that a "get()" on never set + # should implicitly set the value to None. Leaving INIT_OK + # set here means we are consistent whether or not we did a get + # first. + # see test_use_object_set_None vs. test_use_object_get_first_set_None + # in test_attributes.py old = self.get(state, dict_, passive=PASSIVE_NO_FETCH) if check_old is not None and \ @@ -777,6 +784,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): state_str(state), self.key )) + value = self.fire_replace_event(state, dict_, value, old, initiator) dict_[self.key] = value @@ -793,8 +801,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl): def fire_replace_event(self, state, dict_, value, previous, initiator): if self.trackparent: if (previous is not value and - previous is not None and - previous is not PASSIVE_NO_RESULT): + previous not in (None, PASSIVE_NO_RESULT, NEVER_SET)): self.sethasparent(instance_state(previous), state, False) for fn in self.dispatch.set: @@ -1080,7 +1087,7 @@ def backref_listeners(attribute, key, uselist): def emit_backref_from_scalar_set_event(state, child, oldchild, initiator): if oldchild is child: return child - if oldchild is not None and oldchild is not PASSIVE_NO_RESULT: + if oldchild is not None and oldchild not in (PASSIVE_NO_RESULT, NEVER_SET): # With lazy=None, there's no guarantee that the full collection is # present when updating via a backref. old_state, old_dict = instance_state(oldchild),\ @@ -1208,7 +1215,7 @@ class History(History): return not bool( (self.added or self.deleted) - or self.unchanged and self.unchanged != [None] + or self.unchanged ) def sum(self): diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py index 34a2af391c..0d5a4f909e 100644 --- a/lib/sqlalchemy/orm/dependency.py +++ b/lib/sqlalchemy/orm/dependency.py @@ -741,10 +741,15 @@ class ManyToOneDP(DependencyProcessor): self.key, attributes.PASSIVE_NO_INITIALIZE) if history: - for child in history.added: - self._synchronize(state, child, None, False, - uowcommit, "add") - + if history.added: + for child in history.added: + self._synchronize(state, child, None, False, + uowcommit, "add") + elif history.unchanged == [None]: + # this is to appease the case where our row + # here is in fact going to be a so-called "row switch", + # where an INSERT becomes an UPDATE. See #3060. + self._synchronize(state, None, None, True, uowcommit) if self.post_update: self._post_update(state, uowcommit, history.sum()) diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py index 1bd432f155..49d9d11b9a 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -385,6 +385,12 @@ def _collect_update_commands(base_mapper, uowtransaction, if value is None: hasnull = True params[col._label] = value + + # see #3060. Need to consider an "unchanged" None + # as potentially history for now. + elif row_switch and history.unchanged == [None]: + params[col.key] = None + hasdata = True if hasdata: if hasnull: raise orm_exc.FlushError( diff --git a/lib/sqlalchemy/orm/sync.py b/lib/sqlalchemy/orm/sync.py index cf735fc53e..aed98bdf07 100644 --- a/lib/sqlalchemy/orm/sync.py +++ b/lib/sqlalchemy/orm/sync.py @@ -46,7 +46,10 @@ def populate(source, source_mapper, dest, dest_mapper, def clear(dest, dest_mapper, synchronize_pairs): for l, r in synchronize_pairs: - if r.primary_key: + if r.primary_key and \ + dest_mapper._get_state_attr_by_column( + dest, dest.dict, r) is not None: + raise AssertionError( "Dependency rule tried to blank-out primary key " "column '%s' on instance '%s'" % diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py index ccb1effdb0..b44f883c93 100644 --- a/test/orm/test_attributes.py +++ b/test/orm/test_attributes.py @@ -1957,12 +1957,22 @@ class HistoryTest(fixtures.TestBase): Foo, Bar = self._two_obj_fixture(uselist=False) f = Foo() f.someattr = None + # we'd expect ([None], (), ()), however because + # we set to None w/o setting history if we were to "get" first, + # it is more consistent that this doesn't set history. + eq_(self._someattr_history(f), ((), [None], ())) + + def test_use_object_get_first_set_None(self): + Foo, Bar = self._two_obj_fixture(uselist=False) + f = Foo() + assert f.someattr is None + f.someattr = None eq_(self._someattr_history(f), ((), [None], ())) def test_use_object_set_dict_set_None(self): Foo, Bar = self._two_obj_fixture(uselist=False) f = Foo() - hi =Bar(name='hi') + hi = Bar(name='hi') f.__dict__['someattr'] = hi f.someattr = None eq_(self._someattr_history(f), ([None], (), [hi])) diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py index b5057aa4ee..a76f928c79 100644 --- a/test/orm/test_unitofworkv2.py +++ b/test/orm/test_unitofworkv2.py @@ -1272,6 +1272,122 @@ class RowswitchAccountingTest(fixtures.MappedTest): eq_(sess.scalar(self.tables.parent.count()), 0) +class RowswitchM2OTest(fixtures.MappedTest): + # tests for #3060 and related issues + @classmethod + def define_tables(cls, metadata): + Table( + 'a', metadata, + Column('id', Integer, primary_key=True), + ) + Table( + 'b', metadata, + Column('id', Integer, primary_key=True), + Column('aid', ForeignKey('a.id')), + Column('cid', ForeignKey('c.id')), + Column('data', String(50)) + ) + Table( + 'c', metadata, + Column('id', Integer, primary_key=True), + ) + + def _fixture(self): + a, b, c = self.tables.a, self.tables.b, self.tables.c + + class A(fixtures.BasicEntity): + pass + class B(fixtures.BasicEntity): + pass + class C(fixtures.BasicEntity): + pass + + + mapper(A, a, properties={ + 'bs': relationship(B, cascade="all, delete-orphan") + }) + mapper(B, b, properties={ + 'c': relationship(C) + }) + mapper(C, c) + return A, B, C + + def test_set_none_replaces_m2o(self): + # we have to deal here with the fact that a + # get of an unset attribute implicitly sets it to None + # with no history. So while we'd like "b.x = None" to + # record that "None" was added and we can then actively set it, + # a simple read of "b.x" ruins that; we'd have to dramatically + # alter the semantics of get() such that it creates history, which + # would incur extra work within the flush process to deal with + # change that previously showed up as nothing. + + A, B, C = self._fixture() + sess = Session() + + sess.add( + A(id=1, bs=[B(id=1, c=C(id=1))]) + ) + sess.commit() + + a1 = sess.query(A).first() + a1.bs = [B(id=1, c=None)] + sess.commit() + assert a1.bs[0].c is None + + def test_set_none_w_get_replaces_m2o(self): + A, B, C = self._fixture() + sess = Session() + + sess.add( + A(id=1, bs=[B(id=1, c=C(id=1))]) + ) + sess.commit() + + a1 = sess.query(A).first() + b2 = B(id=1) + assert b2.c is None + b2.c = None + a1.bs = [b2] + sess.commit() + assert a1.bs[0].c is None + + def test_set_none_replaces_scalar(self): + # this case worked before #3060, because a straight scalar + # set of None shows up. Howver, as test_set_none_w_get + # shows, we can't rely on this - the get of None will blow + # away the history. + A, B, C = self._fixture() + sess = Session() + + sess.add( + A(id=1, bs=[B(id=1, data='somedata')]) + ) + sess.commit() + + a1 = sess.query(A).first() + a1.bs = [B(id=1, data=None)] + sess.commit() + assert a1.bs[0].data is None + + def test_set_none_w_get_replaces_scalar(self): + A, B, C = self._fixture() + sess = Session() + + sess.add( + A(id=1, bs=[B(id=1, data='somedata')]) + ) + sess.commit() + + a1 = sess.query(A).first() + b2 = B(id=1) + assert b2.data is None + b2.data = None + a1.bs = [b2] + sess.commit() + assert a1.bs[0].data is None + + class BasicStaleChecksTest(fixtures.MappedTest): @classmethod