Code

Opened 7 years ago

Closed 3 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@…> 7 years ago.

Download all attachments as: .zip

Change History (12)

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

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

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

comment:6 Changed 3 years ago by gabrielhurley

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

comment:7 Changed 3 years ago by ojii

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

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

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

comment:10 Changed 3 years ago by ojii

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:11 Changed 3 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.