Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

AR::Base.becomes should not change the STI type #3023

Merged
merged 1 commit into from
@Tho85

The current implementation of AR::Base.becomes doesn't work as expected. Calling subclass_instance.becomes(SuperClass) changes subclass_instance.type to SuperClass, i.e.:

class SuperClass < ActiveRecord::Base
end
class SubClass < SuperClass
end

subclass_instance = SubClass.new
subclass_instance.type # => "SubClass"
subclass_instance.becomes(SuperClass)
subclass_instance.type # => "SuperClass"

I've added the method becomes! to AR::Base which resembles the current behavior and changed AR::Base.becomes back to the old behavior (before e781853).

Refs #98 and https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/5953

@brendon

Just discovered this also. Glad a patch has been created.

@brendon

Just wondering if there's an easy workaround for this problem in the meantime? Would it work to fetch the instance into two variables and then becomes one of them to be the superclass and leave the other as is?

@pwim

Got bitten by this as well. Either this patch should be applied, or this side effect should be explicit in the documentation.

@Tho85

Any chance this patch will be merged into master? We are regularly struggling with this issue.

@ChristianPeters

+1 This is a really unpleasant problem!

You just don't expect this evil side-effect of changing the class of the original object.

Even worse, the documentation reads like as if the original object was not affected: "Returns an instance of the specified klass with the attributes of the current record."

This problem costs hours to identify and it's really time to fix this now.

@reqorder

+1 This is EVIL!!

As ChristianPeters mentions, the Doc is misleading and wrong.
We were having the same issue and were happy, to find this patch!

@brendon

I can't believe that no one from the core team has picked this up. Do you have to be in a special club before your patches are noticed in a timely manner? How can we get their attention?

@moiristo

+1

@NiDi

+1

@kirel

+1

@unimatrixZxero

+1

This needs to be fixed, it has been sitting here for months now. Merge it already!

@hzeus

+1

@rafaelfranca

This pull request cannot be automatically merged. Could you rebase it from master? Also if this is not an valid issue, please close it. I'll be glad to take this pull request to the core team when it is rebased

@Tho85

Just rebased it from master, could you try to merge it again?

@rafaelfranca

For sure. I can't merge it but I can ask to someone merge.

cc @jonleighton @tenderlove

@jonleighton
Collaborator
  • Needs to be changelogged
  • Do we really need the becomes! version? I think we can just change the behaviour of becomes and be done with it.
@Tho85

Just added a changelog entry. I think there are valid use cases for both methods:

  • #becomes is useful if you want to change a type temporarily, i.e. without touching the original object
  • #becomes! comes in handy if you want to change a type permanently, i.e. you save it afterwards. After all, there is a reason why the behavior was introduced in #98.
@tenderlove
Owner

Can you rebase this please?

@Tho85

Done.

@rafaelfranca

@Tho85 it still needs a rebase.

@Tho85

@rafaelfranca Rebased it again, hopefully for the last time...

@rafaelfranca

@Tho85 hahhahaha me too. @tenderlove is it fine to merge?

@volontarian volontarian referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@volontarian volontarian referenced this pull request from a commit
mgawlista Merges open pull request #3023 5c9a5b0
@codepodu

+1 for the merge, got bit by this a couple of weeks ago.

activerecord/lib/active_record/persistence.rb
@@ -147,7 +147,18 @@ def becomes(klass)
became.instance_variable_set("@new_record", new_record?)
became.instance_variable_set("@destroyed", destroyed?)
became.instance_variable_set("@errors", errors)
- became.type = klass.name unless self.class.descends_from_active_record?
+ became
+ end
+
+ # Wrapper around +becomes+ that also changes the instance's sti column value.
+ # This is especially useful if you want to persist the changed class in your
+ # database.
+ #
+ # Note: The old instance's sti column value will be changed too, as both objects
+ # share the same set of attributes.
+ def becomes!(klass)
+ became = becomes(klass)
+ became.send("#{klass.inheritance_column}=", klass.sti_name) unless self.class.descends_from_active_record?
@morgoth
morgoth added a note

This will raise error when inheritance_column is set to nil (disabling STI case)

@rafaelfranca Owner

I think we should not send. Why not set the type as was did in becames?

@Tho85
Tho85 added a note

Using became.type = klass.name doesn't work if you set the inheritance column to something other than type.

Setting inheritance_column to nil is an edge case that has to be considered, I will write a test for it...

@rafaelfranca Owner

Got it. So this send is only to call the method dynamically not to call a private method.

@rafaelfranca Owner

Ok. Ping me when you are done.

@Tho85
Tho85 added a note

This actually works without modification even when setting inheritance_column to nil: If STI is disabled, class.descends_from_active_record? returns true, send is not called, and becomes! behaves exactly like becomes.

If you disable STI, there's no reason to call becomes! anyway, as all the method deals with is STI...

I'm against bloating the test suite by writing a test for a problem that actually doesn't exist, what do you think?

@rafaelfranca Owner

I think we can add this case only to document this behavior if anyone want to change this code. But this is only a suggestion.

@reqorder
reqorder added a note

