convert float and decimal to int for association preload #111

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

dhall commented Jun 5, 2012

Several db's I write tools for have BigDecimal primary keys. Rails and CPK handle this very well except for when I try to do eager loading of associations. This patch fixes this problem for me and I don't think it will cause any problems, but please let me know if you think otherwise. I can demonstrate this with a simple app but couldn't see an easy way to write a failing test to the project without adding new tables/models/associations. If this is the preferred approach let me know and I will.

Contributor
cfis commented Jun 8, 2012

Thanks for the patch.

So without it, what error do you get? And how does Rails already handle this for tables with just one primary key?

Charlie

dhall commented Jun 8, 2012

Thanks for taking a look at this. Rails has an issue with this for a single primary key as well. I've submitted a patch in parallel for it here rails/rails#6637.

Without the patch you get the following (cleaned up a bit) -- let me know if I can do anything else.

> list = Gateway.scoped.includes(:organization)
(Object doesn't support #inspect)
> p list
Gateway Load (0.3ms)  SELECT "gateways".* FROM "gateways" 
Organization Load (0.2ms)  SELECT "organizations".* FROM "organizations" WHERE ((("organizations"."org_id" = 1)))
NoMethodError: undefined method `each' for nil:NilClass
from /Users/dhall/.rvm/gems/ruby-1.9.3-p125/gems/composite_primary_keys-5.0.4/lib/composite_primary_keys/associations/preloader/association.rb:42:in `block in associated_records_by_owner'
from /Users/dhall/.rvm/gems/ruby-1.9.3-p125/gems/composite_primary_keys-5.0.4/lib/composite_primary_keys/associations/preloader/association.rb:33:in `each'
from /Users/dhall/.rvm/gems/ruby-1.9.3-p125/gems/composite_primary_keys-5.0.4/lib/composite_primary_keys/associations/preloader/association.rb:33:in `associated_records_by_owner'
from /Users/dhall/.rvm/gems/ruby-1.9.3-p125/gems/activerecord-3.2.3/lib/active_record/associations/preloader/singular_association.rb:9:in `preload'
from /Users/dhall/.rvm/gems/ruby-1.9.3-p125/gems/activerecord-3.2.3/lib/active_record/associations/preloader/association.rb:19:in `run'
from /Users/dhall/.rvm/gems/ruby-1.9.3-p125/gems/activerecord-3.2.3/lib/active_record/associations/preloader.rb:128:in `block (2 levels) in preload_one'

.... etc

Contributor
cfis commented Jun 13, 2012

Ok, I played around with this a bit more and learned about BigDecimal. I'm inclined not to commit this for two reasons.

  1. I don't think the conversion of float to integer is a good idea because it loses information. Not sure why anyone would want a float as a key, but if they do, then surely they don't want it to become an integer.
  2. As far as the bigdecimal change, its kind of an ugly wart. Probably what would be better is skipping this whole conversion to strings and use the actual objects instead. But ActiveRecord doesn't do that so CPK just follows along. But that seems to me cleaner and avoids this issue entirely. Not sure why there is the conversion to string, I'd have to get familiar with that bit of code again.
@dhall dhall closed this Jun 13, 2012
dhall commented Jun 13, 2012

Charlie, thanks for taking a look at this. Excellent point about float -- honestly I just included it becuase it was the other column type that could have the issue. I'll close this and pursue a change in ActiveRecord that addresses the underlying problem. Thanks for all your work on this problem -- I appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment