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

zharikovpro · 2016-12-22T22:02:03Z

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 · 2016-12-22T22:41:16Z

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

…as a workaround for the issue)

brendon · 2016-12-23T00:22:41Z

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 · 2016-12-23T12:40:04Z

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

zharikovpro · 2016-12-23T16:07:52Z

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.

…ositiveConstraintsTest

zharikovpro · 2016-12-23T20:11:16Z

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.

This reverts commit 1f5aedc.

…ls 3-2

zharikovpro · 2016-12-23T22:57:43Z

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.

brendon

Thanks @zharikovpro for your hard work on this :) I've left a few comments but agree with your approach overall. Let me know if anything doesn't make sense :)

brendon · 2016-12-29T09:11:38Z

lib/acts_as_list/active_record/acts/list.rb

+              # temporary move after bottom with gap, avoiding duplicate values
+              # gap is required to leave room for position increments
+              # positive number will be valid with unique not null check (>= 0) db constraint
+              tmp_position_after_everything = acts_as_list_class.unscoped.maximum(position_column).to_i + 2


Does this unscoping remove the acts_as_list scope? I assume you're not wanting to the position column to be globally unique? just unique per scope?

I assume you're not wanting to the position column to be globally unique?

I want exactly this to bypass unique index on table column constraint. Still, you're right that it's not needed. There could be no such situation when different scopes have same positions in one table with unique index.

brendon · 2016-12-29T09:14:26Z

lib/acts_as_list/active_record/acts/list.rb

+          # unique constraint prevents regular increment_all and forces to do increments one by one
+          # http://stackoverflow.com/questions/7703196/sqlite-increment-unique-integer-field
+          # both SQLite and PostgreSQL (and most probably MySQL too) has same issue
+          update_one_by_one = acts_as_list_list.connection.index_exists?(acts_as_list_list.table_name, position_column, unique: true)


I'm thinking this should be defined more globally (even though it's only used here) as it feels more like a configuration option. What do you think? You could set it at initialisation.

At first I thought that if you have unique index this is not an option, really, but a requirement. Still someone may want to disable this for performance reasons iif DB has deferred unique check on this table, and this will work too. Preparing update to make :sequential_updates option.

Also, this option could be used in other methods in future, definitely the way to go, thanks for the feedback.

brendon · 2016-12-29T09:16:23Z

lib/acts_as_list/active_record/acts/list.rb

+            )
+
+            if update_one_by_one
+              items.order("#{quoted_position_column_with_table_name} ASC").pluck(:id).each do |id|


Why are you plucking the id's only to re-find the records in order to decrement etc...? You could call .reload instead?

brendon · 2016-12-29T09:16:50Z

lib/acts_as_list/active_record/acts/list.rb

@@ -317,32 +317,53 @@ def increment_positions_on_all_items
        # Reorders intermediate items to support moving an item from old_position to new_position.
        def shuffle_positions_on_intermediate_items(old_position, new_position, avoid_id = nil)
          return if old_position == new_position
-          scope = acts_as_list_list
+          scope = acts_as_list_list.select(:id)


If you can remove the pluck then you can remove this too I think.

Yes, pluck will be removed, it's redundant. Still, wouldnt it be performance-wise to only select ids for repositioning? Otherwise DB response will be a little bit larger and slower.

… no pluck

… tests

…MixinNotNullUniquePositiveConstraintsTest

…when corresponding mysql table does not exist

zharikovpro · 2016-12-29T18:01:58Z

@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 · 2017-01-20T21:07:04Z

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

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.

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.
sequential_updates? is defined dynamically, cause it relies on sequential_updates_option from configuration.

Waiting for another great supportive review from you :)

brendon

Thanks @zharikovpro, we're nearly there. I think there are just a few tweaks to make to tidy things up :)

brendon · 2017-01-21T23:07:27Z

lib/acts_as_list/active_record/acts/sequential_updates_method_definer.rb

+    caller_class.class_eval do
+      @sequential_updates = nil
+
+      define_method :sequential_updates? do


class_eval allows us to just write normal Ruby to be executed within the scope of the class. So you can just do

caller_class.class_eval do private def sequential_updates? ... end end

define_method is primarily used to define a method with a string interpolated variable name as far as I've seen it. Give it a go and let me know if it doesn't work as expected.

So you can just do

Nope, calling sequential_updates? that way will produce 'undefined local variable or method `sequential_updates_option'. Cause, well, sequential_updates_option would be really not a local variable nor method (undefined). This definer trick makes it available for the sequential_updates? block, cause block do have access to the caller scope variables.

Another possible option is to store the whole acts_as_list configuration as a class variable after initialization. But this is not in the style of this gem.

Please check a perfect example of doing the same thing inside another definer. It defines method which implementation relies on config option. I do it the same way.

Ah I see :) Sorry about that :) I was focused on the instance variable and didn't realise you were talking about the incoming config :) I agree, no class variables is definitely preferred.

brendon · 2017-01-21T23:19:03Z

lib/acts_as_list/active_record/acts/sequential_updates_method_definer.rb

+module ActiveRecord::Acts::List::SequentialUpdatesDefiner #:nodoc:
+  def self.call(caller_class, column, sequential_updates_option)
+    caller_class.class_eval do
+      @sequential_updates = nil


It seems unintuitive to set a variable at this point. Perhaps set it within def sequential_updates? like so:

@sequential_updates = nil unless defined?(@sequential_updates)

Ok, removed class-level initializer.

brendon · 2017-01-21T23:22:17Z

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

zharikovpro · 2017-01-22T11:32:37Z

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 · 2017-01-22T21:11:41Z

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 :)

brendon · 2017-01-22T21:14:15Z

lib/acts_as_list/active_record/acts/sequential_updates_method_definer.rb

@@ -0,0 +1,21 @@
+module ActiveRecord::Acts::List::SequentialUpdatesDefiner #:nodoc:


Just realised that this class name doesn't match the file name. Can you change the class name as all the other definer classes are ...MethodDefiner :)

Ah, that eagle eye) Fixed.

…ss name

zharikovpro · 2017-01-22T22:25:50Z

@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 · 2017-01-22T22:27:51Z

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

brendon · 2017-01-23T00:09:11Z

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 · 2017-01-23T00:23:28Z

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

…ilds (1.14 is too fresh and causes errors)

zharikovpro · 2017-01-23T00:50:30Z

Fixed! 🎉

brendon · 2017-01-23T00:54:10Z

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 · 2017-01-23T00:57:15Z

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

fabn · 2017-01-23T20:13:24Z

@zharikovpro great job, congratulations.

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

6d7ddae

killed orphaned function store_at_0 (was invented earlier by someone …

567f867

…as a workaround for the issue)

zharikovpro added 7 commits December 23, 2016 19:12

wip: tests

8cd67ca

green tests

17d5ed2

one red test

9aeaa2d

green tests for sequential increments

dac0b19

SQLite and PostgreSQL tests green

dbac159

better test

a89b21f

green tests for SQLite and PostgreSQL for DefaultScopedNotNullUniqueP…

fa4c678

…ositiveConstraintsTest

zharikovpro added 7 commits December 23, 2016 23:54

work in progress: some appraisals fixes

1f5aedc

Revert "work in progress: some appraisals fixes"

51bd155

This reverts commit 1f5aedc.

tests: add_index compatible with rails 3-2

f529ad0

tests: rails 3-2 doesnt have .ids method for AR

0e2ed61

Rakefile restored (corrupted by accident, sorry)

49f5df6

DefaultScopedNotNullUniquePositiveConstraintsTest compatible with Rai…

dbf1807

…ls 3-2

better test index name

83cdf79

brendon requested changes Dec 29, 2016

View reviewed changes

zharikovpro added 6 commits December 29, 2016 14:29

code review for shuffle_positions_on_intermediate_items - no unscope,…

674aa88

… no pluck

sequential_update option for shuffle_positions_on_intermediate_items

3392191

sequential_updates configuration option description

58e81c2

wip: green tests for sequential_updates defaults, red NoAdditionMixin…

0dc8572

… tests

green tests with SequentialUpdatesDefault mixin and SequentialUpdates…

516053b

…MixinNotNullUniquePositiveConstraintsTest

sequential_updates default value detection shouldn't throw exception …

8036261

…when corresponding mysql table does not exist

Merge with origin repo's master

56fd879

removed redundant require

88d9af1

brendon requested changes Jan 21, 2017

View reviewed changes

brendon mentioned this pull request Jan 21, 2017

Comply to tests warnings #248

Merged

zharikovpro added 2 commits January 22, 2017 14:02

review code style tweak

5eb3e6e

code shortened after review

ec8f582

brendon approved these changes Jan 22, 2017

View reviewed changes

brendon reviewed Jan 22, 2017

View reviewed changes

ActiveRecord::Acts::List::SequentialUpdatesMethodDefiner - proper cla…

27e688a

…ss name

use latest stable bundler version which do not cause failed Travis bu…

1d458aa

…ilds (1.14 is too fresh and causes errors)

brendon merged commit cb8dfb5 into brendon:master Jan 23, 2017

zharikovpro deleted the unique_not_null branch January 23, 2017 00:57

This was referenced Jan 23, 2017

Discussion about unique indexes #135

Closed

Broken with unique constraint on position #245

Closed

zharikovpro mentioned this pull request Apr 6, 2017

Unique constraint violation on move_higher, move_lower, destroy #267

Closed

yjukaku mentioned this pull request Jun 5, 2018

Fix unique index constraint failure on item destroy #313

Merged

mgbatchelor mentioned this pull request Oct 22, 2018

Add touch on update configuration #329

Merged

tjwallace mentioned this pull request Apr 13, 2020

Only temporarily move item if using sequential updates #375

Closed

		@@ -0,0 +1,21 @@
		module ActiveRecord::Acts::List::SequentialUpdatesDefiner #:nodoc:

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

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

Conversation

zharikovpro commented Dec 22, 2016

zharikovpro commented Dec 22, 2016

brendon commented Dec 23, 2016

zharikovpro commented Dec 23, 2016

zharikovpro commented Dec 23, 2016 • edited Loading

zharikovpro commented Dec 23, 2016

zharikovpro commented Dec 23, 2016 • edited Loading

brendon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zharikovpro Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

zharikovpro commented Dec 29, 2016 • edited Loading

zharikovpro commented Jan 20, 2017

brendon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zharikovpro Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendon commented Jan 21, 2017

zharikovpro commented Jan 22, 2017

brendon commented Jan 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zharikovpro commented Jan 22, 2017 • edited Loading

zharikovpro commented Jan 22, 2017

brendon commented Jan 23, 2017

zharikovpro commented Jan 23, 2017

zharikovpro commented Jan 23, 2017

brendon commented Jan 23, 2017

zharikovpro commented Jan 23, 2017

fabn commented Jan 23, 2017

zharikovpro commented Dec 23, 2016 •

edited

Loading

zharikovpro commented Dec 23, 2016 •

edited

Loading

zharikovpro Dec 29, 2016 •

edited

Loading

zharikovpro commented Dec 29, 2016 •

edited

Loading

zharikovpro Jan 22, 2017 •

edited

Loading

zharikovpro commented Jan 22, 2017 •

edited

Loading