Opened 5 years ago

Last modified 2 years ago

#14705 new New feature

Model Field Order not influenced by MRO of superclasses

Reported by: vanschelven Owned by: nobody
Component: Database layer (models, ORM) Version: master
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 vanschelven 5 years ago.
failed_attempt (2.9 KB) - added by vanschelven 5 years ago.
ticket14705.diff (5.2 KB) - added by ambv 5 years ago.
Required functionality (sans m2m fields) + tests; based on r15176
14705_model_fields_order.diff (5.4 KB) - added by julien 4 years ago.

Download all attachments as: .zip

Change History (20)

Changed 5 years ago by vanschelven

Changed 5 years ago by vanschelven

comment:1 Changed 5 years ago by vanschelven

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 5 years ago by lrekucki

  • Cc lrekucki@… added

comment:3 follow-up: Changed 5 years ago by 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.

comment:4 in reply to: ↑ 3 Changed 5 years ago by vanschelven

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 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by ambv

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

comment:6 Changed 5 years ago by anonymous

  • Component changed from Uncategorized to Core framework
  • Has patch set

comment:7 Changed 5 years ago by ambv

  • milestone set to 1.3
  • Version changed from 1.2 to SVN

Changed 4 years ago by julien

comment:8 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to Ready 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 Changed 4 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 4 years ago by ambv

  • Owner changed from nobody to russellm

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 Changed 4 years ago by russellm

  • Owner changed from russellm 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 Changed 4 years ago by jaddison

  • Severity set to Normal
  • Type set to New feature

comment:13 Changed 4 years ago by jaddison

  • milestone 1.3 deleted

comment:14 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:15 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:16 Changed 2 years ago by aaugustin

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