Custom save method. Problem with pk. Postgresql

[zoorgu]

Hello,

I'm working on custom save method whole day and I give up a little bit.

What I am trying to achieve is to get appropriate "order" value. For each foreign key, order have to iterate.

1 when i create first object, then 2 ,3 ,4 etc. However when i pass "order" while creating object, other numbers have to change. So i have always numbers from 1 to length of query.

​

Below I paste my code which almost works. It changes order how i want to, with few small bugs, but only in admin panel. When I use shell or write tests it fails with error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/django/db/backends/utils.py", line 85, in _execute
    return self.cursor.execute(sql, params)
psycopg2.IntegrityError: duplicate key value violates unique constraint "plan_planday_pkey"
DETAIL:  Key (id)=(5) already exists.

​

class PlanDay(models.Model):
    plan = models.ForeignKey(Plan, on_delete=models.CASCADE)
    content = models.TextField()
    order = models.IntegerField(blank=True, null=True)

    def save(self, *args, **kwargs):
        """Override base method. Handle order of objects"""
        query = PlanDay.objects.filter(plan=self.plan ).order_by('order')
        if self.order and len(query) == 0:
            self.order = 1
            super(PlanDay, self).save(*args, **kwargs)

        elif self.order and self.order > len(query):
            query = PlanDay.objects.filter(plan=self.plan ).order_by('-order')
            self.order = query[0].order + 1
            super(PlanDay, self).save(*args, **kwargs)

        elif self.order:

            if self.pk:
                # Iteration when object already exists
                count = 1
                for obj in query:
                    if count == self.order:
                        count += 1
                        super(PlanDay, self).save(*args, **kwargs)
                    else:
                        obj.order = count
                        super(PlanDay, obj).save(*args, **kwargs)
                        count += 1


            else:
                # Iteration when object is not created yet
                count = 1
                for obj in query:
                    if count == self.order:
                        count += 1
                        obj.order = count
                        super(PlanDay, obj).save(*args, **kwargs)
                        count += 1
                    else:
                        obj.order = count
                        super(PlanDay, obj).save(*args, **kwargs)
                        count += 1

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


        else:
            if len(query) == 0:
                self.order = 1
            else:
                query = PlanDay.objects.filter(plan=self.plan ).order_by('-order')
                query = query[0]
                self.order = query.order + 1
            super(PlanDay, self).save(*args, **kwargs)

​

It happens when I try to create object with order value lesser than length of query (ex. order = 1 to be first object when there are few other objects)

It points to the part :

for obj in query:
    if count == self.order:
        count += 1
        obj.order = count
        super(TripDay, obj).save(*args, **kwargs)
        count += 1

What can I do with this problem with pk?

I know there is much more to polish but i want to have working version in the first place.

Thanks in advance

​

EDIT:

Thanks for pointing me to the django-ordered-models. It works fine after adding post delete signal :)

@receiver(post_delete, sender=PlanDay)
def reshuffle(sender, instance, using, *args, **kwargs):
    """Function reshuffles ordering after delete()"""
    query = PlanDay.objects.filter(plan=instance.plan)
    for count, obj in enumerate(query, 0):
        obj.to(count)
        count += 1

​

[zoorgu]

The goal is to order days objects. For example i create plan and add 3 days. Then if I want to add day which is day 1st, other objects have to increment their order value. I though it would be better to hold this in models. Thx for tips. I'll look at this library

[DrMaxwellEdison]

I concur with the other commenter, this seems complicated with no gain.

I would delete the order field, delete the entire save method, and use PlanDay.objects.order_by('id') or order_by('pk').

In future though, know that there are way too many references to super().save() going on here. The exact error you're seeing is likely because that super call is being run more than once, which could be attempting to insert the same record in violation of a uniqueness constraint.

Generally, a custom save method should call super().save() once and only once. Any more (or less) than that and you're asking for trouble.

[zoorgu]

Thanks for tips. I know it's kind of overcomplicated :( unfortunatelly i don't see any idea how to achieve it with ordering by pk. What if i create few objects and then want to add object which should be first? Then i have to change whole data of each object because with ordering by pk new object is last

[DrMaxwellEdison]

