Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing test for saving with a new sort_order #36

Merged
merged 9 commits into from
Jan 12, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
*.pot
*.pyc
local_settings.py
.hypothesis/
18 changes: 16 additions & 2 deletions orderable/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,20 @@ def prev(self):
sort_order__lt=self.sort_order
).order_by('-sort_order').first()

@staticmethod
def _update(qs):
"""
Increment the sort_order in a queryset.

Handle IntegrityErrors caused by unique constraints.
"""
try:
with transaction.atomic():
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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a manifestation of #7

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely looks related to #26 / #33.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool thanks :)


def _save(self, objects, old_pos, new_pos):
"""WARNING: Intensive giggery-pokery zone."""
to_shift = objects.exclude(pk=self.pk) if self.pk else objects
Expand All @@ -80,7 +94,7 @@ def _save(self, objects, old_pos, new_pos):
# Increment `sort_order` on objects with:
# sort_order > new_pos.
to_shift = to_shift.filter(sort_order__gte=self.sort_order)
to_shift.update(sort_order=models.F('sort_order') + 1)
self._update(to_shift)
self.sort_order = new_pos

# self.sort_order decreased.
Expand All @@ -90,7 +104,7 @@ def _save(self, objects, old_pos, new_pos):
# Increment `sort_order` on objects with:
# sort_order >= new_pos and sort_order < old_pos
to_shift = to_shift.filter(sort_order__gte=new_pos, sort_order__lt=old_pos)
to_shift.update(sort_order=models.F('sort_order') + 1)
self._update(to_shift)
self.sort_order = new_pos

# self.sort_order increased.
Expand Down
6 changes: 6 additions & 0 deletions orderable/tests/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@
class Task(Orderable):
"""A basic orderable model for tests."""

def __str__(self):
return 'Task {}'.format(self.pk)


class SubTask(Orderable):
"""An orderable model with unique_together."""
task = models.ForeignKey('Task')

class Meta:
unique_together = ('task', 'sort_order')

def __str__(self):
return 'SubTask {}'.format(self.pk)
29 changes: 24 additions & 5 deletions orderable/tests/test_models.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from django.test import TestCase
from hypothesis import example, given
from hypothesis.extra.django import TestCase
from hypothesis.strategies import integers, lists

from .models import SubTask, Task

Expand Down Expand Up @@ -50,12 +52,14 @@ def test_insert_on_create(self):
old_3 = Task.objects.create(sort_order=3)

# Insert between old_1 and old_2
with self.assertNumQueries(4):
with self.assertNumQueries(6):
# Queries:
# Savepoint
# Savepoint
# Bump old_2 to position 3
# Release savepoint
# Release savepoint
# Save new in position 2
# Commit
new = Task.objects.create(sort_order=old_2.sort_order)

tasks = Task.objects.all()
Expand Down Expand Up @@ -114,14 +118,16 @@ def test_decrease_order(self):
item5 = Task.objects.create(sort_order=5)

# Move item4 to position 2
with self.assertNumQueries(6):
with self.assertNumQueries(8):
# Queries:
# Savepoint
# Find end of list
# Move item4 to end of list
# Savepoint
# Bump item2 and item3 on by one
# Release savepoint
# Release savepoint
# Save item4 to new desired position
# Commit
item4.sort_order = item2.sort_order
item4.save()

Expand Down Expand Up @@ -207,3 +213,16 @@ 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Populates the sort_order parameter with a list of positive integers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://persephonemagazine.com/wp-content/uploads/2013/01/gw-itcrowdmosspopcorn.gif

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@example([2, 3, 1])
@example([2, 3, 4])
def test_save_subtask_no_errors(self, sort_orders):
"""Ensure Orderable.save does not raise IntegrityError."""
task = Task.objects.create()

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

subtask.sort_order = 2
subtask.save()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test fail or not? Slightly confused by the issue title. Travis seems to think it's all good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orderable.save used to raise IntegrityError in some cases. This PR fixes that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, ta. Could you update the description?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a docstring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the issue description, it's misleading (but good idea, thanks!)

3 changes: 2 additions & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ django-discover-runner==1.0
flake8==2.2.5
flake8-docstrings==0.2.1
flake8-import-order==0.5.3
psycopg2==2.5.1
hypothesis==2.0.0
psycopg2==2.6.1
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
universal = 1
[flake8]
application-import-names = orderable
ignore = D100,D101,D102,D203,D204
ignore = D100,D101,D102,D104,D105,D203,D204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good old flake8-docstrings. What do you (and the rest of @incuna/backend) think about just not using it? We're ignoring quite a lot of the warnings here - is what's left even all that useful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to remove it, but that might as well be a different PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though actually, there's still a lot of errors we do catch: http://pep257.readthedocs.org/en/latest/error_codes.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no rush. (We'll want to remove it in our other projects as well, if everyone's on board with the idea.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly missing docstrings we ignore, along with whether there are blank lines before/after class docstrings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the list of errors, I think there are only one or two that are particularly significant. A lot of them are things we all do by habit, although they're still useful for newcomers in the future so that's not a good reason to remove it. (There's also a couple of stupid ones like "First line should be in imperative mood" :P)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do do most of these by habit, but that's the point of linting - enforcing good habits.

import-order-style = google
max-complexity = 10
max-line-length = 90
Expand Down