[Sqlalchemy-commits] sqlalchemy: - allow compatibility with string ops passed here fr...

[<co...@sq...>]

details:   http://hg.sqlalchemy.org/sqlalchemy/sqlalchemy/rev/112024eb7fb2
changeset: 8536:112024eb7fb2
user:      Mike Bayer <mi...@zz...>
date:      Mon Aug 13 10:57:33 2012 -0400
description:
- allow compatibility with string ops passed here from custom libraries
Subject: sqlalchemy: - [feature] Adding/removing None from a mapped collection

details:   http://hg.sqlalchemy.org/sqlalchemy/sqlalchemy/rev/858e49f79f8a
changeset: 8537:858e49f79f8a
user:      Mike Bayer <mi...@zz...>
date:      Mon Aug 13 11:00:58 2012 -0400
description:
- [feature] Adding/removing None from a mapped collection
now generates attribute events.  Previously, a None
append would be ignored in some cases.  Related
to [ticket:2229].

- [feature] The presence of None in a mapped collection
now raises an error during flush.   Previously,
None values in collections would be silently ignored.
[ticket:2229]

diffstat:

 CHANGES                           |  10 +++++
 lib/sqlalchemy/orm/attributes.py  |   4 +-
 lib/sqlalchemy/orm/collections.py |   8 ++--
 lib/sqlalchemy/orm/dependency.py  |  18 +++++----
 lib/sqlalchemy/orm/unitofwork.py  |   6 +++
 lib/sqlalchemy/sql/expression.py  |   4 ++
 test/orm/test_attributes.py       |  40 ++++++++++++++++++++
 test/orm/test_cascade.py          |  78 +++++++++++++++++++++++++++++++++-----
 8 files changed, 144 insertions(+), 24 deletions(-)

diffs (truncated from 340 to 300 lines):

diff -r 62942620b0b4 -r 858e49f79f8a CHANGES
--- a/CHANGES	Sun Aug 12 21:07:24 2012 -0400
+++ b/CHANGES	Mon Aug 13 11:00:58 2012 -0400
@@ -123,6 +123,16 @@
     Both features should be avoided, however.
     [ticket:2372]
 
+  - [feature] Adding/removing None from a mapped collection
+    now generates attribute events.  Previously, a None
+    append would be ignored in some cases.  Related
+    to [ticket:2229].
+
+  - [feature] The presence of None in a mapped collection
+    now raises an error during flush.   Previously,
+    None values in collections would be silently ignored.
+    [ticket:2229]
+
   - [bug] Improvements to joined/subquery eager
     loading dealing with chains of subclass entities
     sharing a common base, with no specific "join depth"
diff -r 62942620b0b4 -r 858e49f79f8a lib/sqlalchemy/orm/attributes.py
--- a/lib/sqlalchemy/orm/attributes.py	Sun Aug 12 21:07:24 2012 -0400
+++ b/lib/sqlalchemy/orm/attributes.py	Mon Aug 13 11:00:58 2012 -0400
@@ -1022,6 +1022,9 @@
         return child
 
     def emit_backref_from_collection_append_event(state, child, initiator):
+        if child is None:
+            return
+
         child_state, child_dict = instance_state(child), \
                                     instance_dict(child)
         child_impl = child_state.manager[key].impl
@@ -1207,7 +1210,6 @@
     def from_collection(cls, attribute, state, current):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
         current = getattr(current, '_sa_adapter')
-
         if original is NO_VALUE:
             return cls(list(current), (), ())
         elif original is _NO_HISTORY:
diff -r 62942620b0b4 -r 858e49f79f8a lib/sqlalchemy/orm/collections.py
--- a/lib/sqlalchemy/orm/collections.py	Sun Aug 12 21:07:24 2012 -0400
+++ b/lib/sqlalchemy/orm/collections.py	Mon Aug 13 11:00:58 2012 -0400
@@ -716,7 +716,7 @@
         operation.
 
         """
-        if initiator is not False and item is not None:
+        if initiator is not False:
             if self.invalidated:
                 self._warn_invalidated()
             return self.attr.fire_append_event(
@@ -734,7 +734,7 @@
         an initiator value from a chained operation.
 
         """
