clowne-rb / clowne

A flexible gem for cloning models
https://clowne.evilmartians.io
MIT License
317 stars 18 forks source link

`dependent: :destroy` option on association destroys models related to source after cloning #29

Open curpeng opened 5 years ago

curpeng commented 5 years ago

Associations that contain dependent: :destroy option will be destroyed on source object after the cloning process. It's easy to reproduce in your tests - just add dependent: :destroy to has_one :image, class_name: 'AR::Image' and tests will fail.

curpeng commented 5 years ago

I had debugged the issue a little bit and here is what I found:

This line is destroying association on source model:

  # Clowne::Adapters::ActiveRecord::Associations::HasOne
  record.__send__(:"#{association_name}=", child_clone)

Problem is that record.__send__(:"#{association_name}") is not nil, actually it's association of source object, so when we set new association to child_clone active record destroys source association because of 'dependent: :destroy' option.

To fix it, I think, we need to change the way how gem does initial copy of the object:

# Clowne::Adapters::Base
init_record = init_record(self.class.dup_record(source))

This line calls ActiveRecord's dup method, which duplicates a model + parent associations.

curpeng commented 5 years ago

After a fresh look, I've realized what's going on: I have a model with primary key :id and additional column uuid, my association looks like this:

has_one :image, dependent: :destroy, inverse_of: :post, foreign_key: :post_uuid, primary_key: :uuid

So when the cloning process starts, :id is set to nil, but uuid is not(I have used nullify :uuid), so newly created clone already has a relation of a source object and when clowne sets a cloned version of this association rails destroys original one due to dependent: :destroy option. I've fixed this problem by using init_as:

  init_as do |source|
    clone = source.dup
    clone.uuid = nil
    clone
  end

It would be great to check if an association has dependent option and if an association is already present before clone associations step then raise an error for such idiots like me :) Thanks a lot for such nice gem!

ssnickolay commented 5 years ago

Hi @dolhishev! Your case is interesting and unusual but make sense, yeh. First of all, if this approach with two different columns (id and uuid) is used for all your models, you can define your adapter:


class UUIDActiveRecordAdapter < Clowne::Adapters::ActiveRecord
  class << self
    def dup_record(record)
       super.tap do |clone|
         clone.uuid = nil
       end
    end
  end
end

# and 
class BaseCloner < Clowne::Cloner
  adapter UUIDActiveRecordAdapter
end

class SomeModelCloner < BaseCloner
  # declarations
end

See here and here.

It would be great to check if an association has dependent option and if an association is already present before clone associations step then raise an error for such idiots like me :)

Hm, maybe is too rude to raise an exception but we can handle this case and put a warning message. I'll think about it. And thanks for the report 👍