Opened 8 years ago
Closed 8 years ago
#28019 closed Bug (needsinfo)
Django django.db.models.Model.save() logic bit illogical?
Reported by: | Jacek Kałucki | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.10 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In code below, calling “Foo(name='Foo Bar').save()” method raises ValueError “Cannot force an update in save() with no primary key” - let’s check what goes wrong here:
1) I didn’t pass any parameters so why logic mentions about “force” and “update” since I’m not forcing nor updating but inserting new data row? I’ve figured out that logic adds fields to “update_fields” parameters itself and in this case it’s a “name” which is class Foo property name.
It won’t happened if there is no class inheritance scenario. Why? But it leads us to the next point.
2) Let’s check when the “not pk_set and (force_update or update_fields)” condition is fulfilled.
IMHO if “pk_set” is false there is no need to check “update_fields” because it’s never used in this scenario, it may be used only in case of “pk_set” equals true but these states are mutually exclusive.
My proposal is to revise “get_deferred_fields()” method to handle class attributes correctly in case of inheritance and remove redundant “update_fields” checking on inserts.
Code highlighting:
class Bar(models.Model): name = models.CharField(max_length=100, blank=True) class Foo(Bar): first_name = models.CharField(max_length=50, blank=True) last_name = models.CharField(max_length=50, blank=True) def __init__(self, *args, **kwargs): name = kwargs.get('name') if name: (first_name, last_name) = name.split() kwargs['first_name'] = first_name kwargs['last_name'] = last_name super(Foo, self).__init__(*args, **kwargs) @property def name(self): names = (self.first_name, self.last_name) return " ".join(x for x in names if x) @name.setter def name(self, value): (self.first_name, self.last_name) = value.split()
Change History (2)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
I also cannot reproduce a crash using the provided code. I think we would like you to provide a tested patch to prove that your suggested approach is viable and passes existing tests. It's a bit difficult to assess that by reading a description of the changes. Please reopen the ticket if you can do that. Thanks. (Retitling the ticket's summary would also be useful as it's difficult to get an idea of the proposal based on the current wording.)
I am unable to reproduce the bug on sqlite3 with dev version and 1.10 version. Is there anything else needed to reproduce the bug ?