@Tho85: I am absolutely with you. Why else would you call becomes! if not using STI?

@rafaelfranca Owner

Remember that a good test coverage is better than a fast suite. As a maintainer I don't want to see any regression later in this code, and more open issues ;)

@brendon
brendon added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@morgoth

Are those 2 methods (becomes, becomes!) really useful?
What about removing them in rails 4.0?

@moiristo

I think they are. In my app, I have a class Resource < Document. The Resource class has different validations, therefore I'd like to be able to change the STI class when the code detects that a document should become a resource. Maybe you have an alternative approach for cases like this?

@beerlington

@morgoth I was wondering the same thing. I had an issue with this the other day and posted to the core mailing list. The feeling I get is that some people use it w/out realizing the side effect and others use it for the side effects. If it was up to me, I would pull it out like they did with composed_of, and let people implement it the way they want. I doubt it's used anywhere internally, and I don't see how it's even related to ActiveRecord::Persistence.

@brendon
@beerlington

@brendon I have a similar STI use case as you, but the current state of becomes feels pretty neglected and probably belongs in ActiveRecord::Inheritance

I'd rather see a method where I don't have to specify the parent class. The only time I've ever used becomes has been in views, and I pass in the parent class every time. Maybe I'm missing something, but why would you want it to become something other than its parent?

Something like this would be more useful for me:

class Parent < ActiveRecord::Base; end
class Child < Parent; end

child = Child.new
<%= form_for child.to_sti_parent do %>
<% end %>
@brendon
@beerlington

@brendon Not that you should, but it would work with as much inheritance as you can throw at it. Since only one of the classes can descend directly from ActiveRecord::Base, you could use:

class Parent < ActiveRecord::Base; end
class Child < Parent; end
class SubChild < Child; end

SubChild.base_class # => Parent
@brendon
@morgoth

I never used that feature, so it's hard to me talk about pros, but I'm seeing some downsides:

# STI: Coach < Person
coach = Coach.last
coach.first_name = "John"
coach.changed?
# => true
coach.becomes(Person).changed?
# => false

This one doesn't make sense, however it's possible:

post = Post.last
comment = post.becomes(Comment)
comment.persisted?
# => true
comment.reload
# comment is now record from comments table (with the same id as post)
@dipth

What is the status of this issue?

This problem is making it impossible for us to change the type of a STI record.

[28] pry(main)> rps = RetailerPayoutStrategy.first
  RetailerPayoutStrategy Load (0.4ms)  SELECT `retailer_payout_strategies`.* FROM `retailer_payout_strategies` LIMIT 1
=> #<PrepayRetailerPayoutStrategy id: 1, deal_id: 102, type: "PrepayRetailerPayoutStrategy", payout_delay_in_days: nil, upfront_percent: 4, payout_from: nil, created_at: "2012-07-31 09:56:37", updated_at: "2012-07-31 11:18:01">
[30] pry(main)> rps.becomes(DefaultRetailerPayoutStrategy).save!
   (0.2ms)  BEGIN
   (0.3ms)  UPDATE `retailer_payout_strategies` SET `type` = 'DefaultRetailerPayoutStrategy', `updated_at` = '2012-07-31 11:37:08' WHERE `retailer_payout_strategies`.`type` IN ('DefaultRetailerPayoutStrategy') AND `retailer_payout_strategies`.`id` = 1
   (0.1ms)  COMMIT
=> true

Since becomes changes the type-value in memory to the new type, the database update fails silently because of the WHERE `retailer_payout_strategies`.`type` IN ('DefaultRetailerPayoutStrategy') part.

@morgoth

I'm voting for removing becomes from Rails ;-) - I can prepare patch for that.
After merging this pull request, there will be problem described here: https://github.com/rails/rails/pull/3023/files#r1223769

@rafaelfranca @jonleighton @tenderlove any thoughts on that?

@brendon
@beerlington

@brendon From what I can see, no one is saying they want to remove it because they don't use it. However, leaving flawed features in the framework doesn't benefit anyone in the long term. Removing it and providing something more consistent either within Rails or a separate gem is not a difficult task.

Regarding the documentation, someone a few months ago tried to provide an official guide to STI and maybe it makes sense to bring this back to life.

@rafaelfranca
Owner

Sorry for the delay guys. I never used this feature and needed to read and test the behavior. Also I'm doing a lot of things here.

Yes, I think this should not have that side-effect, and that we need to fix it. But this fix will be only at master. The reason to not apply this to 3-2-stable is that it will change the behavior and someone can use that behavior.

I never used this feature but I don't think that we should remove. Seems that a lot of people use it (see the +1 comments).

To get this merged we need to fix some things first. I'll comment in the diff

@brendon
@avit

I've mostly used this feature in situations when I needed to change the type of the model and persist it, not just masquerade, and more often it's from ancestor to subclass or sibling to sibling. Currently it doesn't make it easy or obvious, see avit@af1d680 for a test example. I've found that only update_attribute works reliably for changing the type on persisted records after using becomes, but it feels like a backdoor since it skips validation and it's easy to screw it up, e.g.

