Opened 13 years ago

Closed 3 years ago

#16176 closed Bug (fixed)

Overwriting a property with field during model inheritance.

Reported by: czepiel.artur@… Owned by: Thomas_Moronez
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: teraom 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

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.

Attachments (1)

patch_and_test.diff (2.8 KB ) - added by Thomas_Moronez 12 years ago.
Sets attributes with property decorators to the class with a None value

Download all attachments as: .zip

Change History (12)

comment:1 by Stephen Burrows, 13 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

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.

comment:2 by teraom, 13 years ago

Cc: teraom added

comment:3 by metzlar@…, 12 years ago

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

comment:4 by Thomas_Moronez, 12 years ago

Owner: changed from nobody to Thomas_Moronez
Status: newassigned

comment:5 by Thomas_Moronez, 12 years ago

Has patch: set
Needs tests: unset

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.

comment:6 by Carl Meyer, 12 years ago

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.

by Thomas_Moronez, 12 years ago

Attachment: patch_and_test.diff added

Sets attributes with property decorators to the class with a None value

comment:7 by Thomas_Moronez, 12 years ago

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.

comment:8 by Tomek Paczkowski, 11 years ago

Patch needs improvement: set

Patch no longer applies cleanly.

comment:9 by Philipp Steinhardt, 9 years ago

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 overwrite all class attributes with None that will be overwritten by fields later (e.g. in add_to_class after the "contribute-to-class" check")
  • delete overwrite class attributes with None 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?

Last edited 9 years ago by Philipp Steinhardt (previous) (diff)

comment:10 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:11 by Carlton Gibson, 3 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top