Updating the position fails uniqueness constraint.

[BookOfGreg]

Hi team,

Problem

insert_at, and potentially other methods in this gem, generates SQL that breaks uniqueness constraints.

Solution

insert_at should order the sortable field highest first to avoid collisions.

To Reproduce

Given this table (Server version: 10.1.22-MariaDB , Pretends to be MySQL 5.7.x)

CREATE TABLE `articles` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `created_at` datetime NOT NULL,
  `updated_at` datetime NOT NULL,
  `position` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`),
  UNIQUE KEY `index_articles_on_position` (`position`)
) ENGINE=InnoDB AUTO_INCREMENT=52 DEFAULT CHARSET=utf8;

And this matching ActiveRecord class

class Article < ActiveRecord::Base
  acts_as_list
end

Article.create(position: 5)
Article.create(position: 4)
Article.create(position: 3)
Article.create(position: 2)
Article.create(position: 1)

a = Article.new
a.insert_at(2)

The insert_at generates the SQL:

UPDATE `articles` SET `position` = (`articles`.`position` + 1), `updated_at` = '2017-05-11 09:11:59.730023' WHERE (1 = 1) AND (`articles`.`position` >= '2');

With error:

Duplicate entry '3' for key 'index_articles_on_position'

I expected SQL of

UPDATE `articles` SET `position` = (`articles`.`position` + 1), `updated_at` = '2017-05-11 09:11:59.730023' WHERE (1 = 1) AND (`articles`.`position` >= '2') ORDER BY position DESC;

[brendon]

Hi @BookOfGreg, can you tell me if you're using the :sequential_updates option. If not, try that and see if it fixed things.

acts_as_list sequential_updates: true

Though @swanandp, I suppose it can't hurt to update in descending order anyway. I've tested it and all the tests pass still. What do you think?

[swanandp]

Agreed @brendon , though we should add this specific test case against it as a failing spec.

[BookOfGreg]

@brendon I wasn't using that option but just chose to roll my own for now as I was only using a small subset of this gems functionality.

I found that if you move things up the list, I needed it in :desc order, if you move it down the list I needed :asc order. Also since I had a default_scope with an order, I needed to reorder to override it.
Think this is relevant to this section, I can see why you asked about sequential updates. If I may ask, why is that not default?: https://github.com/swanandp/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L355

Edit: My implementation:
https://gist.github.com/BookOfGreg/656ab4e6ffbfca75c6e0fe37d1543ff0

Edit2:
Your suspicions were correct.
https://github.com/swanandp/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L333

[brendon]

Hi @BookOfGreg, it's not the default because it wasn't in the past and most people don't run a unique constraint on this column I think, though perhaps they should. I personally don't.

Would you be keen to create a PR for us with tests for this change? Reordering in the correct direction is definitely fine, and we always reorder anyway to remove existing order scopes as you can see elsewhere in the code.

For the tests, it could be as simple as running the sequential update tests with the unique constraints turned on but with sequential_updates: false, then fix what breaks where possible.

[BookOfGreg]

Maybe this deserves to be a separate issue but the gem doesn't seem to be reordering with sequential_updates enabled, when running with sequential updates I'm still seeing this error containing my default order scope:

ORDER BY `articles`.`published_at` DESC, `articles`.`created_at` DESC

I'll be digging more into this in a couple days.

[brendon]

That's interesting. Yes do some more digging. I'm wondering if the sequential updates thing will even be necessary if we make the database shuffle things in the correct order?

[brendon]

Hi @BookOfGreg, how did you get on with this?

[BookOfGreg]

Rolled my own, was much easier than using the gem unfortunately.

[brendon]

All good :) I'll close this for now.