Merge pull request #2 from dabroz/escape-column-name

Bugfix: escaped_sortable_position_column for raw SQL queries

lib/activerecord/sortable/acts_as_sortable.rb

@@ -16,13 +16,8 @@ def acts_as_sortable(&block)
           }
           block.call(options) if block_given?
 
-          cattr_accessor :sortable_relation, instance_reader: false, instance_writer: false
-          cattr_accessor :sortable_append, instance_reader: true, instance_writer: false
-          cattr_accessor :sortable_position_column, instance_reader: true, instance_writer: false
-
-          self.sortable_relation = options[:relation]
-          self.sortable_append = options[:append]
-          self.sortable_position_column = options[:position_column]
+          sortable_relation_create_accessors
+          sortable_relation_provide_accessor_values(options)
 
           scope "ordered_by_#{sortable_position_column}_asc".to_sym, -> { order(arel_table[sortable_position_column].asc) }
 
@@ -31,6 +26,22 @@ def acts_as_sortable(&block)
 
           include ActiveRecord::Sortable::ActsAsSortable::InstanceMethods
         end
+
+        private
+
+        def sortable_relation_create_accessors
+          cattr_accessor :sortable_relation, instance_reader: false, instance_writer: false
+          cattr_accessor :sortable_append, instance_reader: true, instance_writer: false
+          cattr_accessor :sortable_position_column, instance_reader: true, instance_writer: false
+          cattr_accessor :escaped_sortable_position_column, instance_reader: true, instance_writer: false
+        end
+
+        def sortable_relation_provide_accessor_values(options)
+          self.sortable_relation = options[:relation]
+          self.sortable_append = options[:append]
+          self.sortable_position_column = options[:position_column]
+          self.escaped_sortable_position_column = ActiveRecord::Base.connection.quote_column_name(options[:position_column])
+        end
       end
     end
   end

lib/activerecord/sortable/acts_as_sortable/instance_methods.rb

@@ -20,14 +20,14 @@ def move_to!(new_position)
 
         def sortable_relation_shift_left(new_position)
           sortable_relation
-            .where(["#{sortable_position_column} > ? AND #{sortable_position_column} <= ?", send(sortable_position_column), new_position])
-            .update_all sortable_updates_with_timestamps("#{sortable_position_column} = #{sortable_position_column} - 1")
+            .where(["#{escaped_sortable_position_column} > ? AND #{escaped_sortable_position_column} <= ?", send(sortable_position_column), new_position])
+            .update_all sortable_updates_with_timestamps("#{escaped_sortable_position_column} = #{escaped_sortable_position_column} - 1")
         end
 
         def sortable_relation_shift_right(new_position)
           sortable_relation
-            .where(["#{sortable_position_column} >= ? AND #{sortable_position_column} < ?", new_position, send(sortable_position_column)])
-            .update_all sortable_updates_with_timestamps("#{sortable_position_column} = #{sortable_position_column} + 1")
+            .where(["#{escaped_sortable_position_column} >= ? AND #{escaped_sortable_position_column} < ?", new_position, send(sortable_position_column)])
+            .update_all sortable_updates_with_timestamps("#{escaped_sortable_position_column} = #{escaped_sortable_position_column} + 1")
         end
 
         def sortable_relation
@@ -54,7 +54,7 @@ def sortable_append_instance
         end
 
         def sortable_prepend_instance
-          sortable_relation.update_all sortable_updates_with_timestamps("#{sortable_position_column} = #{sortable_position_column} + 1")
+          sortable_relation.update_all sortable_updates_with_timestamps("#{escaped_sortable_position_column} = #{escaped_sortable_position_column} + 1")
           send("#{sortable_position_column}=".to_sym, 0)
         end
 
@@ -71,7 +71,9 @@ def sortable_updates_with_timestamps(base_query)
         end
 
         def sortable_shift_on_destroy
-          sortable_relation.where(["#{sortable_position_column} > ?", send(sortable_position_column)]).update_all sortable_updates_with_timestamps("#{sortable_position_column} = #{sortable_position_column} - 1")
+          position_threshold = send(sortable_position_column)
+          position_update = sortable_updates_with_timestamps("#{escaped_sortable_position_column} = #{escaped_sortable_position_column} - 1")
+          sortable_relation.where(["#{escaped_sortable_position_column} > ?", position_threshold]).update_all position_update
           true
         end
       end

spec/dummy-3.2/app/models/order_thing.rb

@@ -0,0 +1,5 @@
+class OrderThing < ActiveRecord::Base
+  acts_as_sortable do |config|
+    config[:position_column] = :order
+  end
+end

spec/dummy-3.2/db/migrate/20150408101403_create_order_things.rb

