Code

Opened 14 months ago

Closed 10 months ago

Last modified 10 months ago

#19881 closed Cleanup/optimization (fixed)

Document that get_next/previous_by_FOO uses default manager

Reported by: jtiai 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 jtiai 14 months ago.
Testcase to demonstrate bug
get_next_using_manager.patch (1.5 KB) - added by nkryptic 14 months ago.
possible alternative that allows use of alternate manager

Download all attachments as: .zip

Change History (11)

Changed 14 months ago by jtiai

Testcase to demonstrate bug

comment:1 Changed 14 months ago by carljm

  • Component changed from Database layer (models, ORM) to Documentation
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from get_next/previous_by_FOO doesn't work properly with multiple managers to Document that get_next/previous_by_FOO uses default manager
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to 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 Changed 14 months ago by carljm

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 14 months ago by nkryptic

possible alternative that allows use of alternate manager

comment:3 Changed 14 months 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 = 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 Changed 14 months 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 14 months ago by carljm

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 14 months ago by aaugustin

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 10 months ago by bmispelon

  • Has patch set

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

comment:8 Changed 10 months ago by Tim Graham <timograham@…>

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

In ba610cb319d8882a663effcaf0a4e53c04593f98:

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

comment:9 Changed 10 months 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

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.