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)
Change History (25)
comment:1 by , 16 years ago
milestone: | → 1.0 maybe |
---|---|
Triage Stage: | Unreviewed → Design decision needed |
by , 16 years ago
comment:2 by , 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 , 16 years ago
milestone: | 1.0 maybe → post-1.0 |
---|---|
Triage Stage: | Design decision needed → Accepted |
It would be nice, but not necessary.
by , 16 years ago
Attachment: | 8291_2.diff added |
---|
comment:4 by , 16 years ago
Has patch: | set |
---|
comment:6 by , 16 years ago
Cc: | added |
---|
comment:7 by , 16 years ago
Cc: | added |
---|
by , 14 years ago
Attachment: | 8291_1.patch added |
---|
by , 14 years ago
Attachment: | 8291_2.patch added |
---|
comment:9 by , 14 years ago
I have added two patches from other ticket with different approaches and a simple test for this bug.
comment:10 by , 14 years ago
Component: | Core framework → Database layer (models, ORM) |
---|
comment:11 by , 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 , 13 years ago
Owner: | changed from | to
---|---|
UI/UX: | unset |
by , 13 years ago
Attachment: | 8291_django_con11.diff added |
---|
comment:13 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → 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 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Restoring Accepted state; triage state refers to the overall state of the ticket, not the state of the patch.
comment:15 by , 13 years ago
Triage Stage: | Accepted → 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 by , 13 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → 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 by , 13 years ago
Owner: | changed from | to
---|
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 ...
comment:18 by , 13 years ago
Ok looks like setup_joins includes code to specifically handle the pk case:
So I feel fine excluding pk specifically from ordering validation in validation.py. I'll update the patch, removing changes to options.py.
by , 13 years ago
Attachment: | 8291_last_patch_evar.diff added |
---|
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).