Opened 9 years ago

Closed 6 years ago

#5450 closed Cleanup/optimization (invalid)

save_m2m should raise an exception for unsaved models

Reported by: Øyvind Saltvik <oyvind@…> Owned by: Jonas Obrist
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Jacob)

 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)

Attachments (1)

save_m2m_with_new_instance.diff (566 bytes) - added by Øyvind Saltvik <oyvind@…> 9 years ago.

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by Øyvind Saltvik <oyvind@…>

comment:1 Changed 9 years ago by Ø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

comment:2 Changed 9 years ago by 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.

comment:3 Changed 9 years ago by Jacob

Description: modified (diff)
Summary: Save_m2m in forms created by form for model should take a instance argument.save_m2m should raise an exception for unsaved models
Triage Stage: UnreviewedAccepted

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.

comment:4 in reply to:  3 Changed 8 years ago by 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.

comment:5 Changed 8 years ago by Thejaswi Puthraya

Component: UncategorizedDatabase layer (models, ORM)

comment:6 Changed 6 years ago by Gabriel Hurley

Severity: Normal
Type: Cleanup/optimization

comment:7 Changed 6 years ago by Jonas Obrist

Easy pickings: unset
Owner: changed from nobody to Jonas Obrist
UI/UX: unset

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

comment:9 Changed 6 years ago by Jonas Obrist

Resolution: fixed
Status: newclosed

comment:10 Changed 6 years ago by Jonas Obrist

Resolution: fixed
Status: closedreopened

comment:11 Changed 6 years ago by Jonas Obrist

Resolution: invalid
Status: reopenedclosed

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

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