Opened 6 years ago

Closed 6 years ago

#29331 closed Bug (invalid)

Model fields where the field name is shadowed by Python property aren't saved

Reported by: Ben Mathes Owned by: nobody
Component: Database layer (models, ORM) Version: 2.0
Severity: Normal Keywords: models, deferred_field, save
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Ben Mathes)

Reproduced in this github django repo: https://github.com/benmathes/deferred_fields_bug

When defining a model, if you rename a model's db column, django will thing that model is deferred and will not save() it.

e.g.

class SomeModel(models.Model):
    """                                                                                                                                                                                                                                                                                                                                                                   
    a contrived model field where we want a "field" that is stored                                                                                                                                                                                                                                                                                                        
    in a "field" column, but we use @property getter/setters so                                                                                                                                                                                                                                                                                                           
    we name the SomeModel class's attribute as "_field".                                                                                                                                                                                                                                                                                                                  
    """
    name = models.TextField(null=True)
    _field = models.TextField(name="field")

    @property
    def field(self):
        return self._field.upper()

    @field.setter
    def field(self, new_value):
        self._field = new_value.lower()

With a renamed db column, "_field" is in self.__dict__, but "field" is not,

 def get_deferred_fields(self):
     """
     Return a set containing names of deferred fields for this instance.
     """
     return {
         f.attname for f in self._meta.concrete_fields
         if f.attname not in self.__dict__
     }

So field is not saved in .save(), because django _mistakenly_ thinks "field" is deferred, so it is ignored during .save()

# https://github.com/django/django/blob/93331877c81c1c6641b163b97813268f483ede4b/django/db/models/base.py#L712
# ...
#     elif not force_insert and deferred_fields and using == self._state.db:
#         field_names = set()
#         for field in self._meta.concrete_fields:
#             if not field.primary_key and not hasattr(field, 'through'):
#                 field_names.add(field.attname)
# ->      loaded_fields = field_names.difference(deferred_fields)
#         if loaded_fields:
#             update_fields = frozenset(loaded_fields)
#
#     self.save_base(using=using, force_insert=force_insert,
#                    force_update=force_update, update_fields=update_fields)
# ...

Reproduced in this github django repo: https://github.com/benmathes/deferred_fields_bug

Change History (8)

comment:1 by Ben Mathes, 6 years ago

Needs tests: set

comment:2 by Ben Mathes, 6 years ago

Description: modified (diff)

comment:3 by Ben Mathes, 6 years ago

Description: modified (diff)

comment:4 by Tim Graham, 6 years ago

Needs tests: unset
Summary: Model fields where the python attribute name does not match the db column are not savedModel fields where the field name is shadowed by Python property aren't saved
Triage Stage: UnreviewedAccepted

That's interesting, I didn't know about the name argument to Field. Is it documented? The last issue I found for it was #14695.

I'm not sure if this can be fixed. Do you plan to offer a patch? If not, I suppose the restriction could at least be documented.

comment:5 by Ben Mathes, 6 years ago

We're working around it in our specific use case, but not a general solution. Possible the codebase where we found this was mistaking name for db_column, as setting the name kwarg does seem to set the DB column name.

comment:6 by Ben Mathes, 6 years ago

I suspect this is related, since the root cause is a change in get_deferred_attributes

https://github.com/jpwatts/django-positions/issues/49

comment:7 by Simon Charette, 6 years ago

FWIW adjusting your descriptor to use __dict__['field'] instead of __dict__['_field'] should work just fine.

class SomeModel(models.Model):
    """                                                                                                                                                                                                                                                                                                                                                                   
    a contrived model field where we want a "field" that is stored                                                                                                                                                                                                                                                                                                        
    in a "field" column, but we use @property getter/setters so                                                                                                                                                                                                                                                                                                           
    we name the SomeModel class's attribute as "_field".                                                                                                                                                                                                                                                                                                                  
    """
    name = models.TextField(null=True)
    _field = models.TextField(name="field")

    @property
    def field(self):
        return self.__dict__['field'].upper()

    @field.setter
    def field(self, new_value):
        self.__dict__['field'] = new_value.lower()

I don't think we can change the assumption that name either set implicitly or explicitly through a parameter the will be present in __dict__ without breaking a lot of stuff.

Edit: By the way the approach suggested above is exactly what the project your linked to is doing

Last edited 6 years ago by Simon Charette (previous) (diff)

comment:8 by Tim Graham, 6 years ago

Resolution: invalid
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top