Fix bug where order is wrong after adding objects #56
Conversation
This problem was caused by using the count of the objects in the intermediary join table as the starting sort value. This was insufficient because deleting objects would not update the sort values. So imagine with a fresh database, you added 5 objects then deleted 3. The max sort value would be 4 (the 5th object would use the count prior to adding, hence 4), but when you add a new object, its sort value would be 2 (current count of intermediary objects), NOT 5! This fixes that by querying for the maximum sort value in use by the source object's intermediary objects and using that as the starting sort value. Since bulk_create() was introduced in Django 1.4 and there is a dummy atomic() implementation (that actually was not being used), there is no need to use a different implementation to create intermediary objects on Django 1.5. Also, this ensures the right database is being used for the transaction, which was not the case before.
| @@ -316,31 +311,9 @@ def get_rel_to_model_and_object_name(self, klass): | |||
| to_object_name = to_model._meta.object_name | |||
| return to_model, to_object_name | |||
|
|
|||
| def get_intermediate_model_sort_value_field_default(self, klass): | |||
| def default_sort_value(name): | |||
There was a problem hiding this comment.
Why do you remove the callable here to create the default? What's the benefit of calculating in the _add_items method?
There was a problem hiding this comment.
Hi! I'll try my best to explain... First of all, in general, calculating default values shouldn't have to hit the database. Doing so would hide away potential performance issues, and wouldn't really cooperate with Django migrations. It also could be the source of bugs if it depended on the caller knowing that it hits the database and thus ensuring it is called inside the appropriate atomic transaction.
In this case, we were using the get_default value only as a starting point for subsequent values (except in the Django 1.5 case where it was hitting the database every time). Now, you can read inline exactly how the starting point is calculated and be sure it's inside the transaction and thus "correct". Also, this important, it technically wouldn't quite work to keep this calculation inside get_default because it depends on the source object's primary key (it finds the maximum sort value the exists for that source object and increments from there).
Hope that helps to clear things up, thanks!
|
Hey, thanks a lot for this patch. I will review the code and post comments inline. |
|
Thank you for lining out your intend! It makes sense and I merged your code with commit ba6d5db. However this PR is not marked as merged as I modified the commit slightly (whitespace changes). I just released 1.0.2 containing your fix. |
|
Awesome, thanks @gregmuellegger! |
This problem was caused by using the count of the objects in the intermediary join table as the starting sort value. This was insufficient because deleting objects would not update the sort values.
So imagine with a fresh database, you added 5 objects then deleted 3. The max sort value would be 4 (the 5th object would use the count prior to adding, hence 4), but when you add a new object, its sort value would be 2 (current count of intermediary objects), NOT 5!
This fixes that by querying for the maximum sort value in use by the source object's intermediary objects and using that as the starting sort value.
Since bulk_create() was introduced in Django 1.4 and there is a dummy atomic() implementation (that actually was not being used), there is no need to use a different implementation to create intermediary objects on Django 1.5. Also, this ensures the right database is being used for the transaction, which was not the case before.