Add failing test for saving with a new sort_order #36

LilyFoote · 2016-01-11T14:10:50Z

Fix IntegrityError in Orderable.save when qs.update violates a unique constraint.

I used hypothesis because Orderable.save is very complex and has potentially many breaking edge-cases hidden away.

LilyFoote · 2016-01-11T15:25:07Z

orderable/tests/test_models.py

+        task = Task.objects.create()
+
+        for order in sort_orders:
+            SubTask.objects.create(task=task, sort_order=order)


The minimal failing examples all look like this:

sort_orders = [n, n + 1, 1]

The create fails in Orderable._save when it tries to run .update(sort_order=F('sort_order') + 1) on the first two SubTask instances.

I think the problem is that the first SubTask with sort_order == n is incremented to sort_order == n + 1 which conflicts with the second SubTask because of the unique_together between sort_order and task.

This doesn't occur with the Task model because it doesn't have any unique constraints.

LilyFoote · 2016-01-11T15:50:23Z

@meshy I'm not sure how best to fix this. Any ideas?

LilyFoote · 2016-01-12T09:47:14Z

orderable/models.py

+                qs.update(sort_order=models.F('sort_order') + 1)
+        except IntegrityError:
+            for obj in qs.order_by('-sort_order'):
+                qs.filter(pk=obj.pk).update(sort_order=models.F('sort_order') + 1)


This is ugly, but it works...

Unfortunately, Django doesn't have support for DEFERRABLE unique constraints. Ideally, we'd be able to set the constraint as UNIQUE DEFERRABLE INITIALLY IMMEDIATE and then run the update with DEFERRED set.

This doesn't seem right to me -- why is there an integrity error in the first place? The "shift" should have cleared the way, no?

(Please bear in mind that I don't really understand the failing case here.)

Say we have a qs where sort_order looks like this: 1, 2, 3, 4.
We add a new item with sort_order == 2.
Orderable._save creates to_shift which looks like: 2, 3, 4.
The old code then tried to call to_shift.update(sort_order = F('sort_order') + 1).
This works fine without any unique constraints, but if sort_order is unique_together with another field (as in SubTask) the update will raise IntegrityError when it increments 2 to 3. The internal state of to_shift is now 3, 3, 4, which is invalid.

If Django (and the db) supported DEFERRED, we could defer the unique check until the update had completed (3, 4, 5) and everything would work as in the case without unique.

Sounds like a manifestation of #7

It definitely looks related to #26 / #33.

Anyway, I agree this isn't really the "right" fix, but I don't think I can implement the "right" fix without DEFERRED.

What about defining the new sort order by: Max(sort_order) + sort_order + 1? Would that work?

Nice idea. I tried a few variants of it - the first just led to IntegrityError for a slightly more complex example. The second caused sort_order to increase exponentially - I get django.db.utils.DataError eventually.

cool thanks :)

codecov-io · 2016-01-12T09:57:53Z

Current coverage is `81.42%`

Merging #36 into master will increase coverage by +0.97% as of 497679a

@@            master     #36   diff @@
======================================
  Files            2       2       
  Stmts          133     140     +7
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            107     114     +7
  Partial          0       0       
  Missed          26      26

Review entire Coverage Diff as of 497679a

Uncovered Suggestions

+7.86% via orderable/admin.py#52...62
+4.29% via orderable/admin.py#23...28
+2.15% via orderable/models.py#63...65
See 3 more...

Powered by Codecov. Updated on successful CI builds.

LilyFoote · 2016-01-12T11:08:50Z

@kevinetienne @adam-thomas Review?

adam-thomas · 2016-01-12T11:28:32Z

orderable/tests/test_models.py

@@ -207,3 +213,13 @@ def test_changing_parent(self):
        subtask_2.task = task
        subtask_2.save()
        self.assertSequenceEqual(task.subtask_set.all(), [subtask, subtask_2])
+
+    @given(lists(integers(min_value=1), min_size=1, unique=True))


What does this do?

Populates the sort_order parameter with a list of positive integers.

Rather than installing Hypothesis, wouldn't it be better just to do sort_orders = range(1, n)?

(With sort_orders.shuffle() if you need them to be randomly distributed. Or writing out a short list by hand.)

This is much more thorough. I don't trust us to catch all the edge cases without hypothesis.

