Potential 'order_by("name desc")' regression with 1.0+

[Luke Macken]

Hello,

I'm currently hitting a regression with 1.0.0+. This code worked with previous versions.

https://github.com/fedora-infra/bodhi/blob/develop/bodhi/views/generic.py#L42-L56

    query = db.query(
        bodhi.models.User,
        sa.func.count(bodhi.models.User.comments).label('count_1')
    ).join(bodhi.models.Comment)
    query = query\
        .order_by('count_1 desc')\
        .filter(bodhi.models.Comment.timestamp > start_time)

    for user in blacklist:
        query = query.filter(bodhi.models.User.name != user)

    return query\
        .group_by(bodhi.models.User)\
        .limit(5)\
        .all()

ProgrammingError: (psycopg2.ProgrammingError) syntax error at or near "desc"                                                                                  
LINE 2: ...(users.id = comments.user_id) AS count_1, count_1 desc AS an...                                                                                    
                                                             ^                                                                                                
 [SQL: 'SELECT anon_1.users_id AS anon_1_users_id, anon_1.users_name AS anon_1_users_name, anon_1.count_1 AS anon_1_count_1, anon_1.anon_2 AS anon_1_anon_2, re
leases_1.id AS releases_1_id, releases_1.name AS releases_1_name, releases_1.long_name AS releases_1_long_name, releases_1.version AS releases_1_version, relea
ses_1.id_prefix AS releases_1_id_prefix, releases_1.branch AS releases_1_branch, releases_1.dist_tag AS releases_1_dist_tag, releases_1.stable_tag AS releases_
1_stable_tag, releases_1.testing_tag AS releases_1_testing_tag, releases_1.candidate_tag AS releases_1_candidate_tag, releases_1.pending_testing_tag AS release
s_1_pending_testing_tag, releases_1.pending_stable_tag AS releases_1_pending_stable_tag, releases_1.override_tag AS releases_1_override_tag, releases_1.state A
S releases_1_state, builds_1.id AS builds_1_id, builds_1.nvr AS builds_1_nvr, builds_1.inherited AS builds_1_inherited, builds_1.package_id AS builds_1_package
_id, builds_1.release_id AS builds_1_release_id, builds_1.update_id AS builds_1_update_id, packages_1.id AS packages_1_id, packages_1.name AS packages_1_name,
packages_1.requirements AS packages_1_requirements, packages_1.stack_id AS packages_1_stack_id, stacks_1.id AS stacks_1_id, stacks_1.name AS stacks_1_name, sta
cks_1.description AS stacks_1_description, stacks_1.requirements AS stacks_1_requirements, buildroot_overrides_1.id AS buildroot_overrides_1_id, buildroot_over
rides_1.build_id AS buildroot_overrides_1_build_id, buildroot_overrides_1.submitter_id AS buildroot_overrides_1_submitter_id, buildroot_overrides_1.notes AS bu
ildroot_overrides_1_notes, buildroot_overrides_1.submission_date AS buildroot_overrides_1_submission_date, buildroot_overrides_1.expiration_date AS buildroot_o
verrides_1_expiration_date, buildroot_overrides_1.expired_date AS buildroot_overrides_1_expired_date \nFROM (SELECT users.id AS users_id, users.name AS users_n
ame, count(users.id = comments.user_id) AS count_1, count_1 desc AS anon_2 \nFROM users JOIN comments ON users.id = comments.user_id \nWHERE comments.timestamp
 > %(timestamp_1)s AND users.name != %(name_1)s AND users.name != %(name_2)s GROUP BY users.id, users.name ORDER BY count_1 desc \n LIMIT %(param_1)s) AS anon_
1 LEFT OUTER JOIN buildroot_overrides AS buildroot_overrides_1 ON anon_1.users_id = buildroot_overrides_1.submitter_id LEFT OUTER JOIN builds AS builds_1 ON bu
ilds_1.id = buildroot_overrides_1.build_id LEFT OUTER JOIN releases AS releases_1 ON releases_1.id = builds_1.release_id LEFT OUTER JOIN packages AS packages_1
 ON packages_1.id = builds_1.package_id LEFT OUTER JOIN stacks AS stacks_1 ON stacks_1.id = packages_1.stack_id ORDER BY anon_1.anon_2'] [parameters: {'name_2'
: 'autoqa', 'name_1': 'bodhi', 'param_1': 5, 'timestamp_1': datetime.datetime(2015, 4, 20, 18, 59, 41, 509054)}]                             

[Mike Bayer]

if you can provide self-contained tests cases rather than pointing me to full applications, that would be very helpful. As it turns out, I can identify from the query here what it's doing in order to make a test case, but anyone else attempting to triage this wouldn't have enough information to go on.

Here's the test case:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)

    bs = relationship("B", lazy=False)


class B(Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)
    a_id = Column(ForeignKey('a.id'))

e = create_engine("postgresql://scott:tiger@localhost/test", echo=True)
Base.metadata.create_all(e)

s = Session(e)

q = s.query(A, func.count(B.id).label('count_1')).join(A.bs)
q = q.order_by('count_1 desc').group_by(A.id).limit(5)

print(q.all())

What is happening is that new logic described in http://docs.sqlalchemy.org/en/rel_1_0/changelog/migration_10.html#order-by-and-group-by-are-special-cases is being sucked into a behavior used by eager loading, where it attempts to add columns to the columns clause of a subquery so that ORDER BY works when it is wrapped.

The immediate solution is that because you are using text to represent the exact SQL you want, you should be using text() so that Core/ORM knows this is a literal textual construct that isn't to be interpreted:

q = s.query(A, func.count(B.id).label('count_1')).join(A.bs)
q = q.order_by(text('count_1 desc')).group_by(A.id).limit(5)

this unfortunately is another 1.0 regression however as the textual reference produced by order_by('foo') isn't appropriate for copying into the columns clause.

[Mike Bayer]

• altered part of the use contract first set up in #2992; we now skip textual label references when copying ORDER BY elements to the joined-eager-load subquery, as we can't know that these expressions are compatible with this placement; either because they are meant for text(), or because they refer to label names already stated and aren't bound to a table. fixes #3392

→ <<cset 34f98a63b54a>>

[Mike Bayer]

thanks for reporting.

As I'm unable to verify this against your original code it would be very helpful if you can test your code against the latest master revision to verify things are fixed on your end. thanks!

[Luke Macken]

Hey Mike,

I can confirm that this does indeed fix the problem, thank you!

And apologies for the drive-by bug report, I'm usually pretty good about providing isolated test cases.

Cheers!

[Mike Bayer]

ok great, thanks for testing!