Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#8076 closed (fixed)

get_previous(next)_by_foo problem with parent's datetime field

Reported by: sanha <osanha@…> Owned by: jan_oberst
Component: Database layer (models, ORM) Version: dev
Severity: Keywords: 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

model_a has datetime field as t

model_b inherited model_a

when calling modelb.get_next(or previous)_by_t, it was failed.

Attachments (3)

8076_get_prevnext_by_foo.txt (582 bytes ) - added by Bjorn Kristinsson 16 years ago.
adds check for gt, lt
8076_get_prevnext_by_foo.diff (582 bytes ) - added by Bjorn Kristinsson 16 years ago.
Save, with '.diff' extension for proper highlighting…
patch_for_getnext_with_subclasstests.diff (2.9 KB ) - added by jan_oberst 16 years ago.
Added tests for get_next/previous_FIELD / fixed by adding an additional check if the model is a subclass

Download all attachments as: .zip

Change History (13)

comment:1 by Bjorn Kristinsson, 16 years ago

Resolution: invalid
Status: newclosed

get_next_by_FOO only works for a field FOO on the current model.

In your example:
If Model_B has a ForeignKey relation to Model_A, do a get_next_by_t on Model_A, and then loop through Model_A's model_b_set. That way you'll get all the Model_Bs ordered by Model_A's datetime.

comment:2 by Bjorn Kristinsson, 16 years ago

milestone: 1.0
Resolution: invalid
Status: closedreopened
Triage Stage: UnreviewedAccepted

My mistake, this is a valid bug :) Or :( depending on how you look at it.

by Bjorn Kristinsson, 16 years ago

adds check for gt, lt

by Bjorn Kristinsson, 16 years ago

Save, with '.diff' extension for proper highlighting...

comment:3 by Bjorn Kristinsson, 16 years ago

Has patch: set
Owner: changed from nobody to Bjorn Kristinsson
Patch needs improvement: set
Status: reopenednew

Turns out fields/related.py only checks for 'exact', 'in' or 'isnull' lookup_types, but returns a type error otherwise. get_previous_by_FOO and get_next_by_FOO have 'gt' and 'lt' lookup_types respectively.

Leaving this as 'Patch needs improvement' as it only addresses this specific examples; are there other lookup_types that might be sent to related?

comment:4 by Bjorn Kristinsson, 16 years ago

Owner: changed from Bjorn Kristinsson to nobody

comment:5 by jan_oberst, 16 years ago

Owner: changed from nobody to jan_oberst

comment:6 by jan_oberst, 16 years ago

My hackish little patch doesn't solve the actual issue here which lies in primary key lookups on child models.

Django tries to get the first other object with a date that's greater than the current object's date. If there is none it tries to get the object with the same date but highest ID. That's why it calls pk_gt. Usually this just sorts objects of the called object's class by primary key. On inheriting classes it will try to order the related field (the pointer to the superclass). Since related fields don't like to be ordered it fails.

#3246 adds the possibility to do lookups on a RelatedField by passing an object instead of primary key. It also limits lookups on RelatedField to rather 'exact' ID queries. I guess it tries to distinguish from the different querytypes ('exact' and 'in') if it needs to get PKs from more than one object. If there's more than one the pk_trace function is called on every passed object. The resulting list of primary keys is then used to get the objects with the WHERE IN statement.

When called with lt/gt we only have one object (or primary key), so we do not have to call the pk_trace more than once. I don't see why queries for 'lt','gt' shouldn't work.

On the other hand querying on primary keys of related fields isn't really straightforward. The API could behave in a way one wouldn't necessarily expect. Here's just one arbitrary example for this:

# Return all publications that have articles with IDs smaller than 3
Publications.objects.filter(article__id__lt=3)

# This is prohibited by the 'Related Field has invalid lookup' block, but would do exactly the same:
Publications.objects.filter(article__lt=3)

# Now let's have the Article class ordered via model Meta:
class Meta: ordering = ('-pub_date')

# Now one might think that the following two statements do the same:
Publications.objects.filter(article__lt=datetime.now())
Publications.objects.filter(article__pub_date__lt=datetime.now())

by jan_oberst, 16 years ago

Added tests for get_next/previous_FIELD / fixed by adding an additional check if the model is a subclass

comment:7 by Jacob, 16 years ago

Keywords: 1.0-blocker added

comment:8 by Bjorn Kristinsson, 16 years ago

I don't think it's clear what the exact problem is here. My patch fixes the specific issue (as far as my testing goes), but Jan's goes a completely different route.

I can't really follow what his does, but I'm assuming mine fixes a symptom while he's aiming at the cause.

Maybe I just don't get it (actually, I don't), but perhaps a clearer bug description would be helpful to move this one forward?

comment:9 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed

(In [8814]) Fixed #8076: fixed get_(next/previous)_by_date when used with subclasses. Thanks, bjornkri and jan_oberst.

comment:10 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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