instance.becomes(SwirlyThing)  # must pass a const
instance.update_attribute(:type, SwirlyThing)  # wrong: must pass a string
# type column is now yaml-serialized, "--- !ruby/class 'SwirlyThing'\n", hooray.

So, :+1: for keeping something like becomes! for this.

The other common use case is posing as another (generic) class in views. This is mainly to satisfy all the conventions for url helpers, etc. but is lying about the model's class really the right solution for that?

@rafaelfranca

After we merged #7506 this pull request needs a rebase. (I know, again, sorry)

@kayakyakr

This should probably reference the method "ensure_proper_type" from ActiveRecord::Inheritance instead of changing the value itself.

@steveklabnik
Collaborator

@Tho85 ping! Are you still interested in following-through with this pull request? it needs to be rebased.

@Tho85

Rebased it.

@kayakyakr Using ensure_proper_type doesn't work in this case: We have to check whether the current class descends from ActiveRecord::Base instead of the target class.

@carlosantoniodasilva carlosantoniodasilva commented on the diff
activerecord/test/cases/persistence_test.rb
((6 lines not shown))
assert_instance_of Topic, topic
topic.save!
assert_instance_of Topic, Topic.find(topic.id)
end
+ def test_preserve_original_sti_type
+ reply = topics(:second)
+ assert_equal "Reply", reply.type
+
+ topic = reply.becomes(Topic)

This var is not used, perhaps you can test it's type as well, to confirm it's a Topic?

@Tho85
Tho85 added a note

You're right. Testing it for class and STI type now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on the diff
activerecord/CHANGELOG.md
@@ -784,6 +784,13 @@
*Jon Leighton*
+* Don't change STI type when calling ActiveRecord::Base#becomes, add
+ ActiveRecord::Base#becomes!
+
+ See #3023.
+
+ *Thomas Hollstegge*

Please move this changelog to the top.

@Tho85
Tho85 added a note

Done!

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

Can you squash your commits please? I'll ping someone else for a final review before merging, thanks!

@Tho85 Tho85 AR::Base.becomes should not change the STI type
If you want to change the STI type too, use AR::Base.becomes! instead
70fa756
@Tho85

Squashed.

@rafaelfranca rafaelfranca merged commit 70fa756 into from
@rafaelfranca

Thank you

@mbergo mbergo referenced this pull request from a commit in globocom/GloboDNS
@gama gama Add a workaround to Rails pull request 3023
The patch has been pushed to rails 3.2.7. If/when we upgrade, we may
remove the initializer.

For more info, see rails/rails#3023
97aad21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 17, 2012
  1. @Tho85

    AR::Base.becomes should not change the STI type

    Tho85 authored
    If you want to change the STI type too, use AR::Base.becomes! instead
This page is out of date. Refresh to see the latest.
View
7 activerecord/CHANGELOG.md
@@ -1,5 +1,12 @@
## Rails 4.0.0 (unreleased) ##
+* Don't change STI type when calling ActiveRecord::Base#becomes, add
+ ActiveRecord::Base#becomes!
+
+ See #3023.
+
+ *Thomas Hollstegge*

Please move this changelog to the top.

@Tho85
Tho85 added a note

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
* `#pluck` can be used on a relation with `select` clause. Fix #7551
Example:
View
13 activerecord/lib/active_record/persistence.rb
@@ -155,7 +155,18 @@ def becomes(klass)
became.instance_variable_set("@new_record", new_record?)
became.instance_variable_set("@destroyed", destroyed?)
became.instance_variable_set("@errors", errors)
- became.public_send("#{klass.inheritance_column}=", klass.name) unless self.class.descends_from_active_record?
+ became
+ end
+
+ # Wrapper around +becomes+ that also changes the instance's sti column value.
+ # This is especially useful if you want to persist the changed class in your
+ # database.
+ #
+ # Note: The old instance's sti column value will be changed too, as both objects
+ # share the same set of attributes.
+ def becomes!(klass)
+ became = becomes(klass)
+ became.public_send("#{klass.inheritance_column}=", klass.sti_name) unless self.class.descends_from_active_record?
became
end
View
13 activerecord/test/cases/persistence_test.rb
@@ -280,12 +280,23 @@ def test_update_for_record_with_only_primary_key
def test_update_sti_type
assert_instance_of Reply, topics(:second)
- topic = topics(:second).becomes(Topic)
+ topic = topics(:second).becomes!(Topic)
assert_instance_of Topic, topic
topic.save!
assert_instance_of Topic, Topic.find(topic.id)
end
+ def test_preserve_original_sti_type
+ reply = topics(:second)
+ assert_equal "Reply", reply.type
+
+ topic = reply.becomes(Topic)

This var is not used, perhaps you can test it's type as well, to confirm it's a Topic?

@Tho85
Tho85 added a note

You're right. Testing it for class and STI type now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ assert_equal "Reply", reply.type
+
+ assert_instance_of Topic, topic
+ assert_equal "Reply", topic.type
+ end
+
def test_delete
topic = Topic.find(1)
assert_equal topic, topic.delete, 'topic.delete did not return self'
Something went wrong with that request. Please try again.