Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

default_scope treats hashes and relations inconsistently when overwriting

#4598

In working on #4583 Merge default scopes by default, Brian and I discovered some inconsistent behavior. Here are a couple of tests that should both behave the same way, but one passes and one fails:

http://github.com/dchelimsky/rails/commit/6180043572ff30a2e01690b86...

require "cases/helper"
require 'models/developer'

class ArelBugTest < ActiveRecord::TestCase
  fixtures :developers

  # We would expect both to pass or both to fail, but one passes and one fails.

  # fails
  def test_default_scope_called_twice_with_relations
    klass = Class.new(Developer)
    klass.class_eval do 
      default_scope where(:name => 'David')
      default_scope where(:name => 'Jamis')
    end
    assert_equal ["Jamis"], klass.all.map(&:name).uniq.sort
  end
  
  # passes
  def test_default_scope_called_twice_with_hashes
    klass = Class.new(Developer)
    klass.class_eval do 
      default_scope :conditions => { :name => 'David' }
      default_scope :conditions => { :name => 'Jamis' }
    end
    assert_equal ["Jamis"], klass.all.map(&:name).uniq.sort
  end
end

Reported by David Chelimsky · May 14th, 2010 @ 10:41 PM

State: open
Milestone: 3.1
Assigned to: Aaron Patterson Aaron Patterson
Importance: Low

Activity

  1. Neeraj Singh
    Neeraj Singh

    In the first test case where relation is being passed the sql query is being built something like this.

    > us = User.unscoped;
>   us=us.merge(User.where(:name => 'David'));
>   us=us.merge(User.where(:name => 'Jamis'));
>   us.to_sql
 => "SELECT     \"users\".* FROM       \"users\" WHERE     (\"users\".\"name\" = 'Jamis')"

    In the second case where hash is being passed the sql query is being built something like this.

    > us = User.unscoped;
> us = us.where(:name => 'David');
> us = us.where(:name => 'Jamis');
> us.to_sql
  => "SELECT     \"users\".* FROM       \"users\" WHERE     (\"users\".\"name\" = 'David') AND (\"users\".\"name\" = 'Jamis')"

    Before I suggest a fix, I would like to know what direction to proceed.

    Based on ticket #4583 Merge default scopes by default I should assume that the desired behavior should be the second sql so that one can pass more than one default_scope and all the conditions passed should be ANDed to build the final sql.

    May 15th, 2010 @ 04:41 AM

  2. David Chelimsky
    David Chelimsky

    I would expect the same column to get ORed (an IN clause) and different columns to get ANDed:

    user = User.unscoped
user = user.where(:name => 'David')
user = user.where(:name => 'Jamis')
user = user.where(:role => 'admin')
user.to_sql
=> SELECT \"users\".* FROM \"users\" WHERE (\"users\".\"name\" in ('David','Jamis')) AND (\"users\".\"role\" = 'admin')

    Agree?

    May 15th, 2010 @ 04:56 AM

  3. Santiago Pastorino
    Santiago Pastorino

    +1 Agree! i will try a fix

    May 15th, 2010 @ 04:36 PM

  4. Santiago Pastorino
    Santiago Pastorino
    • Milestone cleared.
    • Tag changed from activerecord, bug to activerecord, bug, bugmash
    • State changed from new to open
    • Assigned user changed from José Valim to Santiago Pastorino

    May 15th, 2010 @ 08:49 PM

  5. Santiago Pastorino
    Santiago Pastorino

    Test added here, i don't know if we should test this kind of things or fix in Arel and rely on his behavior.
    Anyways i add the test case only for now.
    I'm going to patch it.

    May 15th, 2010 @ 09:45 PM

  6. Neeraj Singh
    Neeraj Singh
    • Tag changed from activerecord, bug, bugmash to activerecord, bug, bugmash, patch

    Attached is a code patch.

    @Santiago thanks for the test.

    May 16th, 2010 @ 01:17 AM

  7. Rohit Arondekar
    Rohit Arondekar
    • Importance changed from to Low

    The test still fails in 3-0-stable.

      1) Failure:
test_find_all_using_where_twice_should_or_the_relation(RelationTest) [test/cases/relations_test.rb:655]:
<[#<Author id: 1, name: "David", author_address_id: 1, author_address_extra_id: 2>]> expected but was
<[]>.

    However the patch provided by Neeraj doesn't apply any more so I couldn't check if it solves the problem.

    August 4th, 2010 @ 11:47 AM

  8. Rohit Arondekar
    Rohit Arondekar

    I applied Neeraj's patch manually and it made the test pass. Here is a combined patch for 3-0-stable with both the test and fix. Tests are passing.

    August 4th, 2010 @ 01:55 PM

  9. Jeremy Kemper
    Jeremy Kemper
    • Milestone cleared.

    August 30th, 2010 @ 04:10 AM

  10. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2

    October 15th, 2010 @ 11:01 PM

  11. Santiago Pastorino
    Santiago Pastorino
    • Milestone changed from 3.0.2 to 3.1
    • Assigned user changed from Santiago Pastorino to Aaron Patterson

    November 7th, 2010 @ 11:38 AM

  12. Repository
    Repository
    • State changed from open to resolved

    November 18th, 2010 @ 01:14 AM

  13. Repository
    Repository

    November 18th, 2010 @ 01:14 AM

  14. Jeremy Kemper
    Jeremy Kemper
    • State changed from resolved to open
    • Milestone changed from 3.1 to 3.0.5

    http://groups.google.com/group/rubyonrails-core/browse_thread/threa...

    This inconsistency seems ok in context: second hash condition overwrites the first, whereas the second .where relation ANDs on to it.

    February 16th, 2011 @ 05:27 PM

  15. David Chelimsky
    David Chelimsky

    FWIW, I think that having to know that the .where and conditions work differently is confusing and puts an unnecessary burden on users, even if everything is visible in the same context. In our case, we broke the behavior of a gem by adding conditions to the default scope in a model, so we didn't have all the context in one place. Having conditions and .where work differently made it all the more confusing, which is why I submitted this ticket in the first place.

    February 17th, 2011 @ 07:56 AM

  16. Aaron Patterson
    Aaron Patterson
    • Milestone changed from 3.0.5 to 3.1

    I think what we need to do is change this to do an AND in both cases for Rails 3.1. I don't like that we change it to an OR when combining only certain columns. It's like we're tying to read the user's mind, and I am terrible at mind reading.

    February 24th, 2011 @ 02:03 AM