Opened 8 years ago

Closed 4 years ago

#4027 closed New feature (fixed)

Document how to make copies of model instances

Reported by: Marek Kubica Owned by: oinopion
Component: Documentation Version: master
Severity: Normal Keywords: db, copy
Cc: pythonmailing@…, simon@…, forest, 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@…> 8 years ago.
Preliniary copy() patch
django-model-copying.diff (10.2 KB) - added by forest 6 years ago.
New patch implementing Model.copy with recursion. Includes tests.
4027-copy-model-docs.diff (1.6 KB) - added by oinopion 4 years ago.
(Git) diff with documentation update describing copying model instances
4027-copy-model-docs-text.diff (1.6 KB) - added by erikr 4 years ago.
Same as last patch, tiny language corrections added.

Download all attachments as: .zip

Change History (28)

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Copying does not copy ManyToMany field values to ability to make a copies of model instances
  • Triage Stage changed from Unreviewed to Design 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 8 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 8 years ago by Marek Kubica

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

comment:4 Changed 8 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

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 8 years ago by Marek Kubica <pythonmailing@…>

Preliniary copy() patch

comment:5 Changed 8 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 8 years ago by anonymous

  • Cc simon@… added

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

  • Triage Stage changed from Accepted to Design 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 7 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 6 years ago by forest

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

comment:9 Changed 6 years ago by forest

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 6 years ago by forest

  • Needs tests unset
  • Patch needs improvement unset

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

comment:11 Changed 6 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 5 years ago by forest

  • Cc forest 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 5 years ago by mitar

  • Cc mmitar@… added

comment:14 Changed 5 years ago by mtredinnick

  • Component changed from Database layer (models, ORM) to Documentation
  • Has patch unset
  • Triage Stage changed from Design decision needed to Accepted

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 5 years ago by forest

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 5 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 5 years ago by gabrielhurley

  • Summary changed from ability to make a copies of model instances to Document 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 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:19 Changed 4 years ago by oinopion

  • Easy pickings unset
  • Owner changed from nobody to oinopion
  • UI/UX unset

Changed 4 years ago by oinopion

(Git) diff with documentation update describing copying model instances

comment:20 Changed 4 years ago by oinopion

  • Cc tomek+django@… added
  • Status changed from new to assigned

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

comment:21 Changed 4 years ago by oinopion

  • Has patch set
  • Needs documentation unset

Changed 4 years ago by erikr

Same as last patch, tiny language corrections added.

comment:22 Changed 4 years ago by erikr

  • Triage Stage changed from Accepted to Ready 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 4 years ago by bruth

  • 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 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

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