Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Peter Marklund April 22nd, 2009 @ 12:34 PM

  • Tag changed from “2-3-stable, 2.3.2, activerecord, exists, scope” to “2-3-stable, 2.3.2, activerecord, exists, patch, scope”
• 

  Michael Koziarski April 27th, 2009 @ 09:29 AM

  • Assigned user set to “Michael Koziarski”
  • Milestone cleared.

  As discussed on the mailing list, this misbehaviour can cause problems with association_include.

  Shouldn't it be possible to use count == 1 instead of find_initial? That'd at least avoid instantiating an object and firing the relevant callbacks?

• You flagged this item as spam.
  

  Peter Marklund April 27th, 2009 @ 10:17 AM

  Thanks Michael! I think the count approach sounds like a really good. I uploaded a new patch.

  Cheers

  Peter

  • exists_with_include.diff 2 KB
• 

  Frederick Cheung May 10th, 2009 @ 04:26 PM

  Back when the current implementation of exists? came into existance using count == 1 was discussed. Using count has performance implications since it means the database has to find all rows (and count them) rather than just a row

• You flagged this item as spam.
  

  Peter Marklund May 10th, 2009 @ 09:41 PM

  Frederick, thanks for the input on performance implications of count. I just wanted to give a recap of the options discussed here. We have:

  1) The current implementation of exists? that does a connection.select_all table_name.primary_key with :limit => 1 within scopes where eager loaded tables are referenced.

  2) The current patch that uses count(table_name.primary_key) and potentially performs worse.

  3) My original patch that does a find_initial table_name.primary_key. The downside here is that we instantiate AR objects unnecessarily. The upside is that scopes with references to eager loaded tables work, thanks to this piece of logic in find_every:

        if include_associations.any? && references_eager_loaded_tables?(options)
          records = find_with_associations(options)
        else
          records = find_by_sql(construct_finder_sql(options))
          if include_associations.any?
            preload_associations(records, include_associations)
          end
        end
  
  

  Would it make sense to make it possible to do a find_every(:instantiate => false) that skips instantiation? Or would that maybe just be conceptually odd and convolute the code? If we had such an option then exists? using find_initial would be identical to the current implementation (connection.select_all) except when eager loaded tables are referenced in which case those would be included properly and the query wouldn't break.

• You flagged this item as spam.
  

  Peter Marklund May 10th, 2009 @ 09:44 PM

  Just to clarify, the implementation of exists? from my original patch looked like this:

  def exists?(id_or_conditions = {})
    find_initial(
      :select => "#{quoted_table_name}.#{primary_key}",
      :conditions => expand_id_conditions(id_or_conditions)) ? true : false
  end
  
  

  So my idea was to add an :instantiate => false to the find_initial call.

• 

  Michael Koziarski May 14th, 2009 @ 07:11 AM

  Don't worry about skipping instantiation, that's just me being
  paranoid about performance.

  Upload a version with find_initial (the original patch is gone?) and
  I'm happy to apply it.

• You flagged this item as spam.
  

  Peter Marklund May 14th, 2009 @ 08:35 AM

  Hi Michael!
  I agree with you that the find_initial approach seems like the best at this point. I've attached a patch file.

  Thanks!

  Peter

  • exists-with-include.diff 2.1 KB
• 

  Repository May 14th, 2009 @ 09:42 AM

  • State changed from “new” to “committed”

  (from [afcbdfc15f919a470e4cfca97fb0084eebd2ab1f]) Changed ActiveRecord::Base#exists? to invoke find_initial so that it is compatible with, and doesn't lose, :include scopes (references to eager loaded tables)

  Signed-off-by: Michael Koziarski michael@koziarski.com
  [#2543 state:committed] http://github.com/rails/rails/commit/afcbdfc15f919a470e4cfca97fb008...

• 

  Repository May 14th, 2009 @ 09:42 AM

  (from [0380e9ca5fa872e56d0920e6255a2f20b6e01030]) Changed ActiveRecord::Base#exists? to invoke find_initial so that it is compatible with, and doesn't lose, :include scopes (references to eager loaded tables)

  Signed-off-by: Michael Koziarski michael@koziarski.com
  [#2543 state:committed] http://github.com/rails/rails/commit/0380e9ca5fa872e56d0920e6255a2f...

• You flagged this item as spam.
  

  Robin Salkeld July 8th, 2010 @ 07:03 PM

  • Importance changed from “” to “Low”

  Hi all,

  I know this is an old discussion, but I just hit this change when upgrading from 2.3.5 to 2.3.8 and it's caused me a bit of trouble. The issue is the fact that the find_initial call only selects the primary key. This means we now have classes instantiated without any of their expected attributes, and in some cases this can lead to "attribute missing" errors.

  My particular experience with this was due to defining the eql? and hash methods on an ActiveRecord class to use one of the attributes, and at some point during the find flow it called uniq on an Array of them.

  My suggestion would be to either remove the :select option (possibly a performance issue, but doesn't seem that bad to load extra columns) or perhaps refactor things so that exists? and find_initial can call a common method to build up the needed SQL.

  Thanks,
  Robin