Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes problem with replacing has_one association record with itself #12834

Merged
merged 1 commit into from Nov 11, 2013

Conversation

Projects
None yet
4 participants
Contributor

dredozubov commented Nov 10, 2013

ActiveRecord::Schema.define do
  create_table :users do |t|
  end

  create_table :addresses do |t|
    t.integer :user_id
    t.string :street
  end
end

class User < ActiveRecord::Base
  has_one :address, :dependent => :destroy
end

class Address < ActiveRecord::Base
  belongs_to :user
end

 user = User.create!
 address = user.create_address
 address.street = '1 Some St.'
 user.address = address

Fails with "can't modify frozen Hash" when attempting to save update has_one association.

full test gist: https://gist.github.com/scambra/7320315

discussion here: #12775

@senny senny and 1 other commented on an outdated diff Nov 11, 2013

...lib/active_record/associations/has_one_association.rb
@@ -30,7 +30,7 @@ def replace(record, save = true)
save &&= owner.persisted?
transaction_if(save) do
- remove_target!(options[:dependent]) if target && !target.destroyed?
+ remove_target!(options[:dependent]) if target && !target.destroyed? && target != record
@senny

senny Nov 11, 2013

Member

target != record is already used at the top level if statement. Let's extract a local variable to make it expressive that this is an assignment of itself.

@dredozubov

dredozubov Nov 11, 2013

Contributor

seems like a nice idea, will do

Member

senny commented Nov 11, 2013

@dredozubov can you also add an entry to the CHANGELOG?

Contributor

dredozubov commented Nov 11, 2013

i'll rebase and squash commits if everything is ok

@senny senny commented on the diff Nov 11, 2013

...lib/active_record/associations/has_one_association.rb
@@ -26,11 +26,14 @@ def replace(record, save = true)
load_target
return self.target if !(target || record)
- if (target != record) || record.changed?
+
@senny

senny Nov 11, 2013

Member

✂️ only the blank line.

@dredozubov

dredozubov Nov 11, 2013

Contributor

is this a style guide thing or something?

@senny

senny Nov 11, 2013

Member

we like to not add too many blank lines. Maybe removing the one below the assignment is more clear. Having one above and one below is not necessary though.

@dredozubov

dredozubov Nov 11, 2013

Contributor

Ok, i will update corresponding to your suggestions. I usually use blank lines to emphasize some statement, but maybe it's just not need emphasizing in this case. Thanks for quick response. I'll update it right now.

@senny senny commented on an outdated diff Nov 11, 2013

...lib/active_record/associations/has_one_association.rb
@@ -26,11 +26,14 @@ def replace(record, save = true)
load_target
return self.target if !(target || record)
- if (target != record) || record.changed?
+
+ another_record = target != record
@senny

senny Nov 11, 2013

Member

I'm not completely happy with the variable name. Maybe assigning_another_record?

@senny senny commented on an outdated diff Nov 11, 2013

activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+* Fix bug where has_one associaton record update result in crash, when replaced with itself.
+
+ Fixes #12834
@senny

senny Nov 11, 2013

Member

just a nitpick but can you add a . after "Fixes" like so:

Fixes #12834.

Member

senny commented Nov 11, 2013

@dredozubov I added a few cosmetic notices, please update and squash the commits. The patch looks good.

Contributor

dredozubov commented Nov 11, 2013

@senny squashed it and force-pushed, i think that's all

Member

senny commented Nov 11, 2013

@dredozubov great! Thank you for your contribution 💛

senny added a commit that referenced this pull request Nov 11, 2013

Merge pull request #12834 from dredozubov/has_one_association_replace…
…ment

Fixes problem with replacing has_one association record with itself

@senny senny merged commit f580270 into rails:master Nov 11, 2013

@dredozubov dredozubov deleted the dredozubov:has_one_association_replacement branch Nov 11, 2013

senny added a commit that referenced this pull request Nov 11, 2013

Merge pull request #12834 from dredozubov/has_one_association_replace…
…ment

Fixes problem with replacing has_one association record with itself
Conflicts:
	activerecord/CHANGELOG.md
Member

arthurnn commented Dec 13, 2013

is this being back-ported to rails 4.0-stable?

Member

robin850 commented Dec 14, 2013

@arthurnn : This is here I think

Member

senny commented Dec 14, 2013

@arthurnn yes it was backported. Commit is linked right above your comment 😁

Member

arthurnn commented Dec 14, 2013

yep. realized after a few min i posted this. sorry :)

On Saturday, December 14, 2013, Yves Senn wrote:

@arthurnn https://github.com/arthurnn yes it was backported. Commit is
linked right above your comment [image: 😁]


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

Arthur Nogueira Neves
Follow me @arthurnn http://www.twitter.com/arthurnn89

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