Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#29408 closed Cleanup/optimization (fixed)

Add validation of related fields and lookups in model Meta.ordering

Reported by: Shadi Akiki Owned by: Hasan Ramezani
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Jeff Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (17)

comment:1 by Carlton Gibson, 8 years ago

Component: Uncategorized → Database layer (models, ORM)
Summary: ordering by field from related model does not validate if field exists → Add validation of ordering by a field from related model.
Triage Stage: Unreviewed → Accepted
Type: Uncategorized → Cleanup/optimization
Version: 2.0 → master

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 by Windson yang, 8 years ago

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

comment:3 by Jeff, 8 years ago

Cc: Jeff added

comment:4 by Hasan Ramezani, 8 years ago

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: new → assigned
Last edited 8 years ago by Tim Graham (previous) (diff)

comment:5 by Tim Graham, 8 years ago

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 by Hasan Ramezani, 8 years ago

patch updated and new method added.

comment:7 by Hasan Ramezani, 8 years ago

Patch needs improvement: unset

comment:8 by Hasan Ramezani, 8 years ago

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

comment:9 by Carlton Gibson, 8 years ago

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.)

comment:10 by Hasan Ramezani, 8 years ago

PR updated. JSON paths handled and some tests added for it

comment:11 by Hasan Ramezani, 8 years ago

Patch needs improvement: unset

comment:12 by Carlton Gibson, 8 years ago

Patch needs improvement: set

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.

comment:13 by Hasan Ramezani, 8 years ago

Patch needs improvement: unset

Requested changes are done

comment:14 by Carlton Gibson, 8 years ago

Triage Stage: Accepted → Ready for checkin

comment:15 by Mariusz Felisiak, 8 years ago

Summary: Add validation of related fields in model Meta.ordering → Add validation of related fields and lookups in model Meta.ordering

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 8 years ago

Resolution: → fixed
Status: assigned → closed

In 440505cb:

Fixed #29408 -- Added validation of related fields and lookups in model Meta.ordering.

comment:17 by GitHub <noreply@…>, 8 years ago

In f69c7bbd:

Refs #29408 -- Cosmetic edits for validation of related fields and lookups in model Meta.ordering.

Follow up to 440505cb2cadbe1a5b9fba246bcde6c04f51d07e.

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