Opened 17 years ago

Closed 12 years ago

Last modified 3 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: dev
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@…> 17 years ago.
Preliniary copy() patch
django-model-copying.diff (10.2 KB ) - added by Forest Bond 15 years ago.
New patch implementing Model.copy with recursion. Includes tests.
4027-copy-model-docs.diff (1.6 KB ) - added by Tomek Paczkowski 13 years ago.
(Git) diff with documentation update describing copying model instances
4027-copy-model-docs-text.diff (1.6 KB ) - added by Sasha Romijn 13 years ago.
Same as last patch, tiny language corrections added.

Download all attachments as: .zip

Change History (30)

comment:1 by Gary Wilson <gary.wilson@…>, 17 years ago

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 by Marek Kubica, 17 years ago

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 by Marek Kubica, 17 years ago

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

comment:4 by Malcolm Tredinnick, 17 years ago

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.

by Marek Kubica <pythonmailing@…>, 17 years ago

Attachment: 4027-model-copy.diff added

Preliniary copy() patch

comment:5 by Marek Kubica <pythonmailing@…>, 17 years ago

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 by anonymous, 17 years ago

Cc: simon@… added

comment:7 by Simon Litchfield <simon@…>, 17 years ago

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 by miracle2k, 16 years ago

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.

by Forest Bond, 15 years ago

Attachment: django-model-copying.diff added

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

comment:9 by Forest Bond, 15 years ago

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 by Forest Bond, 15 years ago

Needs tests: unset
Patch needs improvement: unset

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

comment:11 by LukeMurphey, 15 years ago

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 by Forest Bond, 14 years ago

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 by Mitar, 14 years ago

Cc: mmitar@… added

comment:14 by Malcolm Tredinnick, 14 years ago

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 by Forest Bond, 14 years ago

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 by Mitar, 14 years ago

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 by Gabriel Hurley, 13 years ago

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 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: New feature

comment:19 by Tomek Paczkowski, 13 years ago

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

by Tomek Paczkowski, 13 years ago

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

(Git) diff with documentation update describing copying model instances

comment:20 by Tomek Paczkowski, 13 years ago

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 by Tomek Paczkowski, 13 years ago

Has patch: set
Needs documentation: unset

by Sasha Romijn, 13 years ago

Same as last patch, tiny language corrections added.

comment:22 by Sasha Romijn, 13 years ago

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 by Byron Ruth, 13 years ago

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 by Luke Plant, 12 years ago

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.

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 0fd05df:

Refs #4027 -- Added Model._state.adding to docs about copying model instances.

comment:26 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In cf05f9f2:

[3.2.x] Refs #4027 -- Added Model._state.adding to docs about copying model instances.

Backport of 0fd05df7b5690fb1b675e1b4d9c92bb22ff74360 from master

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