Custom fields using attname, Model __init__ and Deserialization: hard-coded magic for relation fields

[Paul Garner]

I'm struggling with the title for this issue, sorry.

Basically: if you have custom field type where attname is different to name (which can be desirable for same reason it's done in ForeignKey fields) then parts of Django don't work and are not practical to override.

Your field will work okay until you try to do this:

    obj = MyModel(my_custom_field_name=val)

At this point Model.__init__ in django/db/models/base.py complains with:

    TypeError: 'my_custom_field_name' is an invalid keyword argument for this function

Looking in __init__ it is clear why:

You've got a check for isinstance(field.rel, ManyToOneRel) to decide if is_related_object = True and then:

    if is_related_object:
        setattr(self, field.name, rel_obj)
    else:
        setattr(self, field.attname, val)

ie it decides which value to populate on the model. After that we loop through kwargs and check:

    if isinstance(getattr(self.__class__, prop), property):
        setattr(self, prop, kwargs.pop(prop))

Which fails because is_related_object=False for our custom field so attname has been set on the model but name was used in kwargs. After that the TypeError is raised.

Ok you say, you should just always instantiate your model using the attname as a kwarg instead of name.

Unfortunately the Django serialization apparatus has more hard-coded magic that makes ForeignKey and ManyToMany fields work, but ensures your custom field will fail in this regard.

In django/core/serializers/python.py Deserializer class we have checks for if field.rel with special cases for ManyToManyRel and ManyToOneRel, falling through to...

        else:
            data[field.name] = field.to_python(field_value)
    
    yield base.DeserializedObject(Model(**data), m2m_data)

...instantiating the model with field.name as the default case.

Ok, so I tried adding a dummy rel=ManyToOneRel attribute on my custom field to trick Model.__init__ ... but then that triggers more magic in the Serializer class, which understandably tries to treat it as a ForeignKey field.

Right now I can't see any way to cleanly fake my way out.

Adding this line to django/core/serializers/python.py Deserializer fixed it for me:

            elif field.name != field.attname:
                data[field.attname] = field.to_python(field_value)
                
            # Handle all other fields
            else:
                data[field.name] = field.to_python(field_value)

[Paul Garner]

fix Deserializer

[Paul Garner]

A minimal test case

[Paul Garner]

I've added a patch and test case. This fixes things for me.

[Paul Garner]

​http://djangosnippets.org/snippets/2294/
This field sets both name and attname in its descriptor, using instance.__dict__[self.field.name]. That is different to other Django fields but allows the model to be initialised with either attname or name as a kwarg.

However it needs different values supplied depending on if you use name or attname in the Model kwargs... my patch breaks this field by causing it to be initialised by attname, but with a name type value.

It seems we should have a flag on Field classes so they can tell Django whether they should be initialised by attname or name.

[Paul Garner]

Apologies...

Everything works fine for me without any patch if I make my custom descriptor subclass property ...it's nothing to do with assigning directly to instance.__dict__[self.field.name]

I was copying Django's existing descriptors, all of which seem to subclass object instead.

Perhaps this could be documented.

[Paul Garner]

Although actually my minimal test case is still broken...!

Just my actual working code uses something more like the field in the djangosnippet above, i.e. Field class + Descriptor ...so I'm happy to get that working at least.

[Anssi Kääriäinen]

In the patch there is this code:

            elif field.name != field.attname:
                data[field.attname] = field.to_python(field_value)
                
            # Handle all other fields
            else:
                data[field.name] = field.to_python(field_value)

Why not write it just as

# Handle all other fields
else:
    data[field.attname] = field.to_python(field_value)

As far as I see that is equivalent to what is done in the patch. This also models what __init__ is doing. It expects all other fields by attname.

To get the patch in ready for checkin stage, you will need to write your test case as a normal Django test case, that is a patch to tests directory. tests/modeltests/serializers/ seems to be a good directory for this. It would also be good to check if other deserializers suffer from this problem.

[Anssi Kääriäinen]

Ok, I was a bit quick to mark this accepted. There is no need for Django to fix this, as attname seems to be something that is internal to Django. There is this documentation in django/db/models/fields/__init__.py:

# A guide to Field parameters:
#
#   * name:      The name of the field specifed in the model.
#   * attname:   The attribute to use on the model object. This is the same as
#                "name", except in the case of ForeignKeys, where "_id" is
#                appended.
#   * db_column: The db_column specified in the model (or None).
#   * column:    The database column for this field. This is the same as
#                "attname", except if db_column is specified.   

I couldn't find anything interesting about attname from Django's documentation.

So, now my take is that if this is going to be fixed, the right fix is to define what attname and name really mean. And then fix the code to match that definition. Currently the definition for attname seems to be that it is to be used only with foreign keys.

Marking this design decision needed.

[Aymeric Augustin]

Agreed — currently, attname is an internal implementation detail, it isn't documented. The way Model.__init__ is written clearly shows that attname is only designed to support relations.

If we want to add support for custom fields with attname != name and a read/write accessor created in contribute_to_class, that's a much broader project that needs a detailed proposal on the django-developers mailing list.