Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#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


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)

bug_19881.tar.gz (5.8 KB) - added by Jani Tiainen 4 years ago.
Testcase to demonstrate bug
get_next_using_manager.patch (1.5 KB) - added by nkryptic 4 years ago.
possible alternative that allows use of alternate manager

Download all attachments as: .zip

Change History (11)

Changed 4 years ago by Jani Tiainen

Attachment: bug_19881.tar.gz added

Testcase to demonstrate bug

comment:1 Changed 4 years ago by Carl Meyer

Component: Database layer (models, ORM)Documentation
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: get_next/previous_by_FOO doesn't work properly with multiple managersDocument that get_next/previous_by_FOO uses default manager
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/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 Changed 4 years ago by Carl Meyer

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.

Changed 4 years ago by nkryptic

possible alternative that allows use of alternate manager

comment:3 Changed 4 years ago by nkryptic

Cc: nkryptic 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 ='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 Changed 4 years ago by nkryptic

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 Changed 4 years ago by Carl Meyer

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 Changed 4 years ago by Aymeric Augustin

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 Changed 3 years ago by Baptiste Mispelon

Has patch: set

Pull request (for the documentation) here:

comment:8 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In ba610cb319d8882a663effcaf0a4e53c04593f98:

Fixed #19881 -- Documented that get_next/previous_by_FOO uses default manager.

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 9b0604e5149e20fc98f07256af60eac87421a346:

[1.5.x] Fixed #19881 -- Documented that get_next/previous_by_FOO uses default manager.

Backport of ba610cb319 from master

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