Skip to content

Commit

Permalink
Relax table name detection in from to allow any extension like INDE…
Browse files Browse the repository at this point in the history
…X hint

rails#35360 allows table name qualified if `from` has original table name.
But that is still too strict. We have a valid use case that `from` with
INDEX hint (e.g. `from("comments USE INDEX (PRIMARY)")`).
So I've relaxed the table name detection in `from` to allow any
extension like INDEX hint.

Fixes rails#35359.
  • Loading branch information
kamipo committed Mar 1, 2019
1 parent 0c4bf98 commit bff5e79
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 12 deletions.
7 changes: 5 additions & 2 deletions activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1091,14 +1091,17 @@ def arel_column(field)
field = klass.attribute_alias(field) if klass.attribute_alias?(field)
from = from_clause.name || from_clause.value

if klass.columns_hash.key?(field) &&
(!from || from == table.name || from == connection.quote_table_name(table.name))
if klass.columns_hash.key?(field) && (!from || table_name_matches?(from))
arel_attribute(field)
else
yield
end
end

def table_name_matches?(from)
/(?:\A|(?<!FROM)\s)(?:\b#{table.name}\b|#{connection.quote_table_name(table.name)})(?!\.)/i.match?(from.to_s)
end

def reverse_sql_order(order_query)
if order_query.empty?
return [arel_attribute(primary_key).desc] if primary_key
Expand Down
32 changes: 22 additions & 10 deletions activerecord/test/cases/relations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,27 +182,27 @@ def test_finding_with_subquery_without_select_does_not_change_the_select
end
end

def test_select_with_original_table_name_in_from
def test_select_with_from_includes_original_table_name
relation = Comment.joins(:post).select(:id).order(:id)
subquery = Comment.from(Comment.table_name).joins(:post).select(:id).order(:id)
subquery = Comment.from("#{Comment.table_name} /*! USE INDEX (PRIMARY) */").joins(:post).select(:id).order(:id)
assert_equal relation.map(&:id), subquery.map(&:id)
end

def test_pluck_with_original_table_name_in_from
def test_pluck_with_from_includes_original_table_name
relation = Comment.joins(:post).order(:id)
subquery = Comment.from(Comment.table_name).joins(:post).order(:id)
subquery = Comment.from("#{Comment.table_name} /*! USE INDEX (PRIMARY) */").joins(:post).order(:id)
assert_equal relation.pluck(:id), subquery.pluck(:id)
end

def test_select_with_quoted_original_table_name_in_from
def test_select_with_from_includes_quoted_original_table_name
relation = Comment.joins(:post).select(:id).order(:id)
subquery = Comment.from(Comment.quoted_table_name).joins(:post).select(:id).order(:id)
subquery = Comment.from("#{Comment.quoted_table_name} /*! USE INDEX (PRIMARY) */").joins(:post).select(:id).order(:id)
assert_equal relation.map(&:id), subquery.map(&:id)
end

def test_pluck_with_quoted_original_table_name_in_from
def test_pluck_with_from_includes_quoted_original_table_name
relation = Comment.joins(:post).order(:id)
subquery = Comment.from(Comment.quoted_table_name).joins(:post).order(:id)
subquery = Comment.from("#{Comment.quoted_table_name} /*! USE INDEX (PRIMARY) */").joins(:post).order(:id)
assert_equal relation.pluck(:id), subquery.pluck(:id)
end

Expand All @@ -221,13 +221,25 @@ def test_pluck_with_subquery_in_from_uses_original_table_name

def test_select_with_subquery_in_from_does_not_use_original_table_name
relation = Comment.group(:type).select("COUNT(post_id) AS post_count, type")
subquery = Comment.from(relation).select("type", "post_count")
subquery = Comment.from(relation, "grouped_#{Comment.table_name}").select("type", "post_count")
assert_equal(relation.map(&:post_count).sort, subquery.map(&:post_count).sort)
end

def test_group_with_subquery_in_from_does_not_use_original_table_name
relation = Comment.group(:type).select("COUNT(post_id) AS post_count,type")
subquery = Comment.from(relation).group("type").average("post_count")
subquery = Comment.from(relation, "grouped_#{Comment.table_name}").group("type").average("post_count")
assert_equal(relation.map(&:post_count).sort, subquery.values.sort)
end

def test_select_with_subquery_string_in_from_does_not_use_original_table_name
relation = Comment.group(:type).select("COUNT(post_id) AS post_count, type")
subquery = Comment.from("(#{relation.to_sql}) #{Comment.table_name}_grouped").select("type", "post_count")
assert_equal(relation.map(&:post_count).sort, subquery.map(&:post_count).sort)
end

def test_group_with_subquery_string_in_from_does_not_use_original_table_name
relation = Comment.group(:type).select("COUNT(post_id) AS post_count,type")
subquery = Comment.from("(#{relation.to_sql}) #{Comment.table_name}_grouped").group("type").average("post_count")
assert_equal(relation.map(&:post_count).sort, subquery.values.sort)
end

Expand Down

0 comments on commit bff5e79

Please sign in to comment.