insert_at respects unique not null check (>= 0) db constraints (#246)

.travis.yml

@@ -5,7 +5,7 @@ cache: bundler
 # and https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments
 sudo: false
 before_install:
-  - gem update bundler
+  - gem install bundler -v 1.13.7
 before_script:
   - mysql -e 'create database acts_as_list;'
   - psql -c 'create database acts_as_list;' -U postgres

lib/acts_as_list.rb

@@ -5,4 +5,5 @@
 require "acts_as_list/active_record/acts/add_new_at_method_definer"
 require "acts_as_list/active_record/acts/aux_method_definer"
 require "acts_as_list/active_record/acts/callback_definer"
-require 'acts_as_list/active_record/acts/no_update'
+require 'acts_as_list/active_record/acts/no_update'
+require "acts_as_list/active_record/acts/sequential_updates_method_definer"

lib/acts_as_list/active_record/acts/list.rb

@@ -10,8 +10,12 @@ class << ActiveRecord::Base
   #   act more like an array in its indexing.
   # * +add_new_at+ - specifies whether objects get added to the :top or :bottom of the list. (default: +bottom+)
   #                   `nil` will result in new items not being added to the list on create.
+  # * +sequential_updates+ - specifies whether insert_at should update objects positions during shuffling
+  #   one by one to respect position column unique not null constraint. 
+  #   Defaults to true if position column has unique index, otherwise false.
+  #   If constraint is <tt>deferrable initially deferred<tt>, overriding it with false will speed up insert_at.
   def acts_as_list(options = {})
-    configuration = { column: "position", scope: "1 = 1", top_of_list: 1, add_new_at: :bottom}
+    configuration = { column: "position", scope: "1 = 1", top_of_list: 1, add_new_at: :bottom }
     configuration.update(options) if options.is_a?(Hash)
 
     caller_class = self
@@ -23,6 +27,7 @@ def acts_as_list(options = {})
 
     ActiveRecord::Acts::List::AuxMethodDefiner.call(caller_class)
     ActiveRecord::Acts::List::CallbackDefiner.call(caller_class, configuration[:add_new_at])
+    ActiveRecord::Acts::List::SequentialUpdatesMethodDefiner.call(caller_class, configuration[:column], configuration[:sequential_updates])
 
     include ActiveRecord::Acts::List::InstanceMethods
     include ActiveRecord::Acts::List::NoUpdate
@@ -316,6 +321,10 @@ def increment_positions_on_all_items
         end
 
         # Reorders intermediate items to support moving an item from old_position to new_position.
+        # unique constraint prevents regular increment_all and forces to do increments one by one
+        # http://stackoverflow.com/questions/7703196/sqlite-increment-unique-integer-field
+        # both SQLite and PostgreSQL (and most probably MySQL too) has same issue
+        # that's why *sequential_updates?* check alters implementation behavior
         def shuffle_positions_on_intermediate_items(old_position, new_position, avoid_id = nil)
           return if old_position == new_position
           scope = acts_as_list_list
@@ -329,47 +338,59 @@ def shuffle_positions_on_intermediate_items(old_position, new_position, avoid_id
             #
             # e.g., if moving an item from 2 to 5,
             # move [3, 4, 5] to [2, 3, 4]
-            scope.where(
+            items = scope.where(
               "#{quoted_position_column_with_table_name} > ?", old_position
             ).where(
               "#{quoted_position_column_with_table_name} <= ?", new_position
-            ).decrement_all
+            )
+
+            if sequential_updates?
+              items.order("#{quoted_position_column_with_table_name} ASC").each do |item|
+                item.decrement!(position_column)
+              end
+            else
+              items.decrement_all
+            end
           else
             # Increment position of intermediate items
             #
             # e.g., if moving an item from 5 to 2,
             # move [2, 3, 4] to [3, 4, 5]
-            scope.where(
+            items = scope.where(
               "#{quoted_position_column_with_table_name} >= ?", new_position
             ).where(
               "#{quoted_position_column_with_table_name} < ?", old_position
-            ).increment_all
+            )
+
+            if sequential_updates?
+              items.order("#{quoted_position_column_with_table_name} DESC").each do |item|
+                item.increment!(position_column)
+              end
+            else
+              items.increment_all
+            end
           end
         end
-
+        
         def insert_at_position(position)
           return set_list_position(position) if new_record?
           with_lock do
             if in_list?
               old_position = send(position_column).to_i
               return if position == old_position
-              shuffle_positions_on_intermediate_items(old_position, position)
+              # temporary move after bottom with gap, avoiding duplicate values
+              # gap is required to leave room for position increments
+              # positive number will be valid with unique not null check (>= 0) db constraint
+              temporary_position = acts_as_list_class.maximum(position_column).to_i + 2
+              set_list_position(temporary_position)
+              shuffle_positions_on_intermediate_items(old_position, position, id)
             else
               increment_positions_on_lower_items(position)
             end
             set_list_position(position)
           end
         end
 
-        # used by insert_at_position instead of remove_from_list, as postgresql raises error if position_column has non-null constraint
-        def store_at_0
-          if in_list?
-            old_position = send(position_column).to_i
-            set_list_position(0)
-            decrement_positions_on_lower_items(old_position)
-          end
-        end
-
         def update_positions
           old_position = send("#{position_column}_was") || bottom_position_in_list + 1
           new_position = send(position_column).to_i

lib/acts_as_list/active_record/acts/sequential_updates_method_definer.rb

@@ -0,0 +1,21 @@
+module ActiveRecord::Acts::List::SequentialUpdatesMethodDefiner #:nodoc:
+  def self.call(caller_class, column, sequential_updates_option)
+    caller_class.class_eval do
+      define_method :sequential_updates? do
+        if !defined?(@sequential_updates)
+          if sequential_updates_option.nil?
+            table_exists = caller_class.connection.table_exists?(caller_class.table_name)
+            index_exists = caller_class.connection.index_exists?(caller_class.table_name, column, unique: true)
+            @sequential_updates = table_exists && index_exists
+          else
+            @sequential_updates = sequential_updates_option
+          end
+        else
+          @sequential_updates
+        end
+      end
+
+      private :sequential_updates?
+    end
+  end
+end

test/test_list.rb

@@ -7,10 +7,13 @@
 
 def setup_db(position_options = {})
   $default_position = position_options[:default]
+
+  # sqlite cannot drop/rename/alter columns and add constraints after table creation
+  sqlite = ENV.fetch("DB", "sqlite") == "sqlite"
 
   # AR caches columns options like defaults etc. Clear them!
   ActiveRecord::Base.connection.create_table :mixins do |t|
-    t.column :pos, :integer, position_options
+    t.column :pos, :integer, position_options unless position_options[:positive] && sqlite
     t.column :active, :boolean, default: true
     t.column :parent_id, :integer
     t.column :parent_type, :string
@@ -19,6 +22,20 @@ def setup_db(position_options = {})
     t.column :state, :integer
   end
 
+  if position_options[:unique] && !(sqlite && position_options[:positive])
+    ActiveRecord::Base.connection.add_index :mixins, :pos, unique: true
+  end
+
+  if position_options[:positive]
+    if sqlite
+      # SQLite cannot add constraint after table creation, also cannot add unique inside ADD COLUMN
+      ActiveRecord::Base.connection.execute('ALTER TABLE mixins ADD COLUMN pos integer8 NOT NULL CHECK (pos > 0) DEFAULT 1')
+      ActiveRecord::Base.connection.execute('CREATE UNIQUE INDEX index_mixins_on_pos ON mixins(pos)')
+    else
+      ActiveRecord::Base.connection.execute('ALTER TABLE mixins ADD CONSTRAINT pos_check CHECK (pos > 0)')
+    end
+  end
+
   # This table is used to test table names and column names quoting
   ActiveRecord::Base.connection.create_table 'table-name' do |t|
     t.column :order, :integer
@@ -115,6 +132,14 @@ def self.for_active_false_tests
   end
 end
 
+class SequentialUpdatesDefault < Mixin
+  acts_as_list column: "pos"
+end
+
+class SequentialUpdatesFalseMixin < Mixin
+  acts_as_list column: "pos", sequential_updates: false
+end
+
 class TopAdditionMixin < Mixin
   acts_as_list column: "pos", add_new_at: :top, scope: :parent_id
 end
@@ -818,3 +843,42 @@ def test_nil_position_ordering
     assert_equal [2, 3, 1], DefaultScopedMixin.all.map(&:id)
   end
 end
+
+class SequentialUpdatesOptionDefaultTest < ActsAsListTestCase
+  def setup
+    setup_db
+  end
+
+  def test_sequential_updates_default_to_false_without_unique_index
+    assert_equal false, SequentialUpdatesDefault.new.send(:sequential_updates?)
+  end
+end
+
+class SequentialUpdatesMixinNotNullUniquePositiveConstraintsTest < ActsAsListTestCase
+  def setup
+    setup_db null: false, unique: true, positive: true
+    (1..4).each { |counter| SequentialUpdatesDefault.create!({pos: counter}) }
+  end
+
+  def test_sequential_updates_default_to_true_with_unique_index
+    assert_equal true, SequentialUpdatesDefault.new.send(:sequential_updates?)
+  end
+
+  def test_sequential_updates_option_override_with_false
+    assert_equal false, SequentialUpdatesFalseMixin.new.send(:sequential_updates?)
+  end
+
+  def test_insert_at
+    new = SequentialUpdatesDefault.create
+    assert_equal 5, new.pos
+
+    new.insert_at(1)
+    assert_equal 1, new.pos
+
+    new.insert_at(5)
+    assert_equal 5, new.pos
+
+    new.insert_at(3)
+    assert_equal 3, new.pos
+  end
+end