Deprecation warning in Rails 5.1

afpr252 commented 4 years ago

Hi. We're using the latest version of the thinking sphinx gem (4.4.1) and we recently upgraded to Rails 5.1. Since that upgrade, we've been seeing deprecation warnings in the gem.

Problem

These are the logs we have:

DEPRECATION WARNING: The behavior of 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_changes? instead.
DEPRECATION WARNING: The behavior of changed_attributes 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_changes.transform_values(&:first) instead.

The code that is calling this deprecated method is on gems/thinking-sphinx-4.4.1/lib/thinking_sphinx/active_record/callbacks/delta_callbacks.rb#new_or_changed?

  def new_or_changed?
    instance.new_record? || instance.changed?
  end

The deprecated method is https://apidock.com/rails/ActiveModel/Dirty/changed%3F and the new method that replaces it is https://apidock.com/rails/ActiveRecord/AttributeMethods/Dirty/saved_changes%3F.

Steps to reproduce the issue

The way we reproduced the issue was to create a callback in a model that issues an update which then triggers the thinking sphinx gem.

class User < ApplicationModel
  ...
  # this will trigger the affected code inside a ActiveRecord callback
  after_update{ update!(some_attribute: nil) }
end

ThinkingSphinx::Index.define :user, with: :active_record, delta: ThinkingSphinx::Deltas::SidekiqDelta do
  ...
end

Then open a rails console and:

user = User.last
user.update!(some_other_attribute: nil)

Interim solution

As expected, we managed to solve the deprecation warning by monkey-patching the affected class:

class ThinkingSphinx::ActiveRecord::Callbacks::DeltaCallbacks
  private

  def new_or_changed?
    instance.new_record? || instance.saved_changes?
  end
end

However, we would rather to have the fix in upstream.

Do you want me to open a PR with the way we fixed the deprecation warning or is there a better solution given that this new method is not available on Rails prior to 5.1?

pat commented 4 years ago

Hi André - thanks for reporting this, and for all the excellent detail!

This has come up previously - see #1059 and #1085 - but now you've got me second-guessing my thinking (though it's hard to remember how I thought through it all three years ago). It could be that even without changes to the code the original behaviour will continue to work - it's just that the deprecation warnings from Rails aren't super nuanced.

Or perhaps I've got it wrong… I'm generally using real-time indices (with TS and Rails 6) these days rather than SQL-backed indices and deltas, though the test suite covers both.

I'll try to find some time to put together a test app using deltas and Rails 5.2 to confirm things behave as they should, though not sure when I'll be able to get to that… certainly, if you want to give that a shot, that'd be great!

afpr252 commented 4 years ago

First of all, sorry for not having looked at previous issues. I now understand what you mean by deprecation warnings from Rails aren't super nuanced. https://github.com/pat/thinking-sphinx/issues/1059 enlightened me on that. We'll look deeper into this warning and come back to present our findings.

afpr252 commented 4 years ago

So actually my suggestion as interim solution is actually wrong because saved_changes? will return false given this is run inside the before_save callback, so I no longer recommend that.

Another solution we've found out that seems to work correctly and fixes the deprecation warning is by using has_changes_to_save?. However, this method is only available starting at ActiveRecord 5.1 according to Apidock. If you agree with using this, I can open a PR that conditionally uses this method if ActiveRecord is >= 5.1.

pat commented 4 years ago

If you could, that'd be great. Generally I refer to ActiveRecord::VERSION::STRING.to_f for such things.

It'd also be good to get a test in place that confirms the correct behaviour with these nested callbacks - which probably means adding a new model to spec/internal/app/models and the corresponding table to spec/internal/db/schema.rb to avoid complicating matters in other models.

pat commented 3 years ago

@afpr252 Just realised I've left this issue dormant for over a year, sorry! 😅 did you manage to get anywhere with a fix, even if it's not polished up into a PR?

pat / thinking-sphinx