This project is archived and is in readonly mode.
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:
Comments and changes to this ticket
-
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?
-
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 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 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 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. -
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 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... -
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>
People watching this ticket
Attachments
Referenced by
- 3165 ActiveRecord::MissingAttributeError after update to rails v 2.3.4 This is being caused by the fix introduced in #2543, whic...
- 3165 ActiveRecord::MissingAttributeError after update to rails v 2.3.4 This bug bit me as well. Instead of changing the select q...
- 2543 Make ActiveRecord::Base.exists? invoke find_initial to support :include scopes Signed-off-by: Michael Koziarski michael@koziarski.com [#...
- 2543 Make ActiveRecord::Base.exists? invoke find_initial to support :include scopes Signed-off-by: Michael Koziarski michael@koziarski.com [#...
- 5801 ActiveRecord::Base#exists? raises exception when using :include in default_scope This error appears to be related to the change introduced...
- 5801 ActiveRecord::Base#exists? raises exception when using :include in default_scope This error appears to be related to the change introduced...