Django

Code

Ticket #8076 (closed: fixed)

Opened 4 months ago

Last modified 3 months ago

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

Reported by: sanha <osanha@gmail.com> Assigned to: jan_oberst
Milestone: 1.0 Component: Database layer (models, ORM)
Version: SVN Keywords: 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

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

8076_get_prevnext_by_foo.txt (0.6 kB) - added by bjornkri on 08/09/08 03:10:45.
adds check for gt, lt
8076_get_prevnext_by_foo.diff (0.6 kB) - added by bjornkri on 08/09/08 03:11:47.
Save, with '.diff' extension for proper highlighting…
patch_for_getnext_with_subclasstests.diff (2.9 kB) - added by jan_oberst on 08/09/08 08:03:14.
Added tests for get_next/previous_FIELD / fixed by adding an additional check if the model is a subclass

Change History

08/08/08 14:53:16 changed by bjornkri

  • status changed from new to closed.
  • needs_better_patch changed.
  • resolution set to invalid.
  • needs_tests changed.
  • needs_docs changed.

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.

08/08/08 15:12:46 changed by bjornkri

  • status changed from closed to reopened.
  • stage changed from Unreviewed to Accepted.
  • resolution deleted.
  • milestone set to 1.0.

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

08/09/08 03:10:45 changed by bjornkri

  • attachment 8076_get_prevnext_by_foo.txt added.

adds check for gt, lt

08/09/08 03:11:47 changed by bjornkri

  • attachment 8076_get_prevnext_by_foo.diff added.

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

08/09/08 03:14:31 changed by bjornkri

  • owner changed from nobody to bjornkri.
  • needs_better_patch set to 1.
  • has_patch set to 1.
  • 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?

08/09/08 04:11:27 changed by bjornkri

  • owner changed from bjornkri to nobody.

08/09/08 05:18:32 changed by jan_oberst

  • owner changed from nobody to jan_oberst.

08/09/08 08:01:51 changed 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())

08/09/08 08:03:14 changed by jan_oberst

  • attachment patch_for_getnext_with_subclasstests.diff added.

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

08/31/08 15:47:30 changed by jacob

  • keywords set to 1.0-blocker.

09/01/08 02:30:12 changed 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?

09/01/08 16:04:06 changed by jacob

  • status changed from new to closed.
  • resolution set to fixed.

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


Add/Change #8076 (get_previous(next)_by_foo problem with parent's datetime field)




Change Properties
Action