#19881 closed Cleanup/optimization (fixed)
Document that get_next/previous_by_FOO uses default manager
Reported by: | Jani Tiainen | Owned by: | nobody |
---|---|---|---|
Component: | Documentation | Version: | 1.4 |
Severity: | Normal | Keywords: | |
Cc: | nkryptic | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Model methods get_next_by_FOO() and get_previous_by_FOO() doesn't honor original manager used but uses always default manager (first manager declared on a model).
This might potentially return incorrect data, for example in cases where softdelete is used. Default manager would return all objects but normally user uses manager that returns only non-deleted objects. In that case get_next|pervious_by_FOO would return incorrect data.
Possible workaround would be using proxy models.
Attachments (2)
Change History (11)
by , 12 years ago
Attachment: | bug_19881.tar.gz added |
---|
comment:1 by , 12 years ago
Component: | Database layer (models, ORM) → Documentation |
---|---|
Summary: | get_next/previous_by_FOO doesn't work properly with multiple managers → Document that get_next/previous_by_FOO uses default manager |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
Model instances don't have a memory of how they were queried for, and the only way to "fix" this would be to add such a memory. I think the downsides of adding that additional state to model instances far outweighs the benefit of fixing this behavior; it would require much stronger coupling between both managers and the querysets they return, and querysets and the instances they query for. get_next
and get_previous
operate with respect to the entire set of model instances of that type, not some arbitrarily narrowed set. If you need them to operate with respect to a narrowed set, you can pass additional filter arguments to them.
It would be worth documenting this more clearly, however; accepting and changing the component to documentation.
comment:2 by , 12 years ago
Also, thanks for the report and for providing a demonstration test case! I would have found it much harder to understand the report without the testcase. In future, though, it's easiest to work with a test case if provided as a patch file against Django's own test suite (master branch) rather than as a zipped project.
by , 12 years ago
Attachment: | get_next_using_manager.patch added |
---|
possible alternative that allows use of alternate manager
comment:3 by , 12 years ago
Cc: | added |
---|
I added a patch which provides an alternative implementation for _get_next_or_previous_by_FIELD
. It simply checks for a kwarg
of using_manager
, which the method will attempt to use instead, but defaults to using the _default_manager
. The default behavior wouldn't change, though. So, using the example from the initial test case from jtiai, to get the behavior he wants would look like:
man1 = Person.men.get(last_name='SquarePants') man2 = man1.get_previous_by_birthday(using_manager='men')
While the manager in the test case is fairly simple and you could repeat the filtering it performs when calling get_previous_by_birthday, given more complicated managers it starts to get ugly.
This is definitely just a proof of concept, at this point. If anyone wants this pursued further, I'd be happy to create a pull request for the full implementation with tests.
comment:4 by , 12 years ago
Another possibility I discussed with jtiai, which could be included in the updated documentation, is to use a Proxy model with it's default manager set to the expected manager. Then, if using the proxy model for the initial query, the instance calling get_previous_by_birthday
would pass the test.
comment:5 by , 12 years ago
I don't think this is worth adding another keyword argument to these functions. It's a bit of a smell to have "special" kwarg names for a function that otherwise interprets all its kwargs as field filters. And the new argument doesn't fully solve the problem as phrased in the ticket; you'd still get the "wrong" filtering initially and have to go digging to figure out a) what's happening and b) the name of the keyword arg you need to use to work around it. If people are going to need to do that digging anyway, I think the manual-filtering and proxy-model workarounds are good enough, and both work today.
So I still think this should just be documented, with mention of both filtering manually and using a proxy model.
comment:6 by , 12 years ago
I agree with carljm that the only reasonable thing to do given the existing API is to document this behavior.
To solve the problem, we'd have to introduce a new API hooked on QuerySet and not Model. I'm thinking of something like:
QuerySet.get_next(instance, ('-date', 'name')) QuerySet.get_previous(instance, ('-date', 'name'))
comment:7 by , 11 years ago
Has patch: | set |
---|
Pull request (for the documentation) here: https://github.com/django/django/pull/1290.
comment:8 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Testcase to demonstrate bug