Skip to content
This repository
Browse code

Merge pull request #9426 from exviva/nested_attributes_reuse_existing…

…_new_record

Do not overwrite manually built records during one-to-one nested attribute assignment
Conflicts:
	activerecord/CHANGELOG.md
  • Loading branch information...
commit 2fc4793dd35a567b7885989ff5948fc9816d1839 1 parent a87fc1a
Jon Leighton jonleighton authored
27 activerecord/CHANGELOG.md
Source Rendered
... ... @@ -1,6 +1,33 @@
1 1 ## unreleased ##
  2 +
2 3 * Fix `add_column` with `array` option when using PostgreSQL. Fixes #10432
3 4
  5 +* Do not overwrite manually built records during one-to-one nested attribute assignment
  6 +
  7 + For one-to-one nested associations, if you build the new (in-memory)
  8 + child object yourself before assignment, then the NestedAttributes
  9 + module will not overwrite it, e.g.:
  10 +
  11 + class Member < ActiveRecord::Base
  12 + has_one :avatar
  13 + accepts_nested_attributes_for :avatar
  14 +
  15 + def avatar
  16 + super || build_avatar(width: 200)
  17 + end
  18 + end
  19 +
  20 + member = Member.new
  21 + member.avatar_attributes = {icon: 'sad'}
  22 + member.avatar.width # => 200
  23 +
  24 + *Olek Janiszewski*
  25 +
  26 +* fixes bug introduced by #3329. Now, when autosaving associations,
  27 + deletions happen before inserts and saves. This prevents a 'duplicate
  28 + unique value' database error that would occur if a record being created had
  29 + the same value on a unique indexed field as that of a record being destroyed.
  30 +
4 31 *Adam Anderson*
5 32
6 33
12 activerecord/lib/active_record/associations/association.rb
@@ -168,6 +168,13 @@ def marshal_load(data)
168 168 @reflection = @owner.class.reflect_on_association(reflection_name)
169 169 end
170 170
  171 + def initialize_attributes(record) #:nodoc:
  172 + skip_assign = [reflection.foreign_key, reflection.type].compact
  173 + attributes = create_scope.except(*(record.changed - skip_assign))
  174 + record.assign_attributes(attributes)
  175 + set_inverse_instance(record)
  176 + end
  177 +
171 178 private
172 179
173 180 def find_target?
@@ -237,10 +244,7 @@ def stale_state
237 244
238 245 def build_record(attributes)
239 246 reflection.build_association(attributes) do |record|
240   - skip_assign = [reflection.foreign_key, reflection.type].compact
241   - attributes = create_scope.except(*(record.changed - skip_assign))
242   - record.assign_attributes(attributes)
243   - set_inverse_instance(record)
  247 + initialize_attributes(record)
244 248 end
245 249 end
246 250 end
39 activerecord/lib/active_record/nested_attributes.rb
@@ -229,6 +229,23 @@ class TooManyRecords < ActiveRecordError
229 229 # belongs_to :member, inverse_of: :posts
230 230 # validates_presence_of :member
231 231 # end
  232 + #
  233 + # For one-to-one nested associations, if you build the new (in-memory)
  234 + # child object yourself before assignment, then this module will not
  235 + # overwrite it, e.g.:
  236 + #
  237 + # class Member < ActiveRecord::Base
  238 + # has_one :avatar
  239 + # accepts_nested_attributes_for :avatar
  240 + #
  241 + # def avatar
  242 + # super || build_avatar(width: 200)
  243 + # end
  244 + # end
  245 + #
  246 + # member = Member.new
  247 + # member.avatar_attributes = {icon: 'sad'}
  248 + # member.avatar.width # => 200
232 249 module ClassMethods
233 250 REJECT_ALL_BLANK_PROC = proc { |attributes| attributes.all? { |key, value| key == '_destroy' || value.blank? } }
234 251
@@ -356,20 +373,28 @@ def _destroy
356 373 def assign_nested_attributes_for_one_to_one_association(association_name, attributes)
357 374 options = self.nested_attributes_options[association_name]
358 375 attributes = attributes.with_indifferent_access
  376 + existing_record = send(association_name)
359 377
360   - if (options[:update_only] || !attributes['id'].blank?) && (record = send(association_name)) &&
361   - (options[:update_only] || record.id.to_s == attributes['id'].to_s)
362   - assign_to_or_mark_for_destruction(record, attributes, options[:allow_destroy]) unless call_reject_if(association_name, attributes)
  378 + if (options[:update_only] || !attributes['id'].blank?) && existing_record &&
  379 + (options[:update_only] || existing_record.id.to_s == attributes['id'].to_s)
  380 + assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) unless call_reject_if(association_name, attributes)
363 381
364 382 elsif attributes['id'].present?
365 383 raise_nested_attributes_record_not_found!(association_name, attributes['id'])
366 384
367 385 elsif !reject_new_record?(association_name, attributes)
368   - method = "build_#{association_name}"
369   - if respond_to?(method)
370   - send(method, attributes.except(*UNASSIGNABLE_KEYS))
  386 + assignable_attributes = attributes.except(*UNASSIGNABLE_KEYS)
  387 +
  388 + if existing_record && existing_record.new_record?
  389 + existing_record.assign_attributes(assignable_attributes)
  390 + association(association_name).initialize_attributes(existing_record)
371 391 else
372   - raise ArgumentError, "Cannot build association `#{association_name}'. Are you trying to build a polymorphic one-to-one association?"
  392 + method = "build_#{association_name}"
  393 + if respond_to?(method)
  394 + send(method, assignable_attributes)
  395 + else
  396 + raise ArgumentError, "Cannot build association `#{association_name}'. Are you trying to build a polymorphic one-to-one association?"
  397 + end
373 398 end
374 399 end
375 400 end
14 activerecord/test/cases/nested_attributes_test.rb
@@ -131,6 +131,20 @@ def test_reject_if_with_a_proc_which_returns_true_always_for_has_one
131 131 assert_equal 's1', ship.reload.name
132 132 end
133 133
  134 + def test_reuse_already_built_new_record
  135 + pirate = Pirate.new
  136 + ship_built_first = pirate.build_ship
  137 + pirate.ship_attributes = { name: 'Ship 1' }
  138 + assert_equal ship_built_first.object_id, pirate.ship.object_id
  139 + end
  140 +
  141 + def test_do_not_allow_assigning_foreign_key_when_reusing_existing_new_record
  142 + pirate = Pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?")
  143 + pirate.build_ship
  144 + pirate.ship_attributes = { name: 'Ship 1', pirate_id: pirate.id + 1 }
  145 + assert_equal pirate.id, pirate.ship.pirate_id
  146 + end
  147 +
134 148 def test_reject_if_with_a_proc_which_returns_true_always_for_has_many
135 149 Man.accepts_nested_attributes_for :interests, :reject_if => proc {|attributes| true }
136 150 man = Man.create(name: "John")

0 comments on commit 2fc4793

Please sign in to comment.
Something went wrong with that request. Please try again.