hzamani / active_record-acts_as

Simulate multi-table inheritance for activerecord models
MIT License
252 stars 86 forks source link

Column name collisions in queries #58

Closed extrajordanary closed 8 years ago

extrajordanary commented 8 years ago

Can't do a simple search against columns with the same name on both the actable and acting_as models. Could be more, but by default this will always include the created_at and updated_at fields.

irb(main):038:0* ActingAsModel.where("created_at > ?", time)
ActiveRecord::StatementInvalid: PG::AmbiguousColumn: ERROR:  column reference "created_at" is ambiguous
LINE 1: ..." AND "actable_model"."actable_type" = $1 WHERE (created_at...

The current workaround is that you have to search against the actable model and add the actable_type for the model you are actually searching.

ActableModel.where("created_at > ?", time).where(:actable_type => "ActingAsModel")
hzamani commented 8 years ago

Timestamps aren't needed on both models, and other names should not be same on both classes.

stephen-puiszis commented 8 years ago

@hzamani @extrajordanary Then shouldn't updating a sub-classed model update the timestamps on the parent? The parent_class.updated_at timestamp should update when the sub_class is updated. Gem v1.0.6, Rails 4.2.2


[4] pry(main)> @bank_financials = BankFinancials.last
=> #<BankFinancials:0x0000011186c718
 id: 42547,
 growth_in_equity: 0.7000001,
 <omitted columns>

 [5] pry(main)> @bank_financials.acting_as
=> #<Financials:0x00000111867d30
 id: 80493,
 actable_id: 42547,
 actable_type: "BankFinancials",
 <omitted columns>
 created_at: Thu, 21 Jan 2016 15:41:43 CST -06:00,
 updated_at: Thu, 21 Jan 2016 15:41:43 CST -06:00>

 [6] pry(main)> @bank_financials.update_attributes(growth_in_equity: 0.700)
   (1.1ms)  BEGIN
  Institution Load (0.9ms)  SELECT  "institutions".* FROM "institutions" WHERE "institutions"."id" = $1 LIMIT 1  [["id", 32008]]
  Financials Exists (2.2ms)  SELECT  1 AS one FROM "financials" WHERE ("financials"."quarter" = 2 AND "financials"."id" != 80493 AND "financials"."year" = 2015 AND "financials"."actable_type" = 'BankFinancials' AND "financials"."institution_id" = 32008) LIMIT 1
  SQL (2.8ms)  UPDATE "bank_financials" SET "growth_in_equity" = $1 WHERE "bank_financials"."id" = $2  [["growth_in_equity", 0.7], ["id", 42547]]
   (2.1ms)  COMMIT
=> true

[7] pry(main)> @bank_financials.reload.acting_as
  BankFinancials Load (0.3ms)  SELECT  "bank_financials".* FROM "bank_financials" WHERE "bank_financials"."id" = $1 LIMIT 1  [["id", 42547]]
  Financials Load (0.3ms)  SELECT  "financials".* FROM "financials" WHERE "financials"."actable_id" = $1 AND "financials"."actable_type" = $2 LIMIT 1  [["actable_id", 42547], ["actable_type", "BankFinancials"]]
=> #<Financials:0x00000112704cd0
 id: 80493,
 actable_id: 42547,
 actable_type: "BankFinancials",
 <omitted columns>
 created_at: Thu, 21 Jan 2016 15:41:43 CST -06:00,
 updated_at: Thu, 21 Jan 2016 15:41:43 CST -06:00>
hzamani commented 8 years ago

@stephen-puiszis adding timestamp columns to both child and parent tables is a bad design pattern.

extrajordanary commented 8 years ago

@hzamani if that was the way that you designed it and intended it to be used, then perhaps you could mention this in the Readme. I don't see anywhere where it warns you to be sure to remove created_at and updated_at if they already exist and to not make them if you're creating a new model.

stephen-puiszis commented 8 years ago

@hzamani whether or not that's a bad design pattern isn't what I'm asking and is a different conversation. Given the current and intended design of timestamps only on the parent class, I would have expected that if an update occurs on a child record, the associated parent record's timestamp would be updated for the child's update (@child.acting_as.updated_at). However, that is not the case and that looks like a bug.

@extrajordanary agreed

stephen-puiszis commented 8 years ago

@hzamani thank you!

extrajordanary commented 8 years ago

@hzamani Yes, thanks again for clarifying!

kjakub commented 8 years ago

@hzamani i need keep legacy timestamps on subclasses, so i created parent class without timestamp to avoid collision. Do you see any problem with that ?

hzamani commented 8 years ago

@kjakub, that's not a problem, but you can achieve same functionality with timestamps on parent class. And with timestamps on parent you can have things like this: (with readme example)

Product.where(:updated_at > 10.days.ago)
Product.sort(:updated_at).limit(10)
kjakub commented 8 years ago

@hzamani cool, the problem i encountered is created_at timestamp on parent class - after first update is the same updated_at. And I was lazy to write migration. Thanks.