- reverse course in #3061 so that we instead no longer set None in the attribute

when we do a get; we return the None as always but we leave the dict blank
and the loader callable still in place.  The case for this implicit get on a pending object is not
super common and there really should be no change in state at all when this
operation proceeds.   This change is more dramatic as it reverses
a behavior SQLA has had since the first release.
fixes #3061

7 files changed, 144 insertions, 64 deletions

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 9004f5681..862d4103a 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -35,14 +35,11 @@
         Adjustment to attribute mechanics concerning when a value is
         implicitly initialized to None via first access; this action,
         which has always resulted in a population of the attribute,
-        now emits an attribute event just like any other attribute set
-        operation and generates the same kind of history as one.  Additionally,
-        many mapper internal operations will no longer implicitly generate
-        these "None" values when various never-set attributes are checked.
-        These are subtle behavioral fixes to attribute mechanics which provide
-        a better solution to the problem of :ticket:`3060`, which also
-        involves recognition of attributes explicitly set to ``None``
-        vs. attributes that were never set.
+        no longer does so; the None value is returned but the underlying
+        attribute receives no set event.  This is consistent with how collections
+        work and allows attribute mechanics to behave more consistently;
+        in particular, getting an attribute with no value does not squash
+        the event that should proceed if the value is actually set to None.
 
         .. seealso::
 
diff --git a/doc/build/changelog/migration_10.rst b/doc/build/changelog/migration_10.rst
index 1e5156dce..43830197e 100644
--- a/doc/build/changelog/migration_10.rst
+++ b/doc/build/changelog/migration_10.rst
@@ -62,13 +62,14 @@ attribute first instead::
 
 The above means that the behavior of our "set" operation can be corrupted
 by the fact that the value was accessed via "get" earlier.  In 1.0, this
-inconsistency has been resolved::
+inconsistency has been resolved, by no longer actually setting anything
+when the default "getter" is used.
 
 	>>> obj = Foo()
 	>>> obj.someattr
 	None
 	>>> inspect(obj).attrs.someattr.history
-	History(added=[None], unchanged=(), deleted=())  # 1.0
+	History(added=(), unchanged=(), deleted=())  # 1.0
 	>>> obj.someattr = None
 	>>> inspect(obj).attrs.someattr.history
 	History(added=[None], unchanged=(), deleted=())
@@ -94,13 +95,6 @@ if the primary key attributes have no setting at all, whereas the value
 would be ``None`` before, it will now be the :data:`.orm.attributes.NEVER_SET`
 symbol, and no change to the object's state occurs.
 
-Performance-wise, the change has the tradeoff that an attribute will need
-to be considered in a unit of work flush process in more cases than before, if it has
-been accessed and populated with a default value.   However, performance
-is improved in the case where the unit of work inspects a pending object for
-an existing primary key value, as the state of the object won't change
-in the common case that none was set.
-
 :ticket:`3061`
 
 .. _behavioral_changes_core_10:
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index df1f328b7..fecd74f30 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -556,15 +556,7 @@ class AttributeImpl(object):
     def initialize(self, state, dict_):
         """Initialize the given state's attribute with an empty value."""
 
