- 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

7 files changed, 173 insertions, 11 deletions

diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst
index ecebfeab5..512dce091 100644
--- a/doc/build/changelog/changelog_09.rst
+++ b/doc/build/changelog/changelog_09.rst
@@ -16,6 +16,21 @@
 
     .. 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
         :versions: 1.0.0
 
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index bf7dab4e7..09d6e988d 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 34a2af391..0d5a4f909 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 1bd432f15..49d9d11b9 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 cf735fc53..aed98bdf0 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 ccb1effdb..b44f883c9 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 b5057aa4e..a76f928c7 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