scopes combine conditions on same attribute incorrectly #7365

Closed
grosser opened this Issue Aug 16, 2012 · 45 comments

Comments

Projects
None yet
Contributor

grosser commented Aug 16, 2012

This issue exists since 3.0 (a backport would be nice if it gets fixed)

class Car < ActiveRecord::Base
  scope :good, where(:id => 1)
  scope :bad, where(:id => 2)
end

# using scopes
Car.good.bad
SELECT "cars".* FROM "cars" WHERE "cars"."id" = 2

# using wheres
Car.where(:id => 1).where(:id => 2)
SELECT "cars".* FROM "cars" WHERE "cars"."id" = 1 AND "cars"."id" = 2

Does the same for different attributes, especially annoying when you do logic like id => [1,2,3] + id => [2,3,4] and expect only 2 to get returned

jfeaver commented Aug 26, 2012

Found some related issues: #3052, #6596

It appears to be an issue with merge. scope calls merge here deleting duplicates. In #6596, this code in merge is blamed for incorrectly flagging the first scope records as duplicates.

Member

steveklabnik commented Sep 15, 2012

@jonleighton @tenderlove any thoughts?

Owner

senny commented Oct 12, 2012

I investigated and tracked the problem back to the ActiveRecord::Relation::Merger#merged_wheres method. There is a helpful comment which was added on 2012-08-19. It was a fix, which made the behavior described in this ticket the expected behavior.

You can find the discussion and the relevant code here: #7392

Since this was discussed already and the commit was merged as a patch, I think we should close this as it is the expected behavior.

@rafaelfranca @steveklabnik thoughts?

Contributor

grosser commented Oct 12, 2012

If that's expected, it would be nice if scopes and where would behave the same

Owner

senny commented Oct 12, 2012

I think it would be helpful if both methods would behave equally. I stumbled upon the discussion about the "last equality wins" functionality by browsing the source, so I don't have more information beside the referenced PR. I think we have to wait for someone who remembers the discussion to give us a heads-up about.

Member

schneems commented Nov 3, 2012

Do you get the same behavior using class methods?

def self.good
   where(:id => 1)
end
Contributor

pjg commented Jan 5, 2013

@schneems when using class methods (i.e. Cars.good.bad and both .good and .bad are class methods) it behaves as expected, i.e. it merges both conditions: WHERE "cars"."id" = 1 AND "cars"."id" = 2 (on 3.2.10 at least), so I guess it's another argument for not using scopes.

Member

jonleighton commented Jan 11, 2013

I'm closing this as it intentionally works this way. I agree it's kinda confusing, but I don't think there's much we can do at this stage without breaking people's stuff.

Member

jonleighton commented Jan 11, 2013

Reopening after a discussion with @amatsuda.

We should consider removing the use of #merge in the method defined by scope. Somebody needs to try changing it and see what breaks in our test suite. I would also like to track down which commit added the use of #merge to scopes and if there is some explanation there.

@carlosantoniodasilva said he will try to take a look.

jonleighton reopened this Jan 11, 2013

Contributor

PikachuEXE commented Jan 13, 2013

For me

Car.good.bad

makes more sense since an attribute cannot equal to both values (Or can it?)

And I think

Car.where(:id => 1).where(:id => 2)

should do the same as scope (overwriting condition on existing attribute conditions)

Uhm, @PikachuEXE see description of this pull request - I've given reasonable example there:

amatsuda/kaminari#307

Queries like:

Product.where(:number => [1,2,3]).where(:number => [2,3,4]).page(1)

Resulted in SQL like

SELECT * FROM products WHERE number IN (2,3,4) LIMIT 25 OFFSET 0

Which was totally wrong.

Contributor

PikachuEXE commented Jan 15, 2013

@detomastah
Oh I clearly forgot about the in conditions!! (Or I did not understand it when I read the description)
Sorry for that.

Now I +1 this issue

Member

jonleighton commented Jan 18, 2013

This problem with this is that the "expected behaviour" is totally context-specific. For example, I might reasonable expect:

non_admins = [1,2,3]
blocked = [1,3]
User.where(id: non_admins).where(id: blocked)

To produce:

SELECT * FROM users WHERE id IN (1,2,3) AND id IN (1,3);

It's a contrived example, but you get the idea.

Unfortunately I don't know if it's possible to solve this in a way that pleases everyone.

Contributor

pjg commented Jan 18, 2013

@jonleighton this is exactly how it works atm (where clauses are AND-ed together in the resulting SQL) and this is imo the desired behaviour. Scopes behave differently in a way that last scope overwrites the previous ones and this is bad, imo. From what I understand the real blocker is the default_scope that needs to be overwritten if user desires so. So perhaps consider removing default_scope? :trollface:

Member

jonleighton commented Jan 18, 2013

Scopes behave differently in a way that last scope overwrites the previous ones and this is bad, imo.

Sure, but @PikachuEXE was advocating that where().where() should overwrite like scopes. Hard to please everyone, is my point :)

Contributor

