Can I get some eyes on this? It's a relatively trivial change, but
without it Oracle and Postgres generate invalid SQL in particular
TL;DR version: the construct_limited_ids_condition function passes the
order parameters from the relation to the adapter's 'distinct' method,
but it ignores values set with 'reorder'.
On Postgres and Oracle (perhaps others), columns used in the ORDER BY
clause must appear in the SELECT DISTINCT clause or the SQL is
invalid. In the pull request's test, this code:
0').reorder('posts.comments_count DESC', 'posts.taggings_count
generates the following SQL without the patch:
SELECT DISTINCT "posts".id, comments.id AS alias_0 FROM "posts" LEFT
OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" WHERE
"posts"."author_id" = 1 AND (comments.id > 0) ORDER BY
posts.comments_count ASC, posts.taggings_count ASC LIMIT 1
Note that generated SELECT clause references the wrong order, while
the ORDER BY clause is correct.
On Oracle, the situation is even worse - there, the generated SQL
actually references the aliases from the SELECT and either silently
fails (sorting on the wrong thing) for a single column in the reorder,
or blows up with an error if the count of terms in 'order' and
The patch fixes the issue by using whatever's been passed to reorder
to build that SQL - a pretty minor change.
On a related note, the current mechanism for handling 'reorder' seems
pretty strange; what's the design motivation behind keeping the values
passed to reorder separate from order? As a side-effect of that
implementation, this code:
SomeModel.order('foo ASC').reorder('bar DESC').order('baz ASC')
does not (as might be expected) sort the returned record by baz - the
reorder overrides all previous *and* subsequent orders.