changing a relation from lazy to dynamic generates incorrect SQL

[Former user]

Firstly. Fantastic software guys! Here's the problem: changing lazy=True to lazy='dynamic' on an orm.relation function with a 'secondary_join' and an 'order_by' filter results in broken SQL. Essentially the secondary table is aliased in the SQL but the order_by reference to the secondary is not. I have a small program that exhibits the problem on a MySQL database.

[Former user]

generates the exception when dynamic relation is accessed

[Former user]

output from SQLAlchemy v0.5.5 of broken.py

[Mike Bayer]

I haven't tried the test yet but it's almost certainly that you're trying to order by the "secondary" table. "seconadary" is only appropriate when you absolutely don't care at all about any other columns in that table besides the foreign keys to the related tables. otherwise you need to use the association object pattern.

[Former user]

OK I've looked more closely at this. I guess the reason I think this is a defect is that it is a surprise that changing a perfectly good relation from lazy=True to lazy='dynamic' (so that one can get at the generated query object for filtering etc.) subsequently breaks the code.

Reason

The reason is that the underlying query-generation code happens in strategies.LoadLazyAttribute.__call__ for the one (lazy=True) and dynamic.AppenderMixin._clone for the other (lazy='dynamic'). They generate different query objects. The dynamically generated code aliases the secondary table whereas the lazy=True does not. My code breaks because and order_by clause references the secondary table.

IMHO The generated SQL that fetches the underlying data should really be identical. Or am I missing something important?

The problem occurs because orm.dynamic.AppenderMixin._clone use a query.with_parent API method that aliases the secondary table whereas the orm.strategies.LoadLazyAttribute.__call__ uses LazyLoader.lazy_clause directly... which doesn't alias the secondary table.

Fix

The best fix -- unless I'm missing something -- would be to use the same underlying query generation method... but I don't know enough about the insides of SQLA to do that. But here is a simple fix...

1) add an alias_secondary key to properties.RelationProperty._optimized_compare viz:

    def _optimized_compare(self, value, value_is_parent=False, adapt_source=None,alias_secondary=True):
        if value is not None:
            value = attributes.instance_state(value)
        return self._get_strategy(strategies.LazyLoader).\
                lazy_clause(value, reverse_direction=not value_is_parent, alias_secondary=alias_secondary, adapt_source=adapt_source)

This should break nothing :-)

2) add a query._with_parent method to query.Query that basically mimics the query.with_parent function but give access to the alias_secondary keyword. The API with_parent might call out to this

    def _with_parent(self, instance, property,alias_secondary=True):
        mapper = object_mapper(instance)
    # argument checks omitted....
        prop = mapper.get_property(property, resolve_synonyms=True)
        return self.filter(prop._optimized_compare(instance, value_is_parent=True, alias_secondary=alias_secondary))

3) change the query.with_parent(instance,self.attr.key) in dynamic.AppenderMixin._clone to query._with_parent(instance,self.attr.key,alias_secondary=False).

(and change query.order_by ... to query.order_by(*util.to_list(self.attr.order_by))to mimic thelazy=True` way while we are at it)

These changes Workforme.

[Mike Bayer]

the test is what happens if you use "lazy=False" with your test. that would establish what kind of aliasing we're doing for "order by" and would guide decisions regarding "dynamic".

[Former user]

with lazy=False the generated SQL is the same as for lazy=True (the relation is self-referential... does it fall back to a lazyload?) and no table aliasing occurs. When I add a join_depth=1 then the tables are aliased but the order_by expression is adapted to the correct (secondary) table alias. here in strategies.EagerLoader._create_eager_join

if self.parent_property.order_by:
            context.eager_order_by += eagerjoin._target_adapter.copy_and_process(util.to_list(self.parent_property.order_by))

the problem is that using query.with_parent for dynamic eventually calls LazyLoader.lazy_clause which uses a ClauseAdapter on the parent_property.secondary.alias() It's this alias I can't get to....

If I could pickup that alias then I could adapt the order_by clause before adding it to the query object.

[Mike Bayer]

OK, will have a look at your proposed patches soon.

[Mike Bayer]

OK your fix is the correct approach here. Having the "secondary" table buried in an inaccessible alias is generally inconvenient and it shouldn't be an issue for it to be present as itself. So for 0.6 this is a go.

But it does potentially break a user application which was working around this by re-introducing the "secondary" table in a dynamic query. So I'm a little worried about introducing this in 0.5...its a rare case but the ideal is that ppl can upgrade to 0.5.7 with no changes in behavior. For now I'm targeting this at 0.6. Let me know if you agree that's reasonable.

[Mike Bayer]

85d49bde73b0b131e4b11fee05d0c7a6de626de1.

[Mike Bayer]

Removing milestone: 0.6.0 (automated comment)