AssociationsPreload casts foreign keys to integers
Reported by Chris Hapgood | April 21st, 2008 @ 08:52 PM
In several places in the association preload code, foreign keys are converted to integers (to_i). For example, in the `preload_has_many_association` method, there is this line:
add_preloaded_records_to_collection(id_to_record_map[through_record[through_primary_key].to_i], ...
It appears these casts are required because the `preload_has_and_belongs_to_many_association` method does not fetch the _parent_record_id in such as way as to preserve the DB type.
Nowhere else in AR are keys forced to integers and nowhere else (AFAICT) do db queries result in ambiguous return types. Consequently, AssociationPreload is going to single handedly break a lot of legacy apps and some plugins (multipart keys, UUID keys, etc.). It also sets a precedent of not being able to assume keys are returned as their native type.
Unfortunately, my DB adapter fu is not strong enough to understand WHY the query in `preload_has_and_belongs_to_many_association` confuses the adapter. Any help would be greatly appreciated as I try to run this problem down...
NB: Previously discussed patch (also on Trac) is inadequate to fix the problem. No patch is currently available.
Comments and changes to this ticket
-
Frederick Cheung April 22nd, 2008 @ 12:23 AM
Are you running afoul of a (lack of) quoting problem? I've got that on my todo list.
-
Chris Hapgood April 22nd, 2008 @ 02:10 PM
Frederick,
I tried to find you online yesterday to discuss this as I saw your name all over the patch.
I don't think it's a quoting problem. I think it is a much more fundamental problem in several of the Rails DB adapters.
Quick Summary: When extending the "selected" fields of a record with a calculated or custom field, AR cannot determine the type of the field. For example
extended_post = Post.find(:first, :select => "posts.*, posts.id as _custom")
In this example, extended_post['_custom'] is a string even though the id column is an integer. Now that we have fallen into the trap, there is only one way out: assume all keys are integers and cast. Game Over.
It would appear that AssociationsPreload is the first place in Rails where extended records are exploited internally. And it exposes the weakness in the DB adapters. I can confirm that the MySQL adapter has this problem, and the SQLite3 adapter. Rumour is that the Oracle adapter does not have this problem.
What to do?
1. Fix the DB adapters to return type information with custom fields. While this is the ideal solution, it's not likely to happen soon. And it is probably impossible with some adapters.
2. Back out the AssociaitonsPreload code. Sadly, I think we are too far gone for this to be realistic. I know a lot of people put a lot of time into this performance optimization, and realistically, I know a lot of people don't care about non-integer keys.
3. Recode AP to not use extended fields. I don't know enough about AP to assess if this is realistic - I would appreciate comments.
4. Recode AP to examine the foreign key type indirectly and cast only as required. If Forum has_many Posts, AR knows the type of field for forums.#{primary_key} (DESCRIBE) and thus it knows the foreign key posts.forum_id when AP is preloading from posts. By consulting this information, it would be possible to cast only when there is a mismatch between the custom field's type and the expected foreign key type.
Your thoughts?
-
Frederick Cheung April 22nd, 2008 @ 02:27 PM
Could we not go the other way and assume all keys are strings ?
-
Chris Hapgood April 22nd, 2008 @ 02:32 PM
Frederick,
I love you man, but I just don't think that would work. It would require unprecedented Ruby fu to always pull off the "123" == 123 when comparing keys (uniq, amongst others). Or force the keys to strings EVERYWHERE, which would be a backwards compatible nightmare (but otherwise kinda smart).
Or am I missing something?
-
Frederick Cheung April 22nd, 2008 @ 02:44 PM
Not everywhere, just for the bits in association preload where it's trying to match up records to their owners. I'll give it a go and see if it works
-
Chris Hapgood April 22nd, 2008 @ 03:28 PM
Ahhh... I think I see where you are going with this. Casting _parent_record_id to a string (and everything it's compared to) would not lose any fidelity incomparisons (which is not the case for to_i) and since it's only used internally in AP, who cares if it doesn't match the expected class! Brilliant.
-
Frederick Cheung April 22nd, 2008 @ 03:30 PM
Exactly. If you look at the old eager loading code it basically did that.
-
Frederick Cheung April 25th, 2008 @ 10:31 PM
Make AP not care about whether keys are strings or integers
-
-
Chris Hapgood May 1st, 2008 @ 01:34 AM
Thanks Frederick. Can you tell me where/how to see the patch?
-Chris
-
-
-
Repository May 1st, 2008 @ 05:31 AM
- → State changed from “new” to “resolved”
(from [6f20efdaf733db26fbf337da73121983785064d5]) Fixed AssociationsPreload such that it doesnt require foreign keys to be integers (fcheung) [#33 state:resolved]
-
Chris Hapgood May 1st, 2008 @ 02:04 PM
I am really pleased that this ticket was acted on so quickly.
I know that non-integer keys are not exactly mainstream RoR (although if you saw all the mileage I get out of UUIDs, you might wish they were). That just makes the response on this patch all the more impressive.
Frederick: thanks!
Please Login or create a free account to add a new comment.
You can update this ticket by sending an email to from your email client. (help)
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Source available from github
The Git repository resides at http://github.com/rails
Check out the current development trunk (Edge Rails) with:
git clone git://github.com/rails/rails.git
Creating or reviewing a patch
See the contributor guide.
Creating a feature request
Please don't. If you want a new feature in Rails, you'll have to pull up your sleeves and get busy yourself. Or convince someone else to do it. See the contributor guide on how to get going. But posting them here is just going to lead to ticket root.
Creating a bug report
When creating a bug report, be sure to include as much relevant information as possible. Post the code sample that causes the problem. Preferably, alter the unit tests and show through either changed or added tests how the expected behavior is not occuring.
Security vulnerabilities should be reported via an email to security@rubyonrails.org, do not use trac for reporting security vulnerabilities. All content in trac is publicly available as soon as it is posted.
Then don't get your hopes up. Unless you have a "Code Red, Mission Critical, The World is Coming to an End" kinda bug, you're creating this ticket in the hope that others with the same problem will be able to collaborate with you on solving it. Do not expect that the ticket automatically will see any activity or that others will jump to fix it. Creating a ticket like this is mostly to help yourself start on the path of fixing the problem and for others to sign on to with a "I'm having this problem too".
