This project is archived and is in readonly mode.

#33 ✓resolved
Chris Hapgood

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

    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

    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

    Frederick Cheung April 22nd, 2008 @ 02:27 PM

    Could we not go the other way and assume all keys are strings ?

  • Chris Hapgood

    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

    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

    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

    Frederick Cheung April 22nd, 2008 @ 03:30 PM

    Exactly. If you look at the old eager loading code it basically did that.

  • Frederick Cheung

    Frederick Cheung April 25th, 2008 @ 10:31 PM

    Make AP not care about whether keys are strings or integers

  • Frederick Cheung
  • Chris Hapgood

    Chris Hapgood May 1st, 2008 @ 01:34 AM

    Thanks Frederick. Can you tell me where/how to see the patch?

    -Chris

  • Frederick Cheung

    Frederick Cheung May 1st, 2008 @ 02:04 AM

    It's attached to this ticket :-)

  • Chris Hapgood
  • Repository

    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]

    http://github.com/rails/rails/co...

  • Chris Hapgood

    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!

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