Rails Ruby on Rails

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.

• You flagged this item as spam.
  

  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 ?

• You flagged this item as spam.
  

  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

• You flagged this item as spam.
  

  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

  • association_preload_should_not_assume_integer_keys.patch 7.5 KB
• 

  Frederick Cheung April 30th, 2008 @ 11:17 PM

  rebased patch against current

  • 0001-Association-preload-should-not-assume-integer-keys.patch 8.3 KB

• You flagged this item as spam.
  

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

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

  -Chris

• 

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

  It's attached to this ticket :-)

• You flagged this item as spam.
  

  Chris Hapgood May 1st, 2008 @ 02:08 AM

  I need some sleep.

• 

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

• You flagged this item as spam.
  

  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!