Hold implicitly created collections in a pending area

Accessing a collection-oriented attribute on a newly created object no
longer mutates ``__dict__``, but still returns an empty collection as has
always been the case.   This allows collection-oriented attributes to work
consistently in comparison to scalar attributes which return ``None``, but
also don't mutate ``__dict__``.  In order to accommodate for the collection
being mutated, the same empty collection is returned each time once
initially created, and when it is mutated (e.g. an item appended, added,
etc.) it is then moved into ``__dict__``.  This removes the last of
mutating side-effects on read-only attribute access within the ORM.

Fixes: #4519
Change-Id: I06a058d24e6eb24b5c6b6092d3f8b31cf9c244ae

6 files changed, 79 insertions, 20 deletions

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 321ab7d6f..31c351bb0 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -653,8 +653,8 @@ class AttributeImpl(object):
         """
         raise NotImplementedError()
 
-    def initialize(self, state, dict_):
-        """Initialize the given state's attribute with an empty value."""
+    def _default_value(self, state, dict_):
+        """Produce an empty value for an uninitialized scalar attribute."""
 
         value = None
         for fn in self.dispatch.init_scalar:
@@ -710,8 +710,7 @@ class AttributeImpl(object):
             if not passive & INIT_OK:
                 return NO_VALUE
             else:
-                # Return a new, empty value
-                return self.initialize(state, dict_)
+                return self._default_value(state, dict_)
 
     def append(self, state, dict_, value, initiator, passive=PASSIVE_OFF):
         self.set(state, dict_, value, initiator, passive=passive)
@@ -1184,11 +1183,14 @@ class CollectionAttributeImpl(AttributeImpl):
         # del is a no-op if collection not present.
         del dict_[self.key]
 
-    def initialize(self, state, dict_):
-        """Initialize this attribute with an empty collection."""
+    def _default_value(self, state, dict_):
+        """Produce an empty collection for an un-initialized attribute"""
 
-        _, user_data = self._initialize_collection(state)
-        dict_[self.key] = user_data
+        if self.key in state._empty_collections:
+            return state._empty_collections[self.key]
+
+        adapter, user_data = self._initialize_collection(state)
+        adapter._set_empty(user_data)
         return user_data
 
     def _initialize_collection(self, state):
@@ -1287,7 +1289,7 @@ class CollectionAttributeImpl(AttributeImpl):
 
         old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT)
         if old is PASSIVE_NO_RESULT:
-            old = self.initialize(state, dict_)
+            old = self._default_value(state, dict_)
         elif old is orig_iterable:
             # ignore re-assignment of the current collection, as happens
             # implicitly with in-place operators (foo.collection |= other)
@@ -1699,7 +1701,6 @@ class History(History):
     @classmethod
     def from_collection(cls, attribute, state, current):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
-
         if current is NO_VALUE:
             return cls((), (), ())
 
@@ -1892,8 +1893,10 @@ def init_state_collection(state, dict_, key):
     """Initialize a collection attribute and return the collection adapter."""
 
     attr = state.manager[key].impl
-    user_data = attr.initialize(state, dict_)
-    return attr.get_collection(state, dict_, user_data)
+    user_data = attr._default_value(state, dict_)
+    adapter = attr.get_collection(state, dict_, user_data)
+    adapter._reset_empty()
+    return adapter
 
 
 def set_committed_value(instance, key, value):
diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py
index 6bd009fb8..1f50c7b09 100644
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -614,6 +614,7 @@ class CollectionAdapter(object):
         "owner_state",
         "_converter",
         "invalidated",
+        "empty",
     )
 
     def __init__(self, attr, owner_state, data):
@@ -624,6 +625,7 @@ class CollectionAdapter(object):
         data._sa_adapter = self
         self._converter = data._sa_converter
         self.invalidated = False
+        self.empty = False
 
     def _warn_invalidated(self):
         util.warn("This collection has been invalidated.")
@@ -651,12 +653,39 @@ class CollectionAdapter(object):
 
         self._data()._sa_appender(item, _sa_initiator=initiator)
 
+    def _set_empty(self, user_data):
+        assert (
+            not self.empty
+        ), "This collection adapter is alreay in the 'empty' state"
+        self.empty = True
+        self.owner_state._empty_collections[self._key] = user_data
+
+    def _reset_empty(self):
+        assert (
+            self.empty
+        ), "This collection adapter is not in the 'empty' state"
+        self.empty = False
+        self.owner_state.dict[
+            self._key
+        ] = self.owner_state._empty_collections.pop(self._key)
+
+    def _refuse_empty(self):
+        raise sa_exc.InvalidRequestError(
+            "This is a special 'empty' collection which cannot accommodate "
+            "internal mutation operations"
+        )
+
     def append_without_event(self, item):
         """Add or restore an entity to the collection, firing no events."""
+
+        if self.empty:
+            self._refuse_empty()
         self._data()._sa_appender(item, _sa_initiator=False)
 
     def append_multiple_without_event(self, items):
         """Add or restore an entity to the collection, firing no events."""
