bulk update through act_as_list_no_update

[randoum]

To newcomers : The conversation for this PR continues here #244

Allow to perform bulk update by disabling act_as_list callbacks

ActiveRecord::Base.act_as_list_no_update do
  TodoItem.find(5).update position: 17
  TodoItem.find(8).update position: 11
end

inspired from http://api.rubyonrails.org/classes/ActiveRecord/NoTouching/ClassMethods.html

[randoum]

@swanandp the CI error comes from bundle

Please install the sqlite3 adapter: gem install activerecord-sqlite3-adapter (sqlite3 is not part of the bundle. Add it to Gemfile.)

Shall I do something about that?

[swanandp]

I think that calls for a separate PR. Go for it!

On Sat, Oct 3, 2015 at 11:05 AM, randoum notifications@github.com wrote:

@swanandp https://github.com/swanandp the CI error comes from bundle

Please install the sqlite3 adapter: gem install activerecord-sqlite3-adapter (sqlite3 is not part of the bundle. Add it to Gemfile.)

Shall I do something about that?

— Reply to this email directly or view it on GitHub https://github.com/swanandp/acts_as_list/pull/171#issuecomment-145201692 .

[randoum]

Ok, noted. Let's focus on this one first if you don't mind. What's your feedback?

[randoum]

@swanandp any feedback? Thanks.

[swanandp]

Sorry, got held up in other things, will come back to this shortly.

[randoum]

Ok sure.

[randoum]

@swanandp any feedback? Thanks.

[fabn]

@randoum tests have been fixed in master. Could you please rebase and fix hound issues as well?

Thanks.

[randoum]

Rebased and pushed.

Guys, houndCI is really an annoyance.

• I had to change all my single quoted strings into double quoted string. That's OK for consistency, but the rest of the code that is not concerned by my commit is left single quoteed. So there is no point, tough I started to fix some of them.
• Same with line length, I started to fix and revert the changes.
• And with the private that need an empty line before and after, so did I but the error still here. And again other parts of the code does not respect this convention.

I'm done. And I really suggest you remove houndCI, it's a pure waste of time, or if you really want to keep it please set-it-up less restrictive and update all your code for consistency.

Let me know if I can provide more help, I'll be happy to (except concerning houndCI :D) Cheers

[randoum]

@fabn @swanandp I was going through my old notifications, and remember about this PR. You guys made me work a lot to comply with houndCI, which I did. But then I received no news from you. You shall understand that providing work then being ignored is a bit frustrating.

[swanandp]

Hi @randoum, really sorry you feel frustrated. We turned off hound because it was turning out to be more an annoyance than a good tool.

And thanks a lot for taking the time to fix the hound issues, we really appreciate it.

Regards, Swanand

On Sunday 20 March 2016, randoum notifications@github.com wrote:

@fabn https://github.com/fabn @swanandp https://github.com/swanandp I was going through my old notifications, and remember about this PR. You guys made me work a lot to comply with houndCI, which I did. But then I received no news from you. You shall understand that providing work then being ignored is a bit frustrating.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/swanandp/acts_as_list/pull/171#issuecomment-198866517

[brendon]

Hi @randoum, I've had a look over this and it looks ok. @swanandp are you happy to merge this? I can do it for you if you like?

[brendon]

@randoum, could you rebase this. I'll look to merge it then :)

[brendon]

@randoum, I've updated master again. Can you rebase this and check your code carefully as some of the initialisation routine has been changed and you'll need to mirror this in your disabled class.

Can you also respond to let us know you're still around and interested in working on this? Otherwise we'll have to close the issue and PR until someone wants to push it.

[randoum]

Copy, thanks Give me some times and I will do it. Regards

On May 2, 2016, at 01:02, Brendon Muir notifications@github.com wrote:

@randoum https://github.com/randoum, I've updated master again. Can you rebase this and check your code carefully as some of the initialisation routine has been changed and you'll need to mirror this in your disabled class.

Can you also respond to let us know you're still around and interested in working on this? Otherwise we'll have to close the issue and PR until someone wants to push it.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/swanandp/acts_as_list/pull/171#issuecomment-216080016

[brendon]

Thanks @randoum :) That's all good. Just wanted to make sure you were still around :)

[randoum]

Done. Please see my comment in lib/acts_as_list/active_record/acts/list.rb:400

[brendon]

Hi @randoum :) Welcome back :)

This should be a quick thing to fix, but I think you should be using the native conditional functionality of callbacks here instead of these return statements.

So:

after_destroy :decrement_positions_on_lower_items, unless: acts_as_list_callbacks_disabled?

Are you able to fix that up?

[randoum]

