Discussion about unique indexes

[fabn]

Hi @swanandp this PR is not meant to be merged but only to start a discussion on uniqueness constraints.

In current codebase unique indexes cannot be added to position columns otherwise lot of tests will fail. This is because there are some queries which swap values between rows and other issues with update order.

For instance a simple UPDATE table SET position = position + 1 will lead to duplicate values unless updates are executed in order (with a non standard SQL ORDER BY in UPDATE queries).

I think is worth to mention this in the readme because for list behavior I'd expect to be able to do one of the following

add_index :table, :position, unique: true # for unscoped lists
add_index :table, [:parent_id, :position], unique: true # for scoped lists

In any case it will be possible to make unique index working by using this approach in MySQL and using DEFERRABLE constraints in Postgres. I don't know if this is possible in SQLite.

Also a lot of queries can be rewritten using a standard CASE statement (example) to halve the number of executed queries and to avoid unique issue in Postgres.

If you're interested in this I can try to identify problematic method and they can be rewritten by wrapping them in a new method, something like

# Obviously this need to be customized according to adapter, this version is for MySQL
def without_unique_checks(&block)
  connection.execute('SET UNIQUE_CHECKS = 0;')
  block.call
  connection.execute('SET UNIQUE_CHECKS = 1;')
end

# For instance
def move_to_top
  without_unique_checks do
    increment_positions_on_higher_items
    assume_top_position
  end
end

What do you think?

[fabn]

Another comment on this, the CASE approach will also solve potiential issues like the one I got in #131 and the one shown in this screencast: http://rhnh.net/2010/06/30/acts-as-list-will-break-in-production

[swanandp]

Thanks @fabn , I will look at it shortly. Currently off on a vacation!

[unabl4]

Is this still alive?

[brendon]

I think this is worthy of pursuing. It would reduce bugs around multiple processes working on the list at the same time (with locking) etc...

[swanandp]

Definitely worth pursuing. @fabn the current implementation might not work for MySQL. Have you tested if it does?

[fabn]

@swanandp I did not tested it, it was just a proof of concept. I'm afraid it won't work in MySQL and there's no trace of DEFERRED CONSTRAINT support in both MySQL and Sqlite.

[brendon]

That's unfortunate. Do you want to close this for now?

[fabn]

That's unfortunate. Do you want to close this for now?

I don't know. I still think that we need to support uniqueness constraint, imho it's very important that this gem supports it. We have 2 choices here:

1. Use the single query approach and make it working with unique constraint under PostgreSQL and leave the situation as is for other DBs.
2. Rewrite some methods to work with unique constraint.

Latter approach can be implemented by clearing current element position before updating other list elements, in this way even with immediate constraints there won't be any issue in database.

I did a quick test in a local branch with this change applied on top of unique index commit in this PR

diff --git a/lib/acts_as_list/active_record/acts/list.rb b/lib/acts_as_list/active_record/acts/list.rb
index f7adb0a..004c188 100644
--- a/lib/acts_as_list/active_record/acts/list.rb
+++ b/lib/acts_as_list/active_record/acts/list.rb
@@ -146,6 +146,7 @@ module ActiveRecord
           return unless lower_item

           acts_as_list_class.transaction do
+            set_list_position(nil) # Avoid duplicate positions
             lower_item.decrement_position
             increment_position
           end
@@ -156,6 +157,7 @@ module ActiveRecord
           return unless higher_item

           acts_as_list_class.transaction do
+            set_list_position(nil) # Avoid duplicate positions
             higher_item.increment_position
             decrement_position
           end
@@ -166,6 +168,7 @@ module ActiveRecord
         def move_to_bottom
           return unless in_list?
           acts_as_list_class.transaction do
+            set_list_position(nil) # Avoid duplicate positions
             decrement_positions_on_lower_items
             assume_bottom_position
           end
@@ -176,6 +179,7 @@ module ActiveRecord
         def move_to_top
           return unless in_list?
           acts_as_list_class.transaction do
+            set_list_position(nil) # Avoid duplicate positions
             increment_positions_on_higher_items
             assume_top_position
           end
@@ -184,8 +188,9 @@ module ActiveRecord
         # Removes the item from the list.
         def remove_from_list
           if in_list?
-            decrement_positions_on_lower_items
-            set_list_position(nil)
+            current_position = send(position_column).to_i
+            set_list_position(nil) # Avoid duplicate positions
+            decrement_positions_on_lower_items(current_position)
           end
         end

I still have a lot of failing tests but they decreased from 62 in master with unique index to 45 after this change. There is still a lot of work to do because all methods used to adjust list positions (decrement_positions_on_lower_items, increment_positions_on_lower_items, shuffle_positions_on_intermediate_items) rely on self.position being present to do their job.

There is a lot of work to do to make this working but it might be worth to try.

[brendon]

Ok, yes you have a good point. I did think about nulling the value and there's certainly nothing wrong with that, especially within a transaction. Definitely keep going if you're happy to :)

[brendon]

@fabn, how about using a temporary table to generate the updates then merge this all at once? I've seen mention of that out there. Not sure if it's workable across all database types. At least we have a test suite that tests all the databases now though.

[zharikovpro]

Wonder why it was not posted/discussed there.. maybe this solution could be incorporated into codebase?

[brendon]

#245

[zharikovpro]

I did think about nulling the value and there's certainly nothing wrong with that, especially within a transaction

This will fail not null constraint. Say, I want to enforce not null constraints to have list of items without missing pieces. It's important case. null can be replaced with -1, for example. It wont allow strict constraint like (position > 0), still way better than nullable column.

[brendon]

Closed by #246

[faucct]

Can anyone clarify why this wasn't accepted? The CASE expression way would work in any database.

[brendon]

Hi @faucct, I think this PR wasn't meant to be merged (See the opening comment). If you'd like to pursue this though, check through all the referenced tickets in this thread to get a sense of the history of the problem and what is currently in place (#246), then maybe put up a PR for us to see? :)