order_with_respect_to fails to correctly track _order after deleting entries

[Chris]

When using a model that orders itself with respect to another entry the "_order" value is not set correctly after an entry has been deleted, and isn't set correct if the Foreign key is changed to point from one 'Question' to another (Question here the class with the foreighn key as in the django docs), this can result in incorrect ordering, and duplicate "_order" values for objects with the same foreign key id

To reproduce this problem is simple, use the admin interface to add one question and three answers (related to that question) into the db. These will have _order values of 0, 1 and 2 (correct). Now delete the first answer and add a new answer using the admin interface and the _order value for this new answer will be 2, resulting in the three answers having values of 1, 2 and 2 (where the duplication is clearly bad - this should either still be 0, 1 and 2 with multiple entries having been changed to accomodate, or should be 1, 2 and 3 - preserving the order). The value assigned to _order seems simply to be the number of entries related to that Question, however more intelligence is needed here.

The same issue occurs when moving an answer from one Question to another, in that the answer's _order value is unchanged, when it should probably change dependent on the list it is getting inserted into.

class Question(models.Model):
    q = models.CharField(max_length=200)

class Answer(models.Model):
    question = models.ForeignKey(Question)
    a = models.CharField(max_length=200)

    class Meta:
        order_with_respect_to = 'question'

As I see it there are two ways to solve this:

• when an Answer is saved to the db, it should get the first number larger than all current numbers
• or when an Answer is moved or deleted the remaining entries should be shuffled around so that there ordering is still complete

Either way, _order and the fk should probably be unique together as this would enforce it

This testing was done using the admin interface to add and remove the classes on Django 1.1.1 with MYSQL

[Russell Keith-Magee]

I can't reproduce this - or for the most part, even work out what is being reported.

Instructions like "Now delete the first answer and add a new answer using the admin interface" are incredibly vague. How should I delete the first answer? Using the inline? Directly using the Answer model's admin page? Using the admin delete action on the Answer changelist? How should I add the new answer? Using the inline? Does it matter if this is a tabular or stacked inline? Should I use the admin pages for Answer to add the object? If you're reporting a problem that you see through the admin interface, you need to be *extremely* precise, because there are usually multiple ways to achieve any given goal, and the exact method can have a profound result on the outcome.

Also, if this is related to a problem in admin, the specific ModelAdmin/Inline declaration should also be provided. You provide models, but you don't provide admin definitions. It's not clear where "_order" comes into this whole picture. Using simple admin definitions, there is no _order value on the model or form.

Closing "worksforme"; please reopen if you can provide more specific instructions that can be used to reproduce a problem.

[Chris]

Sorry for being unclear I didn't mean for this to be taken as an issue with the admin interface (I was just using it for convenience). I believe that this is an issue with the models themselves in that the save() function needs to be more intelligent or the delete() function needs to do more cleanup. The code required to reproduce this is here:

class Question(models.Model):
        q = models.CharField(max_length=200)
        def __unicode__(self):
                return self.q

class Answer(models.Model):
        question = models.ForeignKey(Question)
        a = models.CharField(max_length=200)
        class Meta:
                order_with_respect_to = 'question'
        def __unicode__(self):
                return self.a

Run these commands in a 'python manage.py shell' and they have the same results (while avoiding any possible complications with the admin interface):

from mysite.scutle.models import Question, Answer
question = Question(q="Question 1")
question.save()
a1 = question.answer_set.create(a="Answer 1")
a1.save()  #_order=0
a2 = question.answer_set.create(a="Answer 2")
a2.save()  #_order=1
a3 = question.answer_set.create(a="Answer 3")
a3.save()  #_order=2
a4 = question.answer_set.create(a="Answer 4")
a4.save()  #_order=3
question.answer_set.all()
# You should get: [<Answer: Answer 1>, <Answer: Answer 2>, <Answer: Answer 3>, <Answer: Answer 4>] 
a1.delete()
a2.delete()
a5 = question.answer_set.create(a="Answer 5")
a5.save()  # Here _order=2 (duplication of order value of a3)
question.answer_set.all()
# You now get [<Answer: Answer 5>, <Answer: Answer 3>, <Answer: Answer 4>] which is incorrect (at least to my understanding of the code)
a6 = question.answer_set.create(a="Answer 6")
a6.save()
a7 = question.answer_set.create(a="Answer 7")
a7.save()
question.answer_set.all()
# They get even more odd when you add more, this will result in:
#[<Answer: Answer 5>, <Answer: Answer 3>, <Answer: Answer 6>, <Answer: Answer 4>, <Answer: Answer 7>] This is clearly out of order

