Touch when reordering

[botandrose]

Ran into an issue today with acts_as_list and fragment caching. I have a list of items that are expensive to render, so they are each being fragment cached. These items also can be reordered by drag-and-drop. However, only the item that was being dragged got its updated_at timestamp touched, so only that item's fragment cache got busted. The remaining items had stale caches with stale position information, even though this information had been updated in the db. Thus, this PR.

What do you think?

[houndci-bot]

Assignment Branch Condition size for test_removing_item_touches_all_lower_items is too high. [17/15]

[botandrose]

Test failure appears to be a red herring. Same failure is currently on master... looks like something related to JRuby dependencies.

[fabn]

@botandrose tests have been fixed in master. Could you please rebase?

Thanks.

[botandrose]

💚

[botandrose]

Ping

[brendon]

Can you rebase this @botandrose? I'll look at merging it then.

[botandrose]

Rebased.

[brendon]

Thanks for this :) Just a couple of questions.

1. Does it work to just chain .touch before the .update_all. Seems like this would make it easier to support different Rails versions as .touch's functionality has changed over time.
2. Failing that, you could have a custom method that executes .update_all for both the change required, plus updating any necessary timestamp columns. I like this less than option 1.

[botandrose]

Hi @brendon, thank you for taking a look at this!

1. Unfortunately, ActiveRecord does not support calling touch on a relation... only on single ActiveRecord instances. Hence my manual touch_all implementation.
2. To make sure I understand, you are suggesting that we take the approach I propose in this PR, with the alteration that we merge the touch_all and the update_all into one db query?

[brendon]

Ah I see. Yes that's a pain hey?

I don't think it'd hurt to introduce a new method (update_all_with_touch) or something like that that will accept the standard arguments for update_all and adds the touch functionality. It would certainly be dryer :)

[botandrose]

Ha, no big deal, I just wanted to make sure I understood what you were suggesting. I definitely agree with the overall sentiment. The duplication in this diff makes me cringe! I didn't want to go too deep into refactoring before knowing whether or not this was likely to be accepted. I'll give it a shot and report back!

[botandrose]

@brendon Okay, I've got something working here that I'm reasonably happy with. I've extracted most of the duplication to private methods, and the update is now performed with a single db query. WDYT?

[brendon]

Thanks for that, that's a lot cleaner.

Sorry to be a pain, but I've had another thought. I think we can avoid passing that scope around. Can you create class methods that can be chained off the scope? This is the same way update_all works already. What do you think?

[botandrose]

@brendon Ah, great idea! That's the API I wanted, but for some reason I was thinking that we'd have to monkeypatch AR::Relation in order to get it... I totally forgot about class methods being magically extended onto AR::Relation.

This is a definite improvement overall, I think, but there is getting to be quite a bit of indirection, and instantiating a new record just to get the timestamp fields is rather unfortunate. What do you think? Good to merge, or do you think we can improve this further?

[brendon]

Since this is very unlikey to change (could it ever?), perhaps we should be caching the timestamp attributes that we're going to update when acts_as_list is invoked?

[botandrose]

Hmm, the more I think about this, the more I actually think simply calling .new here is the way to go.

Since AR::Base#timestamp_attributes_for_update_in_model depends on #timestamp_attributes_for_update which is intended to be potentially overridden in the concrete class's implementation, we can't trust that the output of that method is static. It probably will be 99% of the time, but its possible that some people will experience weirdness if we cache.

Also, I'm pretty sure we can't reliably call .new in the .acts_as_list macro command, because the class definition hasn't finished executing yet, as this invocation is generally at the top.

With those drawbacks in mind, using .new here doesn't really seem so bad. It's definitely a bit strange, but I'm not seeing any other real downsides. I don't think it will have any performance or memory implications, because its only getting called once. Thoughts?

[brendon]

Getting close! :) I've just added some notes above. I'm keen to cache as much as we can, and also make this as flexible as possible so that it doesn't confuse anyone in the future. Let me know what you think.

[brendon]

