Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7447 closed (fixed)

QuerySet.update() bypasses custom Model.save() methods

Reported by: emulbreh Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: qsrf-cleanup
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This is symmetrical to #6915 (and therefore should be handled simmilar). Options:

  • iterate the queryset, update and save() each instance individually. Because this would also work for sliced querysets #7298 would deserve a better fix then.
  • assert not self.has_custom_save(), "Cannot update a query for a model with a custom save() method.". This will probably break ManyRelated managers.
  • only add a big warning to QuerySet.update() and Model.save() doc. This doesn't help at all to solve the issue.

+1 for the first option.

Attachments (0)

Change History (9)

comment:1 Changed 6 years ago by emulbreh

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Basically the same issue: How will QuerySet.update() work with model validation (#6845)?

comment:2 Changed 6 years ago by emulbreh

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 6 years ago by jacob

  • milestone set to 1.0

comment:4 Changed 6 years ago by anonymous

If update() is changed to iterate over every model, it's going to be much, much slower for querysets with thousands or more records.

Is there a way to check for a custom save method in the model, and only iterate if a custom save exists?

comment:5 Changed 6 years ago by anonymous

Alternatively, add a new method, similar to update (let's call it "queryset.save_data()" for example), and guarantee that that iterates no matter what, and leave the update() method alone.

Then, clearly document the fact that update() bypasses custom saves and is fast, save_data() uses custom saves but is slow.

comment:6 Changed 6 years ago by emulbreh

It should be possible to detect a custom update() method. I attempted to the same for delete() in my patch for #6915. But it's pretty much untested and therefore probably broken ..

comment:7 Changed 6 years ago by mtredinnick

Changing the current behaviour isn't going to happen. The update() method is intended to be a bulk SQL operation. If you want to call the save() method on every item in the queryset, loop over the queryset. It's two lines of code. I will commit an update to the documentation for update() to mention that it's a direct SQL operation.

(This isn't quite the same as delete() for internal implementation reasons, so #6915 is still open for that reason.)

comment:8 Changed 6 years ago by mtredinnick

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

(In [7884]) Documented that the update() method on querysets is a direct SQL call, not the
same as looping over the queryset and calling save() on each item (which is
less efficient). Fixed #7447.

comment:9 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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.