From: Mike Bayer Date: Thu, 23 May 2019 22:01:07 +0000 (-0400) Subject: Unify NO_VALUE and NEVER_SET X-Git-Tag: rel_1_4_0b1~862^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f146f19d4bf1f9150785e22d37a62dcbe3436c9a;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 index 0000000000..c4629db36f --- /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 01f19d9917..321ab7d6fb 100644 --- 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 f809d5891b..4d308d26b3 100644 --- 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 ccf05a7833..3c26f7247d 100644 --- 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 17488f2365..8d244c4fa4 100644 --- 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 8d1e82fb54..0ababc2625 100644 --- 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")], ()), )