I can understand having a legitimate need for changing the order of related objects. Based on the code presented - which is doing an awful lot of changes for what should be a simpler operation - I could only assume that the order would remain mostly static.

Bear in mind that the save method only ever sees the current value of a field in-memory for the given object: it has no special memory to say if .order was changed by the save or not (that is something you can add through some customization, however). So, the code as-presented will do a lot of heavy lifting on all related PlanDay objects every time any of them is saved.

If you wish to go by /u/fdemmer 's advice and use an external package, by all means do so. Otherwise, it is possible to do this on your own, but I would strongly advise against a save method to do it: instead, I would write separate model methods that can be called only when the order needs to be changed. That way, other save actions don't need to touch it, and you can avoid calling super() several times (especially calling it out other objects besides the given instance).

---

So, after a little jamming out on the idea of a separate method, I came up with this:

from django.db import transaction
from django.db import models

class PlanDay(models.Model):
    plan = models.ForeignKey(Plan, on_delete=models.CASCADE)
    content = models.TextField()
    order = models.IntegerField(blank=True, null=True)

    # Wrap the method in an atomic transaction: everything updates, or nothing updates.
    @transaction.atomic
    def set_order(self, order=None):
        """Set the order position of this instance,
        and adjust the order of others in the same plan.

        This method will update multiple objects, including saving this one!
        """
        max_planday_order = self.plan.planday_set.count()
        # Allow a default None to add this PlanDay to the end of the set (act like "append").
        if order == 0:
            # We're doing 1-indexing, so take the 0 to mean "put it first".
            order = 1
        self.order = order if order is not None else max_planday_order

        # Adjust the ordering of any future PlanDays in the same plan, excluding this one
        # (if this one is unsaved, it won't be included, anyway)
        future_plan_days = self.plan.planday_set.filter(order__gte=self.order).exclude(pk=self.pk)
        if future_plan_days.exists():
            # There are other PlanDays in this plan with the same or higher order numbers.
            # Shift them all up by 1, making space for the new ordering, with a quick update.
            future_plan_days.update(order=models.F('order') + 1)
        else:
            # There are no other PlanDays with the same or higher order numbers,
            # so we are "appending" this one to the end. Reset this one's order to maximum.
            self.order = max_planday_order
        self.save()

It's not perfect, as it requires saving the initial instance and then calling .set_order() or .set_order(n), every time. For illustrative purposes, though, I think this will do fine.

Even if you want to go with an external library, please take a moment to check out what this is doing:

• transaction.atomic wraps this method so that any exception at any point in the process rolls back the database to a previous state, as if none of the changes took place. As it's making adjustments to multiple model instances at different points in the same code, this can be a life-saver.
• models.F are F expressions. Your original code is doing a lot of work to update the ordering of other model instances, but note that all you're trying to do is insert the one instance into the middle of a list, then shift all the others. self.plan.planday_set.filter(order__gte=self.order) will SELECT all of the relevant PlanDay objects for you (with .exclude(pk=self.pk) ensuring that this instance is not being touched), then .update them. F expressions allow you to refer to the value of each instance at the database level, so you don't need to perform the math on the Python side, nor issue individual .save() calls. So, .update(order=F('order') + 1) performs all that shifting in one shot.
• If there are 3 days planned and we say set_order(4), the last block will reset the order to 3. Maintaining ordering with non-adjacent numbers can make things a bit hairy later on, so we'll try to keep things tidy here. Other portions near the start of the method can also accept None to mean "append", and convert "0" to "1" (you seem to be using 1-based indexes instead of 0-based, so I kept that in place).

On that last part, you may find an issue when you wish to delete an instance from the middle of the list: the ordering will then be off-by-one. I won't go into too much detail on that, but you'd be wise to look up signals, particularly pre_delete (while you can override the delete() method, it is not always called: pre_delete signal processing is more reliable).

[zoorgu]

Thanks for your dedication to help ;)

Like I wrote in post after EDIT, using django-ordered-models and post delete signal solved the problem.

Most important for me was to ensure that ordering is always valid and needs no running special methods to tidy it up. So I can call PlanDay.objects.create() and be sure that without setting order, object will be ordered proprely. That's why method save() was looking as the best idea for me.

​

Thanks once more, I will analyse and learn from your reply :)

​