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
@rmascarenhas rmascarenhas commented on an outdated diff Dec 9, 2012
activerecord/test/cases/relation_scoping_test.rb
+ 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
@rmascarenhas rmascarenhas commented on an outdated diff Dec 9, 2012
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
@rmascarenhas rmascarenhas commented on an outdated diff Dec 9, 2012
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
@rmascarenhas rmascarenhas commented on an outdated diff Dec 9, 2012
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
Ruby on Rails member

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 Dec 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment