[Django] #29408: ordering by field from related model does not validate if field exists
Django
-----------------------------------------+------------------------
Reporter: Shadi Akiki | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
When the `ordering` class member in `Meta` of a model contains a field
from a related model, and that field does not exist, django's
`makemigrations` does not throw an error. However, if it is a direct field
member of the same class, `makemigrations` does throw an error.
Example below tested on Django 2.0.5
{{{
from django.db import models
# Create your models here.
class Agreement(models.Model):
agreement_id = models.AutoField(verbose_name='ID', serialize=False,
auto_created=True, primary_key=True)
#class Meta:
# generates error in makemigrations
# app.Agreement: (models.E015) 'ordering' refers to the nonexistent
field 'id'.
# ordering = ['id']
class Order(models.Model):
agreement = models.ForeignKey(Agreement, models.DO_NOTHING)
class Meta:
# does not generate error in makemigrations
# but does so during runtime
# e.g. [x for x in Order.objects.all()]
ordering = ['agreement__id']
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29408>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* component: Uncategorized => Database layer (models, ORM)
* version: 2.0 => master
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
I'm going to accept this provisionally. There's
[https://github.com/django/django/blob/2dcc5d629a6439b5547cdd6e67815cabf608fcd4/django/db/models/base.py#L1598-L1600
a `FIXME` in `models/base.py`] specifically about this:
{{{
# Skip ordering in the format field1__field2 (FIXME: checking
# this format would be nice, but it's a little fiddly).
fields = (f for f in fields if LOOKUP_SEP not in f)
}}}
Added in
[https://github.com/django/django/commit/d818e0c9b2b88276cc499974f9eee893170bf0a8
d818e0c9b2b88276cc499974f9eee893170bf0a8].
Either we should address this, or remove the comment and close as
`wontfix` if "fiddly" turns out to be more effort than it's worth.
A test case and a patch showing what "fiddly" actually entails would be
great.
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:1>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Windson yang):
I think we can just address this in the document and don't fix it.
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:2>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jeff):
* cc: Jeff (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* status: new => assigned
* owner: nobody => Hasan Ramezani
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Hasan Ramezani):
patch updated and new method added.
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Hasan Ramezani):
Any updates? if there is something to change please inform me. I am ready.
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
Left comments on PR: patch would need to handle JSON paths, which should
be valid in `ordering` since #24747. (Similar issue arises in #29622.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Hasan Ramezani):
PR updated. `JSON paths` handled and some tests added for it
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
Patch is looking good. There's just a few minor comments on the PR, but
when those are addressed it should be ready to move forwards.
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* needs_better_patch: 1 => 0
Comment:
Requested changes are done
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"440505cb2cadbe1a5b9fba246bcde6c04f51d07e" 440505cb]:
{{{
#!CommitTicketReference repository=""
revision="440505cb2cadbe1a5b9fba246bcde6c04f51d07e"
Fixed #29408 -- Added validation of related fields and lookups in model
Meta.ordering.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Shadi Akiki | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"f69c7bbdceba43789bfba935dd8fa3daaa7f36c4" f69c7bbd]:
{{{
#!CommitTicketReference repository=""
revision="f69c7bbdceba43789bfba935dd8fa3daaa7f36c4"
Refs #29408 -- Cosmetic edits for validation of related fields and lookups
in model Meta.ordering.
Follow up to 440505cb2cadbe1a5b9fba246bcde6c04f51d07e.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29408#comment:17>