Skip to content

Commit fe25c50

Browse files
committed
Fix update_all producing a broken query:
- Fix rails#54560 - `update_all` would produce a broken query when using more than one JOIN on the target. ```ruby Developer.join(:computer, :mentor).update_all(ready: true) ``` Before ```sql UPDATE developers SET ready = true FROM computers JOIN mentors on mentors.id = developers.mentor_id WHERE developer.computer_id = computer.id; # We can't make a JOIN on the developer table, this should in the FROM clause. ``` After ```sql UPDATE developers SET ready = true FROM computers, mentors WHERE developer.computer_id = computer.id AND developers.mentor_id = mentors.id; ```
1 parent 00cc4ff commit fe25c50

File tree

3 files changed

+51
-6
lines changed

3 files changed

+51
-6
lines changed

activerecord/lib/arel/visitors/postgresql.rb

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,12 @@ def visit_Arel_Nodes_UpdateStatement(o, collector)
2929
collect_nodes_for o.values, collector, " SET "
3030
collector << " FROM "
3131
first_join, *remaining_joins = o.relation.right
32-
visit first_join.left, collector
32+
from_items = remaining_joins.extract! do |join|
33+
join.right.expr.right.relation == o.relation.left
34+
end
35+
36+
from_where = [first_join.left] + from_items.map(&:left)
37+
collect_nodes_for from_where, collector, " ", ", "
3338

3439
if remaining_joins && !remaining_joins.empty?
3540
collector << " "
@@ -39,7 +44,8 @@ def visit_Arel_Nodes_UpdateStatement(o, collector)
3944
end
4045
end
4146

42-
collect_nodes_for [first_join.right.expr] + o.wheres, collector, " WHERE ", " AND "
47+
from_where = [first_join.right.expr] + from_items.map { |i| i.right.expr }
48+
collect_nodes_for from_where + o.wheres, collector, " WHERE ", " AND "
4349
else
4450
collector = visit o.relation, collector
4551
collect_nodes_for o.values, collector, " SET "

activerecord/lib/arel/visitors/sqlite.rb

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,12 @@ def visit_Arel_Nodes_UpdateStatement(o, collector)
3030

3131
collector << " FROM "
3232
first_join, *remaining_joins = o.relation.right
33-
visit first_join.left, collector
33+
from_items = remaining_joins.extract! do |join|
34+
join.right.expr.right.relation == o.relation.left
35+
end
36+
37+
from_where = [first_join.left] + from_items.map(&:left)
38+
collect_nodes_for from_where, collector, " ", ", "
3439

3540
if remaining_joins && !remaining_joins.empty?
3641
collector << " "
@@ -40,7 +45,8 @@ def visit_Arel_Nodes_UpdateStatement(o, collector)
4045
end
4146
end
4247

43-
collect_nodes_for [first_join.right.expr] + o.wheres, collector, " WHERE ", " AND "
48+
from_where = [first_join.right.expr] + from_items.map { |i| i.right.expr }
49+
collect_nodes_for from_where + o.wheres, collector, " WHERE ", " AND "
4450
else
4551
collector = visit o.relation, collector
4652
collect_nodes_for o.values, collector, " SET "

activerecord/test/cases/relation/update_all_test.rb

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@
44
require "models/author"
55
require "models/category"
66
require "models/comment"
7+
require "models/company"
78
require "models/computer"
9+
require "models/contract"
810
require "models/developer"
11+
require "models/mentor"
912
require "models/owner"
1013
require "models/post"
1114
require "models/person"
@@ -18,7 +21,7 @@
1821
require "models/cpk"
1922

2023
class UpdateAllTest < ActiveRecord::TestCase
21-
fixtures :authors, :author_addresses, :comments, :developers, :owners, :posts, :people, :pets, :toys, :tags,
24+
fixtures :authors, :author_addresses, :comments, :companies, :developers, :owners, :posts, :people, :pets, :toys, :tags,
2225
:taggings, "warehouse-things", :cpk_orders, :cpk_order_agreements
2326

2427
class TopicWithCallbacks < ActiveRecord::Base
@@ -102,7 +105,7 @@ def test_dynamic_update_all_with_one_joined_table
102105
end
103106
end
104107

105-
def test_dynamic_update_all_with_two_joined_table
108+
def test_dynamic_update_all_with_one_join_on_the_target_and_one_indirect_join
106109
update_fragment = if current_adapter?(:TrilogyAdapter, :Mysql2Adapter)
107110
"toys.name = owners.name"
108111
else # PostgreSQLAdapter, SQLite3Adapter
@@ -118,6 +121,36 @@ def test_dynamic_update_all_with_two_joined_table
118121
end
119122
end
120123

124+
def test_dynamic_update_all_with_two_joins_on_the_target
125+
update_fragment = if current_adapter?(:TrilogyAdapter, :Mysql2Adapter)
126+
"developers.name = mentors.name"
127+
else # PostgreSQLAdapter, SQLite3Adapter
128+
"name = mentors.name"
129+
end
130+
131+
jamis, david, poor_jamis = developers(:jamis, :david, :poor_jamis)
132+
jamis.update_columns(
133+
firm_id: companies(:first_firm).id,
134+
mentor_id: Mentor.create!(name: "John").id,
135+
)
136+
david.update_columns(
137+
firm_id: companies(:another_firm).id,
138+
mentor_id: Mentor.create!(name: "Goliath").id,
139+
)
140+
poor_jamis.update_columns(
141+
firm_id: companies(:another_firm).id,
142+
mentor_id: Mentor.create!(name: "Doe").id,
143+
)
144+
145+
developers = Developer.joins(:firm, :mentor)
146+
assert_equal 3, developers.count
147+
assert_equal 3, developers.update_all(update_fragment)
148+
149+
developers.each do |developer|
150+
assert_equal developer.name, developer.mentor.name
151+
end
152+
end
153+
121154
def test_update_all_with_left_joins
122155
pets = Pet.left_joins(:toys).where(toys: { name: "Bone" })
123156

0 commit comments

Comments
 (0)