[MODEL] Fixed the handling of changed attributes in Indexing to work with older Rails versions

This patch builds on work in elastic#738 by @jkeam and elastic#758 by @Geesu to prevent deprecation warnings on Rails 5.1
due to changes in the handling of "changed attributes", originally reported by @davedash in elastic#714.

It's primary focus is to restore the compatibility with older Rails versions (so we don't break compatibility
without proper version bump and in a single isolated commit), and to make the naming a bit more descriptive.

First, the condition has been changed to work with the `changes_to_save` and `changes` methods, as opposed
to the original `changed_attributes` / `attributes_in_database` naming. This communicates much more clearly
the intent, and the original usage of `changed_attributes` has been misleading.

Also, the "internal instance variable" which keeps track of the changes has been renamed to `@__changed_model_attributes`,
in order to cleary differentiate it from the `changed_attributes` method name in older ActiveRecord versions.

Closes elastic#758

Author: karmi

elasticsearch-model/lib/elasticsearch/model/indexing.rb

@@ -307,17 +307,23 @@ module InstanceMethods
 
         def self.included(base)
           # Register callback for storing changed attributes for models
-          # which implement `before_save` and `attributes_in_database` methods
+          # which implement `before_save` and return changed attributes
+          # (ie. when `Elasticsearch::Model` is included)
           #
           # @note This is typically triggered only when the module would be
           #       included in the model directly, not within the proxy.
           #
           # @see #update_document
           #
-          base.before_save do |instance|
-            instance.instance_variable_set(:@__attributes_in_database,
-                                  Hash[ instance.changes_to_save.map { |key, value| [key, value.last] } ])
-          end if base.respond_to?(:before_save) && base.instance_methods.include?(:attributes_in_database)
+          base.before_save do |i|
+            if i.class.instance_methods.include?(:changes_to_save) # Rails 5.1
+              i.instance_variable_set(:@__changed_model_attributes,
+                                      Hash[ i.changes_to_save.map { |key, value| [key, value.last] } ])
+            elsif i.class.instance_methods.include?(:changes)
+              i.instance_variable_set(:@__changed_model_attributes,
+                                      Hash[ i.changes.map { |key, value| [key, value.last] } ])
+            end
+          end if base.respond_to?(:before_save)
         end
 
         # Serializes the model instance into JSON (by calling `as_indexed_json`),
@@ -391,7 +397,7 @@ def delete_document(options={})
         # @see http://rubydoc.info/gems/elasticsearch-api/Elasticsearch/API/Actions:update
         #
         def update_document(options={})
-          if attributes_in_database = self.instance_variable_get(:@__attributes_in_database)
+          if attributes_in_database = self.instance_variable_get(:@__changed_model_attributes)
             attributes = if respond_to?(:as_indexed_json)
               self.as_indexed_json.select { |k,v| attributes_in_database.keys.map(&:to_s).include? k.to_s }
             else

elasticsearch-model/lib/elasticsearch/model/proxy.rb

@@ -54,15 +54,21 @@ def __elasticsearch__ &block
           end
 
           # Register a callback for storing changed attributes for models which implement
-          # `before_save` and `attributes_in_database` methods (when `Elasticsearch::Model` is included)
+          # `before_save` method and return changed attributes (ie. when `Elasticsearch::Model` is included)
           #
           # @see http://api.rubyonrails.org/classes/ActiveModel/Dirty.html
           #
           before_save do |i|
-            changed_attr = i.__elasticsearch__.instance_variable_get(:@__attributes_in_database) || {}
-            i.__elasticsearch__.instance_variable_set(:@__attributes_in_database,
-                                                      changed_attr.merge(Hash[ i.changes_to_save.map { |key, value| [key, value.last] } ]))
-          end if respond_to?(:before_save) && instance_methods.include?(:attributes_in_database)
+            if i.class.instance_methods.include?(:changes_to_save) # Rails 5.1
+              a = i.__elasticsearch__.instance_variable_get(:@__changed_model_attributes) || {}
+              i.__elasticsearch__.instance_variable_set(:@__changed_model_attributes,
+                                                        a.merge(Hash[ i.changes_to_save.map { |key, value| [key, value.last] } ]))
+            elsif i.class.instance_methods.include?(:changes)
+              a = i.__elasticsearch__.instance_variable_get(:@__changed_model_attributes) || {}
+              i.__elasticsearch__.instance_variable_set(:@__changed_model_attributes,
+                                                        a.merge(Hash[ i.changes.map { |key, value| [key, value.last] } ]))
+            end
+          end if respond_to?(:before_save)
         end
       end
 

elasticsearch-model/test/unit/indexing_test.rb

@@ -171,8 +171,6 @@ def self.before_save(&block)
           (@callbacks ||= {})[block.hash] = block
         end
 
-        def attributes_in_database; [:foo]; end
-
         def changes_to_save
           {:foo => ['One', 'Two']}
         end
@@ -186,8 +184,6 @@ def self.before_save(&block)
           (@callbacks ||= {})[block.hash] = block
         end
 
-        def attributes_in_database; [:foo, :bar]; end
-
         def changes_to_save
           {:foo => ['A', 'B'], :bar => ['C', 'D']}
         end
@@ -197,15 +193,28 @@ def as_indexed_json(options={})
         end
       end
 
