aliased() comparators can be tripped up

new tests for test/orm/query.py OperatorTest using the Node setup from SelfReferentialTest:

nalias = aliased(Node)

self._test(
        nalias.children.any(Node.data=='some data'), 
        "EXISTS (SELECT 1 FROM nodes WHERE "
        "nodes_1.id = nodes.parent_id AND nodes.data = :data_1)")

self._test(
        Node.children.any(nalias.data=='some data'), 
        "EXISTS (SELECT 1 FROM nodes AS nodes_1 WHERE "
        "nodes.id = nodes_1.parent_id AND nodes_1.data = :data_1)"
        )



ualias = aliased(User)
self._test(ualias.id.between(User.id, User.id), "users_1.id BETWEEN users.id AND users.id")

the failures are related to sqlalchemy/orm/util.py AliasedComparator - the operate and reverse_operate methods alias the full clause, which in the case of between over-aliases.

In the node examples, the "auto aliasing" feature which is designed for Node.foo.any(Node.bar==x) takes place, then AliasedComparator's aliasing takes place and it blows up.

The fact that AliasedComparator works at all is the product of the intricacies of comparison - if left and right both feature __eq__, its called in terms of the right side so useralias.id == user.id works because AliasedComparator.operate() is never called. If comparing useralias.id==useralias2.id, it works because the aliasing doesn't affect things which are already aliased. So when this thing does break, its totally weird and arbitrary.

The better way to do this would be to do away with AliasedComparator. Instead, create a clone of the target PropComparator, and just have its __clause_element__() method return the aliased construct (or do some similar kind of method overriding). The PropComparator will then need to be sensitive to its clause element, and pass it along to all SQL construction operations as the "left side". This removes the complexity of having to "post-guess" how to alias the clause.

As a bonus, it's a very slight behavioral contract change to user-defined comparators, so an 0.5.0 milestone. They already have a __clause_element__() method and the change would be that they'd want to use this method if aliasing is expected to be used. As an alternative, we can use a new method like __aliased__() to indicate a PropComparator which is compatible with the new style and perhaps fall back to the old method for those which don't have it.

Comments (2)