AR scope() that overrides previous where() behaves differently than chained where() #8511

Closed
mipearson opened this Issue Dec 13, 2012 · 11 comments

Comments

Projects
None yet
8 participants
Contributor

mipearson commented Dec 13, 2012

Two examples:

class PaymentGatewayEvent < ActiveRecord::Base
  scope :successful, where(:success => true)
end

PaymentGatewayEvent.where(:success => false).successful.count

# SELECT COUNT(*) FROM `payment_gateway_events`
# WHERE `payment_gateway_events`.`success` = 1

#  => 41062 

PaymentGatewayEvent.where(:success => false).where(:success => true).count 

# SELECT COUNT(*) FROM `payment_gateway_events`
# WHERE `payment_gateway_events`.`success` = 0
# AND `payment_gateway_events`.`success` = 1

#  => 0

Wrapping the scope in a lambda {} doesn't change the behaviour.

We're expecting the second (zero results) behaviour: we have an AR relation that we pass through to a summary function that works out what percentage of operations were successful. It's possible that the AR relation is, as a response to user input, filtering out all unsuccessful transactions.

But the much more worrying issue is that a scope'd where() behaves differently from one that's directly chained.

Rails 3.2.6, Ruby 1.9.3, MySQL (mysql2 driver).

Member

claudiob commented Dec 15, 2012

@mipearson I cannot tell you why, but looks like that is the expected behavior according to the test. Look here for instance (https://github.com/rails/rails/blob/master/activerecord/test/cases/named_scope_test.rb#L316):

def test_chaining_should_use_latest_conditions_when_creating
  post = Topic.rejected.new
  assert !post.approved?

  post = Topic.rejected.approved.new
  assert post.approved?

  post = Topic.approved.rejected.new
  assert !post.approved?

  post = Topic.approved.rejected.approved.new
  assert post.approved?
end
Contributor

timraymond commented Dec 15, 2012

Just from experimenting in the console a bit, it seems like the scope class method is shimming in a call to unscoped. In my app, I tried the following:

def Car < ActiveRecord::Base
  scope :mgs, where(:make => 'MG')
end

Car.mgs.count # => 1
Car.where("make <> 'MG'").mgs.count # => 0
Car.where("make <> 'MG'").where(:make => 'MG').count # => 0

Car.where(:make => 'Ford').count # => 5
Car.where(:make => 'Ford').mgs.count # => 1
Car.where(:make => 'Ford').unscoped.where(:make => 'MG').count # => 1

When looking at the Named scoping module, this definitely happens with Lambdas that are passed in, but I'm not entirely sure why it also happens with an ActiveRecord::Relation. Curiously, the behavior isn't present when the argument to where is a String. Hope this sheds a little more light on what appears to be going on.

Contributor

mipearson commented Dec 16, 2012

So - it's something to do with merging attribute hashes. Any ActiveRecord maintainers want to chime in before I submit a PR to fix this "bug" ?

@claudiob I didn't even know you could create records pre-initialized with scopes. That's clever.

Contributor

mipearson commented Dec 16, 2012

Damn, found it, desired behaviour in https://github.com/rails/rails/blob/master/activerecord/test/cases/named_scope_test.rb#L327

  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

    # Nested hash conditions with same keys
    assert_equal [posts(:sti_comments)], Post.with_special_comments.with_very_special_comments.to_a

    # Nested hash conditions with different keys
    assert_equal [posts(:sti_comments)], Post.with_special_comments.with_post(4).to_a.uniq
  end
Contributor

mipearson commented Dec 16, 2012

Okay. As this is desired behaviour (and therefore "not a bug"), I've posted this to rails-core for discussion there.

Contributor

dmitriy-kiriyenko commented Dec 24, 2012

@mipearson, posted where? I also think this can't be a desired behavious, for this totally ruins the abstraction "scopes are methods" and also makes refactoring "extract/inline method" unsafe.

Owner

rafaelfranca commented Dec 24, 2012

@jonleighton could you take a look at this issue please?

jonleighton was assigned Dec 24, 2012

Owner

rafaelfranca commented Mar 21, 2013

This was fixed on master. Closing

Nice, and what about "old"-3.2.13 rails users? oO

Looks like the fix on master happened due to this issue: #7365.

A few comments in that thread note a concern that changing this behavior will break other people's stuff. I imagine that reduces the likelihood of a backport.

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