Invert order when incrementing to circumvent unique index violations

[swelther]

I use this gem in combination with an unique index. When I try to insert a new item at the top of the list I get Mysql2::Error: Duplicate entry error in the increment_positions_on_lower_items method.

This is because MySQL checks on each updated row if it violates the unique index, even when the update statement is called inside of an transaction.

The reversed order fixes this bug by incrementing the 'highest' values first so the unique index is still unique on every row.

[swelther]

Hi @swelther, thanks for your PR :)

You might have already found the work done previously on sequential updates. Is this just something they missed? Would you mind double-checking their work to see if this problem wasn't already solved in another way? (e.g. by enabling sequential updates--which is off by default)

Interesting, I'm 99% sure I tried with sequential updates but it didn't work - at least I remember like :D It seems it does, but I wonder if it is a good idea? Especially for big tables, wouldn't it execute N update statements instead of one? Which can have a big performance impact.

[swelther]

Could be anyway a good idea to document the sequential_updates option in the readme 😉

[brendon]

Indeed, the sequential updates code makes a lot more queries but that was their solution to the problem. If you think there's a more efficient way, we should explore that, but I think it'd be best to make sure all the sequential update code is updated to the new method. I hope that makes sense :D I'm a bit time poor but if you are happy to work on it, I'll be happy to review the PR's :)

[brendon]

I agree RE the documentation. Again, if you're interested, I'd happily accept a PR documenting the sequential updates feature.

[swelther]

I think I know what you mean :D I'll take a look and check if I can change the sequential update code. Could it be a good idea to make this configurable? Like either use the n updates-version or the reversed order update one? I'm not an database expert, maybe there are situations when the later one does not work?

Anyway, will make a PR to document the option :)

[brendon]

Thanks @swelther, I really appreciate that. I think it'd be worthwhile looking at if the reversed order updates version could be the only one if it works for all the existing tests. There are tests around sequential updates already. The n-updates version always bothered me a little bit but it worked for the people who wanted it :)

[swelther]

Alrighty, I'll take care of it :)

[swelther]

It seems my changes work with MySQL, but not Sqlite and Postgresql.

BTW: Adding sequential_updates: true does not fix my problem. I think the increment_positions_on_lower_items method is missing the sequentially check.

I still think it would be nice to use the one-statement update instead of the slower n-statement one :) Maybe I find an idea how to do that.

[brendon]

Thanks @swelther, let me know once you're ready for me to look at your solution :)

[swelther]

@brendon Please have a look :) I think it is as good as it can be at this time ;)

[brendon]

Looks good to me. I'll merge this. If you ever feel like progressing the full-transaction approach to reordering (if it's even possible), I'll be more than happy to review your PR's :D

[swelther]

@brendon I will, but for now it is ok to do the sequential update one because there are not that many items that it would hurt :)

May I ask you to build a new release? Would help me finishing my feature that depends on this bug to be fixed :)

[brendon]

Hi @swelther, I sure can :) I've released 1.0.1 :)

[swelther]

@brendon Thank you very much :)