Allow the building of STI associations #5813

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@diminish7
Contributor

diminish7 commented Apr 11, 2012

When building an active record object from an association with parent.children.build, parent.build_child, or the equivalent with nested form attributes, this change allows you to pass the inheritance column attribute to specify the subclass when the associated class uses STI. e.g.

    parent.children.build(:type => "OldestChild")

This has been the subject of a few other pull requests/issues, and has been closed out for security reasons. I believe this patch addresses those security issues by

  1. Ensuring that the class name is a valid class and that it is in the ancestors of the super class that the association is expecting. You cannot build any random object, it must be in the base association class's hierarchy.
  2. Respecting protected attributes. By default the inheritance column is protected. In order to build an association using a subclass, the inheritance column must be explicitly made accessible, either by overriding attributes_protected_by_default, or with attr_accessible.

Here are the previously closed issues for reference:

@shannonrush

This comment has been minimized.

Show comment Hide comment
@shannonrush

shannonrush Apr 11, 2012

+1

+1

@searls

This comment has been minimized.

Show comment Hide comment
@searls

searls Apr 11, 2012

+1

searls commented Apr 11, 2012

+1

@rdammkoehler

This comment has been minimized.

Show comment Hide comment
@rdammkoehler

rdammkoehler Apr 11, 2012

+1

+1

@charley

This comment has been minimized.

Show comment Hide comment
@charley

charley Apr 11, 2012

+1

charley commented Apr 11, 2012

+1

@chrisjpowers

This comment has been minimized.

Show comment Hide comment
@chrisjpowers

chrisjpowers Apr 11, 2012

+1

+1

@TimWalker

This comment has been minimized.

Show comment Hide comment
@TimWalker

TimWalker Apr 11, 2012

+1

+1

@mvanholstyn

This comment has been minimized.

Show comment Hide comment
@mvanholstyn

mvanholstyn Apr 11, 2012

+1

+1

@dreamfall

This comment has been minimized.

Show comment Hide comment
@dreamfall

dreamfall Apr 11, 2012

Contributor

+1

Contributor

dreamfall commented Apr 11, 2012

+1

@pkananen

This comment has been minimized.

Show comment Hide comment
@pkananen

pkananen Apr 12, 2012

+1

+1

@seeflanigan

This comment has been minimized.

Show comment Hide comment
@seeflanigan

seeflanigan Apr 14, 2012

+1

+1

@flackou

This comment has been minimized.

Show comment Hide comment
@flackou

flackou May 15, 2012

+1

flackou commented May 15, 2012

+1

@pixeltrix

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix May 15, 2012

Member

Why just associations - why not BaseClass.new(:type => 'SubClass') ?

Member

pixeltrix commented May 15, 2012

Why just associations - why not BaseClass.new(:type => 'SubClass') ?

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 May 15, 2012

Contributor

@pixeltrix I feel like #build_association is already having to perform some logic to determine what class to build, and this is just an extension of that behavior. However, it's well established and expected that MyClass.new should return an instance of MyClass, so I'd be uncomfortable with the idea of overriding .new to return an instance of a subclass. I'm not sure you get a whole lot by being able to write BaseClass.new(:type => 'SubClass') rather than just writing SubClass.new, but adding this behavior to #build_association gives you the ability to control the subclass without losing all the stuff you get from #build_association (i.e. adding the new instance to the association)

Contributor

diminish7 commented May 15, 2012

@pixeltrix I feel like #build_association is already having to perform some logic to determine what class to build, and this is just an extension of that behavior. However, it's well established and expected that MyClass.new should return an instance of MyClass, so I'd be uncomfortable with the idea of overriding .new to return an instance of a subclass. I'm not sure you get a whole lot by being able to write BaseClass.new(:type => 'SubClass') rather than just writing SubClass.new, but adding this behavior to #build_association gives you the ability to control the subclass without losing all the stuff you get from #build_association (i.e. adding the new instance to the association)

@pixeltrix

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix May 15, 2012

Member

@diminish7 but you could have an admin form that allowed you to create a number of different subclasses from a popup menu, so you couldn't just do SubClass.new. If it followed attr_accessible rules then it's not going to happen unless you want it to happen.

Member

pixeltrix commented May 15, 2012

@diminish7 but you could have an admin form that allowed you to create a number of different subclasses from a popup menu, so you couldn't just do SubClass.new. If it followed attr_accessible rules then it's not going to happen unless you want it to happen.

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 May 15, 2012

Contributor

