Unify NO_VALUE and NEVER_SET

There's no real difference between these two constants
except they are used in different places and therefore allow
various codepaths to work largely by accident.   These
codepaths should be explicit.   Assign NO_VALUE and NEVER_SET
to the same constant and work towards having just one constant
for "we have no value to return right now".

Fixes: #4696
Change-Id: I7c324967952c1886bf202074d627323a2ad013cc

diff --git a/doc/build/changelog/unreleased_14/4696.rst b/doc/build/changelog/unreleased_14/4696.rst
new file mode 100644 (file)
index 0000000..c4629db
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/4696.rst
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4696
+
+    The internal attribute symbols NO_VALUE and NEVER_SET have been unified, as
+    there was no meaningful difference between these two symbols, other than a
+    few codepaths where they were differentiated in subtle and undocumented
+    ways, these have been fixed.
+

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 01f19d9917b4761e78486545d390e760df35201f..321ab7d6fbdc535d5233af841eb6fe01ec8340f0 100644 (file)
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -28,7 +28,7 @@ from .base import instance_state
 from .base import instance_str
 from .base import LOAD_AGAINST_COMMITTED
 from .base import manager_of_class
-from .base import NEVER_SET
+from .base import NEVER_SET  # noqa
 from .base import NO_AUTOFLUSH
 from .base import NO_CHANGE  # noqa
 from .base import NO_RAISE
@@ -40,7 +40,7 @@ from .base import PASSIVE_NO_INITIALIZE
 from .base import PASSIVE_NO_RESULT
 from .base import PASSIVE_OFF
 from .base import PASSIVE_ONLY_PERSISTENT
-from .base import PASSIVE_RETURN_NEVER_SET
+from .base import PASSIVE_RETURN_NO_VALUE
 from .base import RELATED_OBJECT_OK  # noqa
 from .base import SQL_OK  # noqa
 from .base import state_str
@@ -677,7 +677,7 @@ class AttributeImpl(object):
             key = self.key
             if (
                 key not in state.committed_state
-                or state.committed_state[key] is NEVER_SET
+                or state.committed_state[key] is NO_VALUE
             ):
                 if not passive & CALLABLES_OK:
                     return PASSIVE_NO_RESULT
@@ -692,7 +692,7 @@ class AttributeImpl(object):
                 else:
                     value = ATTR_EMPTY
 
-                if value is PASSIVE_NO_RESULT or value is NEVER_SET:
+                if value is PASSIVE_NO_RESULT or value is NO_VALUE:
                     return value
                 elif value is ATTR_WAS_SET:
                     try:
@@ -708,7 +708,7 @@ class AttributeImpl(object):
                     return self.set_committed_value(state, dict_, value)
 
             if not passive & INIT_OK:
-                return NEVER_SET
+                return NO_VALUE
             else:
                 # Return a new, empty value
                 return self.initialize(state, dict_)
@@ -749,7 +749,7 @@ class AttributeImpl(object):
 
         if self.key in state.committed_state:
             value = state.committed_state[self.key]
-            if value in (NO_VALUE, NEVER_SET):
+            if value is NO_VALUE:
                 return None
             else:
                 return value
@@ -782,7 +782,7 @@ class ScalarAttributeImpl(AttributeImpl):
 
     def delete(self, state, dict_):
         if self.dispatch._active_history:
-            old = self.get(state, dict_, PASSIVE_RETURN_NEVER_SET)
+            old = self.get(state, dict_, PASSIVE_RETURN_NO_VALUE)
         else:
             old = dict_.get(self.key, NO_VALUE)
 
@@ -802,6 +802,8 @@ class ScalarAttributeImpl(AttributeImpl):
     def get_history(self, state, dict_, passive=PASSIVE_OFF):
         if self.key in dict_:
             return History.from_scalar_attribute(self, state, dict_[self.key])
+        elif self.key in state.committed_state:
+            return History.from_scalar_attribute(self, state, NO_VALUE)
         else:
             if passive & INIT_OK:
                 passive ^= INIT_OK
@@ -822,7 +824,7 @@ class ScalarAttributeImpl(AttributeImpl):
         pop=False,
     ):
         if self.dispatch._active_history:
-            old = self.get(state, dict_, PASSIVE_RETURN_NEVER_SET)
+            old = self.get(state, dict_, PASSIVE_RETURN_NO_VALUE)
         else:
             old = dict_.get(self.key, NO_VALUE)
 
@@ -920,7 +922,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
         if (
             current is not None
             and current is not PASSIVE_NO_RESULT
-            and current is not NEVER_SET
+            and current is not NO_VALUE
         ):
             ret = [(instance_state(current), current)]
         else:
@@ -931,7 +933,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
             if (
                 original is not None
                 and original is not PASSIVE_NO_RESULT
-                and original is not NEVER_SET
+                and original is not NO_VALUE
                 and original is not current
             ):
 
