save_m2m should raise an exception for unsaved models

[Øyvind Saltvik <oyvind@…>]

 object = form.save(commit=False)
 object.foofield = 'something'
 object.save()
 form.save_m2m() # would save to a unsaved instance

 form.save_m2m(object) # would be better

(See below for more discussion -JKM)

[Øyvind Saltvik <oyvind@…>]

    object = form.save(commit=False)
    object.foofield = 'something'

    object.save() form.save_m2m() # would save to a unsaved instance

    form.save_m2m(object) # would be better

[Russell Keith-Magee]

I can see the problem you're trying to avoid, but this isn't the right approach. The m2m data from a form should only ever be used with a very specific instance - allowing users to specify an arbitrary instance isn't the way to solve the problem. Plus, the following would still be possible:

object = form.save(commit=False)
object.foofield = 'something'
form.save_m2m(object)

which is just as invalid.

The better solution here would be to put in a check at the start of save_m2m to see if the object has been saved. A simple pk check would probably suffice, raising an exception if the object isn't saved.

[Jacob]

Agreed with Russell -- save_m2m() is very specific to that instance; better behavior would be to raise an error if the object doesn't have a pk. I'm coopting this ticket to handle that issue instead.

[anonymous]

Replying to jacob:

Agreed with Russell -- save_m2m() is very specific to that instance; better behavior would be to raise an error if the object doesn't have a pk. I'm coopting this ticket to handle that issue instead.

Just checking if pk is set isn't a good solution too because it can be set programmatically before save() is called. It is definitely needed to have some kind of read-only attribute, say Model.in_db, telling whether model has been really saved (or read from) db.

[Jonas Obrist]

An error is actually raised when trying to use save_m2m after a no-commit save:

ValueError: 'Article' instance needs to have a primary key value before a many-to-many relationship can be used.

That comes from line 492 in django.db.models.fields.related

[Jonas Obrist]

Closed this issue with the wrong resolution, it wasn't fixed but rather the proposed solution (raising an exception) is already there.