From 6e43a207c6d3072d10b494d605b7bb6635043e30 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 1 Apr 2019 15:04:11 +0900 Subject: Revert unused code and re-using query annotation for `update_all` and `delete_all` This partly reverts #35617. #35617 includes unused code (for `InsertStatement`) and re-using query annotation for `update_all` and `delete_all`, which has not been discussed yet. If a relation has any annotation, I think it is mostly for SELECT query, so re-using annotation by default is not always desired behavior for me. We should discuss about desired behavior before publishing the implementation. --- activerecord/lib/active_record/relation.rb | 3 --- activerecord/lib/arel/nodes/delete_statement.rb | 9 +++----- activerecord/lib/arel/nodes/insert_statement.rb | 9 +++----- activerecord/lib/arel/nodes/select_core.rb | 1 - activerecord/lib/arel/nodes/update_statement.rb | 9 +++----- activerecord/lib/arel/select_manager.rb | 4 ---- activerecord/lib/arel/tree_manager.rb | 5 ---- activerecord/lib/arel/visitors/to_sql.rb | 6 ++--- .../test/cases/arel/delete_manager_test.rb | 18 --------------- .../test/cases/arel/nodes/delete_statement_test.rb | 8 ------- .../test/cases/arel/nodes/insert_statement_test.rb | 8 ------- .../test/cases/arel/nodes/update_statement_test.rb | 8 ------- .../test/cases/arel/update_manager_test.rb | 24 ------------------- .../test/cases/relation/delete_all_test.rb | 19 --------------- .../test/cases/relation/update_all_test.rb | 27 ---------------------- 15 files changed, 11 insertions(+), 147 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 36c2422d84..dd821431e1 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -389,8 +389,6 @@ module ActiveRecord stmt.set Arel.sql(klass.sanitize_sql_for_assignment(updates, table.name)) end - stmt.comment(*arel.comment_node.values) if arel.comment_node - @klass.connection.update stmt, "#{@klass} Update All" end @@ -506,7 +504,6 @@ module ActiveRecord stmt.offset(arel.offset) stmt.order(*arel.orders) stmt.wheres = arel.constraints - stmt.comment(*arel.comment_node.values) if arel.comment_node affected = @klass.connection.delete(stmt, "#{@klass} Destroy") diff --git a/activerecord/lib/arel/nodes/delete_statement.rb b/activerecord/lib/arel/nodes/delete_statement.rb index 56249b2bad..a419975335 100644 --- a/activerecord/lib/arel/nodes/delete_statement.rb +++ b/activerecord/lib/arel/nodes/delete_statement.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Nodes class DeleteStatement < Arel::Nodes::Node - attr_accessor :left, :right, :orders, :limit, :offset, :key, :comment + attr_accessor :left, :right, :orders, :limit, :offset, :key alias :relation :left alias :relation= :left= @@ -18,18 +18,16 @@ module Arel # :nodoc: all @limit = nil @offset = nil @key = nil - @comment = nil end def initialize_copy(other) super @left = @left.clone if @left @right = @right.clone if @right - @comment = @comment.clone if @comment end def hash - [self.class, @left, @right, @orders, @limit, @offset, @key, @comment].hash + [self.class, @left, @right, @orders, @limit, @offset, @key].hash end def eql?(other) @@ -39,8 +37,7 @@ module Arel # :nodoc: all self.orders == other.orders && self.limit == other.limit && self.offset == other.offset && - self.key == other.key && - self.comment == other.comment + self.key == other.key end alias :== :eql? end diff --git a/activerecord/lib/arel/nodes/insert_statement.rb b/activerecord/lib/arel/nodes/insert_statement.rb index 8430dd23da..d28fd1f6c8 100644 --- a/activerecord/lib/arel/nodes/insert_statement.rb +++ b/activerecord/lib/arel/nodes/insert_statement.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Nodes class InsertStatement < Arel::Nodes::Node - attr_accessor :relation, :columns, :values, :select, :comment + attr_accessor :relation, :columns, :values, :select def initialize super() @@ -11,7 +11,6 @@ module Arel # :nodoc: all @columns = [] @values = nil @select = nil - @comment = nil end def initialize_copy(other) @@ -19,11 +18,10 @@ module Arel # :nodoc: all @columns = @columns.clone @values = @values.clone if @values @select = @select.clone if @select - @comment = @comment.clone if @comment end def hash - [@relation, @columns, @values, @select, @comment].hash + [@relation, @columns, @values, @select].hash end def eql?(other) @@ -31,8 +29,7 @@ module Arel # :nodoc: all self.relation == other.relation && self.columns == other.columns && self.select == other.select && - self.values == other.values && - self.comment == other.comment + self.values == other.values end alias :== :eql? end diff --git a/activerecord/lib/arel/nodes/select_core.rb b/activerecord/lib/arel/nodes/select_core.rb index b6154b7ff4..11b4f39ece 100644 --- a/activerecord/lib/arel/nodes/select_core.rb +++ b/activerecord/lib/arel/nodes/select_core.rb @@ -40,7 +40,6 @@ module Arel # :nodoc: all @groups = @groups.clone @havings = @havings.clone @windows = @windows.clone - @comment = @comment.clone if @comment end def hash diff --git a/activerecord/lib/arel/nodes/update_statement.rb b/activerecord/lib/arel/nodes/update_statement.rb index 015bcd7613..cfaa19e392 100644 --- a/activerecord/lib/arel/nodes/update_statement.rb +++ b/activerecord/lib/arel/nodes/update_statement.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Nodes class UpdateStatement < Arel::Nodes::Node - attr_accessor :relation, :wheres, :values, :orders, :limit, :offset, :key, :comment + attr_accessor :relation, :wheres, :values, :orders, :limit, :offset, :key def initialize @relation = nil @@ -13,18 +13,16 @@ module Arel # :nodoc: all @limit = nil @offset = nil @key = nil - @comment = nil end def initialize_copy(other) super @wheres = @wheres.clone @values = @values.clone - @comment = @comment.clone if @comment end def hash - [@relation, @wheres, @values, @orders, @limit, @offset, @key, @comment].hash + [@relation, @wheres, @values, @orders, @limit, @offset, @key].hash end def eql?(other) @@ -35,8 +33,7 @@ module Arel # :nodoc: all self.orders == other.orders && self.limit == other.limit && self.offset == other.offset && - self.key == other.key && - self.comment == other.comment + self.key == other.key end alias :== :eql? end diff --git a/activerecord/lib/arel/select_manager.rb b/activerecord/lib/arel/select_manager.rb index 4e9f527235..ddc9e394dd 100644 --- a/activerecord/lib/arel/select_manager.rb +++ b/activerecord/lib/arel/select_manager.rb @@ -249,10 +249,6 @@ module Arel # :nodoc: all self end - def comment_node - @ctx.comment - end - private def collapse(exprs) exprs = exprs.compact diff --git a/activerecord/lib/arel/tree_manager.rb b/activerecord/lib/arel/tree_manager.rb index 326c4f995c..0476399618 100644 --- a/activerecord/lib/arel/tree_manager.rb +++ b/activerecord/lib/arel/tree_manager.rb @@ -36,11 +36,6 @@ module Arel # :nodoc: all @ast.wheres << expr self end - - def comment(*values) - @ast.comment = Nodes::Comment.new(values) - self - end end attr_reader :ast diff --git a/activerecord/lib/arel/visitors/to_sql.rb b/activerecord/lib/arel/visitors/to_sql.rb index 4192d9efdc..277d553e6c 100644 --- a/activerecord/lib/arel/visitors/to_sql.rb +++ b/activerecord/lib/arel/visitors/to_sql.rb @@ -35,7 +35,6 @@ module Arel # :nodoc: all collect_nodes_for o.wheres, collector, " WHERE ", " AND " collect_nodes_for o.orders, collector, " ORDER BY " maybe_visit o.limit, collector - maybe_visit o.comment, collector end def visit_Arel_Nodes_UpdateStatement(o, collector) @@ -48,7 +47,6 @@ module Arel # :nodoc: all collect_nodes_for o.wheres, collector, " WHERE ", " AND " collect_nodes_for o.orders, collector, " ORDER BY " maybe_visit o.limit, collector - maybe_visit o.comment, collector end def visit_Arel_Nodes_InsertStatement(o, collector) @@ -64,9 +62,9 @@ module Arel # :nodoc: all maybe_visit o.values, collector elsif o.select maybe_visit o.select, collector + else + collector end - - maybe_visit o.comment, collector end def visit_Arel_Nodes_Exists(o, collector) diff --git a/activerecord/test/cases/arel/delete_manager_test.rb b/activerecord/test/cases/arel/delete_manager_test.rb index 63cd1bffe3..0bad02f4d2 100644 --- a/activerecord/test/cases/arel/delete_manager_test.rb +++ b/activerecord/test/cases/arel/delete_manager_test.rb @@ -49,23 +49,5 @@ module Arel dm.where(table[:id].eq(10)).must_equal dm end end - - describe "comment" do - it "chains" do - manager = Arel::DeleteManager.new - manager.comment("deleting").must_equal manager - end - - it "appends a comment to the generated query" do - table = Table.new(:users) - dm = Arel::DeleteManager.new - dm.from table - dm.comment("deletion") - assert_match(%r{DELETE FROM "users" /\* deletion \*/}, dm.to_sql) - - dm.comment("deletion", "with", "comment") - assert_match(%r{DELETE FROM "users" /\* deletion \*/ /\* with \*/ /\* comment \*/}, dm.to_sql) - end - end end end diff --git a/activerecord/test/cases/arel/nodes/delete_statement_test.rb b/activerecord/test/cases/arel/nodes/delete_statement_test.rb index 8ba268653d..3f078063a4 100644 --- a/activerecord/test/cases/arel/nodes/delete_statement_test.rb +++ b/activerecord/test/cases/arel/nodes/delete_statement_test.rb @@ -18,10 +18,8 @@ describe Arel::Nodes::DeleteStatement do it "is equal with equal ivars" do statement1 = Arel::Nodes::DeleteStatement.new statement1.wheres = %w[a b c] - statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::DeleteStatement.new statement2.wheres = %w[a b c] - statement2.comment = Arel::Nodes::Comment.new(["comment"]) array = [statement1, statement2] assert_equal 1, array.uniq.size end @@ -29,14 +27,8 @@ describe Arel::Nodes::DeleteStatement do it "is not equal with different ivars" do statement1 = Arel::Nodes::DeleteStatement.new statement1.wheres = %w[a b c] - statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::DeleteStatement.new statement2.wheres = %w[1 2 3] - statement2.comment = Arel::Nodes::Comment.new(["comment"]) - array = [statement1, statement2] - assert_equal 2, array.uniq.size - statement2.wheres = %w[a b c] - statement2.comment = Arel::Nodes::Comment.new(["other"]) array = [statement1, statement2] assert_equal 2, array.uniq.size end diff --git a/activerecord/test/cases/arel/nodes/insert_statement_test.rb b/activerecord/test/cases/arel/nodes/insert_statement_test.rb index 036576b231..252a0d0d0b 100644 --- a/activerecord/test/cases/arel/nodes/insert_statement_test.rb +++ b/activerecord/test/cases/arel/nodes/insert_statement_test.rb @@ -23,11 +23,9 @@ describe Arel::Nodes::InsertStatement do statement1 = Arel::Nodes::InsertStatement.new statement1.columns = %w[a b c] statement1.values = %w[x y z] - statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::InsertStatement.new statement2.columns = %w[a b c] statement2.values = %w[x y z] - statement2.comment = Arel::Nodes::Comment.new(["comment"]) array = [statement1, statement2] assert_equal 1, array.uniq.size end @@ -36,15 +34,9 @@ describe Arel::Nodes::InsertStatement do statement1 = Arel::Nodes::InsertStatement.new statement1.columns = %w[a b c] statement1.values = %w[x y z] - statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::InsertStatement.new statement2.columns = %w[a b c] statement2.values = %w[1 2 3] - statement2.comment = Arel::Nodes::Comment.new(["comment"]) - array = [statement1, statement2] - assert_equal 2, array.uniq.size - statement2.values = %w[x y z] - statement2.comment = Arel::Nodes::Comment.new("other") array = [statement1, statement2] assert_equal 2, array.uniq.size end diff --git a/activerecord/test/cases/arel/nodes/update_statement_test.rb b/activerecord/test/cases/arel/nodes/update_statement_test.rb index f133ddf7eb..a83ce32f68 100644 --- a/activerecord/test/cases/arel/nodes/update_statement_test.rb +++ b/activerecord/test/cases/arel/nodes/update_statement_test.rb @@ -27,7 +27,6 @@ describe Arel::Nodes::UpdateStatement do statement1.orders = %w[x y z] statement1.limit = 42 statement1.key = "zomg" - statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::UpdateStatement.new statement2.relation = "zomg" statement2.wheres = 2 @@ -35,7 +34,6 @@ describe Arel::Nodes::UpdateStatement do statement2.orders = %w[x y z] statement2.limit = 42 statement2.key = "zomg" - statement2.comment = Arel::Nodes::Comment.new(["comment"]) array = [statement1, statement2] assert_equal 1, array.uniq.size end @@ -48,7 +46,6 @@ describe Arel::Nodes::UpdateStatement do statement1.orders = %w[x y z] statement1.limit = 42 statement1.key = "zomg" - statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::UpdateStatement.new statement2.relation = "zomg" statement2.wheres = 2 @@ -56,11 +53,6 @@ describe Arel::Nodes::UpdateStatement do statement2.orders = %w[x y z] statement2.limit = 42 statement2.key = "wth" - statement2.comment = Arel::Nodes::Comment.new(["comment"]) - array = [statement1, statement2] - assert_equal 2, array.uniq.size - statement2.key = "zomg" - statement2.comment = Arel::Nodes::Comment.new(["other"]) array = [statement1, statement2] assert_equal 2, array.uniq.size end diff --git a/activerecord/test/cases/arel/update_manager_test.rb b/activerecord/test/cases/arel/update_manager_test.rb index e13cb6aa52..cc1b9ac5b3 100644 --- a/activerecord/test/cases/arel/update_manager_test.rb +++ b/activerecord/test/cases/arel/update_manager_test.rb @@ -122,29 +122,5 @@ module Arel @um.key.must_equal @table[:foo] end end - - describe "comment" do - it "chains" do - manager = Arel::UpdateManager.new - manager.comment("updating").must_equal manager - end - - it "appends a comment to the generated query" do - table = Table.new :users - - manager = Arel::UpdateManager.new - manager.table table - - manager.comment("updating") - manager.to_sql.must_be_like %{ - UPDATE "users" /* updating */ - } - - manager.comment("updating", "with", "comment") - manager.to_sql.must_be_like %{ - UPDATE "users" /* updating */ /* with */ /* comment */ - } - end - end end end diff --git a/activerecord/test/cases/relation/delete_all_test.rb b/activerecord/test/cases/relation/delete_all_test.rb index 9b76936b7e..d1c13fa1b5 100644 --- a/activerecord/test/cases/relation/delete_all_test.rb +++ b/activerecord/test/cases/relation/delete_all_test.rb @@ -99,23 +99,4 @@ class DeleteAllTest < ActiveRecord::TestCase assert_raise(ActiveRecord::RecordNotFound) { posts(:thinking) } assert posts(:welcome) end - - def test_delete_all_with_annotation_includes_a_query_comment - davids = Author.where(name: "David").annotate("deleting all") - - assert_sql(%r{/\* deleting all \*/}) do - assert_difference("Author.count", -1) { davids.delete_all } - end - end - - def test_delete_all_without_annotation_does_not_include_an_empty_comment - davids = Author.where(name: "David") - - log = capture_sql do - assert_difference("Author.count", -1) { davids.delete_all } - end - - assert_not_predicate log, :empty? - assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? - end end diff --git a/activerecord/test/cases/relation/update_all_test.rb b/activerecord/test/cases/relation/update_all_test.rb index 526f926841..0500574f28 100644 --- a/activerecord/test/cases/relation/update_all_test.rb +++ b/activerecord/test/cases/relation/update_all_test.rb @@ -241,33 +241,6 @@ class UpdateAllTest < ActiveRecord::TestCase end end - def test_update_all_with_annotation_includes_a_query_comment - tag = Tag.first - - assert_sql(%r{/\* updating all \*/}) do - Post.tagged_with(tag.id).annotate("updating all").update_all(title: "rofl") - end - - posts = Post.tagged_with(tag.id).all.to_a - assert_operator posts.length, :>, 0 - posts.each { |post| assert_equal "rofl", post.title } - end - - def test_update_all_without_annotation_does_not_include_an_empty_comment - tag = Tag.first - - log = capture_sql do - Post.tagged_with(tag.id).update_all(title: "rofl") - end - - assert_not_predicate log, :empty? - assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? - - posts = Post.tagged_with(tag.id).all.to_a - assert_operator posts.length, :>, 0 - posts.each { |post| assert_equal "rofl", post.title } - end - # Oracle UPDATE does not support ORDER BY unless current_adapter?(:OracleAdapter) def test_update_all_ignores_order_without_limit_from_association -- cgit v1.2.3