Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix update_all() with default_scope with joins().where(condition_on_joined_table) #8449

Closed
wants to merge 2 commits into from

3 participants

@derekkraan

There was a bug where you could use Model.joins(...).where(condition_on_joined_table).update_all(...), but the equivalent with a default scope didn't work. Basically, AR was using the where clause but not bothering to join the table, resulting in SQL errors.

After I fixed that, I found another bug where using update_all() on an ambiguous column name (in the mysql adaptor) resulted in more SQL errors. So I reproduced that in a test and fixed it too.

Feedback welcome. Let me know if it's unclear what I'm trying to do with the tests that I wrote.

@derekkraan derekkraan Make default scope with complex conditions (ie, conditions on joined
tables) work with update_all. With delete_all we raise an exception.
Fix delete_all on mysql when you try to update a column that mysql finds
ambiguous.
296603b
activerecord/test/cases/relation_scoping_test.rb
((5 lines not shown))
+ def test_default_scope_filters_on_joins
+ assert_equal 1, DeveloperFilteredOnJoins.all.count
+ assert_equal DeveloperFilteredOnJoins.all.first, developers(:david).becomes(DeveloperFilteredOnJoins)
+ end
+
+ def test_update_all_default_scope_filters_on_joins
+ DeveloperFilteredOnJoins.update_all(:salary => 65000)
+ assert_equal 65000, Developer.find(developers(:david).id).salary
+
+ # has not changed jamis
+ assert_not_equal 65000, Developer.find(developers(:jamis).id).salary
+ end
+
+ def test_delete_all_default_scope_filters_on_joins
+ assert_raise(ActiveRecord::ActiveRecordError) { DeveloperFilteredOnJoins.delete_all }
+ end

These tests have wrong indentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/test/models/author.rb
@@ -198,3 +201,9 @@ class AuthorFavorite < ActiveRecord::Base
belongs_to :author
belongs_to :favorite_author, :class_name => "Author"
end
+
+class AuthorWithPositiveReview < Author
+ def self.has_written_positive_reviews
+ joins(:reviews).where(:reviews => {:positive => true})

{ :positive => true }

Spaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/test/models/book.rb
@@ -6,4 +6,14 @@ class Book < ActiveRecord::Base
has_many :subscriptions
has_many :subscribers, :through => :subscriptions
+
+ has_many :reviews
+end
+
+class BookPositiveReview < Book
+ default_scope { positive_reviews }
+
+ def self.positive_reviews
+ joins(:reviews).where(:reviews => {:positive => true})

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/test/models/review.rb
@@ -0,0 +1,4 @@
+class Review < ActiveRecord::Base
+ belongs_to :author
+ belongs_to :book

Wrong indentation here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rmascarenhas

Seems ok.

However, pull requests should target master instead of a specific release.

@steveklabnik
Collaborator

Yes. @derekkraan is this issue present on master? if it is, please close this and re-open one against master. If not, this is fine.

@derekkraan

@steveklabnik This issue is indeed present on master (just checked now). I'll close this pull request and open a new one against master.

@derekkraan derekkraan closed this
@mnrtks mnrtks referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@mnrtks mnrtks referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@mnrtks mnrtks referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 7, 2012
  1. @derekkraan

    Make default scope with complex conditions (ie, conditions on joined

    derekkraan authored
    tables) work with update_all. With delete_all we raise an exception.
    Fix delete_all on mysql when you try to update a column that mysql finds
    ambiguous.
Commits on Dec 10, 2012
  1. @derekkraan

    Fix whitespace.

    derekkraan authored
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/associations/has_many_association.rb
@@ -95,7 +95,7 @@ def delete_records(records, method)
if method == :delete_all
update_counter(-scope.delete_all)
else
- update_counter(-scope.update_all(reflection.foreign_key => nil))
+ update_counter(-scope.update_all("#{reflection.table_name}.#{reflection.foreign_key}" => nil))
end
end
end
View
12 activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -155,6 +155,18 @@ def quote_table_name(name)
quote_column_name(name)
end
+ # Override to return the quoted table name for assignment. Defaults to
+ # table quoting.
+ #
+ # This works for mysql and mysql2 where table.column can be used to
+ # resolve ambiguity.
+ #
+ # We override this in the sqlite and postgresql adaptors to use only
+ # the column name (as per syntax requirements).
+ def quote_table_name_for_assignment(name)
+ quote_table_name(name)
+ end
+
# Returns a bind substitution value given a +column+ and list of current
# +binds+
def substitute_at(column, index)
View
4 activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -496,6 +496,10 @@ def quote_column_name(name) #:nodoc:
PGconn.quote_ident(name.to_s)
end
+ def quote_table_name_for_assignment(name)
+ quote_column_name(name.to_s.gsub(/.*\./, ''))
+ end
+
# Quote date/time values for use in SQL input. Includes microseconds
# if the value is a Time responding to usec.
def quoted_date(value) #:nodoc:
View
4 activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
@@ -186,6 +186,10 @@ def quote_column_name(name) #:nodoc:
%Q("#{name.to_s.gsub('"', '""')}")
end
+ def quote_table_name_for_assignment(name)
+ quote_column_name(name.to_s.gsub(/.*\./, ''))
+ end
+
# Quote date/time values for use in SQL input. Includes microseconds
# if the value is a Time responding to usec.
def quoted_date(value) #:nodoc:
View
3  activerecord/lib/active_record/relation.rb
@@ -283,7 +283,7 @@ def update_all(updates, conditions = nil, options = {})
stmt.table(table)
stmt.key = table[primary_key]
- if joins_values.any?
+ if with_default_scope.joins_values.any?
@klass.connection.join_to_update(stmt, arel)
else
stmt.take(arel.limit)
@@ -404,6 +404,7 @@ def destroy(id)
# +after_destroy+ callbacks, use the +destroy_all+ method instead.
def delete_all(conditions = nil)
raise ActiveRecordError.new("delete_all doesn't support limit scope") if self.limit_value
+ raise ActiveRecordError.new("delete_all doesn't support conditions on joined tables in delete statements.") if with_default_scope.joins_values.any?
IdentityMap.repository[symbolized_base_class] = {} if IdentityMap.enabled?
if conditions
View
2  activerecord/lib/active_record/sanitization.rb
@@ -102,7 +102,7 @@ def sanitize_sql_hash_for_conditions(attrs, default_table_name = self.table_name
# # => "status = NULL , group_id = 1"
def sanitize_sql_hash_for_assignment(attrs)
attrs.map do |attr, value|
- "#{connection.quote_column_name(attr)} = #{quote_bound_value(value)}"
+ "#{connection.quote_table_name_for_assignment(attr)} = #{quote_bound_value(value)}"
end.join(', ')
end
View
11 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -8,6 +8,8 @@
require 'models/category'
require 'models/post'
require 'models/author'
+require 'models/book'
+require 'models/review'
require 'models/essay'
require 'models/comment'
require 'models/person'
@@ -102,7 +104,8 @@ def test_should_generate_valid_sql
class HasManyAssociationsTest < ActiveRecord::TestCase
fixtures :accounts, :categories, :companies, :developers, :projects,
:developers_projects, :topics, :authors, :comments,
- :people, :posts, :readers, :taggings, :cars, :essays
+ :people, :posts, :readers, :taggings, :cars, :essays,
+ :books, :reviews
def setup
Client.destroyed_client_ids.clear
@@ -1749,4 +1752,10 @@ def test_replace_returns_target
assert_equal [bulb1, bulb3], car.bulbs
assert_equal [bulb1, bulb3], result
end
+
+ def test_delete_records_with_complex_joins
+ david = authors(:david).becomes(AuthorWithPositiveReview)
+ david.books_with_positive_reviews = []
+ assert_equal [], david.books_with_positive_reviews
+ end
end
View
17 activerecord/test/cases/relation_scoping_test.rb
@@ -161,6 +161,23 @@ def test_ensure_that_method_scoping_is_correctly_restored
assert !Developer.scoped.where_values.include?("name = 'Jamis'")
end
+
+ def test_default_scope_filters_on_joins
+ assert_equal 1, DeveloperFilteredOnJoins.all.count
+ assert_equal DeveloperFilteredOnJoins.all.first, developers(:david).becomes(DeveloperFilteredOnJoins)
+ end
+
+ def test_update_all_default_scope_filters_on_joins
+ DeveloperFilteredOnJoins.update_all(:salary => 65000)
+ assert_equal 65000, Developer.find(developers(:david).id).salary
+
+ # has not changed jamis
+ assert_not_equal 65000, Developer.find(developers(:jamis).id).salary
+ end
+
+ def test_delete_all_default_scope_filters_on_joins
+ assert_raise(ActiveRecord::ActiveRecordError) { DeveloperFilteredOnJoins.delete_all }
+ end
end
class NestedRelationScopingTest < ActiveRecord::TestCase
View
14 activerecord/test/fixtures/reviews.yml
@@ -0,0 +1,14 @@
+positive_review:
+ author_id: 2
+ book_id: 1
+ positive: true
+
+david_positive_review:
+ author_id: 1
+ book_id: 2
+ positive: true
+
+negative_review:
+ author_id: 3
+ book_id: 1
+ positive: false
View
9 activerecord/test/models/author.rb
@@ -104,6 +104,7 @@ def testing_proxy_target
has_many :tags_with_primary_key, :through => :posts
has_many :books
+ has_many :books_with_positive_reviews, :class_name => 'BookPositiveReview'
has_many :subscriptions, :through => :books
has_many :subscribers, :through => :subscriptions, :order => "subscribers.nick" # through has_many :through (on through reflection)
has_many :distinct_subscribers, :through => :subscriptions, :source => :subscriber, :select => "DISTINCT subscribers.*", :order => "subscribers.nick"
@@ -140,6 +141,8 @@ def testing_proxy_target
has_many :posts_with_default_include, :class_name => 'PostWithDefaultInclude'
has_many :comments_on_posts_with_default_include, :through => :posts_with_default_include, :source => :comments
+ has_many :reviews
+
scope :relation_include_posts, includes(:posts)
scope :relation_include_tags, includes(:tags)
@@ -198,3 +201,9 @@ class AuthorFavorite < ActiveRecord::Base
belongs_to :author
belongs_to :favorite_author, :class_name => "Author"
end
+
+class AuthorWithPositiveReview < Author
+ def self.has_written_positive_reviews
+ joins(:reviews).where(:reviews => { :positive => true })
+ end
+end
View
10 activerecord/test/models/book.rb
@@ -6,4 +6,14 @@ class Book < ActiveRecord::Base
has_many :subscriptions
has_many :subscribers, :through => :subscriptions
+
+ has_many :reviews
+end
+
+class BookPositiveReview < Book
+ default_scope { positive_reviews }
+
+ def self.positive_reviews
+ joins(:reviews).where(:reviews => { :positive => true })
+ end
end
View
9 activerecord/test/models/developer.rb
@@ -106,6 +106,15 @@ class DeveloperWithIncludes < ActiveRecord::Base
default_scope includes(:audit_logs)
end
+class DeveloperFilteredOnJoins < ActiveRecord::Base
+ self.table_name = 'developers'
+ has_and_belongs_to_many :projects, :foreign_key => 'developer_id', :join_table => 'developers_projects', :order => 'projects.id'
+
+ def self.default_scope
+ joins(:projects).where(:projects => { :name => 'Active Controller' })
+ end
+end
+
class DeveloperOrderedBySalary < ActiveRecord::Base
self.table_name = 'developers'
default_scope :order => 'salary DESC'
View
4 activerecord/test/models/review.rb
@@ -0,0 +1,4 @@
+class Review < ActiveRecord::Base
+ belongs_to :author
+ belongs_to :book
+end
View
6 activerecord/test/schema/schema.rb
@@ -554,6 +554,12 @@ def create_table(*args, &block)
t.integer :lock_version, :default => 0
end
+ create_table :reviews, :force => true do |t|
+ t.integer :author_id
+ t.integer :book_id
+ t.boolean :positive
+ end
+
create_table :shape_expressions, :force => true do |t|
t.string :paint_type
t.integer :paint_id
Something went wrong with that request. Please try again.