Opened 8 years ago

Closed 4 years ago

#5450 closed Cleanup/optimization (invalid)

save_m2m should raise an exception for unsaved models

Reported by: Øyvind Saltvik <oyvind@…> Owned by: ojii
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@…> 8 years ago.

Download all attachments as: .zip

Change History (12)

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

comment:1 Changed 8 years ago by Øyvind Saltvik <oyvind@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
    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 8 years ago by russellm

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 follow-up: Changed 8 years ago by jacob

  • Description modified (diff)
  • Summary changed from Save_m2m in forms created by form for model should take a instance argument. to save_m2m should raise an exception for unsaved models
  • Triage Stage changed from Unreviewed to 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 in reply to: ↑ 3 Changed 7 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 6 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to Database layer (models, ORM)

comment:6 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:7 Changed 4 years ago by ojii

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

comment:8 Changed 4 years ago by ojii

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

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

comment:10 Changed 4 years ago by ojii

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:11 Changed 4 years ago by ojii

  • Resolution set to invalid
  • Status changed from reopened to closed

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