-
-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
insert_at respects unique not null check (>= 0) db constraints #246
Conversation
P.S. Battle-tested on a real project with unique not null constraint. Works like a charm. |
…as a workaround for the issue)
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? |
It's thread safe because of with_lock. Will write minimal tests. |
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
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. |
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this option could be used in other methods in future, definitely the way to go, thanks for the feedback.
) | ||
|
||
if update_one_by_one | ||
items.order("#{quoted_position_column_with_table_name} ASC").pluck(:id).each do |id| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you plucking the id's only to re-find the records in order to decrement etc...? You could call .reload
instead?
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can remove the pluck
then you can remove this too I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…MixinNotNullUniquePositiveConstraintsTest
…when corresponding mysql table does not exist
@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 🎉 |
@brendon, your review suggestions were implemented. Some notes:
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.
Waiting for another great supportive review from you :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @zharikovpro, we're nearly there. I think there are just a few tweaks to make to tidy things up :)
caller_class.class_eval do | ||
@sequential_updates = nil | ||
|
||
define_method :sequential_updates? do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
module ActiveRecord::Acts::List::SequentialUpdatesDefiner #:nodoc: | ||
def self.call(caller_class, column, sequential_updates_option) | ||
caller_class.class_eval do | ||
@sequential_updates = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, removed class-level initializer.
@zharikovpro, definitely test the private methods unit test style. :) |
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. |
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 :) |
@@ -0,0 +1,21 @@ | |||
module ActiveRecord::Acts::List::SequentialUpdatesDefiner #:nodoc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that eagle eye) Fixed.
@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. |
By the way, bundler 1.14 announcement is not in official blog, yet. |
Good idea @zharikovpro :) I'm happy for you to fix the bundler version in the |
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)
Fixed! 🎉 |
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 :) |
Cheers! Can't wait to update gem version in production 😎 |
@zharikovpro great job, congratulations. |
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.