diff --git a/.travis.yml b/.travis.yml index f5d5b966..17ff275e 100644 --- a/.travis.yml +++ b/.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 diff --git a/lib/acts_as_list.rb b/lib/acts_as_list.rb index e0e99e2d..bcf3647a 100644 --- a/lib/acts_as_list.rb +++ b/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' \ No newline at end of file +require 'acts_as_list/active_record/acts/no_update' +require "acts_as_list/active_record/acts/sequential_updates_method_definer" diff --git a/lib/acts_as_list/active_record/acts/list.rb b/lib/acts_as_list/active_record/acts/list.rb index 4f95ee78..dcc08346 100644 --- a/lib/acts_as_list/active_record/acts/list.rb +++ b/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 deferrable initially deferred, 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,31 +338,52 @@ 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 @@ -361,15 +391,6 @@ def insert_at_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 diff --git a/lib/acts_as_list/active_record/acts/sequential_updates_method_definer.rb b/lib/acts_as_list/active_record/acts/sequential_updates_method_definer.rb new file mode 100644 index 00000000..84a20e5b --- /dev/null +++ b/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 diff --git a/test/test_list.rb b/test/test_list.rb index d84f1197..76a7bb99 100644 --- a/test/test_list.rb +++ b/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