-        old = NEVER_SET
-        value = None
-        if self.dispatch.set:
-            value = self.fire_replace_event(state, dict_,
-                                                None, old, None)
-        state._modified_event(dict_, self, old)
-
-        dict_[self.key] = value
-        return value
+        return None
 
     def get(self, state, dict_, passive=PASSIVE_OFF):
         """Retrieve a value from the given object.
diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py
index ab9b9f5d5..59b0078ea 100644
--- a/test/orm/test_attributes.py
+++ b/test/orm/test_attributes.py
@@ -949,7 +949,7 @@ class GetNoValueTest(fixtures.ORMTest):
             attr.get(state, dict_, passive=attributes.PASSIVE_OFF),
             None
         )
-        assert 'attr' in dict_
+        assert 'attr' not in dict_
 
 class UtilTest(fixtures.ORMTest):
     def test_helpers(self):
@@ -2386,7 +2386,7 @@ class LazyloadHistoryTest(fixtures.TestBase):
             'bar'), ((), (), ['hi']))
         assert f.bar is None
         eq_(attributes.get_state_history(attributes.instance_state(f),
-            'bar'), ([None], (), ['hi']))
+            'bar'), ((), (), ['hi']))
 
     def test_scalar_via_lazyload_with_active(self):
         # TODO: break into individual tests
@@ -2430,7 +2430,7 @@ class LazyloadHistoryTest(fixtures.TestBase):
             'bar'), ((), (), ['hi']))
         assert f.bar is None
         eq_(attributes.get_state_history(attributes.instance_state(f),
-            'bar'), ([None], (), ['hi']))
+            'bar'), ((), (), ['hi']))
 
     def test_scalar_object_via_lazyload(self):
         # TODO: break into individual tests
@@ -2475,7 +2475,7 @@ class LazyloadHistoryTest(fixtures.TestBase):
             'bar'), ((), (), [bar1]))
         assert f.bar is None
         eq_(attributes.get_state_history(attributes.instance_state(f),
-            'bar'), ([None], (), [bar1]))
+            'bar'), ((), (), [bar1]))
 
 class ListenerTest(fixtures.ORMTest):
     def test_receive_changes(self):
@@ -2569,11 +2569,8 @@ class ListenerTest(fixtures.ORMTest):
 
         f1 = Foo()
         eq_(f1.bar, None)
-        eq_(canary.mock_calls, [
-                call(
-                    f1, None, attributes.NEVER_SET,
-                    attributes.Event(Foo.bar.impl, attributes.OP_REPLACE))
-        ])
+        # reversal of approach in #3061
+        eq_(canary.mock_calls, [])
 
     def test_none_init_object(self):
         canary = Mock()
@@ -2586,11 +2583,23 @@ class ListenerTest(fixtures.ORMTest):
 
         f1 = Foo()
         eq_(f1.bar, None)
-        eq_(canary.mock_calls, [
-                call(
-                    f1, None, attributes.NEVER_SET,
-                    attributes.Event(Foo.bar.impl, attributes.OP_REPLACE))
-        ])
+        # reversal of approach in #3061
+        eq_(canary.mock_calls, [])
+
+    def test_none_init_collection(self):
+        canary = Mock()
+        class Foo(object):
+            pass
+        instrumentation.register_class(Foo)
+        attributes.register_attribute(Foo, 'bar', useobject=True, uselist=True)
+
+        event.listen(Foo.bar, "set", canary)
+
+        f1 = Foo()
+        eq_(f1.bar, [])
+        # reversal of approach in #3061
+        eq_(canary.mock_calls, [])
+
 
     def test_propagate(self):
         classes = [None, None, None]
diff --git a/test/orm/test_inspect.py b/test/orm/test_inspect.py
index ee3fe213e..4b69c2a71 100644
--- a/test/orm/test_inspect.py
+++ b/test/orm/test_inspect.py
@@ -341,10 +341,10 @@ class TestORMInspection(_fixtures.FixtureTest):
             insp.attrs.id.value,
             None
         )
-        # now the None is there
+        # nope, still not set
         eq_(
             insp.attrs.id.loaded_value,
-            None
+            NO_VALUE
         )
 
     def test_instance_state_attr_passive_value_collection(self):
diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py
index f2bcd2036..16abbb37b 100644
--- a/test/orm/test_relationships.py
+++ b/test/orm/test_relationships.py
@@ -230,6 +230,116 @@ class DependencyTwoParentTest(fixtures.MappedTest):
         session.delete(c)
         session.flush()
 
+class M2ODontOverwriteFKTest(fixtures.MappedTest):
+    @classmethod
+    def define_tables(cls, metadata):
+        Table(
+            'a', metadata,
+            Column('id', Integer, primary_key=True),
+            Column('bid', ForeignKey('b.id'))
+        )
+        Table(
+            'b', metadata,
+            Column('id', Integer, primary_key=True),
+        )
+
+    def _fixture(self, uselist=False):
+        a, b = self.tables.a, self.tables.b
+
+        class A(fixtures.BasicEntity):
+            pass
+        class B(fixtures.BasicEntity):
+            pass
+
+
+        mapper(A, a, properties={
+                'b': relationship(B, uselist=uselist)
+            })
+        mapper(B, b)
+        return A, B
+
+    def test_joinedload_doesnt_produce_bogus_event(self):
+        A, B = self._fixture()
+        sess = Session()
+
+        b1 = B()
+        sess.add(b1)
+        sess.flush()
+
+        a1 = A()
+        sess.add(a1)
+        sess.commit()
+
+        # test that was broken by #3060
+        from sqlalchemy.orm import joinedload
+        a1 = sess.query(A).options(joinedload("b")).first()
+        a1.bid = b1.id
+        sess.flush()
+
+        eq_(a1.bid, b1.id)
+
+    def test_init_doesnt_produce_scalar_event(self):
+        A, B = self._fixture()
+        sess = Session()
+
+        b1 = B()
+        sess.add(b1)
+        sess.flush()
+
+        a1 = A()
+        assert a1.b is None
+        a1.bid = b1.id
+        sess.add(a1)
+        sess.flush()
+        assert a1.bid is not None
+
+    def test_init_doesnt_produce_collection_event(self):
+        A, B = self._fixture(uselist=True)
+        sess = Session()
+
+        b1 = B()
+        sess.add(b1)
+        sess.flush()
+
+        a1 = A()
+        assert a1.b == []
+        a1.bid = b1.id
+        sess.add(a1)
+        sess.flush()
+        assert a1.bid is not None
+
+    def test_scalar_relationship_overrides_fk(self):
+        A, B = self._fixture()
+        sess = Session()
+
+        b1 = B()
+        sess.add(b1)
+        sess.flush()
+
+        a1 = A()
+        a1.bid = b1.id
+        a1.b = None
+        sess.add(a1)
+        sess.flush()
+        assert a1.bid is None
+
+    def test_collection_relationship_overrides_fk(self):
+        A, B = self._fixture(uselist=True)
+        sess = Session()
+
+        b1 = B()
+        sess.add(b1)
+        sess.flush()
+
+        a1 = A()
+        a1.bid = b1.id
+        a1.b = []
+        sess.add(a1)
+        sess.flush()
+        # this is weird
+        assert a1.bid is not None
+
+
 
 class DirectSelfRefFKTest(fixtures.MappedTest, AssertsCompiledSQL):
     """Tests the ultimate join condition, a single column
diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py
index 787c104e7..9fedc9590 100644
--- a/test/orm/test_unitofworkv2.py
+++ b/test/orm/test_unitofworkv2.py
@@ -1314,7 +1314,6 @@ class RowswitchM2OTest(fixtures.MappedTest):
         mapper(C, c)
         return A, B, C
 
-    @testing.fails()
     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
@@ -1338,7 +1337,6 @@ class RowswitchM2OTest(fixtures.MappedTest):
         sess.commit()
         assert a1.bs[0].c is None
 
-    @testing.fails()
     def test_set_none_w_get_replaces_m2o(self):
         A, B, C = self._fixture()
         sess = Session()
@@ -1391,26 +1389,6 @@ class RowswitchM2OTest(fixtures.MappedTest):
         sess.commit()
         assert a1.bs[0].data is None
 
-    def test_joinedload_doesnt_produce_bogus_event(self):
-        A, B, C = self._fixture()
-        sess = Session()
-
-        c1 = C()
-        sess.add(c1)
-        sess.flush()
-
-        b1 = B()
-        sess.add(b1)
-        sess.commit()
-
-        # test that was broken by #3060
-        from sqlalchemy.orm import joinedload
-        b1 = sess.query(B).options(joinedload("c")).first()
-        b1.cid = c1.id
-        sess.flush()
-
-        assert b1.cid == c1.id
-
 
 class BasicStaleChecksTest(fixtures.MappedTest):
     @classmethod