Opened 13 years ago

Closed 9 years ago

#15184 closed Bug (fixed)

Error when subclassing models.ForeignKey field

Reported by: rupa108 Owned by: furious_luke
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: ForeignKey, models, subclassing
Cc: furious_luke Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Hello,

subclassing from

djago.db.models.ForeignKey

does not work as documented.

As soon as I do:

__metaclass__ = models.SubfieldBase

accessing a model instance gives the error:

Traceback (most recent call last):
  File "/home/roman/devel/kundebunt_new/src/kundebunt/contrib/views.py", line 474, in __call__
    call_method_return = call_method(request,obj)
  File "/home/roman/devel/kundebunt_new/src/kundebunt/contrib/views.py", line 561, in view
    fields = self._get_record_properties_data(obj)
  File "/home/roman/devel/kundebunt_new/src/kundebunt/contrib/views.py", line 533, in _get_record_properties_data
    value = self._get_field_data(record,spalte)
  File "/home/roman/devel/kundebunt_new/src/kundebunt/contrib/views.py", line 268, in _get_field_data
    if getattr(record,spalte['name']) == None:
  File "/home/roman/devel/django-trunk/django/db/models/fields/subclassing.py", line 96, in __get__
    return obj.__dict__[self.field.name]
KeyError: 'kunde'

Please see http://groups.google.com/group/django-users/browse_thread/thread/129bbebfb9df206f/2fbe2264a59ded79?lnk=gst&q=roman#2fbe2264a59ded79

Regards

Roman

Attachments (2)

foreignkey_subclass_patch.diff (4.3 KB ) - added by furious_luke 13 years ago.
Patch to correct a bug in subclass ForeignKeyField.
foreignkey_subclass_patch.20141018.diff (4.6 KB ) - added by Luke Hodkinson 9 years ago.
An updated version of the previous patch. This one works cleanly with the current master branch.

Download all attachments as: .zip

Change History (18)

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

Resolution: invalid
Status: newclosed

As it says in the docs:
"
If you're handling custom Python types, such as our Hand class, we need to make sure that when Django initializes an instance of our model and assigns a database value to our custom field attribute, we convert that value into the appropriate Python object. The details of how this happens internally are a little complex, but the code you need to write in your Field class is simple: make sure your field subclass uses a special metaclass:
"

That is, you only use SubfieldBase if your Field type will be handling custom Python types -- such as the Hand object in the custom fields example. If you're just looking to add behavior to an existing Django field type, you don't have to use the metaclass.

in reply to:  1 comment:2 by rupa108, 13 years ago

Resolution: invalid
Status: closedreopened

Hello,

Replying to russellm:

As it says in the docs:

thanks for your reply and sorry if I'm wrong here, however:

I read the docs, and I think I understand them. And I do use SubfieldBase successfully in various places and everything works as expected.
It does not work when subclassing from ForeignKey.

Of course I implement to_python() and occaionally formfield() in my derived classes and also get_prep_lookup(), that's why I need to SubfieldBase ...

But it does not work when subclassing from ForeignKey.

Here is another example:

class FkStandortAX(models.ForeignKey):
    __metaclass__ = models.SubfieldBase
    description = "ForeignKey displaying address field prepared for Ajax Autocompletion"
           
    def to_python(self,value):
        standort = dict(
            name =  value.name,
            strasse = value.adresse and  value.adresse.strasse,
            kfz = value.adresse and value.adresse.land and value.adresse.land.kfz,
            plz = value.adresse and value.adresse.plz,
            ort = value.adresse and value.adresse.ort)
        
        return u'{name} -- {strasse}, {kfz}{plz} {ort}'.format(**standort)

The column definition in the model:

standort = FkStandortAX(verbose_name=_(u"Standort"),to="popkern.Person", db_column='standort')

When using it:

In [1]: from kundebunt.housing import models    

In [2]: r=models.RZ.objects.all()[3]

