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.

[VERIFIED] batches: :conditions for each are applied to each Model.find within the each loop

#2227

Consider the following code:


# app/models/song.rb
class Song < ActiveRecord::Base
  def self.print_pairs(name)
    Song.each(:conditions => ['name = ?', name]) do |song|
        puts song[:name]
        another_song = Song.first(:conditions => ['name <> ?', name])
        puts another_song[:name]
      end
    end
end

I would call it by passing a song name, and it would print out the name of the same song, and the name of another song (with a different name). After applying patch #2201, the .each function has become .find_each

The problem that I have found is that the :conditions applied to Song.each (line 4) is also automatically applied to the Song.first (line 6), and as a consequence Song.first returns an error, since it cannot find a song that matches both conditions ['name = ?', name] and ['name <> ?', name].

I understand that the solution to this problem is to call the inside find as following (line 6):


another_song = Song.with_exclusive_scope { first(:conditions => ['name <> ?', name]) }

However it is not clear whether this is a desired behavior or rather a bug. In this case I have not defined any default_scope for the model Song that has to be overridden by calling with_exclusive_scope, I am just calling Song.find within a Song.each(:conditions) loop.

To replicate this behavior:


rails music
cd music
script/generate scaffold song name:string
rake db:create
rake db:migrate
script/console


Song.create(:name => "Keep The Faith")
Song.create(:name => "Bed Of Roses")
class Song < ActiveRecord::Base
  def self.print_pairs(name)
    Song.each(:conditions => ['name = ?', name]) do |song|
        puts song[:name]
        another_song = Song.with_exclusive_scope { first(:conditions => ['name <> ?', name]) }
        puts another_song[:name]
      end
    end
end
Song.print_pairs('Bed Of Roses')

Once again, the same occurs also after patch #2201, calling .find_each rather than .each, and also using a named_scope to declare the conditions.

Reported by claudiob (at gmail) · March 13th, 2009 @ 09:56 AM

State: resolved
Milestone: 2.x
Assigned to: Pratik Pratik
Importance: none

Activity

  1. Pratik
    Pratik
    • Assigned user set to DHH

    March 13th, 2009 @ 10:12 AM

  2. Matthew Beale
    Matthew Beale

    This is really nasty. I've attached a patch (with tests) that doesn't use with_scope in find_in_batches.

    Check this blog post:

    http://davedupre.com/2009/05/20/gotcha-with-find_each-and-find_in_b...

    A really simple example of why scopes here are very dangerous:

    Balloons.count #=> 10
@clown = Clown.find(2)
@clown.balloons.count #=> 3
@clown.balloons.collect {|b| b.color } #=> [ 'red', blue', 'green' ]
Balloons.find_each(:conditions => { :color => 'red' }) do
  @clown = Clown.find(2)
  @clown.balloons.count #=> 1
  @clown.balloons.collect {|b| b.color } #=> [ 'red' ]
  # Where did all my friggin balloons go?
  Balloons.all.collect { |b| b.color }.uniq #=> [ 'red' ]
end

    This means inside a find_each it's impossible to ask Balloons for a non-red object. Also hard to debug :-)

    June 5th, 2009 @ 07:23 PM

  3. Matthew Beale
    Matthew Beale

    If this is closed, these can probably be marked dupes:

    https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

    This guy covers a bug with using "id" explicitly:

    https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

    June 5th, 2009 @ 07:55 PM

  4. Eugene Pimenov
    Eugene Pimenov
    • Tag changed from 2.3-rc1, 2.3-rc2, :conditions, active_record, find, named_scope to 2.3-rc1, 2.3-rc2, :conditions, active_record, find, named_scope, patch

    More elegant fix

    July 2nd, 2009 @ 03:10 AM

  5. Jacob Kjeldahl
    Jacob Kjeldahl
    • Tag changed from 2.3-rc1, 2.3-rc2, :conditions, active_record, find, named_scope, patch to 2-3-stable, 2.3-rc1, 2.3-rc2, :conditions, active_record, find, named_scope, patch

    I have verified that the 0001-find_in_batches-shouldn-t-clog-conditions-for-find-c.patch applies cleany to 2.3-stable and the tests are running.

    September 3rd, 2009 @ 02:18 PM

  6. Thong Kuah
    Thong Kuah

    +1 I have tried this patch(0001-find_in_batches-shouldn-t-clog-conditions-for-find-c.patch) against 2-3-stable, and the tests work well. Nice test - shows how innocuous find(:id) fails in a find_each block.

    September 8th, 2009 @ 01:07 PM

  7. Valentin Mihov
    Valentin Mihov

    https://rails.lighthouseapp.com/projects/8994/tickets/2791-activere...

    is the same is this one. I tested the patch on 2-3 stable and it applies and works cleanly. I like the fix. It is better than the two I offered.

    I am attaching an additional unit test from #2791. It should be applied over the patch of this issue.

    September 11th, 2009 @ 09:33 AM

  8. Thong Kuah
    Thong Kuah
    • Title changed from batches: :conditions for each are applied to each Model.find within the each loop to [VERIFIED] batches: :conditions for each are applied to each Model.find within the each loop

    state:verified

    September 11th, 2009 @ 10:21 AM

  9. DHH
    DHH
    • Assigned user changed from DHH to Pratik

    December 28th, 2009 @ 07:37 PM

  10. Brian Armstrong
    Brian Armstrong

    +1

    February 1st, 2010 @ 08:12 PM

  11. Scott Windsor
    Scott Windsor

    So... this is a bug that's been "verified". Does that mean this fix will get integrated anytime soon?

    This is kind of a horrible bug. I just ran into a case where a model was getting incremented with callbacks and that update was getting scoped. (because update_counters uses update_all). Any chance this will get integrated into 2.3 stable?

    March 25th, 2010 @ 10:52 PM

  12. chris finne
    chris finne

    +1

    April 11th, 2010 @ 01:15 PM

  13. Ryan Bigg
    Ryan Bigg
    • State changed from new to verified

    April 11th, 2010 @ 02:12 PM

  14. Repository
    Repository
    • State changed from verified to resolved

    (from [18ba648e0de032c6cb68cb48c2aaf3a347294e3d]) Implement find_in_batches without with_scope [#2227 state:resolved]

    Signed-off-by: Pratik Naik pratiknaik@gmail.com
    http://github.com/rails/rails/commit/18ba648e0de032c6cb68cb48c2aaf3...

    April 15th, 2010 @ 01:13 AM

  15. Christopher Sun
    Christopher Sun

    Is this patch supposed to fix find_in_batches with named_scopes? I'm seeing issues where conditions provided by named_scopes on a model are being passed through to subsequent finders on any instances obtained through find_in_batches. For example:

    
Subscription.active.commercial.expired_within(1.year.ago,1.year.from_now).find_in_batches(:batch_size => 500) { |subs| Subscription.first }

    My expected behavior was to not get the first subscription within active and commercial subscription scopes.

    I tested the patched in 2.3.3 and 2.3.5 and had the same issue.

    April 20th, 2010 @ 03:51 PM

  16. Pratik
    Pratik

    Please open a new ticket w/ a patch and/or failing test and assign to me.

    Thanks.

    April 20th, 2010 @ 03:55 PM

  17. Greg Hazel
    Greg Hazel
    • Importance changed from to

    Was there ever a new ticket created for this? The bug still exists in ActiveRecord 2.3.11

    March 29th, 2011 @ 12:53 AM