Opened 11 years ago

Closed 11 years ago

Last modified 11 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

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)

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

Download all attachments as: .zip

Change History (11)

by Jani Tiainen, 11 years ago

Attachment: bug_19881.tar.gz added

Testcase to demonstrate bug

comment:1 by Carl Meyer, 11 years ago

Component: Database layer (models, ORM)Documentation
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 by Carl Meyer, 11 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 nkryptic, 11 years ago

possible alternative that allows use of alternate manager

comment:3 by nkryptic, 11 years ago

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 = 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 nkryptic, 11 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 Carl Meyer, 11 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 Aymeric Augustin, 11 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 Baptiste Mispelon, 11 years ago

Has patch: set

Pull request (for the documentation) here: https://github.com/django/django/pull/1290.

comment:8 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In ba610cb319d8882a663effcaf0a4e53c04593f98:

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

comment:9 by Tim Graham <timograham@…>, 11 years ago

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