rails: Default scope is not overridden with scope for same attributes

Hi all,

Ok, I’ve had a think about this. It raises quite a complex set of issues but I’ll try to be as clear as possible.

The basic reason that this issue has come up is that before 94924dc32baf78f13e289172534c2e71c9c8cade, default scopes were treated in a “special” way in AR which required code in all sorts of places to treat default scopes slightly differently. The aim of that commit was to simplify our implementation by removing the special treatment: now default scopes are basically just a way of redefining the output of MyModel.all.

Therefore, I’m pretty reluctant to revert 94924dc32baf78f13e289172534c2e71c9c8cade, as it makes our code quite a bit simpler, and also corrects some inherent flaws in our previous implementation of default scopes (i.e. the issue with #except mentioned in the commit message).

So, let’s assume we won’t revert 94924dc32baf78f13e289172534c2e71c9c8cade, and look at options for correcting the behaviour change presented here.

First, let’s take this assertion:

assert_equal DeveloperCalledJamis.unscoped.where(name: 'David'), DeveloperCalledJamis.where(name: 'David')

(@raysrashmi, I’ve reordered the argument as assert_equal expects the ‘expected’ value to come first - this makes test failure messages read correctly.)

So, if we accept that there shouldn’t be special handling for default scopes, we can say that the following is an equally valid assertion about the behaviour desired above:

assert_equal Developer.where(name: 'David'), Developer.where(name: 'Jamis').where(name: 'David')

But to implement this would directly contradict #7365 (which is already released in Rails 4.0). Therefore I don’t think it’s possible to make this “work” as specified by the test.

Let’s take a look at the second assertion (arguments again reordered for clarity):

assert_equal DeveloperCalledJamis.unscoped.david, DeveloperCalledJamis.david

It’s more realistic to be able to make this work, since we have control over the definition of the david method (in lib/active_record/scoping/named.rb). One thing we could do to make it pass is this:

diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb
index 49cadb6..aa8dd66 100644
--- a/activerecord/lib/active_record/scoping/named.rb
+++ b/activerecord/lib/active_record/scoping/named.rb
@@ -148,7 +148,7 @@ module ActiveRecord
           extension = Module.new(&block) if block

           singleton_class.send(:define_method, name) do |*args|
-            scope = all.scoping { body.call(*args) }
+            scope = all.merge(unscoped { body.call(*args) })
             scope = scope.extending(extension) if extension

             scope || all

However that unfortunately means that Topic.rejected.approved == Topic.approved, which again causes a regression from the behaviour implemented by #7365.

So a more nuanced thing to do would be something like:

• Check if there is any current scope
• If no, merge the scope block into the default scope
• If yes, don’t merge

That would make our assertion pass:

assert_equal DeveloperCalledJamis.unscoped.david, DeveloperCalledJamis.david

But unfortunately the following would fail:

assert_equal DeveloperCalledJamis.unscoped.order('id desc').david, DeveloperCalledJamis.order('id desc').david

So such a change would create inconsistency; not a great thing in my opinion. So I don’t think it’s really feasible to “fix” the behaviour to satisfy this assertion either.

In conclusion I think we have basically two options:

1. Retain some special handling / edge case breakage of default scopes (by reverting or partially reverting 94924dc32baf78f13e289172534c2e71c9c8cade)
2. Accept/document this behaviour change and move on

I favour the second option. I basically think that we should document that people who want this sort of functionality should change this:

class User < ActiveRecord::Base
  default_scope { where state: 'pending' }
  scope :active, -> { where state: 'active' }
  scope :inactive, -> { where state: 'inactive' }
end

To this:

class User < ActiveRecord::Base
  default_scope { where state: 'pending' }
  scope :active, -> { unscoped.where state: 'active' }
  scope :inactive, -> { unscoped.where state: 'inactive' }
end

That’s also more explicit in my opinion as well.