@@ -0,0 +1,11 @@
+class CreateOrderThings < ActiveRecord::Migration
+  def change
+    create_table :order_things do |t|
+      t.integer :order, :null => false
+
+      t.timestamps null: false
+    end
+
+    add_index :order_things, [:order]
+  end
+end

spec/dummy-3.2/db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20140721161122) do
+ActiveRecord::Schema.define(:version => 20150408101403) do
 
   create_table "children", :force => true do |t|
     t.string   "name",       :null => false
@@ -31,6 +31,14 @@
 
   add_index "items", ["position"], :name => "index_items_on_position"
 
+  create_table "order_things", :force => true do |t|
+    t.integer  "order",      :null => false
+    t.datetime "created_at", :null => false
+    t.datetime "updated_at", :null => false
+  end
+
+  add_index "order_things", ["order"], :name => "index_order_things_on_order"
+
   create_table "other_things", :force => true do |t|
     t.integer  "place",      :null => false
     t.datetime "created_at", :null => false

spec/dummy-4.0/app/models/order_thing.rb

@@ -0,0 +1,5 @@
+class OrderThing < ActiveRecord::Base
+  acts_as_sortable do |config|
+    config[:position_column] = :order
+  end
+end

spec/dummy-4.0/db/migrate/20150408101403_create_order_things.rb

@@ -0,0 +1,11 @@
+class CreateOrderThings < ActiveRecord::Migration
+  def change
+    create_table :order_things do |t|
+      t.integer :order, :null => false
+
+      t.timestamps null: false
+    end
+
+    add_index :order_things, [:order]
+  end
+end

spec/dummy-4.0/db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20140721161122) do
+ActiveRecord::Schema.define(version: 20150408101403) do
 
   create_table "children", force: true do |t|
     t.string   "name",       null: false
@@ -31,6 +31,14 @@
 
   add_index "items", ["position"], name: "index_items_on_position"
 
+  create_table "order_things", force: true do |t|
+    t.integer  "order",      null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end
+
+  add_index "order_things", ["order"], name: "index_order_things_on_order"
+
   create_table "other_things", force: true do |t|
     t.integer  "place",      null: false
     t.datetime "created_at"

spec/dummy-4.1/app/models/order_thing.rb

@@ -0,0 +1,5 @@
+class OrderThing < ActiveRecord::Base
+  acts_as_sortable do |config|
+    config[:position_column] = :order
+  end
+end

spec/dummy-4.1/db/migrate/20150408101403_create_order_things.rb

@@ -0,0 +1,11 @@
+class CreateOrderThings < ActiveRecord::Migration
+  def change
+    create_table :order_things do |t|
+      t.integer :order, :null => false
+
+      t.timestamps null: false
+    end
+
+    add_index :order_things, [:order]
+  end
+end

spec/dummy-4.1/db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20140721161122) do
+ActiveRecord::Schema.define(version: 20150408101403) do
 
   create_table "children", force: true do |t|
     t.string   "name",       null: false
@@ -31,6 +31,14 @@
 
   add_index "items", ["position"], name: "index_items_on_position"
 
+  create_table "order_things", force: true do |t|
+    t.integer  "order",      null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end
+
+  add_index "order_things", ["order"], name: "index_order_things_on_order"
+
   create_table "other_things", force: true do |t|
     t.integer  "place",      null: false
     t.datetime "created_at"

spec/dummy-4.2/app/models/order_thing.rb

@@ -0,0 +1,5 @@
+class OrderThing < ActiveRecord::Base
+  acts_as_sortable do |config|
+    config[:position_column] = :order
+  end
+end

spec/dummy-4.2/db/migrate/20150408101403_create_order_things.rb

@@ -0,0 +1,11 @@
+class CreateOrderThings < ActiveRecord::Migration
+  def change
+    create_table :order_things do |t|
+      t.integer :order, :null => false
+
+      t.timestamps null: false
+    end
+
+    add_index :order_things, [:order]
+  end
+end

spec/dummy-4.2/db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20140721161122) do
+ActiveRecord::Schema.define(version: 20150408101403) do
 
   create_table "children", force: :cascade do |t|
     t.string   "name",       null: false
@@ -31,6 +31,14 @@
 
   add_index "items", ["position"], name: "index_items_on_position"
 
+  create_table "order_things", force: :cascade do |t|
+    t.integer  "order",      null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end
+
+  add_index "order_things", ["order"], name: "index_order_things_on_order"
+
   create_table "other_things", force: :cascade do |t|
     t.integer  "place",      null: false
     t.datetime "created_at"

spec/order_thing_spec.rb

@@ -0,0 +1,6 @@
+require 'spec_helper'
+
+# test for model with position column named `orded` (which can cause problems with raw SQL queries)
+describe OrderThing do
+  it_behaves_like 'activerecord-sortable', position_column: :order
+end