Opened 5 months ago

Last modified 13 days ago

#29408 assigned Cleanup/optimization

Add validation of related fields in model Meta.ordering

Reported by: Shadi Akiki Owned by: Hasan Ramezani
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Jeff Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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']

Change History (9)

comment:1 Changed 5 months ago by Carlton Gibson

Component: UncategorizedDatabase layer (models, ORM)
Summary: ordering by field from related model does not validate if field existsAdd validation of ordering by a field from related model.
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 2.0master

I'm going to accept this provisionally. There's 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 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.

comment:2 Changed 5 months ago by Windson yang

I think we can just address this in the document and don't fix it.

comment:3 Changed 4 months ago by Jeff

Cc: Jeff added

comment:4 Changed 2 months ago by Hasan Ramezani

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: newassigned
Last edited 2 months ago by Tim Graham (previous) (diff)

comment:5 Changed 2 months ago by Tim Graham

Patch needs improvement: set
Summary: Add validation of ordering by a field from related model.Add validation of related fields in model Meta.ordering

comment:6 Changed 7 weeks ago by Hasan Ramezani

patch updated and new method added.

comment:7 Changed 7 weeks ago by Hasan Ramezani

Patch needs improvement: unset

comment:8 Changed 2 weeks ago by Hasan Ramezani

Any updates? if there is something to change please inform me. I am ready.

comment:9 Changed 13 days ago by Carlton Gibson

Patch needs improvement: set

Left comments on PR: patch would need to handle JSON paths, which should be valid in ordering since #24747. (Similar issue arises in #29622.)

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