Opened 13 years ago

Last modified 11 years ago

#14705 new New feature

Model Field Order not influenced by MRO of superclasses

Reported by: Klaas van Schelven Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: lrekucki@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Looking at the two definitions below,

class OneTwo(AbstractModelOne, AbstractModelTwo):
    pass

class TwoOne(AbstractModelTwo, AbstractModelOne):
    pass

one (I?) would expect the order of the fields in the second definition to be the opposite of the order in the first one. (Since the base classes are referenced in the opposite order). This is, however, not the case.

Django only uses creation_counter to determine the order of the fields. Therefor the actual field order is determined by the order in which the containing abstract classes are evaluated (i.e. their python code is read). If these classes come from multiple sources (apps) the field order may end up to be pretty much random (though deterministic).

The general use case is heavy use of abstract models to mix in information from various sources into actual models. In real apps the two above models are not very likely, but being able to have control over the field order (other than placing the abstract models in a certain order) is important.
I make heavy use abstract models to be able to make my apps reusable. The above situation is an important roadblock to efficient use.

A patch with a failing test is attached.
Haven't yet been able to fix this in a non-intrusive way.
I would be very much obliged if someone can come up with a solution.

Attachments (4)

patch (1.8 KB ) - added by Klaas van Schelven 13 years ago.
failed_attempt (2.9 KB ) - added by Klaas van Schelven 13 years ago.
ticket14705.diff (5.2 KB ) - added by Łukasz Langa 13 years ago.
Required functionality (sans m2m fields) + tests; based on r15176
14705_model_fields_order.diff (5.4 KB ) - added by Julien Phalip 13 years ago.

Download all attachments as: .zip

Change History (20)

by Klaas van Schelven, 13 years ago

Attachment: patch added

by Klaas van Schelven, 13 years ago

Attachment: failed_attempt added

comment:1 by Klaas van Schelven, 13 years ago

The attached file "failed_attempt" shows an initial stab at the problem. Running the tests quickly shows that this indeed does not solve it (other tests regress); but this is where my understanding of the ORM ends for now.

comment:2 by Łukasz Rekucki, 13 years ago

Cc: lrekucki@… added

comment:3 by Łukasz Rekucki, 13 years ago

One thing that I don't understand is why does field order matter to you ? Maybe your problem can be solved easier on a different level.

in reply to:  3 comment:4 by Klaas van Schelven, 13 years ago

Replying to lrekucki:

One thing that I don't understand is why does field order matter to you ? Maybe your problem can be solved easier on a different level.

The field order is mainly important wherever I use ModelForms.
True, I can post-hoc fix everything there, but that doesn't seem very DRY.

comment:5 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedAccepted

by Łukasz Langa, 13 years ago

Attachment: ticket14705.diff added

Required functionality (sans m2m fields) + tests; based on r15176

comment:6 by anonymous, 13 years ago

Component: UncategorizedCore framework
Has patch: set

comment:7 by Łukasz Langa, 13 years ago

milestone: 1.3
Version: 1.2SVN

by Julien Phalip, 13 years ago

comment:8 by Julien Phalip, 13 years ago

Triage Stage: AcceptedReady for checkin

I've updated the patch so that it applies to trunk. I've also renamed the 'bases' attribute to 'model_bases' to avoid any confusion.

I'm RFC'ing this patch as I think it solves an annoying inconsistency. I've just got one little reservation: possibly this will be backwards incompatible for people who were relying on that inconsistency. Although I don't think this is a big deal. I'll leave the core devs to make a call on this.

comment:9 by Russell Keith-Magee, 13 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This patch appears to be headed broadly in the right direction, and it works, but I'm not convinced that it's quite right yet.

In particular, the fact that it is introducing a new model_bases attribute to the Meta class when parents already exists concerns me. Granted, the two aren't completely identical -- parents doesn't include any abstract classes, for example -- but it strikes me that the right solution here isn't just to add a new block of almost identical metadata, but to adjust the existing metadata, and adjust the existing usage.

The Meta class is in bad need of some housekeeping -- I'd rather not knowingly add to that housekeeping work.

comment:10 by Łukasz Langa, 13 years ago

Owner: changed from nobody to Russell Keith-Magee

The reason I introduced model_bases instead of rearranging half the framework around changes in parents was to keep the patch minimal (e.g. easier to get right). On top of that, I believe in backwards compatibility and I'm not sure if there isn't some 3rd party code using parents in ways that could have been broken by changing that.

I am willing to help with any refactoring of Meta necessary. However, this is naturally milestone 1.4, e.g. over 9 months away. Could you please reassess whether we could integrate the patch as it is? Without that change I will need to basically fork Django until another stable release. This is not easy to monkey patch.

comment:11 by Russell Keith-Magee, 13 years ago

Owner: changed from Russell Keith-Magee to nobody

If we were talking about a critical component of Django, I might agree to taking a "fix the problem, then fix the fix" approach. In fact, I did just that with #9964 only days ago. However, I don't think this problem falls into that category. Django has had model inheritance for a while, and this is the first time this problem has been reported. It requires a complex model inheritance structure and a reliance on automated form ordering before it becomes a problem.

I'm sorry that deferring this will be inconvenient to you, but I'd ask you to also consider *my* inconvenience -- the fact that landing a suboptimal bugfix *increases* my work as a framework maintainer in the long run, because at some point, I'll have to reconcile two disparate representations of base classes.

Two issues of protocol:

  • Backwards compatibility isn't strictly an issue here, because _meta isn't stable API. There are certain aspects of _meta that I'd be very pained to change (like the basic field management stuff that has been that way since the beginning), but I don't think parents falls into that list.
  • Please don't assign bugs to other people without their permission.

comment:12 by James Addison, 13 years ago

Severity: Normal
Type: New feature

comment:13 by James Addison, 13 years ago

milestone: 1.3

comment:14 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:15 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:16 by Aymeric Augustin, 11 years ago

Component: Core (Other)Database layer (models, ORM)
Note: See TracTickets for help on using tickets.
Back to Top