+        if self.empty:
+            self._refuse_empty()
         appender = self._data()._sa_appender
         for item in items:
             appender(item, _sa_initiator=False)
@@ -670,11 +699,15 @@ class CollectionAdapter(object):
 
     def remove_without_event(self, item):
         """Remove an entity from the collection, firing no events."""
+        if self.empty:
+            self._refuse_empty()
         self._data()._sa_remover(item, _sa_initiator=False)
 
     def clear_with_event(self, initiator=None):
         """Empty the collection, firing a mutation event for each entity."""
 
+        if self.empty:
+            self._refuse_empty()
         remover = self._data()._sa_remover
         for item in list(self):
             remover(item, _sa_initiator=initiator)
@@ -682,6 +715,8 @@ class CollectionAdapter(object):
     def clear_without_event(self):
         """Empty the collection, firing no events."""
 
+        if self.empty:
+            self._refuse_empty()
         remover = self._data()._sa_remover
         for item in list(self):
             remover(item, _sa_initiator=False)
@@ -712,6 +747,10 @@ class CollectionAdapter(object):
         if initiator is not False:
             if self.invalidated:
                 self._warn_invalidated()
+
+            if self.empty:
+                self._reset_empty()
+
             return self.attr.fire_append_event(
                 self.owner_state, self.owner_state.dict, item, initiator
             )
@@ -729,6 +768,10 @@ class CollectionAdapter(object):
         if initiator is not False:
             if self.invalidated:
                 self._warn_invalidated()
+
+            if self.empty:
+                self._reset_empty()
+
             self.attr.fire_remove_event(
                 self.owner_state, self.owner_state.dict, item, initiator
             )
@@ -753,6 +796,7 @@ class CollectionAdapter(object):
             "owner_cls": self.owner_state.class_,
             "data": self.data,
             "invalidated": self.invalidated,
+            "empty": self.empty,
         }
 
     def __setstate__(self, d):
@@ -763,6 +807,7 @@ class CollectionAdapter(object):
         d["data"]._sa_adapter = self
         self.invalidated = d["invalidated"]
         self.attr = getattr(d["owner_cls"], self._key).impl
+        self.empty = d.get("empty", False)
 
 
 def bulk_replace(values, existing_adapter, new_adapter, initiator=None):
diff --git a/lib/sqlalchemy/orm/events.py b/lib/sqlalchemy/orm/events.py
index d73a20e93..5bf6ff418 100644
--- a/lib/sqlalchemy/orm/events.py
+++ b/lib/sqlalchemy/orm/events.py
@@ -2267,6 +2267,9 @@ class AttributeEvents(event.Events):
 
         .. seealso::
 
+            :meth:`.AttributeEvents.init_collection` - collection version
+            of this event
+
             :class:`.AttributeEvents` - background on listener options such
             as propagation to subclasses.
 
@@ -2312,6 +2315,9 @@ class AttributeEvents(event.Events):
             :class:`.AttributeEvents` - background on listener options such
             as propagation to subclasses.
 
+            :meth:`.AttributeEvents.init_scalar` - "scalar" version of this
+            event.
+
         """
 
     def dispose_collection(self, target, collection, collection_adapter):
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 8b03955e2..0d7ce8bbf 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -1653,12 +1653,13 @@ class RelationshipProperty(StrategizedProperty):
             return
 
         if self.uselist:
-            instances = source_state.get_impl(self.key).get(
-                source_state, source_dict
-            )
-            if hasattr(instances, "_sa_adapter"):
-                # convert collections to adapters to get a true iterator
-                instances = instances._sa_adapter
+            impl = source_state.get_impl(self.key)
+            instances_iterable = impl.get_collection(source_state, source_dict)
+
+            # if this is a CollectionAttributeImpl, then empty should
+            # be False, otherwise "self.key in source_dict" should not be
+            # True
+            assert not instances_iterable.empty if impl.collection else True
 
             if load:
                 # for a full merge, pre-load the destination collection,
@@ -1669,7 +1670,7 @@ class RelationshipProperty(StrategizedProperty):
                 dest_state.get_impl(self.key).get(dest_state, dest_dict)
 
             dest_list = []
-            for current in instances:
+            for current in instances_iterable:
                 current_state = attributes.instance_state(current)
                 current_dict = attributes.instance_dict(current)
                 _recursive[(current_state, self)] = True
diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index c6252b6b8..f6c06acc8 100644
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -311,6 +311,10 @@ class InstanceState(interfaces.InspectionAttrInfo):
         return {}
 
     @util.memoized_property
+    def _empty_collections(self):
+        return {}
+
+    @util.memoized_property
     def mapper(self):
         """Return the :class:`.Mapper` used for this mapped object."""
         return self.manager.mapper
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index d05adecdb..b0dffe5dd 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -488,7 +488,7 @@ class NoLoader(AbstractRelationshipLoader):
     ):
         def invoke_no_load(state, dict_, row):
             if self.uselist:
-                state.manager.get_impl(self.key).initialize(state, dict_)
+                attributes.init_state_collection(state, dict_, self.key)
             else:
                 dict_[self.key] = None