Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

is_a? Confuses association consistency #49

Open
lowjoel opened this issue Nov 4, 2015 · 8 comments
Open

is_a? Confuses association consistency #49

lowjoel opened this issue Nov 4, 2015 · 8 comments

Comments

@lowjoel
Copy link
Contributor

lowjoel commented Nov 4, 2015

AR checks that when assigning a variable to a relation (has_many/has_one) that the type of the variable assigned is consistent with the association. ActsAs overrides is_a?, which confuses this check. Anything that acts as the base type would be added, which could potentially give the wrong ID (since the ancestor ID and subclass ID are different)

I'm not sure what's a good fix for this, just sounding it out.

@lowjoel
Copy link
Contributor Author

lowjoel commented Nov 7, 2015

Maybe override ActiveRecord::Associations::Association::raise_on_type_mismatch! to be aware of actable classes?

@hzamani
Copy link
Owner

hzamani commented Nov 8, 2015

Maybe it's better to not override is_a?

@lowjoel
Copy link
Contributor Author

lowjoel commented Nov 9, 2015

But this gem has been overriding is_a? for the longest time, right? That would break backwards compatibility.

Another alternative I can think of is to make sure every subclass has the exact same ID as the parent class, but I think that might require more hacking to AR than it's worth.

@lowjoel
Copy link
Contributor Author

lowjoel commented Nov 20, 2015

Also, in README:

In has_many case you can use subclasses:

store = Store.create
store.products << Pen.create
store.products.first
  # => #<Product: ...>

That does not actually work, because the pen#id is not the same as product#id, which I guess is what this issue is about...

@ball-hayden
Copy link

I've recently run into what I believe to be the consequences of this...
A simplified example (as much as possible, at least):

# app/models/subscription.rb
class Subscription
  belongs_to :subscription_plan
end
# app/models/subscription_plan.rb
class SubscriptionPlan
  actable
end
# app/models/subscription_plans/subscription_plan_a.rb
class SubscriptionPlans::SubscriptionPlanA
  acts_as :subscription_plan
end
# app/models/subscription_plans/subscription_plan_b.rb
class SubscriptionPlans::SubscriptionPlanB
  acts_as :subscription_plan
end
subscription = Subscription.new
plan_a = SubscriptionPlanA.new
subscription.subscription_plan = plan_a
subscription.save

plan_a.acting_as.id # => 1
subscription.subscription_plan_id # => 1

plan_b = SubscriptionPlanB.new
subscription.subscription_plan = plan_b
subscription.save

plan_b.acting_as.id # => 2
subscription.subscription_plan_id # => 1 (!!)

I'm currently looking at monkey patching AR to have it automatically convert any subclass to it's acting_as_model before setting the foreign_key on the child model. This works to a point (autosaving the subclass doesn't seem to work at the moment), but it may provide a bit of inspiration...

# config/initializers/actable_belongs_to.rb
# Fixes issues when an acts_as subclass is assigned to a belongs_to relation
module ActableBelongsTo
  def replace(record)
    if record.respond_to?(:acting_as)
      super(record.acting_as)
    else
      super
    end
  end
end

ActiveRecord::Associations::BelongsToAssociation.prepend ActableBelongsTo

@lowjoel
Copy link
Contributor Author

lowjoel commented Dec 21, 2015

Yup, looks like what bit me.

I think that might work for a belongs_to relation, but doesn't solve the general case where we have a has_many relation that links to an actable table.

@hzamani hzamani mentioned this issue Dec 22, 2015
@hzamani
Copy link
Owner

hzamani commented Jan 26, 2016

What about changing primary key on acting_as models? After that id would resolve to actable model.

@lowjoel
Copy link
Contributor Author

lowjoel commented Jan 26, 2016

That could work, but I think that would make handling the acting_as models harder to manipulate, since almost all code assumes #id.

A more drastic change is to flip the association to be a belongs_to on the actable. This could be a step towards multi-level multi-table inheritance, i.e. C inherits from B which inherits from A, A only stores the type of C, the ID for A B and C are all the same. I don't think this is currently possible with the current ActsAs gem, makes all of them interchangeable (expected from inheritance, and we get to keep overriding is_a?)

That could even allow us at some point to even use normal Ruby inheritance, just throwing out a wild idea.

(if anything, trying to debug both acts_as_relation and AR::acts_as has really improved my metaprogramming-fu 😛)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants