Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions lib/mobility/plugins/active_record/query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -187,21 +187,25 @@ def order(opts, *rest)

if ::ActiveRecord::VERSION::MAJOR >= 8
# Fix for https://github.com/shioyama/mobility/pull/654#issuecomment-2503479112
# TODO: Make this better
# When counting with select values that include mobility-translated attributes,
# we need to filter out the aliased columns that Mobility adds (e.g. __mobility_title_en__)
# to prevent errors in the COUNT SQL generation.
def select_for_count
return super unless klass.respond_to?(:mobility_attribute?)

if select_values.any? { |value| value.right.start_with?(ATTRIBUTE_ALIAS_PREFIX) }
filtered_select_values = select_values.map do |value|
value.right.start_with?(ATTRIBUTE_ALIAS_PREFIX) ? value.left : value
end
# Check if any select values are Mobility attribute aliases
has_mobility_aliases = select_values.any? { |value| value.respond_to?(:right) && value.right.start_with?(ATTRIBUTE_ALIAS_PREFIX) }

# Copied from lib/active_record/relation/calculations.rb
with_connection do |conn|
arel_columns(filtered_select_values).map { |column| conn.visitor.compile(column) }.join(", ")
end
else
super
return super unless has_mobility_aliases

# Filter out Mobility aliases, replacing them with the underlying columns
filtered_select_values = select_values.map do |value|
value.respond_to?(:right) && value.right.start_with?(ATTRIBUTE_ALIAS_PREFIX) ? value.left : value
end

# Copied from lib/active_record/relation/calculations.rb
with_connection do |conn|
arel_columns(filtered_select_values).map { |column| conn.visitor.compile(column) }.join(", ")
end
end
end
Expand Down
45 changes: 45 additions & 0 deletions spec/mobility/plugins/active_record/query_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -218,4 +218,49 @@
expect(Article.i18n.where(title: "Title", content: "Content")).to eq([article2])
end
end

describe "regression: select_for_count with mixed select values" do
# Test for bug where select_for_count called .right on string values
# causing NoMethodError when select_values contained non-Arel nodes
if ::ActiveRecord::VERSION::MAJOR >= 8
before do
stub_const 'Article', Class.new(ActiveRecord::Base)
translates Article, :title, backend: :table
end

it "handles count with regular column in select" do
Article.create(title: "Article 1")
Article.create(title: "Article 2")

# The original bug: calling .right on string values like "id"
# This should not raise NoMethodError: undefined method `right' for "id":String
expect { Article.select(:id).count }.not_to raise_error
expect(Article.select(:id).count).to eq(2)
end

it "handles count with translated column in select" do
Article.create(title: "Article 1")
Article.create(title: "Article 2")

# When selecting translated attributes, Mobility creates Arel::Nodes::As with aliases
# This should properly filter out the aliased columns for counting
relation = Article.i18n.select(:title)

expect { relation.count }.not_to raise_error
expect(relation.count).to eq(2)
end

it "handles count without select when mobility is present" do
Article.create(title: "Article 1")
Article.create(title: "Article 2")

# Ensure normal counting still works on models with mobility
expect { Article.count }.not_to raise_error
expect(Article.count).to eq(2)

expect { Article.i18n.count }.not_to raise_error
expect(Article.i18n.count).to eq(2)
end
end
end
end
Loading