Opened 16 years ago

Closed 13 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: dev
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 16 years ago.
8291_2.diff (2.2 KB ) - added by David Gouldin 16 years ago.
8291_1.patch (1003 bytes ) - added by Filip Gruszczyński 14 years ago.
8291_2.patch (1.5 KB ) - added by Filip Gruszczyński 14 years ago.
8291_django_con11.diff (3.3 KB ) - added by Marc Egli 13 years ago.
8291_last_patch_evar.diff (3.6 KB ) - added by David Gouldin 13 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 by Malcolm Tredinnick, 16 years ago

milestone: 1.0 maybe
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).

by evan_schulz, 16 years ago

Attachment: 8291.diff added

comment:2 by evan_schulz, 16 years ago

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 by Jacob, 16 years ago

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

It would be nice, but not necessary.

by David Gouldin, 16 years ago

Attachment: 8291_2.diff added

comment:4 by David Gouldin, 16 years ago

Has patch: set

comment:5 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:6 by David Gouldin, 16 years ago

Cc: dgouldin@… added

comment:7 by anonymous, 16 years ago

Cc: oliver@… added

comment:8 by Karen Tracey, 14 years ago

#13297 reported this again and has patches.

by Filip Gruszczyński, 14 years ago

Attachment: 8291_1.patch added

by Filip Gruszczyński, 14 years ago

Attachment: 8291_2.patch added

comment:9 by Filip Gruszczyński, 14 years ago

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

comment:10 by Julien Phalip, 14 years ago

Component: Core frameworkDatabase layer (models, ORM)

comment:11 by Julien Phalip, 14 years ago

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 by anonymous, 13 years ago

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

by Marc Egli, 13 years ago

Attachment: 8291_django_con11.diff added

comment:13 by Marc Egli, 13 years ago

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

Triage Stage: UnreviewedAccepted

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

comment:15 by Ivan Sagalaev, 13 years ago

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 by Luke Plant, 13 years ago

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

Owner: changed from Marc Egli to David Gouldin

It looks like the resolution of ordering fields now gets run through Query.setup_joins which uses get_field_by_name by resolve the pk alias to the actual primary key 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 ...

Version 0, edited 13 years ago by David Gouldin (next)

comment:18 by David Gouldin, 13 years ago

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.

by David Gouldin, 13 years ago

Attachment: 8291_last_patch_evar.diff added

comment:19 by Julien Phalip, 13 years ago

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