+      class ::DummyIndexingModelWithOldDirty
+        extend  Elasticsearch::Model::Indexing::ClassMethods
+        include Elasticsearch::Model::Indexing::InstanceMethods
+
+        def self.before_save(&block)
+          (@callbacks ||= {})[block.hash] = block
+        end
+
+        def changes
+          {:foo => ['One', 'Two']}
+        end
+      end
+
       should "register before_save callback when included" do
         ::DummyIndexingModelWithCallbacks.expects(:before_save).returns(true)
         ::DummyIndexingModelWithCallbacks.__send__ :include, Elasticsearch::Model::Indexing::InstanceMethods
       end
 
-      should "set the @__attributes_in_database variable before save" do
+      should "set the @__changed_model_attributes variable before save" do
         instance = ::DummyIndexingModelWithCallbacks.new
         instance.expects(:instance_variable_set).with do |name, value|
-          assert_equal :@__attributes_in_database, name
+          assert_equal :@__changed_model_attributes, name
           assert_equal({foo: 'Two'}, value)
           true
         end
@@ -217,6 +226,23 @@ def as_indexed_json(options={})
         end
       end
 
+      # https://github.com/elastic/elasticsearch-rails/issues/714
+      # https://github.com/rails/rails/pull/25337#issuecomment-225166796
+      should "set the @__changed_model_attributes variable before save for old ActiveModel::Dirty" do
+        instance = ::DummyIndexingModelWithOldDirty.new
+        instance.expects(:instance_variable_set).with do |name, value|
+          assert_equal :@__changed_model_attributes, name
+          assert_equal({foo: 'Two'}, value)
+          true
+        end
+
+        ::DummyIndexingModelWithOldDirty.__send__ :include, Elasticsearch::Model::Indexing::InstanceMethods
+
+        ::DummyIndexingModelWithOldDirty.instance_variable_get(:@callbacks).each do |n,b|
+          instance.instance_eval(&b)
+        end
+      end
+
       should "have the index_document method" do
         client   = mock('client')
         instance = ::DummyIndexingModelWithCallbacks.new
@@ -297,7 +323,7 @@ def as_indexed_json(options={})
         instance = ::DummyIndexingModelWithCallbacks.new
 
         # Reset the fake `changes`
-        instance.instance_variable_set(:@__attributes_in_database, nil)
+        instance.instance_variable_set(:@__changed_model_attributes, nil)
 
         instance.expects(:index_document)
         instance.update_document
@@ -308,7 +334,7 @@ def as_indexed_json(options={})
         instance = ::DummyIndexingModelWithCallbacks.new
 
         # Set the fake `changes` hash
-        instance.instance_variable_set(:@__attributes_in_database, {foo: 'bar'})
+        instance.instance_variable_set(:@__changed_model_attributes, {foo: 'bar'})
 
         client.expects(:update).with do |payload|
           assert_equal 'foo',  payload[:index]
@@ -331,7 +357,7 @@ def as_indexed_json(options={})
         instance = ::DummyIndexingModelWithCallbacksAndCustomAsIndexedJson.new
 
         # Set the fake `changes` hash
-        instance.instance_variable_set(:@__attributes_in_database, {'foo' => 'B', 'bar' => 'D' })
+        instance.instance_variable_set(:@__changed_model_attributes, {'foo' => 'B', 'bar' => 'D' })
 
         client.expects(:update).with do |payload|
           assert_equal({:foo => 'B'}, payload[:body][:doc])
@@ -350,7 +376,7 @@ def as_indexed_json(options={})
         client   = mock('client')
         instance = ::DummyIndexingModelWithCallbacksAndCustomAsIndexedJson.new
 
-        instance.instance_variable_set(:@__attributes_in_database, { 'foo' => { 'bar' => 'BAR'} })
+        instance.instance_variable_set(:@__changed_model_attributes, { 'foo' => { 'bar' => 'BAR'} })
         # Overload as_indexed_json
         instance.expects(:as_indexed_json).returns({ 'foo' => 'BAR' })
 
@@ -372,7 +398,7 @@ def as_indexed_json(options={})
         instance = ::DummyIndexingModelWithCallbacks.new
 
         # Set the fake `changes` hash
-        instance.instance_variable_set(:@__attributes_in_database, {author: 'john'})
+        instance.instance_variable_set(:@__changed_model_attributes, {author: 'john'})
 
         client.expects(:update).with do |payload|
           assert_equal 'foo',  payload[:index]

elasticsearch-model/test/unit/proxy_test.rb

@@ -23,8 +23,6 @@ def self.before_save(&block)
         (@callbacks ||= {})[block.hash] = block
       end
 
-      def attributes_in_database; [:foo]; end
-
       def changes_to_save
         {:foo => ['One', 'Two']}
       end
@@ -43,10 +41,10 @@ def changes_to_save
       DummyProxyModelWithCallbacks.__send__ :include, Elasticsearch::Model::Proxy
     end
 
-    should "set the @__attributes_in_database variable before save" do
+    should "set the @__changed_model_attributes variable before save" do
       instance = ::DummyProxyModelWithCallbacks.new
       instance.__elasticsearch__.expects(:instance_variable_set).with do |name, value|
-        assert_equal :@__attributes_in_database, name
+        assert_equal :@__changed_model_attributes, name
         assert_equal({foo: 'Two'}, value)
         true
       end