[FIXED] save() causing the creation of a new record when trying to update an existing record

Issue

So I have the following model:

class Session(models.Model):
    sessionid = models.CharField(max_length=10, primary_key=True)
    forgepass = models.ForeignKey(ForgeProfile, on_delete=models.CASCADE)
    programid = models.ForeignKey(TrainingProgram, on_delete=models.PROTECT)
    program_day = models.IntegerField()
    hours = models.IntegerField()
    session_date = models.DateField(default=timezone.now())
    verified = models.BooleanField(default=False)


    def save(self, *args, **kwargs):
        session_count = Session.objects.all().count()


        self.sessionid = 'S' + self.forgepass.forgepass + id_gen(session_count)


        super().save(*args, **kwargs)
    

    def __str__(self):
        return '{} - {}'.format(self.sessionid, self.session_date)

When updating an existing record on the backend, a new record will be created with a new sessionid and the updated values instead, while keeping the old record. What exactly am I missing here?

Solution

That's because you each time generate a new primary key, and the primary key is how Django determines whether to create or update.

You thus should only assign a new id if there is no primary key generated, something like:

class Session(models.Model):
    sessionid = models.CharField(max_length=10, primary_key=True)
    forgepass = models.ForeignKey(ForgeProfile, on_delete=models.CASCADE)
    programid = models.ForeignKey(TrainingProgram, on_delete=models.PROTECT)
    program_day = models.IntegerField()
    hours = models.IntegerField()
    session_date = models.DateField(default=timezone.now())
    verified = models.BooleanField(default=False)

    def save(self, *args, **kwargs):
        if not self.sessionid:
            session_count = Session.objects.all().count()
            self.sessionid = (
                'S' + self.forgepass.forgepass + id_gen(session_count)
            )
        return super().save(*args, **kwargs)

    def __str__(self):
        return '{} - {}'.format(self.sessionid, self.session_date)

Now that in itself has a problem: if the self.forgepass.forgepass somehow changes, it will not change the primary key.

But in fact formatting the primary key in any sort of shape is not a good idea. You should consider primary keys as black-box objects: something that is perhaps a str or int, but that is a technical detail: not formatting these.

If you want to assign it a sessionid, you can just add a (unique) extra field, but don't use the primary key for this.

Another problem is that here saving an object will at least require three queries: one to get the number of items, one to get the .forgepass of self.forgepass and finally saving the object. This is even more problematic if you would do bulk creates for example with .bulk_create(…) ^[Django-doc], since then .save(…) ^[Django-doc] is circumvented, so please don't add logic to the .save() handler.

Answered By - willeM_ Van Onsem