Thanks @botandrose. Regarding caching the columns, I think that should be done up top when acts_as_list is called. We're about to merge some biggish changes to the structure of that method for Rails 5 compatibility, so perhaps wait until we've done that, then rebase and add those cached values. I'd imagine it'd be something along the lines of how we cache position_column:

def position_column
    '#{configuration[:column]}'
end

Breaking it down further, timestamp_attributes_for_update_in_model is basically just doing this:

[:updated_at, :updated_on].select { |c| self.class.column_names.include?(c.to_s) }

We could do this in acts_as_list as:

def timestamp_attributes
    [:updated_at, :updated_on].select { |c| column_names.include?(c.to_s) }
end

Also frustratingly this 'instance' method is only calling a class method:

def current_time_from_proper_timezone
    self.class.default_timezone == :utc ? Time.now.utc : Time.now
end

So you can just call: default_timezone == :utc ? Time.now.utc : Time.now and now you don't need to initialise a new object!

I hope that helps :D

[botandrose]

@brendon I think that is a perfectly reasonable approach, except for one big issue. What happens when one is using acts_as_list with an existing database schema that does not conform to Rails conventions?

Just like it is possible to override the position column in acts_as_list, its possible to override the default timestamp columns in ActiveRecord proper. The official way to do this in AR is to override #timestamp_attributes_for_create and #timestamp_attributes_for_update with the desired column names. I've had to do this myself several times over the years when hooking up AR to an existing db.

We agree that this .update_all_with_touch method should attempt to behave as close to the actual #touch method as possible, and I think this is another instance of that.

[brendon]

Ah I see, yes that does make a difference. I did wonder about how one was supposed to override the columns. It's a shame that these are instance methods and not class methods. I can't think of much of a use case for having these fields being configurable per instance of a class. I'll look into this later today and get back to you.

[botandrose]

@brendon Yeah, I definitely agree that they should have been class methods. I can imagine a scenario in which a record is a state machine, and in some states, some fields are touched, and in other states, some other fields are touched, but its pretty contrived. ¯_(ツ)_/¯

[botandrose]

@brendon Did you have a chance to take another look at this? Anything I can do to help get this merged?

[brendon]

Can we just call the method here? Like record.current_time_from_proper_timezone? :)

[botandrose]

Unfortunately, no. Since we're calling an instance method from the class level, we are hitting its public API, and #current_time_from_proper_timezone is private. If it was protected, we could call it directly, I believe.

[brendon]

Ah I see, yea that's a pain :)

[brendon]

Hey sorry about this. I had it marked to take a look but have been under the pump the last couple of weeks :)

Just had a couple of notes about the code, otherwise, you're right, we shouldn't get more specific than the Rails code on this one. I didn't consider people overriding those methods. We're doing some work elsewhere regarding using quoted_table_name to prefix the reference to the position column. I think we need to do this here too.

Also, I've just updated master with Rails 5 support. Can you rebase?

[botandrose]

No worries! Thanks for taking a look! I've rebased, and added a new commit that adds the fully qualified table names where possible. TIL you can't supply the table name for the left-hand side of an assignment (at least not in SQLite) which I guess makes sense, since you're already specifying the table name in the beginning of the UPDATE clause.

[brendon]

Thanks for fixing that one up. I remember seeing it but must have forgotten it.

[brendon]

Ok, I think this one is ready to go. I did just have one small thought about the name of the methods. Do you think touch_and_decrement_all or decrement_and_touch_all would be a better name? The with reminds me of method chaining.

[botandrose]

Yeah, the naming does evoke alias_method_chain.

Now that I think about it, what do you think about just .decrement_all and .increment_all? Ultimately, this PR is addressing the fact that it was surprising that the touching wasn't happening, when one would expect it to. With that in mind, why even bother to point out the touching in the name of those two methods?

.update_all_with_touch still sounds right to me, to differentiate from AR.update_all.

Thoughts?

[brendon]

I agree, let's run with that :) Let me know once you've updated it and I'll merge it.

[botandrose]

Done, and 💚. Thanks very much for your time on this, @brendon !

[brendon]

Excellent! :) Thank you very much for working so hard on this :) It's dedicated contributors like yourself that make these projects run smoothly. I'll probably release a gem in the near future once we've closed off a few more issues.