moiristo / deep_cloneable

This gem gives every ActiveRecord::Base object the possibility to do a deep clone that includes user specified associations.
MIT License
786 stars 88 forks source link

Reusing cloned relation objects #50

Closed Jeehut closed 9 years ago

Jeehut commented 9 years ago

I have an object a of class A that I want to clone. a has a relation to a b of class B and to a c of class C. When cloning a I want to include both b and c relation objects. According to the documentation this can be done like so:

a.deep_clone :include => [:b, :c]

Everything seems fine at the first sight. But here's my problem:

The object b has a relation to the same c that a is already related to. This means object a has a field c_id with value (say) 42 and object b also has a c_id field with the same value 42. When I now run the above code this leads to the following cloned objects:

<A-Class-Object id: 100, b_id: 120, c_id: 140>,
<B-Class-Object id: 120, c_id: 160>,
<C-Class-Object id: 140>,
<C-Class-Object id: 160>

So as you can see this clones object c twice and therefore creates two distinct objects although there was just one object that was used by multiple objects in the original relations.

What I would expect is to detect reused objects via equal _id-fields and only create one object for them which would result to the following objects after cloning:

<A-Class-Object id: 100, b_id: 120, c_id: 140>,
<B-Class-Object id: 120, c_id: 140>,
<C-Class-Object id: 140>

Did I do something wrong or is this behavior just not implemented yet?

I would definitely expect this to be included within this gem so if it's not implemented yet, please don't just close the ticket, I'd love to help getting this done. What do you think, @moiristo?

Btw: I'm using deep_cloneable 2.1.1 in a Rails 3.2.x project.

Jeehut commented 9 years ago

I've forked this project and added a test (including necessary model and database changes) for this behavior. Unfortunately some tests aren't passing right now (even before I did any changes) but the test I added should represent the expected behavior and therefore succeed ones this is implemented (and the problem why the tests don't run is fixed).

moiristo commented 9 years ago

Hi there,

Yes, this problem has been addressed in the gem, namely by using a dictionary. Please see https://github.com/moiristo/deep_cloneable#cloning-really-deep-with-multiple-associations-and-a-dictionary.

Jeehut commented 9 years ago

Thank you, that worked like a charm! Just the naming is counter-intuitive for someone looking for exactly this, so I missed it when I read the docs. Currently it's hard to find it unless you read all the text – I guess the title could be improved to something like: Cloning really deep with multiple associations and object reusage Otherwise it isn't clear what the Dictionary is for.

moiristo commented 9 years ago

Yes, I've been thinking to rewrite the whole readme anyway, so that'll be on my todo-list :)

Jeehut commented 9 years ago

Ok, perfect, then I close this ticket for now trusting on your todo-list. :+1: