Fix AM/AR::Dirty plugin compatibility issues with AR 5.2

[shioyama]

Context

ActiveRecord 5.2.beta.2 has just been released, and we are now testing against it (see #112). There are about twenty specs failing currently, mostly around the Dirty plugin.

The main issue seems to be with rails/rails#28661 (fixing rails/rails#28660), which replaced a __send__(attr) to _attributes(attr), and ultimately to _read_attribute(attr). This is problematic since while in ActiveModel _read_attribute simply maps to __send__, which for a localized accessor like title_en will correctly return the current value of the title, in AR it maps to this:

def _read_attribute(attr_name, &block) # :nodoc
  @attributes.fetch_value(attr_name.to_s, &block)
end

Here, @attributes will not contain the localized translated attributes, so title_en here will return nil. So most of the AR::Dirty specs are failing with changes all coming out as [nil, nil] instead of e.g. [nil, "foo"].

Possible Fix

I'm currently investigating if we could override _read_attribute to do something special for translated attributes, but it's tricky because in the dirty plugin we're using localized attributes to track changes in multiple locales. This is tricky since without a complex regex, we can't really tell if a given attribute passed to _read_attribute is actually a translated attribute or not.

I don't want to be matching regexes every time _read_attribute is called since this is a very frequently-used method and regexes are slow, so this could significantly impact performance. For now just tracking the problem here, I'll see if there are other ways to fix the problem.

[shioyama]

We may need to consider changing from tracking dirty changes on attribute locale accessors, to tracking changes on the attributes themselves.

i.e., instead of what we do currently:

post.title_changes
#=> { "title_en" => [nil, "foo"], "content_en" => [nil, "bar"]}

we would have:

post.title_changes
#=> { "title" => [nil, "foo"], "content" => [nil, "bar"]}

This is, incidentally, what Globalize does. I opted to go with localized accessors so that Mobility could track the full set of locale-specific changes on translated attributes, i.e. if you change the title in English and in French, you will see each change separately. I prefer this approach, but we're running up against problems with how AR is increasingly trying to force everything to specify attribute names upfront, whereas localized names are open-ended (in general, we don't want to assume a set of predefined locales).

That said, currently the dirty plugin depends on fallthrough accessors to actually fetch the localized values of attributes, which uses method_missing and is thus slow. You can get around that by explicitly defining the localized accessors as methods using locale_accessors: [:en, ...] for the accessors you actually use, but I suppose many people don't do that and may thus be incurring a performance hit when using dirty...

[pwim]

There currently seems to be three ways to specify locale accessors:

1. Explicitly
2. Implicitly via I18n.available_locales
3. Dynamically via fallthrough_accessors

Unless I'm missing something, in case 1 and 2, we could explicitly declare the attributes. So the challenging case to handle is 3.

In my case, my app only supports Japanese and English, and am using the locale specific accessors. I'd have no problem with being required to declare them up front.

The case where you don't want to declare them up front is where your app supports arbitrary translation. In this case, you don't know up front which locales your going to need to support, so you can't declare them. Similarly though, if you don't know what locales your app is going to use, you probably aren't going to be using locale specific accessors for the same reason. Instead, you'd be setting the attribute either implicitly via the current locale (post.title = value) or explicitly (post.send(:title=, value, locale: :en)).

This leads me to wonder if there really is a use case that would require someone to use fallthrough_accessors, and there isn't, perhaps we could just deprecate them.

[shioyama]

You're right that it's an edge case we might not want to put too much emphasis on. I guess I like the design of having both fallthrough + explicit locales, whereby you can define explicit locales and yet still fallthrough to any locale.

A use case for this is for example, your app supports mainly a small set of locales, which you set explicitly, but you also want to allow support for new locales organically, perhaps with a performance hit at first.

In the case of the Dirty plugin, fallthrough was (I thought) a nice way to allow the plugin to be used without requiring the user to explicitly specify the locales they want to support upfront (it will work with or without them).

If we did not have fallthrough accessors, I suppose what we would be able to do would be in _read_attribute just to check if the attr is included in attributes with all available locales, which is a fixed array so should not be too heavy performance-wise. With the current spec, we'd need to do a regex match like this one, or maybe something a bit simpler, but my concern is that whereas the current regex is checked in method_missing (so already we're in slow-land), if we put that in _read_attribute then it impacts every attribute fetch.

So I'm not really happy with this. The annoying thing is that everything works fine for ActiveModel, since AM still simply calls __send__. It's ActiveRecord overriding _read_attribute that is the problem.

[pwim]

A use case for this is for example, your app supports mainly a small set of locales, which you set explicitly, but you also want to allow support for new locales organically, perhaps with a performance hit at first.

But do we need explicit accessors for these organic locales? If your app isn't doing something explicitly with Portuguese (for instance), why would you have code like post.title_pt in it? I could see you doing something like iterating through all locales, but then wouldn't your code look like post.title(locale: locale)?

In the case of the Dirty plugin, fallthrough was (I thought) a nice way to allow the plugin to be used without requiring the user to explicitly specify the locales they want to support upfront (it will work with or without them).

Agreed that we don't want to require users to explicitly specify locales to do dirty tracking.

[shioyama]

So here's one solution: 9b55ccf

What I do is to first check if @attributes has the attribute, which will be the case for any column. This avoids checking the regex if we are simply fetching a "normal" attribute. If the attribute is not a key on @attributes, then we resort to first check if it is a translated attribute (without a locale), which is just a check for an element in an array. If we don't find it there, we match against the regex as a last resort, and if it's not there we again fallthrough to super.

This is not a solution yet - we're checking the regex once, then calling the method, which (if no method is defined for that locale accessor) will go to method_missing and again check the regex. To avoid that you could just do what fallthrough accessors is doing direclty in the _read_attribute override, but at that point the whole point of splitting them up as modules is pretty much lost.

So... need to think about this more I guess.

[shioyama]

But do we need explicit accessors for these organic locales? If your app isn't doing something explicitly with Portuguese (for instance), why would you have code like post.title_pt in it? I could see you doing something like iterating through all locales, but then wouldn't your code look like post.title(locale: locale)?

Ah I see, so your point is that we could support open-ended locales, just without supporting explicit locale accessors. That's a good point, and in that case I think it would be natural that you can use the Dirty plugin for locales you want to track, but not for the implicit ones.

[shioyama]

I'm leaning toward leaving fallthrough accessors as a plugin, but making Dirty require a fixed set of locales (from the union of I18n.available_locales and whatever locales are passed into LocaleAccessors, if it is applied to the attribute). This will require a bit more work though. I think you're right though that this is preferable to making the code more complex to handle a very rare use case.