Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Merged
merged 1 commit into from

5 participants

Olek Janiszewski Carlos Antonio da Silva Steve Klabnik Jon Leighton Francesco Rodríguez
Olek Janiszewski

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
Francesco Rodríguez frodsan commented on the diff
...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)
Francesco Rodríguez
frodsan added a note

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

Olek Janiszewski
exviva added a note

@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.

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

Jon Leighton Collaborator

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)

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

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

Olek Janiszewski

@NZKoz or @josevalim could you have a look?

Olek Janiszewski

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

Carlos Antonio da Silva
Steve Klabnik
Collaborator

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

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}

Mind adding spaces inside {}?

Olek Janiszewski
exviva added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carlos Antonio da Silva carlosantoniodasilva commented on the diff
activerecord/lib/active_record/nested_attributes.rb
((5 lines not shown))
- 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)

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

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

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

/cc @jonleighton

Olek Janiszewski

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

Thanks!

Jon Leighton jonleighton commented on the diff
activerecord/lib/active_record/nested_attributes.rb
((12 lines not shown))
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)
Jon Leighton Collaborator

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

Olek Janiszewski
exviva added a note

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

Jon Leighton Collaborator

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?)

Olek Janiszewski
exviva added a note

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.

Olek Janiszewski
exviva added a note

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.

Jon Leighton Collaborator

Makes sense

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

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

Olek Janiszewski

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

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

Jon Leighton
Collaborator

Sorry, this needs another rebase

Olek Janiszewski 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
Jon Leighton jonleighton merged commit f1af9f8 into from
Olek Janiszewski

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

Jon Leighton
Collaborator

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 3, 2013
  1. Olek Janiszewski

    Do not overwrite manually built records during one-to-one nested attr…

    exviva authored
    …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
This page is out of date. Refresh to see the latest.
21 activerecord/CHANGELOG.md
View
@@ -1,3 +1,24 @@
+* Do not overwrite manually built records during one-to-one nested attribute 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
+
+ *Olek Janiszewski*
+
* fixes bug introduced by #3329. Now, when autosaving associations,
deletions happen before inserts and saves. This prevents a 'duplicate
unique value' database error that would occur if a record being created had
12 activerecord/lib/active_record/associations/association.rb
View
@@ -164,6 +164,13 @@ def marshal_load(data)
@reflection = @owner.class.reflect_on_association(reflection_name)
end
+ def initialize_attributes(record) #:nodoc:
Francesco Rodríguez
frodsan added a note

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

Olek Janiszewski
exviva added a note

@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.

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

Jon Leighton Collaborator

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ skip_assign = [reflection.foreign_key, reflection.type].compact
+ attributes = create_scope.except(*(record.changed - skip_assign))
+ record.assign_attributes(attributes)
+ set_inverse_instance(record)
+ end
+
private
def find_target?
@@ -233,10 +240,7 @@ def stale_state
def build_record(attributes)
reflection.build_association(attributes) do |record|
- skip_assign = [reflection.foreign_key, reflection.type].compact
- attributes = create_scope.except(*(record.changed - skip_assign))
- record.assign_attributes(attributes)
- set_inverse_instance(record)
+ initialize_attributes(record)
end
end
end
39 activerecord/lib/active_record/nested_attributes.rb
View
@@ -229,6 +229,23 @@ class TooManyRecords < ActiveRecordError
# belongs_to :member, inverse_of: :posts
# validates_presence_of :member
# end
+ #
+ # For one-to-one nested associations, if you build the new (in-memory)
+ # child object yourself before assignment, then this 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
module ClassMethods
REJECT_ALL_BLANK_PROC = proc { |attributes| attributes.all? { |key, value| key == '_destroy' || value.blank? } }
@@ -356,20 +373,28 @@ def _destroy
def assign_nested_attributes_for_one_to_one_association(association_name, attributes)
options = self.nested_attributes_options[association_name]
attributes = attributes.with_indifferent_access
+ existing_record = send(association_name)
- 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)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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)
Jon Leighton Collaborator

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

Olek Janiszewski
exviva added a note

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

Jon Leighton Collaborator

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?)

Olek Janiszewski
exviva added a note

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.

Olek Janiszewski
exviva added a note

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.

Jon Leighton Collaborator

Makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
else
- raise ArgumentError, "Cannot build association `#{association_name}'. Are you trying to build a polymorphic one-to-one association?"
+ method = "build_#{association_name}"
+ if respond_to?(method)
+ send(method, assignable_attributes)
+ else
+ raise ArgumentError, "Cannot build association `#{association_name}'. Are you trying to build a polymorphic one-to-one association?"
+ end
end
end
end
14 activerecord/test/cases/nested_attributes_test.rb
View
@@ -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 }
+ assert_equal pirate.id, pirate.ship.pirate_id
+ end
+
def test_reject_if_with_a_proc_which_returns_true_always_for_has_many
Man.accepts_nested_attributes_for :interests, :reject_if => proc {|attributes| true }
man = Man.create(name: "John")
Something went wrong with that request. Please try again.