pjg commented Jan 18, 2013

@jonleighton but later on he realized he was wrong ;) To quote him:

Now I +1 this issue

@jonleighton No, he was advocating for making named scopes to work exactly like class method scopes. They behave differently at the moment.

Member

jonleighton commented Jan 18, 2013

Ah yes, right.

Member

neerajdotname commented Feb 27, 2013

Since Rails4 release is being finalized lets see if we can have a consensus to move one way or the other.

As @jonleighton mentioned it is hard to please everyone. But we can have a consistent behavior across scope and where .

Car.good.bad
SELECT "cars".* FROM "cars" WHERE "cars"."id" = 2

# using wheres
Car.where(:id => 1).where(:id => 2)
SELECT "cars".* FROM "cars" WHERE "cars"."id" = 1 AND "cars"."id" = 2

I personally think that if we choose the path of last equality wins then we would be playing whack-a-mole for a while. The most consistent result with a maintainable code would be to just AND all the where and scopes without picking which one wins.

So in the above case Car.good.bad should produce SELECT "cars".* FROM "cars" WHERE "cars"."id" = 1 AND "cars"."id" = 2 .

Any thoughts .

Release of Rails4 is the best opportunity to break some backward compatibility if we have to.

Contributor

PikachuEXE commented Feb 27, 2013

I think AND makes more sense
Say you have named scopes called active and inactive
It is sensible to have nothing by using Model.active.inactive, since there should be nothing with both state

Contributor

grosser commented Feb 27, 2013

👍 for simple logic :)

On Wed, Feb 27, 2013 at 6:37 AM, PikachuEXE notifications@github.comwrote:

I think AND makes more sense
Say you have named scopes called active and inactive
It is sensible to have nothing by using Model.active.inactive, since
there should be nothing with both state


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/issues/7365#issuecomment-14176555
.

@neerajdotname agreed 👍, calling scopes should work pretty much like calling where, they should not override conditions but add on top of them. Removing conditions is a work for different APIs.

Member

neerajdotname commented Feb 27, 2013

@carlosantoniodasilva Does that mean I can start working on a patch which would behave as given below ?

  • AND all scopes Car.active.inactive
  • AND all scopes including a default scope
  • AND all combinations of scopes and where clauses
  • AND all class methods returning where conditions def self.active; where(:active => true); end

This would make code behave consistent .

@jonleighton @ernie @tenderlove Please chime in with your thoughts since this would pretty much revert the work done in #7392 .

@neerajdotname To me that seems a sane default, and it's what @jonleighton have agreed with if I understand it correctly. Lets get feedback from @ernie and @tenderlove before starting any work as you said, but if it's gonna change, now seems a good time.

Contributor

pjg commented Feb 27, 2013

I'm 👍 for treating everything as AND just like @neerajdotname has said.

I also believe doing this: AND all scopes including a default scope will render default_scope pretty pointless which I'm all for doing! (and then removing it completely).

Member

jonleighton commented Mar 1, 2013

I am 👍, I think, but:

  • We shouldn't change the implementation of #merge, just the implementation of scope (to not use #merge)
  • We shouldn't change how default scopes work, so Car.good.bad should be equivalent to Car.default_scope.merge(Car.unscoped.good.bad)
  • I'm not sure if it's too late to do this now, since the beta is already out
Member

jonleighton commented Mar 1, 2013

It looks like there will be a beta2, so I reckon we should try to go for it. @neerajdotname please work up a patch.

Member

jonleighton commented Mar 1, 2013

BTW, I think the current use of #merge in scope is actually just a historical relic of scopes that used the hash syntax. E.g.

scope :good, conditions: { id: 1 }

We need to make sure not to break support for that in activerecord-deprecated_finders.

Contributor

ernie commented Mar 1, 2013

Hey all, sorry for the late reply, I'm at a conference.

I'm also 👍 on this but would definitely love to see default_scope on its way out. A major version release would have been the time to do it but I think that now that we're already at beta it's probably too late.

@jonleighton: It is early, and I have not had coffee, so I may be missing something, but since the goofy "last equality wins" stuff lives in merge, and I think you're 👍 on removing that behavior in explicit calls to merge, but not on implicit ones, I'm not sure we can do it without changing the implementation of merge at least a little bit, or subclassing a NonOverwritingMerger with a simplified version of merged_wheres that we use for certain kinds of merges.

No?

(I should probably add that anything that kills that last equality wins stuff gets and enthusiastic 👍 from me.)

Member

jonleighton commented Mar 1, 2013

@ernie what I mean is that the implementation of scope calls merge, so we can change scope to not call merge without changing merge. make sense?

I don't think we should consider removing default_scope at this stage. I think it's too late for 4.0 and in any case it's been discussed before and decided against.

Contributor

ernie commented Mar 1, 2013

Right. I think we're saying the same thing then. If scope doesn't call merge, it'll still need to duplicate most of merge's behavior, minus last equality wins, though.

Member

jonleighton commented Mar 1, 2013

The implementation is currently:

          singleton_class.send(:define_method, name) do |*args|
            options  = body.respond_to?(:call) ? unscoped { body.call(*args) } : body
            relation = all.merge(options)

            extension ? relation.extending(extension) : relation
          end

I think it can be changed to:

          singleton_class.send(:define_method, name) do |*args|
            extension ? body.call(*args).extending(extension) : body.call(*args)
          end

(With some code to deal with backward-compat where body is not a callable.)

That would make scope :foo, -> { ... } basically identical to def self.foo; ...; end.

@jonleighton wouldn't that kill the scope chainability we always have (since it currently returns a relation no matter what the block returns)?

I've written a blog post about scopes explaining that with them you can have a condition and return nil, because a relation is always returned, and I think that's a plus for scopes I'd not like to have removed.

Member

jonleighton commented Mar 1, 2013

@carlosantoniodasilva yeah it would, but we could easily add a condition for that. Something like

          singleton_class.send(:define_method, name) do |*args|
            scope = extension ? body.call(*args).extending(extension) : body.call(*args)
            scope || all
          end

@jonleighton there we go, just wanted to make sure that wouldn't get lost 👍❤️

I think I tried removing the merge condition a while ago and got some failing tests.. Anyway, lets see if @neerajdotname can get all wrapped up :)

Thanks!

Contributor

ernie commented Mar 1, 2013

@jonleighton Ah yes, I keep forgetting we're forcing callables now. Sorry for the confusion.

Member

neerajdotname commented Mar 1, 2013

I'll start digging into code. Might ping someone if I get stuck since I'm not very familiar with this code.

Contributor

ernie commented Mar 1, 2013

@jonleighton OK, sorry, I know I'm probably being really dense, but I was thinking about it some more, and how would just invoking a callable inside the generated scope method yield the proper return value for the chained scope? It seems like it'd work for the immediate scope, but not for a chained call, because of the block's closure at the time the class was loaded. Isn't that why we merged on the results of the call to begin with?

Member

jonleighton commented Mar 1, 2013

@ernie sorry, I'm not understanding your question :( can you restate it?

Contributor

ernie commented Mar 2, 2013

@jonleighton I am now drinking coffee, and can safely be ignored. Forgot about the the use of scoping when a relation is delegating to the class's scope. Tested this out locally and looks to be a solid fix.

Member

neerajdotname commented Mar 3, 2013

@jonleighton quick update and I need a bit of help.

Changes so far are here https://github.com/neerajdotname/rails/compare/7365-mergin-scopes-and-where .
All the tests are passing except this one https://github.com/rails/rails/blob/master/activerecord/test/cases/named_scope_test.rb#L120

Here is the issue

class Post < ActiveRecord::Base
  scope :published, -> { where(status: 'published') }
end

class SpecialPost < Post
end

> SpecialPost.all
#=> SELECT "posts".* FROM "posts" WHERE "posts"."type" IN ('SpecialPost')

> SpecialPost.published
#=> SELECT "posts".* FROM "posts" WHERE "posts"."status" = 'published' 

Notice that in the last sql IN clause is missing.

When SpecialPost.published is executed then the block is executed and since it is lexically scoped self is Post when this code is executed https://github.com/rails/rails/blob/master/activerecord/lib/active_record/core.rb#L147 . This results in STI where clause not being added.

Previously it was working because the code had all.merge(options) . And all was building relation which was taking care of STI .

I looked around and I was able to make some progress by using instance_exec here https://github.com/rails/activerecord-deprecated_finders/blob/master/lib/active_record/deprecated_finders/base.rb#L28 and by setting @klass to SpeicalPost. But that seems a bit hacky.

Any thoughts on how to take care of STI issue. Maybe there is an easier solution which I'm missing . Thanks.

Member

jonleighton commented Mar 3, 2013

@neerajdotname see the relation method in core.rb. That returns a relation with the STI condition where applicable. So I think it should work if you do something like: relation.scoping { body.call }.

Member

neerajdotname commented Mar 3, 2013

@jonleighton relation.scoping { body.call } did not work because the self issue comes up inside the body.call.

This workaround worked for me and now all tests are passing. neerajdotname/rails@a4eae8b

As you can see I'm getting relation and just picking the where_values.

Full diff is here neerajdotname/rails@master...7365-mergin-scopes-and-where

Member

neerajdotname commented Mar 5, 2013

@jonleighton

PR is here . #9553

Contributor

pjg commented Mar 10, 2013

Nice to see this merged. Good work!

@kennyadsl kennyadsl pushed a commit to kennyadsl/spree that referenced this issue Nov 2, 2013

@huoxito @radar huoxito + radar Fix conflict between product available and search scopes
Search::Base#retrieve_products would add another scope regarding
spree_prices currency to the query. While in Rails < 4 the last scope
would override the first now it just combines all of them with "AND"

see rails/rails#7365 for lots of discussion on
the subject

I let the `currency` argument on the `available` scope for now just so
that we don't need to change too much code at this point
8403be6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment