Opened 7 years ago

Closed 7 years ago

Last modified 3 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: master
Severity: Keywords: 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

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 bjornkri 7 years ago.
adds check for gt, lt
8076_get_prevnext_by_foo.diff (582 bytes) - added by bjornkri 7 years ago.
Save, with '.diff' extension for proper highlighting…
patch_for_getnext_with_subclasstests.diff (2.9 KB) - added by jan_oberst 7 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 Changed 7 years ago by bjornkri

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 Changed 7 years ago by bjornkri

  • milestone set to 1.0
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 7 years ago by bjornkri

adds check for gt, lt

Changed 7 years ago by bjornkri

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

comment:3 Changed 7 years ago by bjornkri

  • Has patch set
  • Owner changed from nobody to bjornkri
  • Patch needs improvement set
  • Status changed from reopened to new

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 Changed 7 years ago by bjornkri

  • Owner changed from bjornkri to nobody

comment:5 Changed 7 years ago by jan_oberst

  • Owner changed from nobody to jan_oberst

comment:6 Changed 7 years ago by jan_oberst

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())

Changed 7 years ago by jan_oberst

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

comment:7 Changed 7 years ago by jacob

  • Keywords 1.0-blocker added

comment:8 Changed 7 years ago by bjornkri

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 Changed 7 years ago by jacob

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

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

comment:10 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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