@@ -998,7 +1000,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
             if previous is not value and previous not in (
                 None,
                 PASSIVE_NO_RESULT,
-                NEVER_SET,
+                NO_VALUE,
             ):
                 self.sethasparent(instance_state(previous), state, False)
 
@@ -1109,7 +1111,7 @@ class CollectionAttributeImpl(AttributeImpl):
 
         if self.key in state.committed_state:
             original = state.committed_state[self.key]
-            if original not in (NO_VALUE, NEVER_SET):
+            if original is not NO_VALUE:
                 current_states = [
                     ((c is not None) and instance_state(c) or None, c)
                     for c in current
@@ -1142,7 +1144,7 @@ class CollectionAttributeImpl(AttributeImpl):
         for fn in self.dispatch.append:
             value = fn(state, value, initiator or self._append_token)
 
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
         if self.trackparent and value is not None:
             self.sethasparent(instance_state(value), state, True)
@@ -1158,7 +1160,7 @@ class CollectionAttributeImpl(AttributeImpl):
         operations (even though set.pop is the one where it is really needed).
 
         """
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
     def fire_remove_event(self, state, dict_, value, initiator):
         if self.trackparent and value is not None:
@@ -1167,13 +1169,13 @@ class CollectionAttributeImpl(AttributeImpl):
         for fn in self.dispatch.remove:
             fn(state, value, initiator or self._remove_token)
 
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
     def delete(self, state, dict_):
         if self.key not in dict_:
             return
 
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
         collection = self.get_collection(state, state.dict)
         collection.clear_with_event()
@@ -1386,7 +1388,7 @@ def backref_listeners(attribute, key, uselist):
         if (
             oldchild is not None
             and oldchild is not PASSIVE_NO_RESULT
-            and oldchild is not NEVER_SET
+            and oldchild is not NO_VALUE
         ):
             # With lazy=None, there's no guarantee that the full collection is
             # present when updating via a backref.
@@ -1481,7 +1483,7 @@ def backref_listeners(attribute, key, uselist):
         if (
             child is not None
             and child is not PASSIVE_NO_RESULT
-            and child is not NEVER_SET
+            and child is not NO_VALUE
         ):
             child_state, child_dict = (
                 instance_state(child),
@@ -1549,9 +1551,7 @@ def backref_listeners(attribute, key, uselist):
 
 
 _NO_HISTORY = util.symbol("NO_HISTORY")
-_NO_STATE_SYMBOLS = frozenset(
-    [id(PASSIVE_NO_RESULT), id(NO_VALUE), id(NEVER_SET)]
-)
+_NO_STATE_SYMBOLS = frozenset([id(PASSIVE_NO_RESULT), id(NO_VALUE)])
 
 History = util.namedtuple("History", ["added", "unchanged", "deleted"])
 
@@ -1637,12 +1637,15 @@ class History(History):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
 
         if original is _NO_HISTORY:
-            if current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), ())
             else:
                 return cls((), [current], ())
         # don't let ClauseElement expressions here trip things up
-        elif attribute.is_equal(current, original) is True:
+        elif (
+            current is not NO_VALUE
+            and attribute.is_equal(current, original) is True
+        ):
             return cls((), [current], ())
         else:
             # current convention on native scalars is to not
@@ -1658,7 +1661,7 @@ class History(History):
                     current = None
             else:
                 deleted = [original]
-            if current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), deleted)
             else:
                 return cls([current], (), deleted)
@@ -1668,11 +1671,11 @@ class History(History):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
 
         if original is _NO_HISTORY:
-            if current is NO_VALUE or current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), ())
             else:
                 return cls((), [current], ())
-        elif current is original and current is not NEVER_SET:
+        elif current is original and current is not NO_VALUE:
             return cls((), [current], ())
         else:
             # current convention on related objects is to not
@@ -1688,7 +1691,7 @@ class History(History):
                     current = None
             else:
                 deleted = [original]
-            if current is NO_VALUE or current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), deleted)
             else:
                 return cls([current], (), deleted)
@@ -1697,11 +1700,11 @@ class History(History):
     def from_collection(cls, attribute, state, current):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
 
-        if current is NO_VALUE or current is NEVER_SET:
+        if current is NO_VALUE:
             return cls((), (), ())
 
         current = getattr(current, "_sa_adapter")
-        if original in (NO_VALUE, NEVER_SET):
+        if original is NO_VALUE:
             return cls(list(current), (), ())
         elif original is _NO_HISTORY:
             return cls((), list(current), ())

diff --git a/lib/sqlalchemy/orm/base.py b/lib/sqlalchemy/orm/base.py
index f809d5891b763e8e176a8b6826e7c3c25f766b19..4d308d26b3d4fbd7d944cff70b081a27da5fc391 100644 (file)
--- a/lib/sqlalchemy/orm/base.py
+++ b/lib/sqlalchemy/orm/base.py
@@ -45,13 +45,12 @@ NO_VALUE = util.symbol(
     and flags indicated we were not to load it.
     """,
 )
