Forbid field shadowing in model multi-inheritance
|Reported by:||Anssi Kääriäinen||Owned by:||nobody|
|Component:||Database layer (models, ORM)||Version:||1.3|
|Cc:||Anssi Kääriäinen, krzysiumed@…, chrismedrela||Triage Stage:||Accepted|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
I was working on ticket #13803 and got a little carried away. I ended up forbidding field shadowing in model multi-inheritance cases. The attached patch also includes checks for field.attname clashing with field.name, disallowing 'pk' as a field name (what #13803 is about) and
__ in field names.
Except for the multi-inheritance the validations should be easily seen as necessary ones.
Now, why forbid field shadowing for multi-inheritance? The basic problem is that in the DB you can have different values for the similarly named fields, but in Python you can have just a single value. The most trivial example is this:
class A: f = models.IntegerField() class B: f = models.DateField() class C(A, B) pass
In this case it is obvious that things will not work as expected as you simply can't save the C instance's value for f as both date and integer.
The above example has one even more serious problem: there is a clash between the id fields of A and B. This can lead to pretty confusing situations. The most obvious one is that both A and B get their id value from the sequence when saving a new model. However A can get 10 as the last value, B 11. Now, you have a C model with a value of both 10 and 11 for the id. Save it and suddenly you will duplicate, or even overwrite an object from either B or C model. If 10 is in effect, you will overwrite B with id 10, if 11 is in effect you will create a new A with value 11. In the last case the sequence will be out of sync, and next time you save A you will get a
I am not exactly sure if the above is what happens. Other possibility is that A's value will be used as B's id value on the first save already. In this case things work like: Save C -> Save A (gets 10 as id) -> Save B (sees 10 as id) -> overwrite of B instance of id value 10.
Thus, field shadowing should be forbidden. This will be backwards incompatible (there is even one model having this problem in the test suite). But there is little hope of fixing this without making the code a lot more complex. I think this case can be classified as a data corrupting bug (due to the id problem), and thus backwards incompatibility is not in effect. The backwards compatibility is solvable by changing the field names to non-clashing ones in one of the parent models. This is a problem only in multi-inheritance cases, which should not be too commonly used.
I think I have seen some tickets where the real underlying problem has been model-inheritance with field shadowing. However, I can't find them now. I don't think this is anything too critical, but this would be a nice thing to fix. Things will seem to work at first, but then later on you will get weird data overwrite problems which can be pretty hard to debug.
Change History (24)
comment:2 Changed 5 years ago by
|Patch needs improvement:||set|
|Triage Stage:||Unreviewed → Accepted|
comment:5 Changed 5 years ago by
|Patch needs improvement:||unset|