Fix _changed? BC break and introduce algolia_dirty? method

[julienbourdeau]

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Related Issue	Fix #140
Need Doc update	yes

Describe your change

We recently made some change to handle the new will_save_change_to_#{attr}? instead of the deprecated #{attr}_changed?. See #325

I didn't realize that this was breaking backward compat' because we check your ActiveRecord version. So if you upgrade, existing methods named #{attr}_changed? won't be called.

I refactored it to the "first responding method", both will be tested, regardless of your Rails version. I added more tests for this.

algolia_dirty? method

I added this in the same PR but it could be removed to merge the fix independantly. I added a method called algolia_dirty?, you can define it to override the calls to all _changed? method. I think this could be useful in case you need to define many _changed? method or to handle multiple relationships.

Please any feedback on this one is appreciated 🙏

class Ebook < ActiveRecord::Base
  include AlgoliaSearch

  algoliasearch :synchronous => true, :index_name => safe_index_name("eBooks")do
    attributesToIndex [:name]
  end

  def algolia_dirty?
    self.author.custom_modified_recently?
  end
end

📝 DOC: Doc update is one the way.

[jeromedalbert]

Wouldn't this code essentially always call #{attr_name}_changed?, since on any random Rails model, calling model.respond_to?(:xxx_changed?) returns true for all model attributes?

If so, this would essentially nullify #325, which means that on Rails 5.1 this gem would revert to its old behavior i.e. it would start emitting warnings again for all algolia attributes with the same name as a model attribute.

As a reminder, the deprecation warning in Rails 5.1 was:

DEPRECATION WARNING: The behavior of attribute_changed? inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method after save returned (e.g. the opposite of what it returns now). To maintain the current behavior, use saved_change_to_attribute? instead.

I believe this warning was removed in Rails 5.2 as they changed the behavior (I guess). But xxx_changed? still exists and will likely always exist.

So unless there is some way to know if xxx_changed was defined by the end user, instead of redefined by Rails, I am not sure this fix would work.

[julienbourdeau]

Yes, I agree. I don't know why I didn't see it myself. I guess after trying a few things, I started ignoring some cases. I was really looking to restore the previous behavior. 😅
I definitely don't want to see those warning in more recent rails version 👍

[jeromedalbert]

What is the problem this PR is trying to fix? Is it to call xxx_changed? methods again? But why call them again, since their behavior is deprecated/changed in Rails?

Is it because people redefine xxx_changed? But why would they do that? The only use case I can see is for custom/complex attributes as documented in https://github.com/algolia/algoliasearch-rails#custom-attribute-definition:

## Custom attribute definition

You can use a block to specify a complex attribute value

```ruby
class Contact < ActiveRecord::Base
  include AlgoliaSearch

  algoliasearch do
    attribute :email
    attribute :full_name do
      "#{first_name} #{last_name}"
    end
    add_attribute :full_name2
  end

  def full_name2
    "#{first_name} #{last_name}"
  end
end

***Notes:*** As soon as you use such code to define extra attributes, the gem is not anymore able to detect if the attribute has changed (the code uses Rails's `#{attribute}_changed?` method to detect that). As a consequence, your record will be pushed to the API even if its attributes didn't change. You can work-around this behavior creating a `_changed?` method:

```ruby
class Contact < ActiveRecord::Base
  include AlgoliaSearch

  algoliasearch do
    attribute :email
    attribute :full_name do
      "#{first_name} #{last_name}"
    end
  end

  def full_name_changed?
    first_name_changed? || last_name_changed?
  end
end

^ If the goal of this PR is to support this use case and only this use case (i.e. assuming people only define _changed for custom/complex attributes), then I see these possible solutions:

1. Make the code in this PR call xxx_changed? only if xxx is a custom/complex attribute (a custom/complex attribute is basically an attribute that is not a Rails attribute i.e.!has_attribute?(:xxx)). Otherwise call will_save_change_to_xxx.

2. Change the doc instructions+code not to use xxx_changed? for complex attributes, i.e. replace _changed by a new term in order not to conflict with Rails terminology, and instruct users to update their codebases to use that new term (ouch). Indeed, going forward the terminology for changed? means something different from its previous meaning. Which is unfortunate because this makes the Algolia doc current instructions for xxx_changed? diverge with the new meaning of Rails xxx_changed?.

   Idea for a new term: algolia_xxx_dirty?

3. Just revert Don't use deprecated attribute_changed? #325, i.e. just use xxx_changed? all the time but somehow make Rails 5.1 silence warnings every time xxx_changed? is called. Not sure this is possible.

4. Just revert Don't use deprecated attribute_changed? #325 and do nothing. This means that all users of Rails 5.1 may receive a huge amount of Rails warnings in their specs and potentially server logs, and they won't be able to do anything about them.

[simonireilly]

I think you are handling this correctly 👍

There will be many apps that use <= rails 5.1 for years to come so it should be supported.

There are maybe 20 ways to do this but...

I think the loop to call both the methods is simple and good! If we try to be clever we just create edge cases!!

👍 for your implementation.

[julienbourdeau]

Thank you @jeromedalbert and @simonireilly for your precious feedback, I really appreciate it 🙏

I looked for a better solution inspired by all @jeromedalbert's ideas. I think I found a fairly safe way to check whether #{attr_name}_changed? is user-defined or autmatic Rails method.

rails_magic_def = object.method(method_name).source_location[0].end_with?("active_model/attribute_methods.rb")

I think now, all the different scenario are handled, backward-compatibility is restored and deprecated method shouldn't be used. I'm going to add more tests to ensure that 👍

Again, any feedback is more than welcome. Thank you very much for helping me on this!

[jeromedalbert]

Wow that is some Jedi-level code introspection right there. 😂

Looks good to me.

[redox]

Why do we need that @julienbourdeau ? Doesn't seem to be a very popular library.

[julienbourdeau]

source_location is only available after in ruby 1.9, this gem backports the feature to 1.8.
I thought __file__ should work with ruby < 1.9 but I got undefined method in Travis as well.

One other solution would be to not have this feature in Ruby < 1.9 and in this case we always call attr_changed? method. In this case, people won't be able to use the new name but you can't run Rails 5 on ruby 1.8, I don't think it's important.

Now, that I think about it, It might be the best solution. WDYT?

[redox]

I think always calling the attr_changed? method is fine, it's what we've done before; no?

In this case, people won't be able to use the new name but you can't run Rails 5 on ruby 1.8, I don't think it's important.

Correct

Now, that I think about it, It might be the best solution. WDYT?

Most probably. Also, what you've done is not enough: the Gemfile is handling the dependencies of the project, while the dependencies that are gonna be bundled while releasing the gem is defined in algoliasearch-rails.gemspec. There should be a single mutualized list for both, but the fact we're having extra dependencies based on the ruby VM/versions makes it impossible.

[julienbourdeau]

Thank you Sylvain for reminding me about algoliasearch-rails.gemspec. When writing this "fix" it totally went over my head 😅 🙏