Opened 7 years ago

Closed 7 years ago

#17080 closed Bug (wontfix)

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

Reported by: anentropic Owned by: nobody
Component: Core (Serialization) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


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/ 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,, rel_obj)
        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/ Deserializer class we have checks for if field.rel with special cases for ManyToManyRel and ManyToOneRel, falling through to...

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

...instantiating the model with 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/ Deserializer fixed it for me:

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

Attachments (2)

Deserializer.patch (847 bytes) - added by anentropic 7 years ago.
fix Deserializer (673 bytes) - added by anentropic 7 years ago.
A minimal test case

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by anentropic

Attachment: Deserializer.patch added

fix Deserializer

Changed 7 years ago by anentropic

Attachment: added

A minimal test case

comment:1 Changed 7 years ago by anentropic

Has patch: set

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

comment:2 Changed 7 years ago by anentropic

Patch needs improvement: set
This field sets both name and attname in its descriptor, using instance.__dict__[]. 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.

comment:3 Changed 7 years ago by anentropic

Resolution: invalid
Status: newclosed


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

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

Perhaps this could be documented.

Last edited 7 years ago by anentropic (previous) (diff)

comment:4 Changed 7 years ago by anentropic

Resolution: invalid
Status: closedreopened

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 I'm happy to get that working at least.

comment:5 Changed 7 years ago by Anssi Kääriäinen

Component: UncategorizedCore (Serialization)
Triage Stage: UnreviewedAccepted

In the patch there is this code:

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

Why not write it just as

# Handle all other fields
    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.

comment:6 Changed 7 years ago by Anssi Kääriäinen

Triage Stage: AcceptedDesign decision needed

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/

# 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.

comment:7 Changed 7 years ago by Aymeric Augustin

Resolution: wontfix
Status: reopenedclosed

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.

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