Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not use Arel.star when ignored_columns #27918

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion activerecord/lib/active_record/relation/query_methods.rb
Expand Up @@ -1042,7 +1042,11 @@ def build_select(arel)
if select_values.any?
arel.project(*arel_columns(select_values.uniq))
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a elsif here?

arel.project(@klass.arel_table[Arel.star])
if @klass.ignored_columns.any?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this conditional. Just always do column_names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need it, but it does make for much nicer looking queries in the common case. 😕

arel.project(*arel_columns(@klass.column_names))
else
arel.project(@klass.arel_table[Arel.star])
end
end
end

Expand Down
22 changes: 11 additions & 11 deletions activerecord/test/cases/adapters/mysql2/explain_test.rb
@@ -1,21 +1,21 @@
require "cases/helper"
require "models/developer"
require "models/computer"
require "models/author"
require "models/post"

class Mysql2ExplainTest < ActiveRecord::Mysql2TestCase
fixtures :developers
fixtures :authors

def test_explain_for_one_query
explain = Developer.where(id: 1).explain
assert_match %(EXPLAIN for: SELECT `developers`.* FROM `developers` WHERE `developers`.`id` = 1), explain
assert_match %r(developers |.* const), explain
explain = Author.where(id: 1).explain
assert_match %(EXPLAIN for: SELECT `authors`.* FROM `authors` WHERE `authors`.`id` = 1), explain
assert_match %r(authors |.* const), explain
end

def test_explain_with_eager_loading
explain = Developer.where(id: 1).includes(:audit_logs).explain
assert_match %(EXPLAIN for: SELECT `developers`.* FROM `developers` WHERE `developers`.`id` = 1), explain
assert_match %r(developers |.* const), explain
assert_match %(EXPLAIN for: SELECT `audit_logs`.* FROM `audit_logs` WHERE `audit_logs`.`developer_id` = 1), explain
assert_match %r(audit_logs |.* ALL), explain
explain = Author.where(id: 1).includes(:posts).explain
assert_match %(EXPLAIN for: SELECT `authors`.* FROM `authors` WHERE `authors`.`id` = 1), explain
assert_match %r(authors |.* const), explain
assert_match %(EXPLAIN for: SELECT `posts`.* FROM `posts` WHERE `posts`.`author_id` = 1), explain
assert_match %r(posts |.* ALL), explain
end
end
21 changes: 21 additions & 0 deletions activerecord/test/cases/base_test.rb
Expand Up @@ -1525,4 +1525,25 @@ def test_default_values_are_deeply_dupped
assert Developer.new.respond_to?(:last_name=)
assert Developer.new.respond_to?(:last_name?)
end

test "when #reload called, ignored columns' attribute methods are not defined" do
developer = Developer.create!(name: "Developer")
refute developer.respond_to?(:first_name)
refute developer.respond_to?(:first_name=)

developer.reload

refute developer.respond_to?(:first_name)
refute developer.respond_to?(:first_name=)
end

test "ignored columns not included in SELECT" do
query = Developer.all.to_sql

# ignored column
refute query.include?("first_name")

# regular column
assert query.include?("name")
end
end
4 changes: 2 additions & 2 deletions activerecord/test/cases/scoping/default_scoping_test.rb
Expand Up @@ -469,9 +469,9 @@ def test_default_scope_is_threadsafe
end

test "a scope can remove the condition from the default scope" do
scope = DeveloperCalledJamis.david2
scope = DeveloperCalledJamis.jamis2
assert_equal 1, scope.where_clause.ast.children.length
assert_equal Developer.where(name: "David"), scope
assert_equal DeveloperCalledJamis.where("salary < 10000").to_a, scope.to_a
end

def test_with_abstract_class_where_clause_should_not_be_duplicated
Expand Down
1 change: 1 addition & 0 deletions activerecord/test/models/developer.rb
Expand Up @@ -175,6 +175,7 @@ class DeveloperCalledJamis < ActiveRecord::Base

default_scope { where(name: "Jamis") }
scope :poor, -> { where("salary < 150000") }
scope :jamis2, -> { unscoped.where("salary < 10000") }
scope :david, -> { where name: "David" }
scope :david2, -> { unscoped.where name: "David" }
end
Expand Down