[Django] #16176: Overwriting a property with field during model inheritance.
Django
----------------------------+----------------------------------------------
Reporter: | Owner: nobody
czepiel.artur@… | Status: new
Type: Bug | Component: Database layer (models, ORM)
Milestone: | Severity: Normal
Version: SVN | Triage Stage: Unreviewed
Keywords: | Easy pickings: 0
Has patch: 0 |
UI/UX: 0 |
----------------------------+----------------------------------------------
Documentation says (in
https://docs.djangoproject.com/en/1.3/topics/db/models/#field-name-hiding-
is-not-permitted paragraph) that:
This restriction only applies to attributes which are Field instances.
Normal Python attributes can be overridden if you wish. It also only
applies to the name of the attribute as Python sees it: if you are
manually specifying the database column name, you can have the same column
name appearing in both a child and an ancestor model for multi-table
inheritance (they are columns in two different database tables).
However.. I came up today with setup like this:
{{{
1 from django.db import models
2
3 # Create your models here.
4
5 class SomeTestModel(models.Model):
6 some_field = models.CharField(max_length=100)
7
8 class Meta:
9 abstract = True
10
11 @property
12 def other_field(self):
13 return "[OTHER] %s" % self.some_field
14
15
16
17 class OtherModel(SomeTestModel):
18 other_field = models.CharField(max_length=100)
19
20
21 class AndMoreOther(SomeTestModel):
22 not_important_field = models.CharField(max_length=100)
}}}
And then if you do:
{{{
>>> from testapp.models import *
>>> o = OtherModel()
Traceback (most recent call last):
File "<console>", line 1, in <module>
File
"/home/arturstudio/PROJEKTY/tempdjango/inh/src/django/django/db/models/base.py",
line 357, in __init__
setattr(self, field.attname, val)
AttributeError: can't set attribute
}}}
Since my models where a lot bigger and more complicate, it took me almost
all day to figure out that the problem was a @property from a base model,
and my suggestion is that there should be at least a warning somewhere
(during model's __init__ perhaps) that could be more precise about why
attribute couldn't been set. (or attribute to which object (either Model
or Field).
I tried it on 1.2 and 1.4 pre-alpha SVN-16338
To reproduce you just need to put the models.py from above in some app.
--
Ticket URL: <https://code.djangoproject.com/ticket/16176>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
czepiel.artur@… | Status: new
Type: Bug | Component: Database layer
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by melinath):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 1
* needs_docs: => 0
Comment:
I can confirm that this bug exists. It happens with CharFields, but not
ForeignKeys. I haven't actually checked, but I'm guessing that the problem
lies in the use of contribute_to_class, which would mean that the
attribute with the field's name would never actually be set on the class -
and thus the property would not be overridden... If that is the problem,
one possible solution would be to just always set the attribute with the
field's name to None.
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:1>
Django
czepiel.artur@… | Status: new
Type: Bug | Component: Database layer
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
* cc: teraom (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:2>
Django
Reporter: czepiel.artur@… | Owner: anonymous
Type: Bug | Status: new
Component: HTTP handling | Version: master
Severity: Release blocker | Resolution:
Keywords: xxxxx | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* keywords: => xxxxx
* owner: nobody => anonymous
* has_patch: 0 => 1
* component: Database layer (models, ORM) => HTTP handling
* severity: Normal => Release blocker
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:3>
Django
Reporter: czepiel.artur@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by metzlar@…):
I made this work by defining the property dynamically inside the parent
model definition:
{{{
class SomeTestModel(models.Model):
some_field = models.CharField(max_length=100)
class Meta:
abstract = True
@classmethod
def __new__(cls, *args, **kwargs):
inst = models.Model.__new__(cls, *args, **kwargs)
if cls is SomeTestModel:
inst.other_field = property(lambda x: "[OTHER] %s" %
getattr(x, 'some_field'))
return inst
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:3>
Django
Reporter: czepiel.artur@… | Owner:
Type: Bug | Thomas_Moronez
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Thomas_Moronez):
* owner: nobody => Thomas_Moronez
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:4>
Django
Reporter: czepiel.artur@… | Owner:
Type: Bug | Thomas_Moronez
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Thomas_Moronez):
* has_patch: 0 => 1
* needs_tests: 1 => 0
Comment:
Added a try ... except statement that throws a more descriptive error
message. That way the user can know the error lies within the use of the
@property descriptor and can simply modify the name of the field.
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:5>
Django
Reporter: czepiel.artur@… | Owner:
Type: Bug | Thomas_Moronez
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by carljm):
I don't think the proposed error-reraising is a good idea. For one thing,
we can't be sure that the `AttributeError` is in fact due to a property
from an abstract base class, it could be due to a definition of
`__setattr__`, a metaclass, or possibly other even more diabolical things.
For another, when we can avoid it it's a bad idea to obscure the original
traceback by catching one exception and raising another one in its place.
melinath's suggestion to set all fields as class attributes so they
override descriptors from a parent class is certainly worth considering,
as it would make models behave more like regular Python classes. This
could result in some backwards-incompatibility; would need to look into it
more to get a clearer sense of what cases might cause trouble, but I think
it's probably acceptable if documented in the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:6>
Django
Reporter: czepiel.artur@… | Owner:
Type: Bug | Thomas_Moronez
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Thomas_Moronez):
I modified the add_to_class method to check if the value of the attribute
is a property. If it is it sets the attribute to the class with a None
value. I took melinath's suggestion too set the field's name to None.This
does fix the bug, and does not break test in model_inheritance_regress.
However, as developer carljm, pointed out, backwards-incompatibility may
be present as the patch adds 4 unit test failures.
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:7>
Django
Reporter: czepiel.artur@… | Owner:
Type: Bug | Thomas_Moronez
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Patch no longer applies cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:8>
Django
Reporter: czepiel.artur@… | Owner:
| Thomas_Moronez
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Phist0ne):
I stumbled upon this bug lately using django 1.8 and am willing to provide
a patch.
The main problem seems that creating an attribute containing a django
field on a model does not overwrite any descriptors (with `__set__`
defined) from ancestor classes, which does not match the behavior of
standard python classes.
It makes sense to me that the attributes corresponding to model fields are
not set until the class is instantiated, since the values for these fields
are not available on class creation.
Therefore my suggested solution would be very similar to Thomas_Moronez's
patch. What confuses me though is that the proposed patch seems to not
call contribute_to_class.
In my opinion there are three possible solutions:
* delete all class attributes that will be overwritten by fields later
(e.g. in add_to_class after the "contribute-to-class" check")
* delete class attributes that have a `__set__` method defined
* set the class attributes to a specific value (e.g. a to be written
class) to mark them as to be overwritten later (melinath's suggestion)
Is this still considered a bug and if so which one is the preferred
solution?
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:9>
Django
Reporter: czepiel.artur@… | Owner:
| Thomas_Moronez
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/14508 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:10>
Django
Reporter: czepiel.artur@… | Owner:
| Thomas_Moronez
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
Fixed in 225d96533a8e05debd402a2bfe566487cc27d95f
--
Ticket URL: <https://code.djangoproject.com/ticket/16176#comment:11>