From 58a4fbfb8f0148e1896568a8faa683ea96a2536e Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 8 Feb 2025 18:16:59 +0000 Subject: [PATCH 1/5] Support joins in update_all Ref https://github.com/rails/rails/pull/53950 --- lib/arel/visitors/sqlserver.rb | 85 +++++++++++++++++++++++++++++----- test/cases/coerced_tests.rb | 55 ++++++++++++++++++---- 2 files changed, 118 insertions(+), 22 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 5890b7fc3..36ef48b87 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -29,9 +29,69 @@ def visit_Arel_Nodes_Concat(o, collector) visit o.right, collector end + # def visit_Arel_Nodes_UpdateStatement(o, collector) + # if has_join_and_composite_primary_key?(o) + # update_statement_using_join(o, collector) + # else + # o.limit = Nodes::Limit.new(9_223_372_036_854_775_807) if o.orders.any? && o.limit.nil? + # + # super + # end + # end + def visit_Arel_Nodes_UpdateStatement(o, collector) - if has_join_and_composite_primary_key?(o) - update_statement_using_join(o, collector) + collector.retryable = false + o = prepare_update_statement(o) + + collector << "UPDATE " + + # UPDATE with JOIN is in the form of: + # + # UPDATE t1 + # SET .. + # FROM t2 + # WHERE t1.join_id = t2.join_id + # + # Or if more than one join is present: + # + # UPDATE t1 + # SET .. + # FROM t2 + # JOIN t3 ON t2.join_id = t3.join_id + # WHERE t1.join_id = t2.join_id + if has_join_sources?(o) + visit o.relation.left, collector + collect_nodes_for o.values, collector, " SET " + collector << " FROM " + first_join, *remaining_joins = o.relation.right + visit first_join.left, collector + + if remaining_joins && !remaining_joins.empty? + collector << " " + remaining_joins.each do |join| + visit join, collector + end + end + + collect_nodes_for [first_join.right.expr] + o.wheres, collector, " WHERE ", " AND " + else + collector = visit o.relation, collector + collect_nodes_for o.values, collector, " SET " + collect_nodes_for o.wheres, collector, " WHERE ", " AND " + end + + collect_nodes_for o.orders, collector, " ORDER BY " + maybe_visit o.limit, collector + end + + # In the simple case, PostgreSQL allows us to place FROM or JOINs directly into the UPDATE + # query. However, this does not allow for LIMIT, OFFSET and ORDER. To support + # these, we must use a subquery. + def prepare_update_statement(o) + + + if has_join_sources?(o) && !has_limit_or_offset_or_orders?(o) && !has_group_by_and_having?(o) + o else o.limit = Nodes::Limit.new(9_223_372_036_854_775_807) if o.orders.any? && o.limit.nil? @@ -39,6 +99,7 @@ def visit_Arel_Nodes_UpdateStatement(o, collector) end end + def visit_Arel_Nodes_DeleteStatement(o, collector) if has_join_and_composite_primary_key?(o) delete_statement_using_join(o, collector) @@ -61,16 +122,16 @@ def delete_statement_using_join(o, collector) collect_nodes_for o.wheres, collector, " WHERE ", " AND " end - def update_statement_using_join(o, collector) - collector.retryable = false - - collector << "UPDATE " - visit o.relation.left, collector - collect_nodes_for o.values, collector, " SET " - collector << " FROM " - visit o.relation, collector - collect_nodes_for o.wheres, collector, " WHERE ", " AND " - end + # def update_statement_using_join(o, collector) + # collector.retryable = false + # + # collector << "UPDATE " + # visit o.relation.left, collector + # collect_nodes_for o.values, collector, " SET " + # collector << " FROM " + # visit o.relation, collector + # collect_nodes_for o.wheres, collector, " WHERE ", " AND " + # end def visit_Arel_Nodes_Lock(o, collector) o.expr = Arel.sql("WITH(UPDLOCK)") if o.expr.to_s =~ /FOR UPDATE/ diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index 58d760341..91ccc7392 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -1303,18 +1303,25 @@ def test_update_coerced require "models/author" class UpdateAllTest < ActiveRecord::TestCase - # Rails test required updating a identity column. + # Regular expression slightly different. coerce_tests! :test_update_all_doesnt_ignore_order def test_update_all_doesnt_ignore_order_coerced - david, mary = authors(:david), authors(:mary) - _(david.id).must_equal 1 - _(mary.id).must_equal 2 - _(david.name).wont_equal mary.name - assert_queries_match(/UPDATE.*\(SELECT \[authors\].\[id\] FROM \[authors\].*ORDER BY \[authors\].\[id\]/i) do - Author.where("[id] > 1").order(:id).update_all(name: "Test") + assert_equal authors(:david).id + 1, authors(:mary).id # make sure there is going to be a duplicate PK error + test_update_with_order_succeeds = lambda do |order| + Author.order(order).update_all("id = id + 1") + rescue ActiveRecord::ActiveRecordError + false + end + + if test_update_with_order_succeeds.call("id DESC") + # test that this wasn't a fluke and using an incorrect order results in an exception + assert_not test_update_with_order_succeeds.call("id ASC") + else + # test that we're failing because the current Arel's engine doesn't support UPDATE ORDER BY queries is using subselects instead + assert_queries_match(/\AUPDATE .+ \(SELECT .* ORDER BY id DESC.*\)/i) do + test_update_with_order_succeeds.call("id DESC") + end end - _(david.reload.name).must_equal "David" - _(mary.reload.name).must_equal "Test" end # SELECT columns must be in the GROUP clause. @@ -1332,6 +1339,34 @@ def test_update_all_with_group_by_coerced post = Post.select(:id, :title).group(:title).joins(:comments).group("posts.id").having("count(comments.id) < #{minimum_comments_count}").first assert_not_equal "ig", post.title end + + # TODO + coerce_tests! :test_dynamic_update_all_with_one_joined_table + def test_dynamic_update_all_with_one_joined_table_coerced + update_fragment = "name = pets.name" + + toys = Toy.joins(:pet) + assert_equal 3, toys.count + assert_equal 3, toys.update_all(update_fragment) + + toys.each do |toy| + assert_equal toy.pet.name, toy.name + end + end + + # TODO + coerce_tests! :test_dynamic_update_all_with_two_joined_table + def test_dynamic_update_all_with_two_joined_table_coerced + update_fragment = "name = owners.name" + + toys = Toy.joins(pet: [:owner]) + assert_equal 3, toys.count + assert_equal 3, toys.update_all(update_fragment) + + toys.each do |toy| + assert_equal toy.pet.owner.name, toy.name + end + end end class DeleteAllTest < ActiveRecord::TestCase @@ -1971,7 +2006,7 @@ def with_marshable_time_defaults # Revert changes @connection.change_column_default(:sst_datatypes, :datetime, current_default) if current_default.present? end - + # We need to give the full paths for this to work. undef_method :schema_dump_5_1_path def schema_dump_5_1_path From dd42907d4265f3d76ee4f1a8c25e32a583ecca17 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Mon, 10 Feb 2025 12:23:47 +0000 Subject: [PATCH 2/5] Update coerced_tests.rb --- test/cases/coerced_tests.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index 91ccc7392..53eec6c92 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -1340,7 +1340,7 @@ def test_update_all_with_group_by_coerced assert_not_equal "ig", post.title end - # TODO + # TODO: https://github.com/rails/rails/pull/54482 coerce_tests! :test_dynamic_update_all_with_one_joined_table def test_dynamic_update_all_with_one_joined_table_coerced update_fragment = "name = pets.name" @@ -1354,7 +1354,7 @@ def test_dynamic_update_all_with_one_joined_table_coerced end end - # TODO + # TODO: https://github.com/rails/rails/pull/54482 coerce_tests! :test_dynamic_update_all_with_two_joined_table def test_dynamic_update_all_with_two_joined_table_coerced update_fragment = "name = owners.name" From 4191d87e5e263d525ea4edd0a4329fac90510ef0 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Mon, 10 Feb 2025 14:06:55 +0000 Subject: [PATCH 3/5] Remove coerced tests --- test/cases/coerced_tests.rb | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index 53eec6c92..4da87a0cf 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -1339,34 +1339,6 @@ def test_update_all_with_group_by_coerced post = Post.select(:id, :title).group(:title).joins(:comments).group("posts.id").having("count(comments.id) < #{minimum_comments_count}").first assert_not_equal "ig", post.title end - - # TODO: https://github.com/rails/rails/pull/54482 - coerce_tests! :test_dynamic_update_all_with_one_joined_table - def test_dynamic_update_all_with_one_joined_table_coerced - update_fragment = "name = pets.name" - - toys = Toy.joins(:pet) - assert_equal 3, toys.count - assert_equal 3, toys.update_all(update_fragment) - - toys.each do |toy| - assert_equal toy.pet.name, toy.name - end - end - - # TODO: https://github.com/rails/rails/pull/54482 - coerce_tests! :test_dynamic_update_all_with_two_joined_table - def test_dynamic_update_all_with_two_joined_table_coerced - update_fragment = "name = owners.name" - - toys = Toy.joins(pet: [:owner]) - assert_equal 3, toys.count - assert_equal 3, toys.update_all(update_fragment) - - toys.each do |toy| - assert_equal toy.pet.owner.name, toy.name - end - end end class DeleteAllTest < ActiveRecord::TestCase From 1afe4c6ecda4a223c6f98d350c50702acfb5308c Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Mon, 10 Feb 2025 14:11:32 +0000 Subject: [PATCH 4/5] Cleanup --- lib/arel/visitors/sqlserver.rb | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 36ef48b87..e0be7d2d4 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -29,16 +29,7 @@ def visit_Arel_Nodes_Concat(o, collector) visit o.right, collector end - # def visit_Arel_Nodes_UpdateStatement(o, collector) - # if has_join_and_composite_primary_key?(o) - # update_statement_using_join(o, collector) - # else - # o.limit = Nodes::Limit.new(9_223_372_036_854_775_807) if o.orders.any? && o.limit.nil? - # - # super - # end - # end - + # Same as SQLite and PostgreSQL. def visit_Arel_Nodes_UpdateStatement(o, collector) collector.retryable = false o = prepare_update_statement(o) @@ -84,12 +75,8 @@ def visit_Arel_Nodes_UpdateStatement(o, collector) maybe_visit o.limit, collector end - # In the simple case, PostgreSQL allows us to place FROM or JOINs directly into the UPDATE - # query. However, this does not allow for LIMIT, OFFSET and ORDER. To support - # these, we must use a subquery. + # Same as PostgreSQL except we need to add limit if using subquery. def prepare_update_statement(o) - - if has_join_sources?(o) && !has_limit_or_offset_or_orders?(o) && !has_group_by_and_having?(o) o else From ccfd73ee66fcf9ce6d2b5e4a50e489ac2cb9cb8e Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Mon, 10 Feb 2025 14:14:01 +0000 Subject: [PATCH 5/5] Cleanup --- lib/arel/visitors/sqlserver.rb | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index e0be7d2d4..a9e8609ba 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -109,17 +109,6 @@ def delete_statement_using_join(o, collector) collect_nodes_for o.wheres, collector, " WHERE ", " AND " end - # def update_statement_using_join(o, collector) - # collector.retryable = false - # - # collector << "UPDATE " - # visit o.relation.left, collector - # collect_nodes_for o.values, collector, " SET " - # collector << " FROM " - # visit o.relation, collector - # collect_nodes_for o.wheres, collector, " WHERE ", " AND " - # end - def visit_Arel_Nodes_Lock(o, collector) o.expr = Arel.sql("WITH(UPDLOCK)") if o.expr.to_s =~ /FOR UPDATE/ collector << " "