Opened 7 years ago

Closed 7 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 kapil garg, 7 years ago

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 ?

comment:2 by Tim Graham, 7 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.)

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