insert_at respects unique not null check (>= 0) db constraints

[zharikovpro]

Following this discussion.

Proposal to resolve db constraints issues. It targets insert_at cause it's often used by UI clients. Great demo of this is ActiveAdmin Reorderable gem.

[zharikovpro]

P.S. Battle-tested on a real project with unique not null constraint. Works like a charm.

[brendon]

Thanks @zharikovpro, that's a good solution. Are you able to add some tests to cover off what this change caters for? Just tests that would have failed without this change.

I assume it's also thread safe since the intermediate change is backed out before the transaction ends?

[zharikovpro]

It's thread safe because of with_lock. Will write minimal tests.

[zharikovpro]

Hmm, you pointed out that we need to go deeper with this issue. With unique constraint query like "UPDATE pos = pos +1" will fail. Here's more details.

Really two options there. One is to make increment in two passes, like

      define_singleton_method :update_all_with_touch_unique_workaround do |updates|
        if connection.index_exists?(caller_class.table_name, column, unique: true)
          # unique constraint prevents regular increment_all, so we use work-around with negative values
          # http://stackoverflow.com/questions/7703196/sqlite-increment-unique-integer-field
          # it's not specific to SQLite only, PostgreSQL has same issue
          update_all_with_touch "#{quoted_position_column} = -#{quoted_position_column_with_table_name}"
          update_all_with_touch updates.sub('= (', '= (-')
        else
          update_all_with_touch updates
        end
      end

I dislike this, cause it still wont allow CHECK > 0 constraint if someone needs it. Also I suppose gem itself does not handle negative positions well. Still, this solution is locally tested by me, holded.

Another approach will be to do one-by-one updates inside increment_all, moving rows from the very bottom to the gap. This will allow CHECK > 0 constraint and surely will prevent someone of entering negative position into db, which is great in my opinion. Performance is the price for correctness in that case, and additional time will be required for me to add tests for the positive CHECK.

@brendon do you think it's the right way to do it? I still want to make it right asap, if you support release.

[zharikovpro]

Ok, finally managed to add db constraints that will fail test suite DefaultScopedNotNullUniquePositiveConstraintsTest on my local machine. Also implemented sequential update for both SQLite and PostgreSQL. Sadly, forgot to test with appraisals. Hope it will be sorted out anytime soon, too.

[zharikovpro]

Green light! One-by-one increments/decrements will only be enabled upon unique index detection. Following the principle of least astonishment and backwards compatibility, old users will get the same exact behaviour as before with new build. And with new gem version when someone adds unique index to the column (can also add check >= 0) it will just work.

[zharikovpro]

@brendon all your comments made sense and changes were made, thank you. Please check update with new sequential_updates option. It has smart default behaviour, covered by tests.

Hope for the review even if not this year ;) and wish you a lot of fun at winter holidays 🎉

[zharikovpro]

@brendon it's great that you enjoy sunny days :) I've been swamped with work and hope will get to your proposals soon. Can you give an example of issues with database connection requirement on load, if it's not too much hassle for you? Making option lazily evaluated will make specs a little more complicated, though I agree it's a good thing to do. As always, thanks for all clues.

[brendon]

Thanks @zharikovpro :) Good to hear you're busy though!

Ideally when a Rails application boots up we shouldn't be accessing the database at this point as we're assuming a lot about the application we're in (perhaps they switch databases as part of the boot up process etc...). In multi-tenancy databases sometimes the application boots up only with access to the central schema (the set of tables that contain a list of all the tenants etc...) then upon the first request, they switch schema's. The current code you have would fail as the default schema would probably not include the table that the model requires.

I know it's a strange detail, but I've personally had this problem in the past. Better to (if we can) not access the database until the first request too.

[zharikovpro]

@brandon, thanks for clarification! This makes perfect sense, and multi-tenant apps are very important. Definitely worth supporting them, sure will do.

[zharikovpro]

@brendon, your review suggestions were implemented. Some notes:

1) sequential_updates? was made private and contains lazily evaluated option in cases when it's not specified via acts_as_list options.

Initially, I thought about making this a class-level function and evaluate only once for every instance. But you showed a case with multiple tenants, where earlier established connection can be altered, so I intentionally made this evaluation instance-level. As I can see, ActiveRecord caches tables and indexes info, so this should result in no performance penalty (i.e. no db hit during this check). If one want to avoid this type of checks at all, he can explicitly set sequential_updates option to true or false upon mixin initialization.

2) It's debatable topic whether or not tests should ever test private methods behavior, like I test sequential_updates? Again, I think it's better than not to test it, cause that "smart detection" in case of missing option may have implementation flaws without it. And testing this indirectly via public-only methods is a lot more complex and completely untransparent.

3) sequential_updates? is defined dynamically, cause it relies on sequential_updates_option from configuration.

Waiting for another great supportive review from you :)

[brendon]

@zharikovpro, definitely test the private methods unit test style. :)

[zharikovpro]

Hmm, somehow Travis build is failing with 'Bundler could not find compatible versions for gem "thread_safe"' - @brendon , can you check it, please? It clearly looks as an issue unrelated to my code changes and I wonder how it can be fixed.

[brendon]

Thanks @zharikovpro, I think travis is having a jruby problem at the moment. I've initiated a rebuild so we'll see if that works :)

Otherwise I'm happy with the changes as they sit now and will approve them once the tests pass :)

[zharikovpro]

@brandon, I've noticed some changes in build logs, they may be related with recently broken jruby builds.

Your latest green master build 4 days ago uses bundler-1.13.7. My green builds use it, too. Exactly 3 days ago bundler 1.14.0 was released, and next day - bundler 1.14.1. Latest failing builds are using it, and bundle install fails.

Smells like an issue with bundler version. Could you try lock bundler version on Travis and see if it helps? I've tested this locally, and it worked on osx, same jruby version, same bundler version. Still, Travis uses linux. And it really looks like bundler version update issue, or a really rare coincidence.

[zharikovpro]

By the way, bundler 1.14 announcement is not in official blog, yet.

[brendon]

Good idea @zharikovpro :) I'm happy for you to fix the bundler version in the travis.yml file in this PR (with a comment about the reason in the file) to get this suite green :)

[zharikovpro]

I do not know how to fix this using travis.yml 😊 , and only can google for a solution. One points to a build customization..

[zharikovpro]

Fixed! 🎉

[brendon]

Well done! I'm going to release 0.9.0 that will include this. Thank you so much for your work on this and I look forward to working with you again in the future :)

[zharikovpro]

Cheers! Can't wait to update gem version in production 😎

[fabn]

@zharikovpro great job, congratulations.