Opened 7 years ago

Closed 4 years ago

#7566 closed (fixed)

dumpdata command should not use model._default_manager.all()

Reported by: anonymous Owned by: nobody
Component: Core (Serialization) Version: master
Severity: Keywords: dumpdata
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

dumpdata command should not use model._default_manager.all() for retrieving objects because model can use custom Manager with redefined .all() method.
better way for retrieving all objects is using QuerySet(model).all()

Attachments (2)

dumpdata-fix.patch (911 bytes) - added by anonymous 7 years ago.
6155.diff (1.5 KB) - added by Karen Tracey <kmtracey@…> 7 years ago.
Patch that updates doc as well

Download all attachments as: .zip

Change History (7)

Changed 7 years ago by anonymous

Changed 7 years ago by Karen Tracey <kmtracey@…>

Patch that updates doc as well

comment:1 Changed 7 years ago by Karen Tracey <kmtracey@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Seems like a good idea, since I'd expect you'd normally want to dump everything even if the default manager excludes some records. I was unaware of that syntax for QuerySet creation when the fix for #6155 went in. But if this change is made then the doc note about the default manager being used needs to be deleted, since it won't be true any more. I updated the patch to do that as well.

comment:2 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Accepted to Design decision needed

I'm -1 on this change. If you have installed a default manager it's because they're the records you consider the "whole set" by default. And that's what dumpdata works with. If you wanted the whole set to be "everything", you wouldn't have overridden the default manager.

comment:3 follow-up: Changed 7 years ago by Karen Tracey <kmtracey@…>

Eh, for my case I don't think of what the default manager returns as the "whole set". It's more the publicly-visible set. There's more in the database (which existed long before I put a Django front-end on it) than I care to expose via the website front-end, and I use excluding default managers to limit what can be seen in the normal case via the web site. That said, I've never actually used dumpdata (I backup/migrate/move the DB with database utilities directly) so I'm not particularly qualified to comment on how it should behave. But if I did need to use it, I think I'd want it to dump all the data.

comment:4 in reply to: ↑ 3 Changed 7 years ago by anonymous

Replying to Karen Tracey <kmtracey@gmail.com>:

Eh, for my case I don't think of what the default manager returns as the "whole set". It's more the publicly-visible set. There's more in the database (which existed long before I put a Django front-end on it) than I care to expose via the website front-end, and I use excluding default managers to limit what can be seen in the normal case via the web site. That said, I've never actually used dumpdata (I backup/migrate/move the DB with database utilities directly) so I'm not particularly qualified to comment on how it should behave. But if I did need to use it, I think I'd want it to dump all the data.

I agree with Karen Tracey. I use custom manager for creating versions of object, they makes visible only last version. If i execute dumpdata, i expect that all objects will be saved. Creating new method for filtering objects eliminates the need for a possible replace of the Manager, because that need to change all of the related sources.

comment:5 Changed 4 years ago by ramiro

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

This was fixed in r13669 with a Solomonic decision.

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