At the end, looking at the SQL tables shows that the "_order" values in the db are set as 2, 3, 2, 3 and 4 for pk's 3, 4, 5, 6 and 7 (in their respective orders). This seems incorrect to me as there is duplication of the order values for answers to the same question, and the ordering doesn't respect the order answers are added to the question.

Calling question.set_answer_order with a list of the pks of the answers does correct this list with the _order values form 0 to 4. This represents a potential solution to this problem: if question.set_answer_order(question.get_answer_order()) is called every time an entry from the list is removed the orders are corrected to be starting from 0 and going to (number of entires - 1), and so new entries (which seem to get given an "_order" value equal to the number of existing entries) will always be added last to the list. So this code works as expected:

from mysite.scutle.models import Question, Answer
question = Question(q="Question 1")
question.save()
a1 = question.answer_set.create(a="Answer 1")
a1.save()
a2 = question.answer_set.create(a="Answer 2")
a2.save()
a3 = question.answer_set.create(a="Answer 3")
a3.save()
a4 = question.answer_set.create(a="Answer 4")
a4.save()
question.answer_set.all()
# You should get: [<Answer: Answer 1>, <Answer: Answer 2>, <Answer: Answer 3>, <Answer: Answer 4>] 
a1.delete()
question.set_answer_order(question.get_answer_order())
a2.delete()
question.set_answer_order(question.get_answer_order())
a5 = question.answer_set.create(a="Answer 5")
a5.save() # gets _order=2 as before, but the other _orders are now 0 and 1 rather than 2 and 3 as in the previous example
question.answer_set.all()
# Output is now in correct order: [<Answer: Answer 3>, <Answer: Answer 4>, <Answer: Answer 5>]
a6 = question.answer_set.create(a="Answer 6")
a6.save()
a7 = question.answer_set.create(a="Answer 7")
a7.save()
question.answer_set.all()
# And ordering is still correct [<Answer: Answer 3>, <Answer: Answer 4>, <Answer: Answer 5>, <Answer: Answer 6>, <Answer: Answer 7>]

This leads to a solution where this is called automatically on a delete() function call to any model with order_with_respect_to set, or alternatively more intelligence around what value "_order" is assigned on a save() call (so that it gets the current largest number + 1)

A similar issue occurs with multiple questions, moving an answer from one question to another doesn't change the value of "_order" (a solution for the ordering issue above might also solve this)

[Russell Keith-Magee]

Ok - that's a lot clearer, and it's clearly a problem. Your suggested solution certainly seems to be on the right track, too.

[Simon Meers]

I think changing the insertion order value calculation is the better solution here; less expensive than reordering everything and simpler than detection deletions *and* parent updates, etc. Patch and tests attached.

[Julien Phalip]

Thanks for the patch, Simon! It looks pretty good to me. I'm just wondering if the tests could be made a little more thorough by adding more items, by doing more delete/add/move operations and by asserting the actual values of each item's _order field. What do you think?

[Simon Meers]

I was steering away from making the tests too explicit, particularly in regard to the value of the _order field -- I'd prefer to test the behaviour rather than the implementation. Currently, for efficiency's sake, if you delete an item you'll get a "gap" in the _order sequence. I don't actually want the "gap" so much that I want to test for it, and would like the tests to continue to pass even if someone later decides to trigger reordering upon modification, or even if they wanted to switch between zero/one-based indexing.

Another test or two based on delete/add/move operations wouldn't hurt, though those I'd written were enough to convince me there were no logic holes.

[Hasan Ramezani]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In f97bbad:

Fixed #13296 -- Fixed ordering by Options.order_with_respect_to after deleting objects.

Thanks Simon Meers for the original patch.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2362f272:

[3.0.x] Fixed #13296 -- Fixed ordering by Options.order_with_respect_to after deleting objects.

Thanks Simon Meers for the original patch.

Backport of f97bbad908df128189eff77d98af9a25ed1ecf23 from master