This project is archived and is in readonly mode.

#2543 ✓committed
Peter Marklund

Make ActiveRecord::Base.exists? invoke find_initial to support :include scopes

Reported by Peter Marklund | April 22nd, 2009 @ 12:32 PM

Currently ActiveRecord::Base.exists? does not support scopes with the :include option and it breaks for example if you have a scope with :include and an :order that depends on that :include. This patch solves this problem by making exists? reuse working code that we already have in the find family of methods (find, find_every, find_initial etc.).

This bug is discussed here:

http://groups.google.com/group/r...

Comments and changes to this ticket

  • Peter Marklund

    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

    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?

  • Peter Marklund

    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

  • Frederick Cheung

    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

  • Peter Marklund

    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.

  • Peter Marklund

    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

    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.

  • Peter Marklund

    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

  • Repository

    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

    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...

  • Robin Salkeld

    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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages