Opened 9 years ago

Closed 5 years ago

#4027 closed New feature (fixed)

Document how to make copies of model instances

Reported by: Marek Kubica Owned by: Tomek Paczkowski
Component: Documentation Version: master
Severity: Normal Keywords: db, copy
Cc: pythonmailing@…, simon@…, Forest Bond, mmitar@…, tomek+django@…, bruth@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Although it does not seem documented anywhere (but I think, it should, as it is handy) model instances can be copied by setting the primary key to None in which case they get a new ID and get saved as new object into the database preserving their old values. Unfortunately, this does not apply to ManyToMany fields of the model: they have to be saved before setting the primary key to None (ModelInstance.manytomanyfield.all()), then the instance has to be saved and afterwards they can be added to the new object as it has again a valid ID and saved for the second time. It seems to me that saving an object twice to circumvent this is somewhat hackish.

The most important thing to me is that copying objects should work in a clean way (setting the primary key to None is more or less clean enough, provided that it is documented somewhere and works as expected). Such a possibility is needed sometimes: I have to maintain a large number of objecs which often differ only in one field, so I can simply copy the old one and change the value.

Attachments (4)

4027-model-copy.diff (1.4 KB) - added by Marek Kubica <pythonmailing@…> 9 years ago.
Preliniary copy() patch
django-model-copying.diff (10.2 KB) - added by Forest Bond 7 years ago.
New patch implementing Model.copy with recursion. Includes tests.
4027-copy-model-docs.diff (1.6 KB) - added by Tomek Paczkowski 5 years ago.
(Git) diff with documentation update describing copying model instances
4027-copy-model-docs-text.diff (1.6 KB) - added by Erik Romijn 5 years ago.
Same as last patch, tiny language corrections added.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 9 years ago by Gary Wilson <gary.wilson@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: Copying does not copy ManyToMany field valuesability to make a copies of model instances
Triage Stage: UnreviewedDesign decision needed

Since Django has no official method for copying model instances, I am going to change this ticket's title to reflect that; otherwise, this ticket should probably be marked invalid. Marek, you might want to start a discussion on the django-dev mailing list about this topic.

comment:2 Changed 9 years ago by Marek Kubica

Thanks Gary, this is the same thing I heard on IRC. An official method for copying is exactly what I need. Fine, I'll start a discussion on this on django-dev.

comment:3 Changed 9 years ago by Marek Kubica

I've posted to django-developers now, the discussion can be found in Gogle Groups.

comment:4 Changed 9 years ago by Malcolm Tredinnick

Triage Stage: Design decision neededAccepted

Many-to-many fields can only be added and saved after the models they are referring to have been saved (or, at least, have a primary key). That cannot be avoided. So it sounds like you need to write an external copy() function that handles this.

Seems like a reasonable thing to do, so feel free to knock up a patch. A method on the Model class wouldn't be insane.

Changed 9 years ago by Marek Kubica <pythonmailing@…>

Attachment: 4027-model-copy.diff added

Preliniary copy() patch

comment:5 Changed 9 years ago by Marek Kubica <pythonmailing@…>

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set

I wrote a first version of the patch: it implements copy() as method of the Model class defined in base.py. It was tested by a small helper script - I'm not sure, but it probably needs some unittests, too. As this was my first patch submitted to Django, I was not sure how to implement it correctly, so this patch is more of a draft that is free to discussion. If you have any comments on it - just post, I'll try to improve this patch to bring it into a stage where it is ready for checkin.

comment:6 Changed 9 years ago by anonymous

Cc: simon@… added

comment:7 Changed 9 years ago by Simon Litchfield <simon@…>

Triage Stage: AcceptedDesign decision needed

I was unable to use this patch, because it implicitly save()s the newly copied instance.

To me, plain old copy() should only copy the attributes.

Perhaps some arguments (eg commit=True), and/or maybe using deepcopy() to include the manytomany's would be a better approach?

comment:8 Changed 8 years ago by miracle2k

With QSRF and model inheritance, setting the pk of a subclass model to None is no longer enough to make a copy. From what I can tell, the best working approach seems to be:

def copy_model_instance(obj):
    from django.db.models import AutoField
    initial = dict([(f.name, getattr(obj, f.name))
                    for f in obj._meta.fields
                    if not isinstance(f, AutoField) and\
                       not f in obj._meta.parents.values()])
    return obj.__class__(**initial)

This is without handling m2m relationships.

Changed 7 years ago by Forest Bond

Attachment: django-model-copying.diff added

New patch implementing Model.copy with recursion. Includes tests.

comment:9 Changed 7 years ago by Forest Bond

Hi,

Attached is a new patch that implements Model.copy with recursion. The patch includes tests (although the tests probably miss some corner cases).

Let me know what you think.

Thanks,
Forest

comment:10 Changed 7 years ago by Forest Bond

Needs tests: unset
Patch needs improvement: unset

Reset a "Needs tests" and "Patch needs improvement" flags pending review of latest patch.

comment:11 Changed 7 years ago by LukeMurphey

Model inheritance makes this more difficult since the inheriting classes uses a base pointer that references the base model. Setting the id to None and calling save() does not create a new instance (at least it doesn't in my case on Django 1.0.2).

comment:12 Changed 6 years ago by Forest Bond

Cc: Forest Bond added

Hi Luke,

My patch does not rely on setting self.id to None.

I will try to bring this ticket up on the mailing list. Perhaps it can be fixed in 1.3.

Thanks,
Forest

comment:13 Changed 6 years ago by Mitar

Cc: mmitar@… added

comment:14 Changed 6 years ago by Malcolm Tredinnick

Component: Database layer (models, ORM)Documentation
Has patch: unset
Triage Stage: Design decision neededAccepted

This is possible in a few lines of code for each model. You can copy many-to-many field by doing new_instance.m2m = old_instance.m2m.all(), for example. Ditto for foreign keys. This isn't always want you want when creating a copy of a model, however (think deep-copy versus shallow-copy), so really has to be left up to the individual case. For any particular model, a duplicate method is short enough already that Django doesn't need additions.

*However*, a documentation patch to explain this little fragment would be useful. We should start capturing some of these little patterns.

comment:15 Changed 6 years ago by Forest Bond

Hi Malcolm,

It sounds like your suggestion is to manually copy each field from the old instance to the new one. Why not provide a method to automate this?

Also, my patch implements recursion (deep copying). This is not a very straight-forward thing to do, so it seems like having one implementation that works in Django core would be useful.

I realize my patch does more than what the ticket reporter was initially looking for. Would you prefer that I create a new ticket to cover this additional functionality? I personally do need support for deep copying.

Thanks,
Forest

comment:16 Changed 6 years ago by Mitar

I also think it would be useful to have a best-practice method on object which covers this (or few of them for different kinds of cloning, all nicely documented) so that programmer does not need to care about internals of the objects (for example, what if objects will represent non-relational data someday, will m2m field be there, will there be something else to also copy?).

comment:17 Changed 6 years ago by Gabriel Hurley

Summary: ability to make a copies of model instancesDocument how to make copies of model instances

In response to Forest and mitar: let's keep this a documentation ticket as Malcolm suggested, and move any deep/shallow copy feature additions to a new ticket (and probably start a mailing list discussion to make sure we have consensus on the feature).

For the time being, a ticket this old really needs some kind of resolution, and documenting a simple and useful pattern that covers 90% of cases is a good idea, even if copy methods do get implemented at a later date.

I would say that either docs/topics/db/models or docs/topics/db/queries is probably the place to put the docs for this. Despite the fact that topics/db/models is a more appropriate section, it actually fits better context-wise with topics/db/models near the sections of updating and deleting objects. A case for going into either one could be made, and ultimately there should probably be a cross-ref from one section to the other whichever way one it ends up in.

comment:18 Changed 5 years ago by Łukasz Rekucki

Severity: Normal
Type: New feature

comment:19 Changed 5 years ago by Tomek Paczkowski

Easy pickings: unset
Owner: changed from nobody to Tomek Paczkowski
UI/UX: unset

Changed 5 years ago by Tomek Paczkowski

Attachment: 4027-copy-model-docs.diff added

(Git) diff with documentation update describing copying model instances

comment:20 Changed 5 years ago by Tomek Paczkowski

Cc: tomek+django@… added
Status: newassigned

I've written short section on copying model instances. I surely need some help with people that know how to write.

comment:21 Changed 5 years ago by Tomek Paczkowski

Has patch: set
Needs documentation: unset

Changed 5 years ago by Erik Romijn

Same as last patch, tiny language corrections added.

comment:22 Changed 5 years ago by Erik Romijn

Triage Stage: AcceptedReady for checkin

I've made a few tiny language improvements. Otherwise, it looks good to me, with clear and simple examples. RFCed.

comment:23 Changed 5 years ago by Byron Ruth

Cc: bruth@… added

I tackled this problem as a separate app: https://github.com/cbmi/django-forkit. It is simply an abstract model that implements a few methods such as fork, reset, and diff. It handles all three relationship types, though there are a few known issues (e.g. M2M through models are not yet supported). Deep diffing is not yet fully baked, and the reset method must propagate that a reset is intended for all related objects (rather than creating new ones). Saving related objects are deferred until the whole hierarchy has been traversed to ensure data integrity (and provides ways of altering the objects post fork/reset).

Here are the tests for it thus far: https://github.com/cbmi/django-forkit/blob/master/forkit/tests/models.py. I would be interested in hearing feedback on a few of the assumptions I made (e.g. which relationships to choose for a shallow copy). And definitely send a pull request or submit a ticket if there are more elegant ways to make use of the Django model internals.

I am posting here only to make others aware of it, but I don't think it is appropriate to go into Django proper.

comment:24 Changed 5 years ago by Luke Plant

Resolution: fixed
Status: assignedclosed

In [17064]:

Fixed #4027 - Document how to make copies of model instances

Thanks to Marek Kubica for the report and initial patch, and to oinopion and
erikr for work on the patch.

Note: See TracTickets for help on using tickets.
Back to Top