The conditional of callback would not cover all cases. Example: decrement_positions_on_lower_items is called from store_at_0. And we also have to plan ahead for futures version when added method may call decrement_positions_on_lower_items.

[brendon]

store_at_0 isn't used now that I look closer at it. Also, are we trying to negate side effects from directly calling these methods, or just saying that we don't want any callbacks triggered when dealing with these objects (CRUD). I'd say if someone uses an acts_as_list method directly on the object, that method should still fire and have effect. Just the automatic features (and as far as I know, they're only triggered through the callbacks) should be disabled.

My fear is that one day someone is going to add a new method and forget to check for the disabled state and that'll cause problems. At least if the check is done on the callback it's more visible, and new callbacks are less likely to be written.

@fabn, do you have an opinion on this?

[randoum]

That's exactly my point, and that's why you should place your conditions statement where the action is made (i.e where the database instructions are), it is much less error prone than to add conditions in all the callers (which includes callbacks).

[brendon]

I can't envisage a situation where one would be invoking the aal methods manually and not want them to take effect? Surely you're only wanting to stop us touching the database as a result of a CRUD on the object (i.e. a touching of the database triggered by our callbacks?). In that case the callbacks are the perfect place to do this. I need a good example of manual method calls that shouldn't take effect. Can you provide one? Perhaps from your original use case?

I'm sorry it's taking so long to get this settled, I'm just not feeling good about the code as it stands and don't want to commit us to future pain for the sake of a side-feature.

The other way might be some sort of adapter abstraction that mutes the database calls globally? Seems like overkill and I'm not sure it's even possible though.

[randoum]

Just that it a best-practice to interrupt the callchain at the central endpoint where the action is taken, and not everywhere the callchain can/may/could start.

But it's your gem so if you want it that way I'll do it, even if it looks like bad coding.

[brendon]

I'd normally agree, but we know the starting points, and they're not added by the end user. Your initial comment on this PR also says it: "Allow to perform bulk update by disabling act_as_list callbacks". It just seems in this case that that's the most logical place to halt something that the gem should have full control over (its own callbacks).

I'm happy to relent if you can show me a case where you're triggering SQL execution via this gem outside the invocation of the callbacks but also without directly using this gems methods (why would you use the methods if you didn't want them to execute SQL?)

@swanandp, if you could chime in on this one I'd really appreciate it :)

[swanandp]

@brendon Sure, I will be able to do a full review this Saturday.

[brendon]

Thanks @swanandp :)

[wikyd]

Hi, I would love to have this functionality and I'm willing to assist however needed. Happy to complete this last refactor, rebase again, add more specs, etc. Whatever it takes :)

[brendon]

Thanks @wikyd, I'm just waiting on @swanandp's opinion on this. As you can see above, I'm not that keen on adding checks everywhere, and would instead prefer limiting the execution of the callbacks themselves.

@swanandp, did you come up with an opinion either way?

[swanandp]

First of all, I am generally wary of false positives around Thread.current and other thread related errors ( i.e. not a fan on Thread local variables).

Secondly, this API is better offered on an object level, so we can disable individual callbacks, Rails style ( e.g. :if => :run_callbacks )

I'd much prefer something like:

TodoItem.find(5).acts_as_list_no_callback do
  update(:position => 5)
end

# or 

TodoItem.find(5).acts_as_list_no_callback do |record|
  record.update(:position => 5)
end

[nickpoorman]

I could really use this right now... 😞 I'm updating thousands of items in a list and it's wreaking havoc on my database.

[brendon]

Hi @nickpoorman, sorry to hear that. As a workaround you could use update_columns to circumvent all the callback logic. Or if you feel like chipping in on this PR please do :) It's at a bit of an impasse at the moment.

@wikyd, now that @swanandp has had his say, would you like to push this through along his recommendations?

[wikyd]

I would like to help, but the proposed changes does not work with our use case. We have a Javascript front-end that allows users to re-order and remove items from the list. Then, we take this set of changes and update our items in a batch use Rails params -> model magic. Turning off callbacks for a specific model does not help in that case.

[brendon]

@wikyd, I've always found this to be the most troubling part of using acts_as_list. Unless you use the relative move methods or set one position at a time, the callbacks can get in the way. I often use this relation extension:

module Sortable
  def sort(array_of_ids)
    array_of_ids.each.with_index(1) do |id, index|
      where(:id => id).update_all(:position => index)
    end
  end
end

As a way to mass-assign positions passed in as a list of id's. It can be limited if the list of id's isn't all the id's in the scope though.

[nickpoorman]

@wikyd - I'm actually doing the same thing. We have a bulk upsert endpoint that does a create/destroy/update all in one. For this route, I had to disable paper trail also, as it was creating massive amounts of rows in our versions table. They seemed to have solved this "disable" issue this way: https://github.com/airblade/paper_trail/blob/05dd9fb40848c2c8e7079eac7fccffb54720db9e/lib/paper_trail/record_trail.rb#L425