+NEVER_SET = NO_VALUE
+"""
+Synonymous with NO_VALUE
 
-NEVER_SET = util.symbol(
-    "NEVER_SET",
-    """Symbol which may be placed as the 'previous' value of an attribute
-    indicating that the attribute had not been assigned to previously.
-    """,
-)
+.. versionchanged:: 1.4   NEVER_SET was merged with NO_VALUE
+"""
 
 NO_CHANGE = util.symbol(
     "NO_CHANGE",
@@ -126,15 +125,15 @@ PASSIVE_OFF = util.symbol(
         RELATED_OBJECT_OK | NON_PERSISTENT_OK | INIT_OK | CALLABLES_OK | SQL_OK
     ),
 )
-PASSIVE_RETURN_NEVER_SET = util.symbol(
-    "PASSIVE_RETURN_NEVER_SET",
+PASSIVE_RETURN_NO_VALUE = util.symbol(
+    "PASSIVE_RETURN_NO_VALUE",
     """PASSIVE_OFF ^ INIT_OK""",
     canonical=PASSIVE_OFF ^ INIT_OK,
 )
 PASSIVE_NO_INITIALIZE = util.symbol(
     "PASSIVE_NO_INITIALIZE",
-    "PASSIVE_RETURN_NEVER_SET ^ CALLABLES_OK",
-    canonical=PASSIVE_RETURN_NEVER_SET ^ CALLABLES_OK,
+    "PASSIVE_RETURN_NO_VALUE ^ CALLABLES_OK",
+    canonical=PASSIVE_RETURN_NO_VALUE ^ CALLABLES_OK,
 )
 PASSIVE_NO_FETCH = util.symbol(
     "PASSIVE_NO_FETCH", "PASSIVE_OFF ^ SQL_OK", canonical=PASSIVE_OFF ^ SQL_OK

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index ccf05a7833ba81ddaed2b76d2bde486419c23fde..3c26f7247dfe0e772a779335276c21d1b25d2d59 100644 (file)
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -2715,7 +2715,7 @@ class Mapper(InspectionAttr):
         return self._identity_key_from_state(state, attributes.PASSIVE_OFF)
 
     def _identity_key_from_state(
-        self, state, passive=attributes.PASSIVE_RETURN_NEVER_SET
+        self, state, passive=attributes.PASSIVE_RETURN_NO_VALUE
     ):
         dict_ = state.dict
         manager = state.manager
@@ -2769,7 +2769,7 @@ class Mapper(InspectionAttr):
         return {prop.key for prop in self._all_pk_props}
 
     def _get_state_attr_by_column(
-        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NEVER_SET
+        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NO_VALUE
     ):
         prop = self._columntoproperty[column]
         return state.manager[prop.key].impl.get(state, dict_, passive=passive)
@@ -2790,7 +2790,7 @@ class Mapper(InspectionAttr):
         )
 
     def _get_committed_state_attr_by_column(
-        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NEVER_SET
+        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NO_VALUE
     ):
 
         prop = self._columntoproperty[column]

diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py
index 17488f2365a1545ef50489a43b0e228fa918bf35..8d244c4fa417348e4cfcc1d84ad476259f7883d2 100644 (file)
--- a/test/orm/test_attributes.py
+++ b/test/orm/test_attributes.py
@@ -1028,31 +1028,27 @@ class GetNoValueTest(fixtures.ORMTest):
             attributes.PASSIVE_NO_RESULT,
         )
 
-    def test_passive_no_result_never_set(self):
-        attr, state, dict_ = self._fixture(attributes.NEVER_SET)
+    def test_passive_no_result_no_value(self):
+        attr, state, dict_ = self._fixture(attributes.NO_VALUE)
         eq_(
             attr.get(state, dict_, passive=attributes.PASSIVE_NO_INITIALIZE),
             attributes.PASSIVE_NO_RESULT,
         )
         assert "attr" not in dict_
 
-    def test_passive_ret_never_set_never_set(self):
-        attr, state, dict_ = self._fixture(attributes.NEVER_SET)
+    def test_passive_ret_no_value(self):
+        attr, state, dict_ = self._fixture(attributes.NO_VALUE)
         eq_(
-            attr.get(
-                state, dict_, passive=attributes.PASSIVE_RETURN_NEVER_SET
-            ),
-            attributes.NEVER_SET,
+            attr.get(state, dict_, passive=attributes.PASSIVE_RETURN_NO_VALUE),
+            attributes.NO_VALUE,
         )
         assert "attr" not in dict_
 
