Opened 7 years ago
Closed 7 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 )
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 , 7 years ago
Needs tests: | set |
---|
comment:2 by , 7 years ago
Description: | modified (diff) |
---|
comment:3 by , 7 years ago
Description: | modified (diff) |
---|
comment:4 by , 7 years ago
Needs tests: | unset |
---|---|
Summary: | Model fields where the python attribute name does not match the db column are not saved → Model fields where the field name is shadowed by Python property aren't saved |
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 7 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 , 7 years ago
I suspect this is related, since the root cause is a change in get_deferred_attributes
comment:7 by , 7 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
comment:8 by , 7 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
That's interesting, I didn't know about the
name
argument toField
. 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.