ie.

@widget.paper_trail.without_versioning do
  @widget.update_attributes :name => 'Ford'
end

...and it works quite well.

[brendon]

@nickpoorman, they're using some kind of class variable to track the disabled state? I'd assume that wasn't thread safe.

[nickpoorman]

@brendon - I believe it is thread safe: https://github.com/airblade/paper_trail/pull/328

[nickpoorman]

@brendon - I see what they did. They used https://github.com/steveklabnik/request_store to store the state. https://github.com/airblade/paper_trail/blob/8b9ce72f2c449becb3d75b094b9b07fb64b6e908/lib/paper_trail.rb#L62

[brendon]

@nickpoorman, ah yes, I use that in my applications too. It's handy, but may be brittle to add as a dependency in our case? Will there be any downsides say in the console?

[brendon]

I noticed ancestry has a similar method though I've not investigated how they do theirs.

[nickpoorman]

@brendon, as far as I can tell request_store is a wrapper for Thread.current with some middleware to clean up after requests so I see no reason why it wouldn't work in the console.

[brendon]

Yes I suppose if the store variable is set and unset cleanly after the block executes then that would be fine.

Here's how ancestry is doing it: https://github.com/stefankroes/ancestry/blob/master/lib/ancestry/instance_methods.rb#L296

It's an instance method but is quite spread out in terms of its application in the code.

@swanandp, do you have any more thoughts given the latest comments here?

[swanandp]

@brendon Definitely need to do the blocking on object level. Let's avoid class or thread level variables unless we have to, and we don't have to here.

[brendon]

Thanks @swanandp. @wikyd, @randoum given this direction, would one of you like to refactor this so that it uses an instance variable and also if possible includes just checks for this variable on the callback declarations rather than inline in the code?

[randoum]

Personally I have zero availability. Just a note about request store. It is used in several gems for quite some times and seems to be maturely thread safe.

[brendon]

Thanks @randoum, that's all good. I have no doubts about the quality of RequestStore but as @swanandp says in this case, it's not necessary to use a thread level variable so I'd rather not add an unnecessary dependency. Thanks for your input on this PR to date. :)

[swanandp]

That's exactly right. I've used RequestStore in the past quite effectively, but I don't think it's needed here.

On Wed, 7 Dec 2016 at 1:45 PM, Brendon Muir notifications@github.com wrote:

Thanks @randoum https://github.com/randoum, that's all good. I have no doubts about the quality of RequestStore but as @swanandp https://github.com/swanandp says in this case, it's not necessary to use a thread level variable so I'd rather not add an unnecessary dependency. Thanks for your input on this PR to date. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/swanandp/acts_as_list/pull/171#issuecomment-265383918, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFjGLOthRgDpAVLGjX2sOPC5PYReqJ0ks5rFmsrgaJpZM4GFe7p .

[randoum]

I am not sure we can do without Thread.current.

If you can do @widget.paper_trail.without_versioning {@widget.update_attributes :name => 'Ford'} it's because the without_versioning statement concerns only @widget, which is a single record.

In our case an act_as_list_no_update block would apply to the behavior of multiple records or a whole model.

In terms of use case, you would never write:

TodoItem.find(5).acts_as_list_no_callback do
  update(:position => 5)
end

Because it's plain useless.

---

Instead, you would be using acts_as_list_no_callback for mass updates, mass ordering, mass import or such. Hence the statements would be concerning multiple records.

How can you write a block statement that concern multiple record? There's only one way, by using a block at the class level :

TodoItem.acts_as_list_no_callback do
  mass_import_method
end

2 choices: class instance variable or Thread.current variable. The second one is way safer.

So I understand you guys fears toward Thread.current, but I'd like to remind you that my original PR was based on http://api.rubyonrails.org/classes/ActiveRecord/NoTouching/ClassMethods.html which is part or rails core and using Thread.current.

If rails core team is using it, we can. We are using Thread.current in a block context, no around thread synchronization or between different webserver sessions, it's safe to use in our case. The only important thing is to clear the thread variable in an ensureblock.

---

Some reading : http://stackoverflow.com/questions/7896298/safety-of-thread-current-usage-in-rails

[brendon]

Hi @randoum, you make a good point :) @swanandp, can you chime in?

@randoum, your latest rebase has reverted a bunch of changes on master (Appraisal config, CHANGELOG etc...)

[randoum]

Yeah I messed up big time with git. I have no idea what I did. Finally I deleted my fork and I already started from a fresh one.

[randoum]

Kindly continue there #244