-    def test_passive_ret_never_set_empty(self):
+    def test_passive_ret_no_value_empty(self):
         attr, state, dict_ = self._fixture(None)
         eq_(
-            attr.get(
-                state, dict_, passive=attributes.PASSIVE_RETURN_NEVER_SET
-            ),
-            attributes.NEVER_SET,
+            attr.get(state, dict_, passive=attributes.PASSIVE_RETURN_NO_VALUE),
+            attributes.NO_VALUE,
         )
         assert "attr" not in dict_
 
@@ -1581,7 +1577,7 @@ class PendingBackrefTest(fixtures.ORMTest):
         )
         eq_(lazy_posts.call_count, 1)
 
-    def test_passive_history_collection_never_set(self):
+    def test_passive_history_collection_no_value(self):
         Post, Blog, lazy_posts = self._fixture()
 
         lazy_posts.return_value = attributes.PASSIVE_NO_RESULT
@@ -1594,10 +1590,10 @@ class PendingBackrefTest(fixtures.ORMTest):
             attributes.instance_dict(b),
         )
 
-        # this sets up NEVER_SET on b.posts
+        # this sets up NO_VALUE on b.posts
         p.blog = b
 
-        eq_(state.committed_state, {"posts": attributes.NEVER_SET})
+        eq_(state.committed_state, {"posts": attributes.NO_VALUE})
         assert "posts" not in dict_
 
         # then suppose the object was made transient again,
@@ -1607,7 +1603,7 @@ class PendingBackrefTest(fixtures.ORMTest):
         p2 = Post("asdf")
         p2.blog = b
 
-        eq_(state.committed_state, {"posts": attributes.NEVER_SET})
+        eq_(state.committed_state, {"posts": attributes.NO_VALUE})
         eq_(dict_["posts"], [p2])
 
         # then this would fail.
@@ -2080,17 +2076,17 @@ class HistoryTest(fixtures.TestBase):
         assert "someattr" not in f.__dict__
         assert "someattr" not in attributes.instance_state(f).committed_state
 
-    def test_collection_never_set(self):
+    def test_collection_no_value(self):
         Foo = self._fixture(uselist=True, useobject=True, active_history=True)
         f = Foo()
         eq_(self._someattr_history(f, passive=True), (None, None, None))
 
-    def test_scalar_obj_never_set(self):
+    def test_scalar_obj_no_value(self):
         Foo = self._fixture(uselist=False, useobject=True, active_history=True)
         f = Foo()
         eq_(self._someattr_history(f, passive=True), (None, None, None))
 
-    def test_scalar_never_set(self):
+    def test_scalar_no_value(self):
         Foo = self._fixture(
             uselist=False, useobject=False, active_history=True
         )
@@ -3483,14 +3479,14 @@ class EventPropagateTest(fixtures.TestBase):
             b = B()
             b.attrib = "foo"
             eq_(b.attrib, "foo")
-            eq_(canary, [("foo", attributes.NEVER_SET)])
+            eq_(canary, [("foo", attributes.NO_VALUE)])
 
             c = C()
             c.attrib = "bar"
             eq_(c.attrib, "bar")
             eq_(
                 canary,
-                [("foo", attributes.NEVER_SET), ("bar", attributes.NEVER_SET)],
+                [("foo", attributes.NO_VALUE), ("bar", attributes.NO_VALUE)],
             )
 
     def test_propagate(self):
@@ -3531,16 +3527,13 @@ class EventPropagateTest(fixtures.TestBase):
             d1 = D()
             b.attrib = d1
             is_(b.attrib, d1)
-            eq_(canary, [(d1, attributes.NEVER_SET)])
+            eq_(canary, [(d1, attributes.NO_VALUE)])
 
             c = C()
             d2 = D()
             c.attrib = d2
             is_(c.attrib, d2)
-            eq_(
-                canary,
-                [(d1, attributes.NEVER_SET), (d2, attributes.NEVER_SET)],
-            )
+            eq_(canary, [(d1, attributes.NO_VALUE), (d2, attributes.NO_VALUE)])
 
     def _test_propagate_fixtures(self, active_history, useobject):
         classes = [None, None, None, None]

diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py
index 8d1e82fb540bf5e8f050454bc67db60d9767b90b..0ababc26250fead04780ea5aa871bd02524db8df 100644 (file)
--- a/test/orm/test_lazy_relations.py
+++ b/test/orm/test_lazy_relations.py
@@ -1096,9 +1096,9 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
-            attributes.NEVER_SET,
+            attributes.NO_VALUE,
         )
         assert "user_id" not in a1.__dict__
         assert "user" not in a1.__dict__
@@ -1109,7 +1109,7 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get_history(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
             ((), (), ()),
         )
@@ -1174,7 +1174,7 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
             User(name="ed"),
         )
@@ -1185,7 +1185,7 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get_history(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
             ((), [User(name="ed")], ()),
         )