7365 merging scopes for where clauses #9553

Merged
merged 1 commit into from Mar 8, 2013

Conversation

Projects
None yet
4 participants
Member

neerajdotname commented Mar 5, 2013

No description provided.

@senny senny commented on an outdated diff Mar 5, 2013

activerecord/CHANGELOG.md
@@ -1,5 +1,11 @@
## Rails 4.0.0 (unreleased) ##
+* Previously Post.active.inactive used to result in Post.inactive
+ since the last where clause used to win while combining scopes.
+ Now all the scopes will be merged using AND. Fixes #7365 .
+
+ Neeraj Singh
@senny

senny Mar 5, 2013

Member

can you put * around your name to format it nicely?

@senny senny commented on an outdated diff Mar 5, 2013

activerecord/test/cases/named_scope_test.rb
@@ -325,11 +325,11 @@ def test_chaining_should_use_latest_conditions_when_creating
def test_chaining_should_use_latest_conditions_when_searching
# Normal hash conditions
- assert_equal Topic.where(:approved => true).to_a, Topic.rejected.approved.to_a
- assert_equal Topic.where(:approved => false).to_a, Topic.approved.rejected.to_a
+ assert_equal Topic.where(:approved => false).where(:approved => true).to_a, Topic.rejected.approved.to_a
+ assert_equal Topic.where(:approved => true).where(:approved => false).to_a, Topic.approved.rejected.to_a
@senny

senny Mar 5, 2013

Member

can you use the ruby 1.9 hash syntax?

Member

senny commented Mar 5, 2013

also can you squash the commits together into a single one?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 5, 2013

activerecord/CHANGELOG.md
@@ -1,5 +1,11 @@
## Rails 4.0.0 (unreleased) ##
+* Previously Post.active.inactive used to result in Post.inactive
@carlosantoniodasilva

carlosantoniodasilva Mar 5, 2013

Owner

Remove extra space and use `` around the code parts.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 5, 2013

activerecord/lib/active_record/scoping/named.rb
@@ -159,10 +159,16 @@ def scope(name, body, &block)
end
singleton_class.send(:define_method, name) do |*args|
- options = body.respond_to?(:call) ? unscoped { body.call(*args) } : body
- relation = all.merge(options)
+ scope = if body.respond_to?(:call)
+ if scoped = extension ? body.call(*args).extending(extension) : body.call(*args)
+ scoped.where_values = scoped.where_values + self.send(:relation).where_values
@carlosantoniodasilva

carlosantoniodasilva Mar 5, 2013

Owner

Can use +=, and is it necessary to use send here?

Member

neerajdotname commented Mar 5, 2013

@senny @carlosantoniodasilva your feedback has been incorporated.

@carlosantoniodasilva relation is a private method hence I'm using send .

@jonleighton jonleighton and 1 other commented on an outdated diff Mar 5, 2013

activerecord/lib/active_record/scoping/named.rb
@@ -159,10 +159,16 @@ def scope(name, body, &block)
end
singleton_class.send(:define_method, name) do |*args|
- options = body.respond_to?(:call) ? unscoped { body.call(*args) } : body
- relation = all.merge(options)
+ scope = if body.respond_to?(:call)
+ if scoped = extension ? body.call(*args).extending(extension) : body.call(*args)
+ scoped.where_values += self.send(:relation).where_values
+ end
@jonleighton

jonleighton Mar 5, 2013

Member

I'm not sure about this solution to the issue we discussed, but I don't have any concrete alternatives without playing with the code. So I'll try to look at it in the next few days.

@jonleighton

jonleighton Mar 5, 2013

Member

This works and is a bit less hacky IMO

diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb
index d1580a1..5388012 100644
--- a/activerecord/lib/active_record/scoping/named.rb
+++ b/activerecord/lib/active_record/scoping/named.rb
@@ -159,14 +159,18 @@ module ActiveRecord
           end

           singleton_class.send(:define_method, name) do |*args|
-            scope = if body.respond_to?(:call)
-                      if scoped = extension ? body.call(*args).extending(extension) : body.call(*args)
-                        scoped.where_values += self.send(:relation).where_values
-                      end
-                      scoped
-                    else
-                      body
-                    end
+            if body.respond_to?(:call)
+              scope = extension ? body.call(*args).extending(extension) : body.call(*args)
+
+              if scope
+                default_scoped = scope.default_scoped
+
+                scope = relation.merge(scope)
+                scope.default_scoped = default_scoped
+              end
+            else
+              scope = body
+            end

             scope || all
           end

I think this patch may cause failures in activerecord-deprecated_finders though, when somebody does:

scope :foo, conditions: { ... }

Could you take a look? Either way that functionality looks to be broken by this so we need to test it somewhere.

@neerajdotname

neerajdotname Mar 6, 2013

Member

@jonleighton changes you suggested worked just fine. All the tests are passing for activerecord and activerecord-deprecated_finders .

scope :foo, conditions: { ... } seems to be holding up. I'll update the PR to reflect the changes you suggested. Thank you.

@jonleighton

jonleighton Mar 8, 2013

Member

scope :foo, conditions: { ... } seems to be holding up.

You're right, this is actually done entirely by activerecord-deprecated_finders so changing this doesn't break it. activerecord-deprecated_finders converts scope :foo, conditions: { ... } into a callable which gets passed to super.

Neeraj Singh and John Leighton Combine scope conditions using AND
Currently Post.active.inactive will result in Post.inactive since
the last where clause wins when scopes are merged.

This pull request will merge all scopes ( barring defaul scope)
using AND.

The default scope will be overridden if another scope acts on the
same where clause.

closes #7365
cd26b6a
Member

neerajdotname commented Mar 8, 2013

@jonleighton Please take a look. Changes you suggested are in .

@jonleighton jonleighton added a commit that referenced this pull request Mar 8, 2013

@jonleighton jonleighton Merge pull request #9553 from neerajdotname/7365-mergin-scopes-and-where
7365 merging scopes for where clauses
f1082b8

@jonleighton jonleighton merged commit f1082b8 into rails:master Mar 8, 2013

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