Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #31966 from kg8m/fix_limited_ids_for
Use column alias of primary_key in limited_ids_for
  • Loading branch information
kamipo committed Feb 28, 2018
1 parent 853054b commit 851618c
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 17 deletions.
8 changes: 8 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
* Fix `#columns_for_distinct` of MySQL and PostgreSQL to make
`ActiveRecord::FinderMethods#limited_ids_for` use correct primary key values
even if `ORDER BY` columns include other table's primary key.

Fixes #28364.

*Takumi Kagiyama*

* Make `reflection.klass` raise if `polymorphic?` not to be misused.

Fixes #31876.
Expand Down
Expand Up @@ -515,7 +515,7 @@ def columns_for_distinct(columns, orders) # :nodoc:
s.gsub(/\s+(?:ASC|DESC)\b/i, "")
}.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }

[super, *order_columns].join(", ")
(order_columns << super).join(", ")
end

def strict_mode?
Expand Down
Expand Up @@ -583,7 +583,7 @@ def columns_for_distinct(columns, orders) #:nodoc:
.gsub(/\s+NULLS\s+(?:FIRST|LAST)\b/i, "")
}.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }

[super, *order_columns].join(", ")
(order_columns << super).join(", ")
end

def update_table_definition(table_name, base) # :nodoc:
Expand Down
10 changes: 5 additions & 5 deletions activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb
Expand Up @@ -25,25 +25,25 @@ def test_columns_for_distinct_zero_orders
end

def test_columns_for_distinct_one_order
assert_equal "posts.id, posts.created_at AS alias_0",
assert_equal "posts.created_at AS alias_0, posts.id",
@conn.columns_for_distinct("posts.id", ["posts.created_at desc"])
end

def test_columns_for_distinct_few_orders
assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1",
assert_equal "posts.created_at AS alias_0, posts.position AS alias_1, posts.id",
@conn.columns_for_distinct("posts.id", ["posts.created_at desc", "posts.position asc"])
end

def test_columns_for_distinct_with_case
assert_equal(
"posts.id, CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0",
"CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0, posts.id",
@conn.columns_for_distinct("posts.id",
["CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END"])
)
end

def test_columns_for_distinct_blank_not_nil_orders
assert_equal "posts.id, posts.created_at AS alias_0",
assert_equal "posts.created_at AS alias_0, posts.id",
@conn.columns_for_distinct("posts.id", ["posts.created_at desc", "", " "])
end

Expand All @@ -52,7 +52,7 @@ def test_columns_for_distinct_with_arel_order
def order.to_sql
"posts.created_at desc"
end
assert_equal "posts.id, posts.created_at AS alias_0",
assert_equal "posts.created_at AS alias_0, posts.id",
@conn.columns_for_distinct("posts.id", [order])
end

Expand Down
Expand Up @@ -263,25 +263,25 @@ def test_columns_for_distinct_zero_orders
end

def test_columns_for_distinct_one_order
assert_equal "posts.id, posts.created_at AS alias_0",
assert_equal "posts.created_at AS alias_0, posts.id",
@connection.columns_for_distinct("posts.id", ["posts.created_at desc"])
end

def test_columns_for_distinct_few_orders
assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1",
assert_equal "posts.created_at AS alias_0, posts.position AS alias_1, posts.id",
@connection.columns_for_distinct("posts.id", ["posts.created_at desc", "posts.position asc"])
end

def test_columns_for_distinct_with_case
assert_equal(
"posts.id, CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0",
"CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0, posts.id",
@connection.columns_for_distinct("posts.id",
["CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END"])
)
end

def test_columns_for_distinct_blank_not_nil_orders
assert_equal "posts.id, posts.created_at AS alias_0",
assert_equal "posts.created_at AS alias_0, posts.id",
@connection.columns_for_distinct("posts.id", ["posts.created_at desc", "", " "])
end

Expand All @@ -290,23 +290,23 @@ def test_columns_for_distinct_with_arel_order
def order.to_sql
"posts.created_at desc"
end
assert_equal "posts.id, posts.created_at AS alias_0",
assert_equal "posts.created_at AS alias_0, posts.id",
@connection.columns_for_distinct("posts.id", [order])
end

def test_columns_for_distinct_with_nulls
assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls first"])
assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls last"])
assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls first"])
assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls last"])
end

def test_columns_for_distinct_without_order_specifiers
assert_equal "posts.title, posts.updater_id AS alias_0",
assert_equal "posts.updater_id AS alias_0, posts.title",
@connection.columns_for_distinct("posts.title", ["posts.updater_id"])

assert_equal "posts.title, posts.updater_id AS alias_0",
assert_equal "posts.updater_id AS alias_0, posts.title",
@connection.columns_for_distinct("posts.title", ["posts.updater_id nulls last"])

assert_equal "posts.title, posts.updater_id AS alias_0",
assert_equal "posts.updater_id AS alias_0, posts.title",
@connection.columns_for_distinct("posts.title", ["posts.updater_id nulls first"])
end

Expand Down
5 changes: 5 additions & 0 deletions activerecord/test/cases/finder_test.rb
Expand Up @@ -1180,6 +1180,11 @@ def test_find_with_order_on_included_associations_with_construct_finder_sql_for_
order("author_addresses_authors.id DESC").limit(3).to_a.size
end

def test_find_with_eager_loading_collection_and_ordering_by_collection_primary_key
assert_equal Post.first, Post.eager_load(comments: :ratings).
order("posts.id, ratings.id, comments.id").first
end

def test_find_with_nil_inside_set_passed_for_one_attribute
client_of = Company.
where(client_of: [2, 1, nil],
Expand Down

0 comments on commit 851618c

Please sign in to comment.