Do not overwrite manually built records during one-to-one nested attribute assignment #9426

Merged
merged 1 commit into from May 3, 2013

Projects

None yet

5 participants

@exviva
Contributor
exviva commented Feb 26, 2013

For one-to-one nested associations, if you build the new (in-memory)
child object yourself before assignment, then the NestedAttributes
module will not overwrite it, e.g.:

class Member < ActiveRecord::Base
  has_one :avatar
  accepts_nested_attributes_for :avatar

  def avatar
    super || build_avatar(width: 200)
  end
end

member = Member.new
member.avatar_attributes = {icon: 'sad'}
member.avatar.width # => 200
@frodsan frodsan commented on the diff Feb 26, 2013
...erecord/lib/active_record/associations/association.rb
@@ -164,6 +164,12 @@ def marshal_load(data)
@reflection = @owner.class.reflect_on_association(reflection_name)
end
+ def initialize_attributes(record)
@frodsan
frodsan Feb 26, 2013 Contributor

If this is going to stay as part of the public API, can you add the required documentation and tests?

@exviva
exviva Feb 26, 2013 Contributor

@frodsan sure thing, I didn't do this in the first place because the surrounding methods don't have any docs, I'm not sure the method name is proper, and to be honest I'm not 100% sure what this code is actually doing :).

I've extracted it to a method to keep the behaviours when NestedAttributes builds a new record or uses an already built new record, as coherent as possible.

Once I get more feedback on the code, I'll add proper test coverage and docs for that method.

@carlosantoniodasilva
carlosantoniodasilva Apr 5, 2013 Member

Should we maybe just nodoc it? I don't think it's supposed to be used publicly.

@jonleighton
jonleighton May 3, 2013 Member

The whole class is nodoc'ed so we don't really need to nodoc this specific method (but it doesn't really matter of course)

@exviva
Contributor
exviva commented Mar 3, 2013

Bump...anyone from core care to review and maybe merge this?

@exviva
Contributor
exviva commented Mar 8, 2013

@NZKoz or @josevalim could you have a look?

@exviva
Contributor
exviva commented Mar 13, 2013

Ok, then maybe @carlosantoniodasilva or @steveklabnik ? :)

@carlosantoniodasilva

Sorry, can't take a look today, but I've bookmarked here to check when I
find some time.

On Wed, Mar 13, 2013 at 3:36 AM, Olek Janiszewski
notifications@github.comwrote:

Ok, then maybe @carlosantoniodasilvahttps://github.com/carlosantoniodasilvaor
@steveklabnik https://github.com/steveklabnik ? :)


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/9426#issuecomment-14826823
.

At.
Carlos Antonio

@steveklabnik
Member

I will defer to @carlosantoniodasilva . I'm not familiar enough with ActiveRecord.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Apr 5, 2013
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
assert_equal 's1', ship.reload.name
end
+ def test_reuse_already_built_new_record
+ pirate = Pirate.new
+ ship_built_first = pirate.build_ship
+ pirate.ship_attributes = {name: 'Ship 1'}
+ assert_equal ship_built_first.object_id, pirate.ship.object_id
+ end
+
+ def test_do_not_allow_assigning_foreign_key_when_reusing_existing_new_record
+ pirate = Pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?")
+ pirate.build_ship
+ pirate.ship_attributes = {name: 'Ship 1', pirate_id: pirate.id + 1}
@carlosantoniodasilva
carlosantoniodasilva Apr 5, 2013 Member

Mind adding spaces inside {}?

@exviva
exviva Apr 5, 2013 Contributor

@carlosantoniodasilva thanks for the review, I incorporated your feedback. Let's wait for @jonleighton to review as well.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Apr 5, 2013
activerecord/lib/active_record/nested_attributes.rb
- 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)
+ if (options[:update_only] || !attributes['id'].blank?) && existing_record &&
+ (options[:update_only] || existing_record.id.to_s == attributes['id'].to_s)
+ assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) unless call_reject_if(association_name, attributes)
@carlosantoniodasilva
carlosantoniodasilva Apr 5, 2013 Member

I dislike a little having to change these lines, makes the diff dirty, but existing_record shows better intent than just record I guess.

@carlosantoniodasilva

Overall I think it looks good. It's gonna need a changelog entry though. Thanks!

/cc @jonleighton

@exviva
Contributor
exviva commented Apr 23, 2013

bump @carlosantoniodasilva @jonleighton could you guys have another look at this?

Thanks!

@jonleighton jonleighton commented on the diff May 3, 2013
activerecord/lib/active_record/nested_attributes.rb
elsif attributes['id'].present?
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))
+ assignable_attributes = attributes.except(*UNASSIGNABLE_KEYS)
+
+ if existing_record && existing_record.new_record?
+ existing_record.assign_attributes(assignable_attributes)
+ association(association_name).initialize_attributes(existing_record)
@jonleighton
jonleighton May 3, 2013 Member

Why are both of these lines necessary? Is the second one not sufficient?

@exviva
exviva May 3, 2013 Contributor

The first one assigns the attributes passed as arguments (assignable_attributes). The second one assigns them from the create_scope of the association.

@jonleighton
jonleighton May 3, 2013 Member

Aha, right. I think it would make sense for the lines to be the other way around? I.e. I should be able to override the create_scope with the assignable attributes, right? (Can you add a test for that?)

@exviva
exviva May 3, 2013 Contributor

I think it should be like this, to mirror the behaviour of Association#build_record. Actually, not the whole create_scope is assigned - only columns that didn't change, foreign key, and potentially STI type column.

The second test is showing that the foreign key cannot be nested-mass-assigned to a new, in-memory record.

@exviva
exviva May 3, 2013 Contributor

The whole dance with introducing a new method is supposed to do almost exactly what build_record would do, only not initialize and assign a new record, but instead assign the attributes to the existing in-memory record.

@jonleighton
jonleighton May 3, 2013 Member

Makes sense

@jonleighton
Member

Added some comments. It also needs a rebase. In general it seems good.

@exviva
Contributor
exviva commented May 3, 2013

@jonleighton thanks for the review, please have another look.

Should this also be backported to 4-0-stable?

@jonleighton
Member

Sorry, this needs another rebase

@exviva exviva Do not overwrite manually built records during one-to-one nested attr…
…ibute assignment

For one-to-one nested associations, if you build the new (in-memory)
child object yourself before assignment, then the NestedAttributes
module will not overwrite it, e.g.:

    class Member < ActiveRecord::Base
      has_one :avatar
      accepts_nested_attributes_for :avatar

      def avatar
        super || build_avatar(width: 200)
      end
    end

    member = Member.new
    member.avatar_attributes = {icon: 'sad'}
    member.avatar.width # => 200
534030c
@exviva
Contributor
exviva commented May 3, 2013
@jonleighton jonleighton merged commit f1af9f8 into rails:master May 3, 2013
@exviva
Contributor
exviva commented May 3, 2013

@jonleighton thanks for merging. Should we backport to 4-0-stable?

@jonleighton
Member

Unsure. I'm not sure if I consider this a feature or a bug fix. I'm trying to clarify on the core team what our backporting policy should be for 4-0-stable (between now and the actual release).

@exviva exviva deleted the exviva:nested_attributes_reuse_existing_new_record branch Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment