Relationship __ne__ does not handle aliased() properly when comparing NULL

[Brandon Clarke]

Found from SO post Here.

Sample code to reproduce:

from sqlalchemy import *
from sqlalchemy.orm import aliased
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import relation, sessionmaker, relationship, backref

Base = declarative_base()

class Parent(Base):
    __tablename__ = 'parent'
    id = Column(Integer, primary_key=True)
    children = relation('Child', back_populates='parents')

class Child(Base):
    __tablename__ = 'child'
    id = Column(Integer, primary_key=True)
    parent_id = Column(Integer, ForeignKey('parent.id'))
    parents = relation('Parent', back_populates='children', uselist=False)

aChild = aliased(Child)

Session = sessionmaker()
Session = Session()

print Session.query(aChild.id).filter(~(aChild.parents == None))
""" SELECT children_1.id AS children_1_id
    FROM children AS children_1
    WHERE children_1.parent_id IS NOT NULL
"""

print Session.query(aChild.id).filter(aChild.parents != None) #failing case
""" SELECT children_1.id AS children_1_id
    FROM children AS children_1, children
    WHERE children.parent_id IS NOT NULL
"""

I did some poking around and have a proposed fix in relationships.py, below:

@@ -1291,8 +1291,8 @@ class RelationshipProperty(StrategizedProperty):
         """
             if isinstance(other, (util.NoneType, expression.Null)):
                 if self.property.direction == MANYTOONE:
-                    return sql.or_(*[x != None for x in
-                                     self.property._calculated_foreign_keys])
+                    return _orm_annotate(~self.property._optimized_compare(
+                        None, adapt_source=self.adapter))

I basically used the logic in place for eq and inverted it. It passes all existing test cases, I'm just working on creating a test case for this failure mode. I was planning on wrapping up a pull request today. If there are any suggestions for where to locate the test, please let me know. First time submitting anything here, so if I'm doing anything wrong, please let me know that too

[Mike Bayer]

two things going on here. One is for this issue, we aren't using self.adapter there. Other one is, it doesn't have the _orm_annotate().

this works also, though I will look into negating optimized compare also as its odd I didn't use that here.

@@ -1291,8 +1292,11 @@ class RelationshipProperty(StrategizedProperty):
             """
             if isinstance(other, (util.NoneType, expression.Null)):
                 if self.property.direction == MANYTOONE:
-                    return sql.or_(*[x != None for x in
+                    crit = sql.or_(*[x != None for x in
                                      self.property._calculated_foreign_keys])
+                    if self.adapter:
+                        crit = self.adapter(crit)
+                    return _orm_annotate(crit)
                 else:
                     return self._criterion_exists()
             elif self.property.uselist:

for the tests I can smoke out the lack of _orm_annotate() like this:

diff --git a/test/orm/inheritance/test_relationship.py b/test/orm/inheritance/test_relationship.py
index db2cd1e..4889ca5 100644
--- a/test/orm/inheritance/test_relationship.py
+++ b/test/orm/inheritance/test_relationship.py
@@ -328,6 +328,7 @@ class SelfReferentialJ2JSelfTest(fixtures.MappedTest):
     def test_relationship_compare(self):
         sess = self._five_obj_fixture()
         e1 = sess.query(Engineer).filter_by(name='e1').one()
+        e2 = sess.query(Engineer).filter_by(name='e2').one()

         eq_(sess.query(Engineer)
                 .join(Engineer.engineers, aliased=True)
@@ -339,6 +340,11 @@ class SelfReferentialJ2JSelfTest(fixtures.MappedTest):
                 .filter(Engineer.reports_to == e1).all(),
             [e1])

+        eq_(sess.query(Engineer)
+                .join(Engineer.engineers, aliased=True)
+                .filter(Engineer.reports_to != None).all(),
+            [e1, e2])
+
 class M2MFilterTest(fixtures.MappedTest):

     run_setup_mappers = 'once'

to find that, I took out the _orm_annotate() from the eq() case and ran the tests to see what fails. I knew that the failure would be in orm/inheritance because that's where the more crazy tests for things that require full annotations typically are. the annotation aspect here refers to flags that you'd see in the object's _annotations dictionary. it's not code that AFAIK anyone other than I am familiar with, or even aware of :) so i wouldn't sweat it.

bugs like this are important so i can patch this one in a couple of minutes here unless you wanted to deliver the pull request yourself, but also I want to decide if i want to use the optimized compare here or not.

[Brandon Clarke]

I'll leave it to you, I'm just looking at the one tree while you've got a view of the whole forest. Glad I could help

[Mike Bayer]

OK, I like the optimized compare approach here. Not as simple as far as how it gets to the answer but goes through consistent channels, so that's good.

for the test, I'll use the same approach here to find it (but I already know it's going to be in test/orm/test_query somewhere most likely). First I'll break the __eq__() case in the same way:

@@ -989,7 +989,7 @@ class RelationshipProperty(StrategizedProperty):
                     return ~self._criterion_exists()
                 else:
                     return _orm_annotate(self.property._optimized_compare(
-                        None, adapt_source=self.adapter))
+                        None,)) # adapt_source=self.adapter))
             elif self.property.uselist:
                 raise sa_exc.InvalidRequestError(
                     "Can't compare a collection to an object or collection; "

here's where it failed:

test/orm/test_query.py::OperatorTest::test_selfref_relationship FAILED

I'm not super thrilled that this is the only failure but we can build on that, so

@@ -916,6 +916,11 @@ class OperatorTest(QueryTest, AssertsCompiledSQL):
         )

         self._test(
+            nalias.parent != None,
+            "nodes_1.parent_id IS NOT NULL"
+        )
+
+        self._test(
             nalias.children == None,
             "NOT (EXISTS ("
             "SELECT 1 FROM nodes WHERE nodes_1.id = nodes.parent_id))",

[Mike Bayer]

ok thats fine

[Mike Bayer]

• Fixed bugs in ORM object comparisons where comparison of many-to-one != None would fail if the source were an aliased class, or if the query needed to apply special aliasing to the expression due to aliased joins or polymorphic querying; also fixed bug in the case where comparing a many-to-one to an object state would fail if the query needed to apply special aliasing due to aliased joins or polymorphic querying. fixes #3310

→ <<cset 305ea84004fe>>

[Mike Bayer]

• Fixed bugs in ORM object comparisons where comparison of many-to-one != None would fail if the source were an aliased class, or if the query needed to apply special aliasing to the expression due to aliased joins or polymorphic querying; also fixed bug in the case where comparing a many-to-one to an object state would fail if the query needed to apply special aliasing due to aliased joins or polymorphic querying. fixes #3310

(cherry picked from commit 305ea84004fe604f461cd3c9438fbc84e3d790b2)

→ <<cset 775ff3419fa0>>