Opened 8 years ago

Closed 5 years ago

#8291 closed Bug (fixed)

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

Reported by: peterd12 Owned by: David Gouldin
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 8 years ago.
8291_2.diff (2.2 KB) - added by David Gouldin 8 years ago.
8291_1.patch (1003 bytes) - added by Filip Gruszczyński 6 years ago.
8291_2.patch (1.5 KB) - added by Filip Gruszczyński 6 years ago.
8291_django_con11.diff (3.3 KB) - added by Marc Egli 5 years ago.
8291_last_patch_evar.diff (3.6 KB) - added by David Gouldin 5 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 8 years ago by Malcolm Tredinnick

milestone: 1.0 maybe
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign 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 8 years ago by evan_schulz

Attachment: 8291.diff added

comment:2 Changed 8 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 8 years ago by Jacob

milestone: 1.0 maybepost-1.0
Triage Stage: Design decision neededAccepted

It would be nice, but not necessary.

Changed 8 years ago by David Gouldin

Attachment: 8291_2.diff added

comment:4 Changed 8 years ago by David Gouldin

Has patch: set

comment:5 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:6 Changed 8 years ago by David Gouldin

Cc: dgouldin@… added

comment:7 Changed 8 years ago by anonymous

Cc: oliver@… added

comment:8 Changed 6 years ago by Karen Tracey

#13297 reported this again and has patches.

Changed 6 years ago by Filip Gruszczyński

Attachment: 8291_1.patch added

Changed 6 years ago by Filip Gruszczyński

Attachment: 8291_2.patch added

comment:9 Changed 6 years ago by Filip Gruszczyński

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

comment:10 Changed 6 years ago by Julien Phalip

Component: Core frameworkDatabase layer (models, ORM)

comment:11 Changed 5 years ago by Julien Phalip

Easy pickings: unset
Patch needs improvement: set
Severity: Normal
Type: 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 5 years ago by anonymous

Owner: changed from nobody to Marc Egli
UI/UX: unset

Changed 5 years ago by Marc Egli

Attachment: 8291_django_con11.diff added

comment:13 Changed 5 years ago by Marc Egli

Patch needs improvement: unset
Triage Stage: AcceptedUnreviewed

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 5 years ago by Karen Tracey

Triage Stage: UnreviewedAccepted

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

comment:15 Changed 5 years ago by Ivan Sagalaev

Triage Stage: AcceptedReady 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 5 years ago by Luke Plant

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 5 years ago by David Gouldin

Owner: changed from Marc Egli to David Gouldin

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 5 years ago by David Gouldin (previous) (diff)

comment:18 Changed 5 years ago by David Gouldin

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 5 years ago by David Gouldin

Attachment: 8291_last_patch_evar.diff added

comment:19 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: newclosed

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