Permalink
Browse files

Pass mass-assignment options to nested models - closes #1673.

  • Loading branch information...
1 parent 987afa5 commit 5d655aabcb47a68e7ce1dfb60222e951a6d30169 @pixeltrix pixeltrix committed Jun 13, 2011
@@ -1717,10 +1717,13 @@ def assign_attributes(new_attributes, options = {})
attributes.each do |k, v|
if k.include?("(")
multi_parameter_attributes << [ k, v ]
- elsif respond_to?("#{k}=")
@josevalim

josevalim Jul 3, 2011

Member

Maybe we should not change the default way ActiveRecord::Base does assignments. This can cause side effects in other methods developers implement and make things slower. Wouldn't it be better to store the current mass assignment role in an instance variable and simply make the #{association}_attributes= method use it without changing their arity?

@pixeltrix

pixeltrix Jul 3, 2011

Owner

I was probably trying too hard to be flexible here - a bit of YAGNI. I was considering that developers might want access to the mass assignment options which they'd get if they changed their method signature, but on reflection there may be methods out there like this:

def status(new_status, changed_at = Time.current)
  self[:status] = new_status
  self[:status_changed_at] = changed_at
end

It's a contrived example but it illustrates the point that there's a possibility of writer methods that have an arity of 2 or more. Do we want to add protected reader methods so that developers can access the current role and options from there own methods, e.g:

  def mass_assignment_role
    @mass_assignment_role ||= :default
  end

  def mass_assignment_options
    @mass_assignment_options ||= {}
  end

  def assign_attributes(new_attributes, options = {})
   return unless new_attributes

   attributes = new_attributes.stringify_keys
   @mass_assignment_options = options
   @mass_assignment_role = options[:as] || :default
    # method implementation
   @mass_assignment_options = nil
   @mass_assignment_role = nil
  end

then the accepts_nested_attributes_for method would generate something like this:

  def pirate_attributes=(attributes)
    assign_nested_attributes_for_one_to_one_association(:pirate, attributes, mass_assignment_options)
  end
@josevalim

josevalim Jul 3, 2011

Member

+1 for protected reader methods. I think we can store just the mass_assignment_option value and nothing more. I would consider this a private API for this while.

- send("#{k}=", v)
else
- raise(UnknownAttributeError, "unknown attribute: #{k}")
+ method_name = "#{k}="
+ if respond_to?(method_name)
+ method(method_name).arity == -2 ? send(method_name, v, options) : send(method_name, v)
+ else
+ raise(UnknownAttributeError, "unknown attribute: #{k}")
+ end
end
end
@@ -276,15 +276,15 @@ def accepts_nested_attributes_for(*attr_names)
type = (reflection.collection? ? :collection : :one_to_one)
- # def pirate_attributes=(attributes)
- # assign_nested_attributes_for_one_to_one_association(:pirate, attributes)
+ # def pirate_attributes=(attributes, assignment_opts = {})
+ # assign_nested_attributes_for_one_to_one_association(:pirate, attributes, assignment_opts)
# end
class_eval <<-eoruby, __FILE__, __LINE__ + 1
if method_defined?(:#{association_name}_attributes=)
remove_method(:#{association_name}_attributes=)
end
- def #{association_name}_attributes=(attributes)
- assign_nested_attributes_for_#{type}_association(:#{association_name}, attributes)
+ def #{association_name}_attributes=(attributes, assignment_opts = {})
+ assign_nested_attributes_for_#{type}_association(:#{association_name}, attributes, assignment_opts)
end
eoruby
else
@@ -319,21 +319,21 @@ def _destroy
# If the given attributes include a matching <tt>:id</tt> attribute, or
# update_only is true, and a <tt>:_destroy</tt> key set to a truthy value,
# then the existing record will be marked for destruction.
- def assign_nested_attributes_for_one_to_one_association(association_name, attributes)
+ def assign_nested_attributes_for_one_to_one_association(association_name, attributes, assignment_opts = {})
options = self.nested_attributes_options[association_name]
attributes = attributes.with_indifferent_access
if (options[:update_only] || !attributes['id'].blank?) && (record = send(association_name)) &&
(options[:update_only] || record.id.to_s == attributes['id'].to_s)
- assign_to_or_mark_for_destruction(record, attributes, options[:allow_destroy]) unless call_reject_if(association_name, attributes)
+ assign_to_or_mark_for_destruction(record, attributes, options[:allow_destroy], assignment_opts) unless call_reject_if(association_name, attributes)
- elsif attributes['id'].present?
+ elsif attributes['id'].present? && !assignment_opts[:without_protection]
raise_nested_attributes_record_not_found(association_name, attributes['id'])
elsif !reject_new_record?(association_name, attributes)
method = "build_#{association_name}"
if respond_to?(method)
- send(method, attributes.except(*UNASSIGNABLE_KEYS))
+ send(method, attributes.except(*unassignable_keys(assignment_opts)), assignment_opts)
else
raise ArgumentError, "Cannot build association #{association_name}. Are you trying to build a polymorphic one-to-one association?"
end
@@ -367,7 +367,7 @@ def assign_nested_attributes_for_one_to_one_association(association_name, attrib
# { :name => 'John' },
# { :id => '2', :_destroy => true }
# ])
- def assign_nested_attributes_for_collection_association(association_name, attributes_collection)
+ def assign_nested_attributes_for_collection_association(association_name, attributes_collection, assignment_opts = {})
options = self.nested_attributes_options[association_name]
unless attributes_collection.is_a?(Hash) || attributes_collection.is_a?(Array)
@@ -401,7 +401,7 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
if attributes['id'].blank?
unless reject_new_record?(association_name, attributes)
- association.build(attributes.except(*UNASSIGNABLE_KEYS))
+ association.build(attributes.except(*unassignable_keys(assignment_opts)), assignment_opts)
end
elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
unless association.loaded? || call_reject_if(association_name, attributes)
@@ -418,8 +418,10 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
end
if !call_reject_if(association_name, attributes)
- assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
+ assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy], assignment_opts)
end
+ elsif assignment_opts[:without_protection]
+ association.build(attributes.except(*unassignable_keys(assignment_opts)), assignment_opts)
else
raise_nested_attributes_record_not_found(association_name, attributes['id'])
end
@@ -428,8 +430,8 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
# Updates a record with the +attributes+ or marks it for destruction if
# +allow_destroy+ is +true+ and has_destroy_flag? returns +true+.
- def assign_to_or_mark_for_destruction(record, attributes, allow_destroy)
- record.attributes = attributes.except(*UNASSIGNABLE_KEYS)
+ def assign_to_or_mark_for_destruction(record, attributes, allow_destroy, assignment_opts)
+ record.assign_attributes(attributes.except(*unassignable_keys(assignment_opts)), assignment_opts)
record.mark_for_destruction if has_destroy_flag?(attributes) && allow_destroy
end
@@ -458,5 +460,9 @@ def call_reject_if(association_name, attributes)
def raise_nested_attributes_record_not_found(association_name, record_id)
raise RecordNotFound, "Couldn't find #{self.class.reflect_on_association(association_name).klass.name} with ID=#{record_id} for #{self.class.name} with ID=#{id}"
end
+
+ def unassignable_keys(assignment_opts)
+ assignment_opts[:without_protection] ? UNASSIGNABLE_KEYS - %w[id] : UNASSIGNABLE_KEYS
+ end
end
end
@@ -552,3 +552,248 @@ def test_has_many_create_with_bang_without_protection
end
end
+
+
+class MassAssignmentSecurityNestedAttributesTest < ActiveRecord::TestCase
+ include MassAssignmentTestHelpers
+
+ def nested_attributes_hash(association, collection = false, except = [:id])
+ if collection
+ { :first_name => 'David' }.merge(:"#{association}_attributes" => [attributes_hash.except(*except)])
+ else
+ { :first_name => 'David' }.merge(:"#{association}_attributes" => attributes_hash.except(*except))
+ end
+ end
+
+ # build
+
+ def test_has_one_new_with_attr_protected_attributes
+ person = LoosePerson.new(nested_attributes_hash(:best_friend))
+ assert_default_attributes(person.best_friend)
+ end
+
+ def test_has_one_new_with_attr_accessible_attributes
+ person = TightPerson.new(nested_attributes_hash(:best_friend))
+ assert_default_attributes(person.best_friend)
+ end
+
+ def test_has_one_new_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.new(nested_attributes_hash(:best_friend), :as => :admin)
+ assert_admin_attributes(person.best_friend)
+ end
+
+ def test_has_one_new_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.new(nested_attributes_hash(:best_friend), :as => :admin)
+ assert_admin_attributes(person.best_friend)
+ end
+
+ def test_has_one_new_without_protection
+ person = LoosePerson.new(nested_attributes_hash(:best_friend, false, nil), :without_protection => true)
+ assert_all_attributes(person.best_friend)
+ end
+
+ def test_belongs_to_new_with_attr_protected_attributes
+ person = LoosePerson.new(nested_attributes_hash(:best_friend_of))
+ assert_default_attributes(person.best_friend_of)
+ end
+
+ def test_belongs_to_new_with_attr_accessible_attributes
+ person = TightPerson.new(nested_attributes_hash(:best_friend_of))
+ assert_default_attributes(person.best_friend_of)
+ end
+
+ def test_belongs_to_new_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.new(nested_attributes_hash(:best_friend_of), :as => :admin)
+ assert_admin_attributes(person.best_friend_of)
+ end
+
+ def test_belongs_to_new_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.new(nested_attributes_hash(:best_friend_of), :as => :admin)
+ assert_admin_attributes(person.best_friend_of)
+ end
+
+ def test_belongs_to_new_without_protection
+ person = LoosePerson.new(nested_attributes_hash(:best_friend_of, false, nil), :without_protection => true)
+ assert_all_attributes(person.best_friend_of)
+ end
+
+ def test_has_many_new_with_attr_protected_attributes
+ person = LoosePerson.new(nested_attributes_hash(:best_friends, true))
+ assert_default_attributes(person.best_friends.first)
+ end
+
+ def test_has_many_new_with_attr_accessible_attributes
+ person = TightPerson.new(nested_attributes_hash(:best_friends, true))
+ assert_default_attributes(person.best_friends.first)
+ end
+
+ def test_has_many_new_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.new(nested_attributes_hash(:best_friends, true), :as => :admin)
+ assert_admin_attributes(person.best_friends.first)
+ end
+
+ def test_has_many_new_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.new(nested_attributes_hash(:best_friends, true), :as => :admin)
+ assert_admin_attributes(person.best_friends.first)
+ end
+
+ def test_has_many_new_without_protection
+ person = LoosePerson.new(nested_attributes_hash(:best_friends, true, nil), :without_protection => true)
+ assert_all_attributes(person.best_friends.first)
+ end
+
+ # create
+
+ def test_has_one_create_with_attr_protected_attributes
+ person = LoosePerson.create(nested_attributes_hash(:best_friend))
+ assert_default_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_with_attr_accessible_attributes
+ person = TightPerson.create(nested_attributes_hash(:best_friend))
+ assert_default_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.create(nested_attributes_hash(:best_friend), :as => :admin)
+ assert_admin_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.create(nested_attributes_hash(:best_friend), :as => :admin)
+ assert_admin_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_without_protection
+ person = LoosePerson.create(nested_attributes_hash(:best_friend, false, nil), :without_protection => true)
+ assert_all_attributes(person.best_friend)
+ end
+
+ def test_belongs_to_create_with_attr_protected_attributes
+ person = LoosePerson.create(nested_attributes_hash(:best_friend_of))
+ assert_default_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_with_attr_accessible_attributes
+ person = TightPerson.create(nested_attributes_hash(:best_friend_of))
+ assert_default_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.create(nested_attributes_hash(:best_friend_of), :as => :admin)
+ assert_admin_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.create(nested_attributes_hash(:best_friend_of), :as => :admin)
+ assert_admin_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_without_protection
+ person = LoosePerson.create(nested_attributes_hash(:best_friend_of, false, nil), :without_protection => true)
+ assert_all_attributes(person.best_friend_of)
+ end
+
+ def test_has_many_create_with_attr_protected_attributes
+ person = LoosePerson.create(nested_attributes_hash(:best_friends, true))
+ assert_default_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_with_attr_accessible_attributes
+ person = TightPerson.create(nested_attributes_hash(:best_friends, true))
+ assert_default_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.create(nested_attributes_hash(:best_friends, true), :as => :admin)
+ assert_admin_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.create(nested_attributes_hash(:best_friends, true), :as => :admin)
+ assert_admin_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_without_protection
+ person = LoosePerson.create(nested_attributes_hash(:best_friends, true, nil), :without_protection => true)
+ assert_all_attributes(person.best_friends.first)
+ end
+
+ # create!
+
+ def test_has_one_create_with_bang_with_attr_protected_attributes
+ person = LoosePerson.create!(nested_attributes_hash(:best_friend))
+ assert_default_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_with_bang_with_attr_accessible_attributes
+ person = TightPerson.create!(nested_attributes_hash(:best_friend))
+ assert_default_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_with_bang_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.create!(nested_attributes_hash(:best_friend), :as => :admin)
+ assert_admin_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_with_bang_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.create!(nested_attributes_hash(:best_friend), :as => :admin)
+ assert_admin_attributes(person.best_friend, true)
+ end
+
+ def test_has_one_create_with_bang_without_protection
+ person = LoosePerson.create!(nested_attributes_hash(:best_friend, false, nil), :without_protection => true)
+ assert_all_attributes(person.best_friend)
+ end
+
+ def test_belongs_to_create_with_bang_with_attr_protected_attributes
+ person = LoosePerson.create!(nested_attributes_hash(:best_friend_of))
+ assert_default_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_with_bang_with_attr_accessible_attributes
+ person = TightPerson.create!(nested_attributes_hash(:best_friend_of))
+ assert_default_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_with_bang_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.create!(nested_attributes_hash(:best_friend_of), :as => :admin)
+ assert_admin_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_with_bang_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.create!(nested_attributes_hash(:best_friend_of), :as => :admin)
+ assert_admin_attributes(person.best_friend_of, true)
+ end
+
+ def test_belongs_to_create_with_bang_without_protection
+ person = LoosePerson.create!(nested_attributes_hash(:best_friend_of, false, nil), :without_protection => true)
+ assert_all_attributes(person.best_friend_of)
+ end
+
+ def test_has_many_create_with_bang_with_attr_protected_attributes
+ person = LoosePerson.create!(nested_attributes_hash(:best_friends, true))
+ assert_default_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_with_bang_with_attr_accessible_attributes
+ person = TightPerson.create!(nested_attributes_hash(:best_friends, true))
+ assert_default_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_with_bang_with_admin_role_with_attr_protected_attributes
+ person = LoosePerson.create!(nested_attributes_hash(:best_friends, true), :as => :admin)
+ assert_admin_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_with_bang_with_admin_role_with_attr_accessible_attributes
+ person = TightPerson.create!(nested_attributes_hash(:best_friends, true), :as => :admin)
+ assert_admin_attributes(person.best_friends.first, true)
+ end
+
+ def test_has_many_create_with_bang_without_protection
+ person = LoosePerson.create!(nested_attributes_hash(:best_friends, true, nil), :without_protection => true)
+ assert_all_attributes(person.best_friends.first)
+ end
+
+end
@@ -59,8 +59,9 @@ class LoosePerson < ActiveRecord::Base
has_one :best_friend, :class_name => 'LoosePerson', :foreign_key => :best_friend_id
belongs_to :best_friend_of, :class_name => 'LoosePerson', :foreign_key => :best_friend_of_id
-
has_many :best_friends, :class_name => 'LoosePerson', :foreign_key => :best_friend_id
+
+ accepts_nested_attributes_for :best_friend, :best_friend_of, :best_friends
end
class LooseDescendant < LoosePerson; end
@@ -70,11 +71,14 @@ class TightPerson < ActiveRecord::Base
attr_accessible :first_name, :gender
attr_accessible :first_name, :gender, :comments, :as => :admin
+ attr_accessible :best_friend_attributes, :best_friend_of_attributes, :best_friends_attributes
+ attr_accessible :best_friend_attributes, :best_friend_of_attributes, :best_friends_attributes, :as => :admin
has_one :best_friend, :class_name => 'TightPerson', :foreign_key => :best_friend_id
belongs_to :best_friend_of, :class_name => 'TightPerson', :foreign_key => :best_friend_of_id
-
has_many :best_friends, :class_name => 'TightPerson', :foreign_key => :best_friend_id
+
+ accepts_nested_attributes_for :best_friend, :best_friend_of, :best_friends
end
class TightDescendant < TightPerson; end

0 comments on commit 5d655aa

Please sign in to comment.