@pixeltrix Sure, I can see that. Let me take a stab at adding that in

Contributor

diminish7 commented May 15, 2012

@pixeltrix Sure, I can see that. Let me take a stab at adding that in

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 May 17, 2012

Contributor

@pixeltrix Just added support for BaseClass.new(:type => 'SubClass). Love to get any feedback on it. Thanks!

Contributor

diminish7 commented May 17, 2012

@pixeltrix Just added support for BaseClass.new(:type => 'SubClass). Love to get any feedback on it. Thanks!

@ticktricktrack

This comment has been minimized.

Show comment Hide comment
@ticktricktrack

ticktricktrack Jul 27, 2012

I feel so dirty adding attr_accessible :type, but without this issue pulled and closed, what else is there to do. I came far too close to ditching STI for a non modular, procedural solution. So +1 from me!

My solution for now: parent.children += [OldestChild.new(cool: stuff)]

I feel so dirty adding attr_accessible :type, but without this issue pulled and closed, what else is there to do. I came far too close to ditching STI for a non modular, procedural solution. So +1 from me!

My solution for now: parent.children += [OldestChild.new(cool: stuff)]

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Oct 29, 2012

Member

3 month ping, where does this stand?

Member

schneems commented Oct 29, 2012

3 month ping, where does this stand?

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Oct 29, 2012

Contributor

This needs a rebase.

Contributor

frodsan commented Oct 29, 2012

This needs a rebase.

@ghost ghost assigned pixeltrix Nov 4, 2012

@schorsch

This comment has been minimized.

Show comment Hide comment
@schorsch

schorsch Nov 8, 2012

+1

Especially needed when creating an object using nested_attributes of different sti types e.g.

class Item 
class LineItem < Item
class Devider < Item
class Subtotal < Item

class Document 
  has_many :items
  accepts_nested_attributes_for :items

Document.new items_attributes:[ {price: 1.0, type: 'LineItem'}, {type:'Devider', name: 'Some Info'}  ]

Without this patch AR instanciates the nested objects above, with class Item so some special methods only available in LineItem (e.g calculate_totals) can only be called after saving and reloading the document object.

schorsch commented Nov 8, 2012

+1

Especially needed when creating an object using nested_attributes of different sti types e.g.

class Item 
class LineItem < Item
class Devider < Item
class Subtotal < Item

class Document 
  has_many :items
  accepts_nested_attributes_for :items

Document.new items_attributes:[ {price: 1.0, type: 'LineItem'}, {type:'Devider', name: 'Some Info'}  ]

Without this patch AR instanciates the nested objects above, with class Item so some special methods only available in LineItem (e.g calculate_totals) can only be called after saving and reloading the document object.

@timraymond

This comment has been minimized.

Show comment Hide comment
@timraymond

timraymond Nov 27, 2012

Contributor

@diminish7 PING Where does this stand?

Contributor

timraymond commented Nov 27, 2012

@diminish7 PING Where does this stand?

@gucki

This comment has been minimized.

Show comment Hide comment
@gucki

gucki Nov 27, 2012

+1

gucki commented Nov 27, 2012

+1

@pascalbetz

This comment has been minimized.

Show comment Hide comment
@pascalbetz

pascalbetz Nov 27, 2012

+1 as well. I have something similar that i copy from project to project. Would be handy if included in Rails.

+1 as well. I have something similar that i copy from project to project. Would be handy if included in Rails.

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 Nov 28, 2012

Contributor

Rebased and fixed conflicts. @pixeltrix - Just wanted to check in. What do you think? Any chance of getting this merged in?

Contributor

diminish7 commented Nov 28, 2012

Rebased and fixed conflicts. @pixeltrix - Just wanted to check in. What do you think? Any chance of getting this merged in?

@pixeltrix

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Nov 28, 2012

Member

@diminish7 can you squash the commits into a single commit and add a CHANGELOG entry - thanks. Also how well does it play together with the protected_attributes gem and the adoption of strong_parameters into core?

@jonleighton @tenderlove any objections to merging this?

Member

pixeltrix commented Nov 28, 2012

@diminish7 can you squash the commits into a single commit and add a CHANGELOG entry - thanks. Also how well does it play together with the protected_attributes gem and the adoption of strong_parameters into core?

@jonleighton @tenderlove any objections to merging this?

@pixeltrix

View changes

activerecord/lib/active_record/reflection.rb
@@ -386,6 +386,7 @@ def derive_join_table
def primary_key(klass)
klass.primary_key || raise(UnknownPrimaryKey.new(klass))
end
+

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Nov 28, 2012

Member

Remove this extra line

@pixeltrix

pixeltrix Nov 28, 2012

Member

Remove this extra line

@pixeltrix

View changes

activerecord/lib/active_record/base.rb
+ class << self
+ alias_method :new_without_sti, :new
+ alias_method :new, :new_with_sti
+ end

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Nov 28, 2012

Member

Do we need to alias method chain it or can we just use super?

@pixeltrix

pixeltrix Nov 28, 2012

Member

Do we need to alias method chain it or can we just use super?

@pixeltrix

View changes

activerecord/lib/active_record/inheritance.rb
+ if (attrs = args.first).is_a?(Hash)
+ if (subclass_name = attrs.with_indifferent_access[inheritance_column]).present?
+ begin
+ subclass = compute_type(subclass_name)

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Nov 28, 2012

Member

Not sure about this - it'll trigger autoloading of constants which may be a security issue as the constant name comes from user input. I think I'd prefer if we got a list of subclasses and then checked against their names. I know that it won't trigger autoloading of subclasses but I tend to use require_dependency to load my subclasses at the end of the base class file so that they're all immediately available anyway.

@pixeltrix

pixeltrix Nov 28, 2012

Member

Not sure about this - it'll trigger autoloading of constants which may be a security issue as the constant name comes from user input. I think I'd prefer if we got a list of subclasses and then checked against their names. I know that it won't trigger autoloading of subclasses but I tend to use require_dependency to load my subclasses at the end of the base class file so that they're all immediately available anyway.

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 Nov 28, 2012

Contributor

Squashed to a single commit. I'll make your other changes at lunch today. Also, this isn't playing nice with strong_parameters right now, I'll fix that this afternoon as well. Thanks for the feedback!

Contributor

diminish7 commented Nov 28, 2012

Squashed to a single commit. I'll make your other changes at lunch today. Also, this isn't playing nice with strong_parameters right now, I'll fix that this afternoon as well. Thanks for the feedback!

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 Nov 28, 2012

Contributor

Oh, I was wrong, it does play nice with strong_parameters. Added a test to prove that out.

Contributor

diminish7 commented Nov 28, 2012

Oh, I was wrong, it does play nice with strong_parameters. Added a test to prove that out.

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 Nov 28, 2012

Contributor

@pixeltrix I think I've covered all your changes except for playing nice with the protected_attributes gem. I think it should be handled from the gem's perspective, so if this gets merged into master, I'll submit a patch to protected_attributes.

Contributor

diminish7 commented Nov 28, 2012

@pixeltrix I think I've covered all your changes except for playing nice with the protected_attributes gem. I think it should be handled from the gem's perspective, so if this gets merged into master, I'll submit a patch to protected_attributes.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Nov 28, 2012

Member

It doesn't merge cleanly anymore

Member

steveklabnik commented Nov 28, 2012

It doesn't merge cleanly anymore

Added STI support to init and building associations
Allows you to do BaseClass.new(:type => "SubClass") as well as
parent.children.build(:type => "SubClass") or parent.build_child
to initialize an STI subclass. Ensures that the class name is a
valid class and that it is in the ancestors of the super class
that the association is expecting.
@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 Nov 28, 2012

Contributor

@steveklabnik There you go, the conflict should be fixed now. Dualing changelog entries.

Contributor

diminish7 commented Nov 28, 2012

@steveklabnik There you go, the conflict should be fixed now. Dualing changelog entries.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Nov 28, 2012

Member

🤘

Member

steveklabnik commented Nov 28, 2012

🤘

@pixeltrix

This comment has been minimized.

Show comment Hide comment
@pixeltrix

pixeltrix Nov 29, 2012

Member

Rebased and merged in 89b5b31

Member

pixeltrix commented Nov 29, 2012

Rebased and merged in 89b5b31

@pixeltrix pixeltrix closed this Nov 29, 2012

@gucki

This comment has been minimized.

Show comment Hide comment
@gucki

gucki Nov 29, 2012

great news, thanks a lot :)

gucki commented Nov 29, 2012

great news, thanks a lot :)

@diminish7

This comment has been minimized.

Show comment Hide comment
@diminish7

diminish7 Nov 29, 2012

Contributor

Awesome, thanks!

Contributor

diminish7 commented Nov 29, 2012

Awesome, thanks!

@dgilperez

This comment has been minimized.

Show comment Hide comment
@dgilperez

dgilperez Dec 5, 2012

🍻 thanks!

🍻 thanks!

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