AssociationsPreload casts foreign keys to integers

Are you running afoul of a (lack of) quoting problem? I've got that on my todo list.

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?

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

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?

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

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.

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

Make AP not care about whether keys are strings or integers

rebased patch against current

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

-Chris

It's attached to this ticket :-)

I need some sleep.

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!