Support unique indexes when rebalancing ranks

[janklimo]

The goal of this PR is to improve support for apps having a unique index on their ranked columns.

The problem we were seeing in our app is that rebalancing would fail if it attempted to move a record to a position that is already in use (see the example in specs).

I went with the simplest approach possible: see if the target value already exists and if it does, decrement the value until we find an available one. I believe it's reasonable to expect there is always breathing room available for us to work with. E.g. given a million records, the gap used for rebalancing is 4295.

Eager to hear your thoughts!

[brendon]

Hi @janklimo :) Thanks for putting this together.

That's an interesting problem. I wonder if an alternative would be to wipe the rank values before rebalancing? We're going to throw them away anyway, and the operation should be safe if done within a transaction. What do you think?

[janklimo]

@brendon that's a good idea, should allow for a cleaner code. I'll look into it.

[brendon]

Thanks @janklimo, I have a flappy test that sometimes fails on Postgres (as you can see in the travis run). I've spent quite a while trying to debug it but I can never reproduce it locally.

There seems to be another failing combination with mysql. It looks like a config error. What do you think? :)

[janklimo]

@brendon I figured out the problem with the failing jruby spec is that that early version of activerecord-jdbcmysql-adapter needs a corresponding version of jdbc-mysql otherwise it will fail with a later one. I'm looking into why the other spec is failing.

[brendon]

Thanks for your hard work @janklimo, Just a quick note, you need to edit the Appraisals file then run appraisal install (or update?) which will update the gem files automatically.

Did you make any headway on that flappy test? Let me know when this is ready to merge :)

[brendon]

Thanks for the update. Can you confirm you ran appraisal install? I assume you did and the file generated matched the one you modified manually? If not, commit that new file and I'll merge this for you :)

[janklimo]

Hey @brendon yes I did and the output matched the changed I made previously.

Hard to say what made that one spec flaky. I cleaned it up a bit so I can debug what's wrong and it started to pass again so I never got to see where the problem was 😅

Just rebased the commits. Over to you 👍

[janklimo]

So it failed again. I'll look into it. Wish I could assign myself/reassign you to make the responsibility clearer.

[brendon]

Haha, yea it's perplexing. Let me know if there's anything I can do to help RE travis :)

[brendon]

Yes that's as far as I got. The items are slightly out of order:

869, 867, 870, 1001 expected 869, 870, 867, 1001 was

(had to use a differ to spot the difference! :D)

[brendon]

That seems to have fixed it? I'm rerunning the test suit to be sure :)

[janklimo]

Hi @brendon no, that's just to prove the error is caused by two records having the same row value before rebalancing. Which, now that I think about it, should never happen:

1. It leads to non-deterministic behavior when rebalancing - order will or will not be maintained. Either way, ranks will not remain equal after being rebalanced.
2. The gem has no API for setting the same value for two records - you need to resort to bypassing callbacks with update_all.

I remember acts_as_list has a section on data integrity. Maybe we should acknowledge the same best practice and have a similar recommendation?

[brendon]

Thanks @janklimo, that could be an idea. I did add a paragraph a while back around this concept:

IMPORTANT: Note that you MUST append _position to the column name when setting a new position on an instance. This is a fake column that can take relative as well as absolute index-based values for position.

It is a bit obscure and to be fair, when I started using this gem, I also got confused and tried to set the column directly.

At least your test clears up that flappy test. Postgres must be more sensitive to this issue for some reason. Are you happy to tidy up that test and back out the debug stuff and I can merge this for you :)

[brendon]

This looks great :) Thanks @janklimo :) I really appreciate your hard work on this.

[brendon]

I've released 0.4.6 for you.

[janklimo]

You're the best, thank you for your help with this @brendon 💯

[abhitrivedi]

reset_ranks! attempts to set the rank column to nil. This fails when the rank column is set as NOT NULL.

[brendon]

Sigh! :D restrictions are fun eh! :D I guess the question is, do you need a NOT NULL restraint on a column completely managed by this gem? As far as the developer is concerned, the gem should be able to use this column as it requires as long as you have a stable outcome (ordering is correct and maintained). We nullify the scope in a transaction, so if it fails it'll roll back to its original state.

If you can find a good way around this, please let me know and I'd be happy to look at a PR :)

[janklimo]

@brendon I could switch back to our previous strategy of not resetting ranks. Instead, we'd check if a rank is taken, then progressively lower the target rank by 1 until a unique value is available.

[brendon]

@janklimo, I'd be reluctant to do this as I think the nullifying solution works well and is cleaner. @abhitrivedi, is it possible to relax your restriction so that it only resolves after the transaction commits?