Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

has_one association and dependent: destroy may remove records it shouldn't #20514

Closed
giovannibonetti opened this issue Jun 11, 2015 · 8 comments

Comments

Projects
None yet
5 participants
@giovannibonetti
Copy link

commented Jun 11, 2015

I had a problem in my app when combining has_one association, a uniqueness index in the database and dependent: destroy. If I try to create a record the association and it already exists in the database, the dependent records are destroyed.
The SQL log shows that the dependent records are deleted just after the transaction rollback. Is it expected?

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', github: 'rails/rails'
  gem 'arel', github: 'rails/arel'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :users, force: true  do |t|
  end

  create_table :author_profiles, force: true  do |t|
    t.integer :user_id
  end

  create_table :books, force: true  do |t|
    t.integer :author_profile_id
  end

  add_index :author_profiles, [:user_id], unique: true
end

class User < ActiveRecord::Base
  has_one :author_profile, dependent: :destroy
end

class AuthorProfile < ActiveRecord::Base
  belongs_to :user
  has_many :books, dependent: :destroy

  validates :user, presence: true, uniqueness: true
end

class Book < ActiveRecord::Base
  belongs_to :author_profile
end

class BugTest < Minitest::Test
  def test_association_stuff
    user = User.create!
    author_profile = user.create_author_profile!

    3.times do
      author_profile.books.create!
    end

    # These tests pass
    assert_equal 1, User.count
    assert_equal 1, AuthorProfile.count
    assert_equal 3, Book.count

    # Let's force the creation of the author profile and see what happens
    user.create_author_profile

    # These tests fail
    assert_equal 3, Book.count
  end
end
@pascalbetz

This comment has been minimized.

Copy link

commented Jun 11, 2015

This is not (directly) connected the uniqueness index or the uniqueness validation.
It looks like when you run user.create_author_profile you are actually deleting the existing/associated AuthorProfile which in turns deletes the associated Books.

If you use AuthorProfile.create(user: user)instead, then the create call fails (if you have a uniqueness constraint) or succeeds but does not delete the existing books (without uniqueness constraint).

@giovannibonetti

This comment has been minimized.

Copy link
Author

commented Jun 11, 2015

Hi, @pascalbetz! Thanks for the workaround.
Nevertheless, I would like official confirmation if it is actually a bug, so that I can start working on a solution.

@pascalbetz

This comment has been minimized.

Copy link

commented Jun 11, 2015

@giovannibonetti
I'd say this is documented behaviour (not very explicit though and i agree that it is suprising that the records are deleted):

Assigns the associate object, extracts the primary key, sets it as the foreign key, and saves the
associate object. To avoid database inconsistencies, permanently deletes an existing associated object
when assigning a new one, even if the new one isn't saved to database.
http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_one

@pascalbetz

This comment has been minimized.

Copy link

commented Jun 11, 2015

Oh and: i would not use the alternate way of creating an AuthorProfile. I just showed it to explain what is going on (has_one association triggering deletion)

@pascalbetz

This comment has been minimized.

Copy link

commented Jun 12, 2015

And more digging (all the book stuff can actually ignored so i simplified a bit)

class BugTest < Minitest::Test
  def test_association_stuff
    user = User.create!
    author_profile = user.create_author_profile!

    assert_equal 1, User.count
    assert_equal 1, AuthorProfile.count

    # deletes the previously associated AuthorProfile AND saves the new one
    new_author_profile = AuthorProfile.new
    user.author_profile = new_author_profile
    assert_equal 1, AuthorProfile.count
    refute AuthorProfile.exists?(id: author_profile.id)

    # does delete the previously associated AuthorProfile
    unsaved_author_profile = user.build_author_profile
    assert_equal 0, AuthorProfile.count
    refute unsaved_author_profile.persisted?

  end
end

I think the differentiation between

bar.foo=Foo.new(delete and save)
and
bar.build_foo(delete and do not save)

is quite tricky (since the second one sets the association as well).

Since it works as described in the documentation i'd say that this is not a bug.

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2015

I would agree this is not a bug. The behavior of a has_one when more than one record exists is considered undefined behavior.

@sgrif sgrif closed this Jun 12, 2015

@Altareq

This comment has been minimized.

Copy link

commented Jul 27, 2015

I think this is a bug and should be reopened.

That is what the documentation says:

create_association(attributes = {})
Returns a new object of the associated type that has been instantiated with attributes, linked to this object through a foreign key, and that has already been saved (if it passed the validation).

However what is happing on Rails.version => "4.2.2", it deletes an existing association after the rollback failed because the uniq constraint is not fulfilled. If there is an already existing association, it should not delete it if the validation of the new one failed. So at the end, what happens is the following: you expect that the new create fails with validation issue(Uniq validation), but you would not expect that the existing association is deleted from the database. So your database gets inconsistent, as you expect that the existing one will not be deleted or that the new one can be created, the DELETE FROM commit happens after the new created association is rolled back. So it should either keep the existing association, or deletes it and creates a new one, but not none of the above which is the least what you can expect, missing data.

@Youngv

This comment has been minimized.

Copy link

commented May 16, 2017

Sometimes it didn't delete an existing association when create_association, but sometimes it does, so weird

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.