Opened 7 years ago

Closed 3 years ago

#8291 closed Bug (fixed)

"pk" alias doesn't work for Meta option "ordering"

Reported by: peterd12 Owned by: dgouldin
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: Meta ordering pk alias
Cc: dgouldin@…, oliver@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Using the model below:

class MyModel(models.Model):
    part_number = models.CharField(max_length=128)

    class Meta:
        ordering = ['pk']

... yields the error message: "ordering" refers to "pk", a field that doesn't exist

Attachments (6)

8291.diff (660 bytes) - added by evan_schulz 7 years ago.
8291_2.diff (2.2 KB) - added by dgouldin 7 years ago.
8291_1.patch (1003 bytes) - added by gruszczy 5 years ago.
8291_2.patch (1.5 KB) - added by gruszczy 5 years ago.
8291_django_con11.diff (3.3 KB) - added by frog32 4 years ago.
8291_last_patch_evar.diff (3.6 KB) - added by dgouldin 3 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by mtredinnick

  • milestone set to 1.0 maybe
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

From memory, it was fiddly to make this work. I'll have another look at it at some point, but it's not really necessary. Just use "id" instead (or whatever you've named your primary key if there's an explicit key).

Changed 7 years ago by evan_schulz

comment:2 Changed 7 years ago by evan_schulz

Simply skipping validation of 'pk' values (see attached diff) seems like it works with some limited testing, though I'm not sure about all potential implications.

While it is certainly not necessary, it would be nice to be able to use the pk alias consistently.

comment:3 Changed 7 years ago by jacob

  • milestone changed from 1.0 maybe to post-1.0
  • Triage Stage changed from Design decision needed to Accepted

It would be nice, but not necessary.

Changed 7 years ago by dgouldin

comment:4 Changed 7 years ago by dgouldin

  • Has patch set

comment:5 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:6 Changed 6 years ago by dgouldin

  • Cc dgouldin@… added

comment:7 Changed 6 years ago by anonymous

  • Cc oliver@… added

comment:8 Changed 5 years ago by kmtracey

#13297 reported this again and has patches.

Changed 5 years ago by gruszczy

Changed 5 years ago by gruszczy

comment:9 Changed 5 years ago by gruszczy

I have added two patches from other ticket with different approaches and a simple test for this bug.

comment:10 Changed 4 years ago by julien

  • Component changed from Core framework to Database layer (models, ORM)

comment:11 Changed 4 years ago by julien

  • Easy pickings unset
  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Bug

dgouldin's patch seems like the most promising and the one with the most thorough tests. However, the tests would need to be rewritten using unittests since this is now Django's preferred way.

comment:12 Changed 4 years ago by anonymous

  • Owner changed from nobody to frog32
  • UI/UX unset

Changed 4 years ago by frog32

comment:13 Changed 4 years ago by frog32

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Unreviewed

i basicly picked the patches from evan_schulz and dgouldin, reordered something to not have code before the docstring and wrote a testcase for it.

comment:14 Changed 4 years ago by kmtracey

  • Triage Stage changed from Unreviewed to Accepted

Restoring Accepted state; triage state refers to the overall state of the ticket, not the state of the patch.

comment:15 Changed 4 years ago by isagalaev

  • Triage Stage changed from Accepted to Ready for checkin

Stumbled on this ticket while trying to fix validation errors in admin_views tests. The latest patch applies, fixes the problem and looks correct to me. Marking RFC.

comment:16 Changed 4 years ago by lukeplant

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

The tests pass without the patch to django/db/models/options.py, haven't investigated why.

It would also be better if the test was put in its own method, for easier testing.

comment:17 Changed 3 years ago by dgouldin

  • Owner changed from frog32 to dgouldin

It looks like the resolution of ordering fields now gets run through Query.setup_joins which builds a list of field aliases, including pk, to use in resolving to the actual model field. So, lukeplant, you're correct that the change to options.py is no longer necessary. Now to look at validation.py and see if there's a better general purpose solution than excluding "pk" from going through ordering validation ...

Last edited 3 years ago by dgouldin (previous) (diff)

comment:18 Changed 3 years ago by dgouldin

Ok looks like setup_joins includes code to specifically handle the pk case:

https://github.com/django/django/blob/3904f208b979e55356eefcd0291fb0d7fdfebe45/django/db/models/sql/query.py#L1284

So I feel fine excluding pk specifically from ordering validation in validation.py. I'll update the patch, removing changes to options.py.

Changed 3 years ago by dgouldin

comment:19 Changed 3 years ago by julien

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

In [17445]:

Fixed #8291 -- Allowed 'pk' to be used as an ordering option in Model.Meta. Thanks to peterd12 for the report and to evan_schulz, gruszczy, frog32 and David Gouldin for their work on the patch.

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