Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix delete_all when chained with joins. #5649

Merged
merged 1 commit into from

3 participants

Rafael Mendonça França Jon Leighton Aaron Patterson
Rafael Mendonça França

I don't know if the MySQL fix is the best one. @tenderlove please review it.

Closes #5202 and #919

Jon Leighton
Collaborator

Rather than creating two separate methods (join_to_update and join_to_delete), I think it would be better to change Arel::DeleteManager to have a where method. Can you look into that? Thanks.

Rafael Mendonça França

Should I add the key method too?

Rafael Mendonça França

hey @jonleighton. We need the two separate methods because in mysql update and delete works completely different. But I changed the code to use Arel::DeleteManager#where (It was already defined)

Aaron Patterson tenderlove merged commit 1c8e222 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 11, 2012
  1. Rafael Mendonça França
This page is out of date. Refresh to see the latest.
20 activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
View
@@ -312,13 +312,27 @@ def sanitize_limit(limit)
# on mysql (even when aliasing the tables), but mysql allows using JOIN directly in
# an UPDATE statement, so in the mysql adapters we redefine this to do that.
def join_to_update(update, select) #:nodoc:
- subselect = select.clone
- subselect.projections = [update.key]
+ key = update.key
+ subselect = subquery_for(key, select)
- update.where update.key.in(subselect)
+ update.where key.in(subselect)
+ end
+
+ def join_to_delete(delete, select, key) #:nodoc:
+ subselect = subquery_for(key, select)
+
+ delete.where key.in(subselect)
end
protected
+
+ # Return a subquery for the given key using the join information.
+ def subquery_for(key, select)
+ subselect = select.clone
+ subselect.projections = [key]
+ subselect
+ end
+
# Returns an array of record hashes with the column names as keys and
# column values as values.
def select(sql, name = nil, binds = [])
24 activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
View
@@ -294,19 +294,10 @@ def release_savepoint
# In the simple case, MySQL allows us to place JOINs directly into the UPDATE
# query. However, this does not allow for LIMIT, OFFSET and ORDER. To support
- # these, we must use a subquery. However, MySQL is too stupid to create a
- # temporary table for this automatically, so we have to give it some prompting
- # in the form of a subsubquery. Ugh!
+ # these, we must use a subquery.
def join_to_update(update, select) #:nodoc:
if select.limit || select.offset || select.orders.any?
- subsubselect = select.clone
- subsubselect.projections = [update.key]
-
- subselect = Arel::SelectManager.new(select.engine)
- subselect.project Arel.sql(update.key.name)
- subselect.from subsubselect.as('__active_record_temp')
-
- update.where update.key.in(subselect)
+ super
else
update.table select.source
update.wheres = select.constraints
@@ -558,6 +549,17 @@ def limited_update_conditions(where_sql, quoted_table_name, quoted_primary_key)
protected
+ # MySQL is too stupid to create a temporary table for use subquery, so we have
+ # to give it some prompting in the form of a subsubquery. Ugh!
+ def subquery_for(key, select)
+ subsubselect = select.clone
+ subsubselect.projections = [key]
+
+ subselect = Arel::SelectManager.new(select.engine)
+ subselect.project Arel.sql(key.name)
+ subselect.from subsubselect.as('__active_record_temp')
+ end
+
def add_index_length(option_strings, column_names, options = {})
if options.is_a?(Hash) && length = options[:length]
case length
12 activerecord/lib/active_record/relation.rb
View
@@ -400,8 +400,16 @@ def delete_all(conditions = nil)
if conditions
where(conditions).delete_all
else
- statement = arel.compile_delete
- affected = @klass.connection.delete(statement, 'SQL', bind_values)
+ stmt = Arel::DeleteManager.new(arel.engine)
+ stmt.from(table)
+
+ if joins_values.any?
+ @klass.connection.join_to_delete(stmt, arel, table[primary_key])
+ else
+ stmt.wheres = arel.constraints
+ end
+
+ affected = @klass.connection.delete(stmt, 'SQL', bind_values)
reset
affected
20 activerecord/test/cases/persistence_test.rb
View
@@ -13,12 +13,14 @@
require 'models/parrot'
require 'models/minivan'
require 'models/person'
+require 'models/pet'
+require 'models/toy'
require 'rexml/document'
require 'active_support/core_ext/exception'
class PersistencesTest < ActiveRecord::TestCase
- fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics, 'warehouse-things', :authors, :categorizations, :categories, :posts, :minivans
+ fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics, 'warehouse-things', :authors, :categorizations, :categories, :posts, :minivans, :pets, :toys
# Oracle UPDATE does not support ORDER BY
unless current_adapter?(:OracleAdapter)
@@ -76,6 +78,22 @@ def test_delete_all
assert_equal Topic.count, Topic.delete_all
end
+ def test_delete_all_with_joins_and_where_part_is_hash
+ where_args = {:toys => {:name => 'Bone'}}
+ count = Pet.joins(:toys).where(where_args).count
+
+ assert_equal count, 1
+ assert_equal count, Pet.joins(:toys).where(where_args).delete_all
+ end
+
+ def test_delete_all_with_joins_and_where_part_is_not_hash
+ where_args = ['toys.name = ?', 'Bone']
+ count = Pet.joins(:toys).where(where_args).count
+
+ assert_equal count, 1
+ assert_equal count, Pet.joins(:toys).where(where_args).delete_all
+ end
+
def test_update_by_condition
Topic.update_all "content = 'bulk updated!'", ["approved = ?", true]
assert_equal "Have a nice day", Topic.find(1).content
Something went wrong with that request. Please try again.