I'll also note that it found an overflow in the sort_order field with one of my attempts to improve _update. This is the sort of edge-case that I would not have found with a simple test.

Found the bugs how? It looks like it improved on the hardcoded list in the test that was there already, but wouldn't changing that to [1,2,3,4] as you described earlier have achieved the same thing? Did you get that from the test output? Do you think you would have been able to find this error using Hypothesis without that test already in place (or without writing it yourself)?

And yes - the point being that there probably aren't many more edge cases or failures in the exact list of sort_orders you have (there are only so many different relevant situations you can construct), but there might be plenty of edge cases or failures elsewhere that this test will never hit. Adding Hypothesis here doesn't guarantee much.

I'll also note that it found an overflow in the sort_order field with one of my attempts to improve _update. This is the sort of edge-case that I would not have found with a simple test.

That sounds good, although I'm a bit surprised/alarmed that sort_order can overflow at all outside of a case where we've basically filled up the database, at which point we have other issues. Theoretically any field with a limited size can overflow in that situation, so I don't know if there's any value in designing around it.

Is there a test for this? What else did it find? If I'm missing information about what you've actually done here, please post it either in the issue or in the code itself.

#36 (comment)

LilyFoote · 2016-01-12T15:17:36Z

@adam-thomas @kevinetienne Merge?

kevinetienne · 2016-01-12T15:42:35Z

Merging, other issues if needed can be done in other PRs

Add failing test for saving with a new sort_order

kevinetienne · 2016-01-12T15:43:14Z

changelog.md :p

LilyFoote added 2 commits January 11, 2016 13:55

Add failing test

8ab614e

Use hypothesis to find a minimal failing example

7515b69

LilyFoote self-assigned this Jan 11, 2016

LilyFoote added the bug label Jan 11, 2016

Simplify hypothesis test further

e61ed80

LilyFoote reviewed Jan 11, 2016
View reviewed changes

LilyFoote added 3 commits January 12, 2016 09:25

Fix Orderable._save for new instances

5f7ccab

Test moving an existing Orderable instance

0a42578

Add Orderable._update helper method

c6cea48

LilyFoote reviewed Jan 12, 2016
View reviewed changes

Ignore more flake8 docstring warnings

9f864ba

adam-thomas reviewed Jan 12, 2016
View reviewed changes

LilyFoote added 2 commits January 12, 2016 11:52

Add docstring to SubTask.save test

6e69fc3

Add two explict examples to hypothesis test

497679a

kevinetienne pushed a commit that referenced this pull request Jan 12, 2016

Merge pull request #36 from incuna/test-reorder

2495aed

Add failing test for saving with a new sort_order

kevinetienne merged commit 2495aed into master Jan 12, 2016

kevinetienne deleted the test-reorder branch January 12, 2016 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add failing test for saving with a new sort_order #36

Add failing test for saving with a new sort_order #36

LilyFoote commented Jan 11, 2016

LilyFoote Jan 11, 2016

LilyFoote Jan 11, 2016

LilyFoote commented Jan 11, 2016

LilyFoote Jan 12, 2016

meshy Jan 12, 2016

meshy Jan 12, 2016

LilyFoote Jan 12, 2016

meshy Jan 12, 2016

LilyFoote Jan 12, 2016

LilyFoote Jan 12, 2016

kevinetienne Jan 12, 2016

LilyFoote Jan 12, 2016

kevinetienne Jan 12, 2016

codecov-io commented Jan 12, 2016

LilyFoote commented Jan 12, 2016

adam-thomas Jan 12, 2016

LilyFoote Jan 12, 2016

adam-thomas Jan 12, 2016

adam-thomas Jan 12, 2016

LilyFoote Jan 12, 2016

meshy Jan 12, 2016

LilyFoote Jan 12, 2016

adam-thomas Jan 12, 2016

adam-thomas Jan 12, 2016

LilyFoote Jan 12, 2016

LilyFoote commented Jan 12, 2016

kevinetienne commented Jan 12, 2016

kevinetienne commented Jan 12, 2016

Add failing test for saving with a new sort_order #36

Add failing test for saving with a new sort_order #36

Conversation

LilyFoote commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LilyFoote commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 12, 2016

Current coverage is 81.42%

LilyFoote commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LilyFoote commented Jan 12, 2016

kevinetienne commented Jan 12, 2016

kevinetienne commented Jan 12, 2016

Current coverage is `81.42%`