From 64b9e93bb571160315987862583a83222e506734 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 20 Nov 2013 21:51:35 +0000 Subject: Fix ActiveRecord::Relation#unscope I'm pretty confused about the addition of this method. The documentation says that it was intended to allow the removal of values from the default scope (in contrast to #except). However it behaves exactly the same as except: https://gist.github.com/jonleighton/7537008 (other than having a slightly enhanced syntax). The removal of the default scope is allowed by 94924dc32baf78f13e289172534c2e71c9c8cade, which was not a change we could make until 4.1 due to the need to deprecate things. However after that change #unscope still gives us nothing that #except doesn't already give us. However there *is* a desire to be able to unscope stuff in a way that persists across merges, which would allow associations to be defined which unscope stuff from the default scope of the associated model. E.g. has_many :comments, -> { unscope where: :trashed } So that's what this change implements. I've also corrected the documentation. I removed the guide references to #except as I think unscope really supercedes #except now. While we're here, there's also a potential desire to be able to write this: has_many :comments, -> { unscoped } However, it doesn't make sense and would not be straightforward to implement. While with #unscope we're specifying exactly what we want to be removed from the relation, with "unscoped" we're just saying that we want it to not have some things which were added earlier on by the default scope. However in the case of an association, we surely don't want *all* conditions to be removed, otherwise the above would just become "SELECT * FROM comments" with no foreign key constraint. To make the above work, we'd have to somehow tag the relation values which get added when evaluating the default scope in order to differentiate them from other relation values. Which is way too much complexity and therefore not worth it when most use cases can be satisfied with unscope. Closes #10643, #11061. --- activerecord/CHANGELOG.md | 5 +++++ activerecord/lib/active_record/relation.rb | 2 +- .../lib/active_record/relation/query_methods.rb | 17 ++++++++++++----- .../cases/associations/has_many_associations_test.rb | 9 +++++++++ activerecord/test/cases/relation/mutation_test.rb | 2 +- activerecord/test/cases/scoping/default_scoping_test.rb | 6 ++++++ activerecord/test/models/car.rb | 1 + 7 files changed, 35 insertions(+), 7 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index cd3ca2ab34..97e5d236a8 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Make ActiveRecord::Relation#unscope affect relations it is merged in + to. + + *Jon Leighton* + * Use strings to represent non-string `order_values`. *Yves Senn* diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 6e0669a77f..745c6cf349 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -7,7 +7,7 @@ module ActiveRecord MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group, :order, :joins, :where, :having, :bind, :references, - :extending] + :extending, :unscope] SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering, :reverse_order, :distinct, :create_with, :uniq] diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index b6a7c25b4b..473762011b 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -341,13 +341,19 @@ module ActiveRecord # User.where(name: "John", active: true).unscope(where: :name) # == User.where(active: true) # - # Note that this method is more generalized than ActiveRecord::SpawnMethods#except - # because #except will only affect a particular relation's values. It won't wipe - # the order, grouping, etc. when that relation is merged. For example: + # This method is similar to except, but unlike + # except, it persists across merges: # - # Post.comments.except(:order) + # User.order('email').merge(User.except(:order)) + # == User.order('email') + # + # User.order('email').merge(User.unscope(:order)) + # == User.all + # + # This means it can be used in association definitions: + # + # has_many :comments, -> { unscope where: :trashed } # - # will still have an order if it comes from the default_scope on Comment. def unscope(*args) check_if_method_has_arguments!(:unscope, args) spawn.unscope!(*args) @@ -355,6 +361,7 @@ module ActiveRecord def unscope!(*args) # :nodoc: args.flatten! + self.unscope_values += args args.each do |scope| case scope diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index b11d27467b..359bcfba5f 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1761,4 +1761,13 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal 1, speedometer.minivans.to_a.size, "Only one association should be present:\n#{speedometer.minivans.to_a}" assert_equal 1, speedometer.reload.minivans.to_a.size end + + test "can unscope the default scope of the associated model" do + car = Car.create! + bulb1 = Bulb.create! name: "defaulty", car: car + bulb2 = Bulb.create! name: "other", car: car + + assert_equal [bulb1], car.bulbs + assert_equal [bulb1, bulb2], car.all_bulbs.sort_by(&:id) + end end diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb index a70f979442..7cb2a19bee 100644 --- a/activerecord/test/cases/relation/mutation_test.rb +++ b/activerecord/test/cases/relation/mutation_test.rb @@ -20,7 +20,7 @@ module ActiveRecord @relation ||= Relation.new FakeKlass.new('posts'), Post.arel_table end - (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order]).each do |method| + (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order, :unscope]).each do |method| test "##{method}!" do assert relation.public_send("#{method}!", :foo).equal?(relation) assert_equal [:foo], relation.public_send("#{method}_values") diff --git a/activerecord/test/cases/scoping/default_scoping_test.rb b/activerecord/test/cases/scoping/default_scoping_test.rb index 2fbe19ab06..11dd32bfb8 100644 --- a/activerecord/test/cases/scoping/default_scoping_test.rb +++ b/activerecord/test/cases/scoping/default_scoping_test.rb @@ -262,6 +262,12 @@ class DefaultScopingTest < ActiveRecord::TestCase end end + def test_unscope_merging + merged = Developer.where(name: "Jamis").merge(Developer.unscope(:where)) + assert merged.where_values.empty? + assert !merged.where(name: "Jon").where_values.empty? + end + def test_order_in_default_scope_should_not_prevail expected = Developer.all.merge!(order: 'salary desc').to_a.collect { |dev| dev.salary } received = DeveloperOrderedBySalary.all.merge!(order: 'salary').to_a.collect { |dev| dev.salary } diff --git a/activerecord/test/models/car.rb b/activerecord/test/models/car.rb index 6d257dbe7e..8f3b70a7c6 100644 --- a/activerecord/test/models/car.rb +++ b/activerecord/test/models/car.rb @@ -1,5 +1,6 @@ class Car < ActiveRecord::Base has_many :bulbs + has_many :all_bulbs, -> { unscope where: :name }, class_name: "Bulb" has_many :funky_bulbs, class_name: 'FunkyBulb', dependent: :destroy has_many :foo_bulbs, -> { where(:name => 'foo') }, :class_name => "Bulb" -- cgit v1.2.3