-        if initiator is not False and item is not None:
+        if initiator is not False:
             if self.invalidated:
                 self._warn_invalidated()
             self.attr.fire_remove_event(
@@ -1032,7 +1032,7 @@
 def __set(collection, item, _sa_initiator=None):
     """Run set events, may eventually be inlined into decorators."""
 
-    if _sa_initiator is not False and item is not None:
+    if _sa_initiator is not False:
         executor = getattr(collection, '_sa_adapter', None)
         if executor:
             item = getattr(executor, 'fire_append_event')(item, _sa_initiator)
@@ -1040,7 +1040,7 @@
 
 def __del(collection, item, _sa_initiator=None):
     """Run del events, may eventually be inlined into decorators."""
-    if _sa_initiator is not False and item is not None:
+    if _sa_initiator is not False:
         executor = getattr(collection, '_sa_adapter', None)
         if executor:
             getattr(executor, 'fire_remove_event')(item, _sa_initiator)
diff -r 62942620b0b4 -r 858e49f79f8a lib/sqlalchemy/orm/dependency.py
--- a/lib/sqlalchemy/orm/dependency.py	Sun Aug 12 21:07:24 2012 -0400
+++ b/lib/sqlalchemy/orm/dependency.py	Mon Aug 13 11:00:58 2012 -0400
@@ -247,7 +247,11 @@
                 self.mapper in uowcommit.mappers
 
     def _verify_canload(self, state):
-        if state is not None and \
+        if self.prop.uselist and state is None:
+            raise exc.FlushError(
+                    "Can't flush None value found in "
+                    "collection %s" % (self.prop, ))
+        elif state is not None and \
             not self.mapper._canload(state,
                             allow_subtypes=not self.enable_typechecks):
             if self.mapper._canload(state, allow_subtypes=True):
@@ -559,10 +563,10 @@
                             pks_changed):
         source = state
         dest = child
+        self._verify_canload(child)
         if dest is None or \
                 (not self.post_update and uowcommit.is_deleted(dest)):
             return
-        self._verify_canload(child)
         if clearkeys:
             sync.clear(dest, self.mapper, self.prop.synchronize_pairs)
         else:
@@ -1032,8 +1036,7 @@
                                                 passive)
             if history:
                 for child in history.added:
-                    if child is None or \
-                            (processed is not None and
+                    if (processed is not None and
                                 (state, child) in processed):
                         continue
                     associationrow = {}
@@ -1044,8 +1047,7 @@
                         continue
                     secondary_insert.append(associationrow)
                 for child in history.deleted:
-                    if child is None or \
-                            (processed is not None and
+                    if (processed is not None and
                             (state, child) in processed):
                         continue
                     associationrow = {}
@@ -1130,6 +1132,8 @@
         if associationrow is None:
             return
 
+        self._verify_canload(child)
+
         if child is not None and not uowcommit.session._contains_state(child):
             if not child.deleted:
                 util.warn(
@@ -1138,8 +1142,6 @@
                     (mapperutil.state_class_str(child), operation, self.prop))
             return False
 
-        self._verify_canload(child)
-
         sync.populate_dict(state, self.parent, associationrow,
                                         self.prop.synchronize_pairs)
         sync.populate_dict(child, self.mapper, associationrow,
diff -r 62942620b0b4 -r 858e49f79f8a lib/sqlalchemy/orm/unitofwork.py
--- a/lib/sqlalchemy/orm/unitofwork.py	Sun Aug 12 21:07:24 2012 -0400
+++ b/lib/sqlalchemy/orm/unitofwork.py	Mon Aug 13 11:00:58 2012 -0400
@@ -29,6 +29,9 @@
         # process "save_update" cascade rules for when
         # an instance is appended to the list of another instance
 
+        if item is None:
+            return
+
         sess = sessionlib._state_session(state)
         if sess:
             prop = state.manager.mapper._props[key]
@@ -40,6 +43,9 @@
         return item
 
     def remove(state, item, initiator):
+        if item is None:
+            return
+
         sess = sessionlib._state_session(state)
         if sess:
             prop = state.manager.mapper._props[key]
diff -r 62942620b0b4 -r 858e49f79f8a lib/sqlalchemy/sql/expression.py
--- a/lib/sqlalchemy/sql/expression.py	Sun Aug 12 21:07:24 2012 -0400
+++ b/lib/sqlalchemy/sql/expression.py	Mon Aug 13 11:00:58 2012 -0400
@@ -3545,6 +3545,10 @@
 
     def __init__(self, left, right, operator, type_=None,
                     negate=None, modifiers=None):
+        # allow compatibility with libraries that
+        # refer to BinaryExpression directly and pass strings
+        if isinstance(operator, basestring):
+            operator = operators.custom_op(operator)
         self.left = _literal_as_text(left).self_group(against=operator)
         self.right = _literal_as_text(right).self_group(against=operator)
         self.operator = operator
diff -r 62942620b0b4 -r 858e49f79f8a test/orm/test_attributes.py
--- a/test/orm/test_attributes.py	Sun Aug 12 21:07:24 2012 -0400
+++ b/test/orm/test_attributes.py	Mon Aug 13 11:00:58 2012 -0400
@@ -2308,6 +2308,46 @@
         f1.barset.add(b1)
         assert f1.barset.pop().data == 'some bar appended'
 
+    def test_none_on_collection_event(self):
+        """test that append/remove of None in collections emits events.
+
+        This is new behavior as of 0.8.
+
+        """
+        class Foo(object):
+            pass
+        class Bar(object):
+            pass
+        instrumentation.register_class(Foo)
+        instrumentation.register_class(Bar)
+        attributes.register_attribute(Foo, 'barlist', uselist=True,
+                useobject=True)
+        canary = []
+        def append(state, child, initiator):
+            canary.append((state, child))
+        def remove(state, child, initiator):
+            canary.append((state, child))
+        event.listen(Foo.barlist, 'append', append)
+        event.listen(Foo.barlist, 'remove', remove)
+
+        b1, b2 = Bar(), Bar()
+        f1 = Foo()
+        f1.barlist.append(None)
+        eq_(canary, [(f1, None)])
+
+        canary[:] = []
+        f1 = Foo()
+        f1.barlist = [None, b2]
+        eq_(canary, [(f1, None), (f1, b2)])
+
+        canary[:] = []
+        f1 = Foo()
+        f1.barlist = [b1, None, b2]
+        eq_(canary, [(f1, b1), (f1, None), (f1, b2)])
+
+        f1.barlist.remove(None)
+        eq_(canary, [(f1, b1), (f1, None), (f1, b2), (f1, None)])
+
     def test_propagate(self):
         classes = [None, None, None]
         canary = []
diff -r 62942620b0b4 -r 858e49f79f8a test/orm/test_cascade.py
--- a/test/orm/test_cascade.py	Sun Aug 12 21:07:24 2012 -0400
+++ b/test/orm/test_cascade.py	Mon Aug 13 11:00:58 2012 -0400
@@ -397,20 +397,20 @@
 
         })
 
-    def test_none_skipped_assignment(self):
-        # [ticket:2229] proposes warning/raising on None
-        # for 0.8
+    def test_none_o2m_collection_assignment(self):
         User, Address = self.classes.User, self.classes.Address
         s = Session()
         u1 = User(name='u', addresses=[None])
         s.add(u1)
         eq_(u1.addresses, [None])
-        s.commit()
-        eq_(u1.addresses, [])
-
-    def test_none_skipped_append(self):
-        # [ticket:2229] proposes warning/raising on None
-        # for 0.8
+        assert_raises_message(
+            orm_exc.FlushError,
+            "Can't flush None value found in collection User.addresses",
+            s.commit
+        )
+        eq_(u1.addresses, [None])
+
+    def test_none_o2m_collection_append(self):
         User, Address = self.classes.User, self.classes.Address
         s = Session()
 
@@ -418,8 +418,13 @@
         s.add(u1)
         u1.addresses.append(None)
         eq_(u1.addresses, [None])
-        s.commit()
-        eq_(u1.addresses, [])
+        assert_raises_message(
+            orm_exc.FlushError,
+            "Can't flush None value found in collection User.addresses",
+            s.commit
+        )
+        eq_(u1.addresses, [None])
+
 
 class O2MCascadeDeleteNoOrphanTest(fixtures.MappedTest):
     run_inserts = None
@@ -1797,6 +1802,57 @@
         assert b1 not in a1.bs
         assert b1 in a2.bs
 
+    def test_none_m2m_collection_assignment(self):
+        a, A, B, b, atob = (self.tables.a,
+                                self.classes.A,
+                                self.classes.B,
+                                self.tables.b,
+                                self.tables.atob)
+
+
+        mapper(A, a, properties={
+            'bs': relationship(B,
+                secondary=atob, backref="as")
+        })
+        mapper(B, b)
+