Opened 17 years ago
Closed 13 years ago
#5450 closed Cleanup/optimization (invalid)
save_m2m should raise an exception for unsaved models
Reported by: | Owned by: | Jonas Obrist | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
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 )
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)
Change History (12)
by , 17 years ago
Attachment: | save_m2m_with_new_instance.diff added |
---|
comment:2 by , 17 years ago
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.
follow-up: 4 comment:3 by , 17 years ago
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: | Unreviewed → Accepted |
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 by , 16 years ago
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 by , 16 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|
comment:6 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:7 by , 13 years ago
Easy pickings: | unset |
---|---|
Owner: | changed from | to
UI/UX: | unset |
comment:8 by , 13 years ago
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 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:10 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:11 by , 13 years ago
Resolution: | → invalid |
---|---|
Status: | reopened → closed |
Closed this issue with the wrong resolution, it wasn't fixed but rather the proposed solution (raising an exception) is already there.