In [3]: r._meta.fields
Out[3]: 
[<django.db.models.fields.AutoField object at 0x1434410>,
 <django.db.models.fields.DateTimeField object at 0x1434250>,
 <django.db.models.fields.CharField object at 0x1434290>,
 <kundebunt.contrib.models.fields.FkStandortAX object at 0x1434310>]

In [4]: [f.name for f in r._meta.fields]
Out[4]: ['id', 'timestamp', 'name', 'standort']

In [5]: r.standort  

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)

/home/roman/devel/kundebunt_new/src/kundebunt/<ipython console> in <module>()

/home/roman/devel/django-trunk/django/db/models/fields/subclassing.pyc in __get__(self, obj, type)
     94         if obj is None:
     95             raise AttributeError('Can only be accessed via an instance.')
---> 96         return obj.__dict__[self.field.name]
     97 
     98     def __set__(self, obj, value):

KeyError: 'standort'

Accessing r.standort should not raise an error here. It's there in the field list but it's not accessible. How is the value that to_python returns supposed to be accessed if not like this?

Regards

Roman

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

Triage Stage: UnreviewedAccepted

Ok - I'm going to mark this accepted -- line 96 of subclassing.py looks like it should be field.attname, not field.name. There may be other problems involved with subclassing ForeignKey that follow on from that fix; that will require some additional investigation.

However, I would make two comments:

  1. When reporting a bug, you need to give us *all* the information you have, not just the bits you want to give us. Nowhere in your mailing list posts or in this ticket did you mention that you were using to_python. That's a pretty important detail for anybody that wants to reproduce the problem. Remember, if we can't reproduce your problem, the problem effectively doesn't exist.
  1. The reason you've found this bug is that you are almost certainly Doing It Wrong (tm). Your model layer shouldn't care whether it's being used for AJAX or not. A foreign key is a pointer to another model. The "value" for that model is something that uniquely references another object. A primary key value (or, by proxy, a Django object that encapsulates that primary key value) is the most sensible representation of that value. The need to be "AJAXified" is a display level concern. AJAX rendering considerations should be in your forms/templating layer, not your data model.

comment:4 by rupa108, 13 years ago

Thank's for taking the time to investigate this and sorry for confusing things. You're right, I certainly got mixed up and the things I'm trying to do should be done differently. Nevertheless I'm glad that you consider to make changes here.

comment:5 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: Bug

comment:6 by furious_luke, 13 years ago

Cc: furious_luke added
Owner: changed from nobody to furious_luke
Status: reopenednew

by furious_luke, 13 years ago

Patch to correct a bug in subclass ForeignKeyField.

comment:7 by furious_luke, 13 years ago

Has patch: set
Resolution: fixed
Status: newclosed

The issue appeared to be that SubfieldBase's descriptor was stomping on ForeignKey's. I've added a check for an existing value under the same name in SubfieldBases's descriptor; this way if the field name is missing from a model's __dict__ attribute we can hand off responsibility to the previously existing descriptor.

A new unit test has been added to check for this problem.

comment:8 by Julien Phalip, 13 years ago

Resolution: fixed
Status: closedreopened

The ticket will be closed as fixed the day that a patch actually gets *checked in* to Django.

comment:9 by furious_luke, 13 years ago

Ah, right you are then. Sorry about that.

comment:10 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:11 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:12 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:13 by Tim Graham, 10 years ago

Patch needs improvement: set

Patch needs to be updated to apply cleanly.

by Luke Hodkinson, 9 years ago

An updated version of the previous patch. This one works cleanly with the current master branch.

comment:14 by Tim Graham, 9 years ago

SubfieldBase has been deprecated in Django 1.8. Can the issue be reproduced without it? If so, please update the patch, thanks. If not, we should close this ticket.

comment:15 by Luke Hodkinson, 9 years ago

The deprecation of SubfieldBase appears to have eliminated the issue!

comment:16 by Simon Charette, 9 years ago

Resolution: fixed
Status: newclosed

@furious-luke, we'll close the ticket as fixed then.

Note: See TracTickets for help on using tickets.
Back to Top