Code

Opened 3 years ago

Closed 3 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

Description

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)

Attachments (2)

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

Download all attachments as: .zip

Change History (9)

Changed 3 years ago by anentropic

fix Deserializer

Changed 3 years ago by anentropic

A minimal test case

comment:1 Changed 3 years ago by anentropic

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 3 years ago by anentropic

  • Patch needs improvement set

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.

comment:3 Changed 3 years ago by anentropic

  • Resolution set to invalid
  • Status changed from new to closed

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.

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

comment:4 Changed 3 years ago by anentropic

  • Resolution invalid deleted
  • Status changed from closed to reopened

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.

comment:5 Changed 3 years ago by akaariai

  • Component changed from Uncategorized to Core (Serialization)
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:6 Changed 3 years ago by akaariai

  • Triage Stage changed from Accepted to Design 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/__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.

comment:7 Changed 3 years ago by aaugustin

  • Resolution set to wontfix
  • Status changed from reopened to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.