[MODEL] Refactor module/mixin organization for clarity (#893)

* [MODEL] Refactor module inclusion for clarity

* [MODEL] Don't use Hash#transform_values as it's not available in ruby 2.2

* [MODEL] Make before_save callbacks consistent

Author: estolfo

Diff for: elasticsearch-model/lib/elasticsearch/model.rb

@@ -89,9 +89,10 @@ module Model
 
     # Adds the `Elasticsearch::Model` functionality to the including class.
     #
-    # * Creates the `__elasticsearch__` class and instance methods, pointing to the proxy object
-    # * Includes the necessary modules in the proxy classes
-    # * Sets up delegation for crucial methods such as `search`, etc.
+    # * Creates the `__elasticsearch__` class and instance method. These methods return a proxy object with
+    #   other common methods defined on them.
+    # * The module includes other modules with further functionality.
+    # * Sets up delegation for common methods such as `import` and `search`.
     #
     # @example Include the module in the `Article` model definition
     #
@@ -108,44 +109,16 @@ def self.included(base)
       base.class_eval do
         include Elasticsearch::Model::Proxy
 
-        Elasticsearch::Model::Proxy::ClassMethodsProxy.class_eval do
-          include Elasticsearch::Model::Client::ClassMethods
-          include Elasticsearch::Model::Naming::ClassMethods
-          include Elasticsearch::Model::Indexing::ClassMethods
-          include Elasticsearch::Model::Searching::ClassMethods
-        end
-
-        Elasticsearch::Model::Proxy::InstanceMethodsProxy.class_eval do
-          include Elasticsearch::Model::Client::InstanceMethods
-          include Elasticsearch::Model::Naming::InstanceMethods
-          include Elasticsearch::Model::Indexing::InstanceMethods
-          include Elasticsearch::Model::Serializing::InstanceMethods
-        end
-
-        Elasticsearch::Model::Proxy::InstanceMethodsProxy.class_eval <<-CODE, __FILE__, __LINE__ + 1
-          def as_indexed_json(options={})
-            target.respond_to?(:as_indexed_json) ? target.__send__(:as_indexed_json, options) : super
-          end
-        CODE
-
-        # Delegate important methods to the `__elasticsearch__` proxy, unless they are defined already
-        #
+        # Delegate common methods to the `__elasticsearch__` ClassMethodsProxy, unless they are defined already
         class << self
           METHODS.each do |method|
-            delegate method, to: :__elasticsearch__ unless self.public_instance_methods.include?(method)
+            delegate method, to: :__elasticsearch__ unless self.respond_to?(method)
           end
         end
-
-        # Mix the importing module into the proxy
-        #
-        self.__elasticsearch__.class_eval do
-          include Elasticsearch::Model::Importing::ClassMethods
-          include Adapter.from_class(base).importing_mixin
-        end
-
-        # Add to the registry if it's a class (and not in intermediate module)
-        Registry.add(base) if base.is_a?(Class)
       end
+
+      # Add to the model to the registry if it's a class (and not in intermediate module)
+      Registry.add(base) if base.is_a?(Class)
     end
 
     # Access the module settings

Diff for: elasticsearch-model/lib/elasticsearch/model/indexing.rb

@@ -338,13 +338,17 @@ def self.included(base)
           #
           # @see #update_document
           #
-          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] } ])
+          base.before_save do |obj|
+            if obj.respond_to?(:changes_to_save) # Rails 5.1
+              changes_to_save = obj.changes_to_save
+            elsif obj.respond_to?(:changes)
+              changes_to_save = obj.changes
+            end
+
+            if changes_to_save
+              attrs = obj.instance_variable_get(:@__changed_model_attributes) || {}
+              latest_changes = changes_to_save.inject({}) { |latest_changes, (k,v)| latest_changes.merge!(k => v.last) }
+              obj.instance_variable_set(:@__changed_model_attributes, attrs.merge(latest_changes))
             end
           end if base.respond_to?(:before_save)
         end

Diff for: elasticsearch-model/lib/elasticsearch/model/proxy.rb

@@ -19,15 +19,15 @@ module Elasticsearch
   module Model
 
     # This module provides a proxy interfacing between the including class and
-    # {Elasticsearch::Model}, preventing the pollution of the including class namespace.
+    # `Elasticsearch::Model`, preventing the pollution of the including class namespace.
     #
     # The only "gateway" between the model and Elasticsearch::Model is the
-    # `__elasticsearch__` class and instance method.
+    # `#__elasticsearch__` class and instance method.
     #
     # The including class must be compatible with
     # [ActiveModel](https://github.com/rails/rails/tree/master/activemodel).
     #
-    # @example Include the {Elasticsearch::Model} module into an `Article` model
+    # @example Include the `Elasticsearch::Model` module into an `Article` model
     #
     #     class Article < ActiveRecord::Base
     #       include Elasticsearch::Model
@@ -53,40 +53,48 @@ module Proxy
       #       module and the functionality is accessible via the proxy.
       #
       def self.included(base)
+
         base.class_eval do
-          # {ClassMethodsProxy} instance, accessed as `MyModel.__elasticsearch__`
-          #
+
+          # `ClassMethodsProxy` instance, accessed as `MyModel.__elasticsearch__`
           def self.__elasticsearch__ &block
             @__elasticsearch__ ||= ClassMethodsProxy.new(self)
             @__elasticsearch__.instance_eval(&block) if block_given?
             @__elasticsearch__
           end
 
-          # {InstanceMethodsProxy}, accessed as `@mymodel.__elasticsearch__`
-          #
-          def __elasticsearch__ &block
-            @__elasticsearch__ ||= InstanceMethodsProxy.new(self)
-            @__elasticsearch__.instance_eval(&block) if block_given?
-            @__elasticsearch__
+          # Mix the importing module into the `ClassMethodsProxy`
+          self.__elasticsearch__.class_eval do
+            include Adapter.from_class(base).importing_mixin
           end
 
           # Register a callback for storing changed attributes for models which implement
           # `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|
-            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] } ]))
+          before_save do |obj|
+            if obj.respond_to?(:changes_to_save) # Rails 5.1
+              changes_to_save = obj.changes_to_save
+            elsif obj.respond_to?(:changes)
+              changes_to_save = obj.changes
+            end
+
+            if changes_to_save
+              attrs = obj.__elasticsearch__.instance_variable_get(:@__changed_model_attributes) || {}
+              latest_changes = changes_to_save.inject({}) { |latest_changes, (k,v)| latest_changes.merge!(k => v.last) }
+              obj.__elasticsearch__.instance_variable_set(:@__changed_model_attributes, attrs.merge(latest_changes))
             end
           end if respond_to?(:before_save)
         end
+
+        # {InstanceMethodsProxy}, accessed as `@mymodel.__elasticsearch__`
+        #
+        def __elasticsearch__ &block
+          @__elasticsearch__ ||= InstanceMethodsProxy.new(self)
+          @__elasticsearch__.instance_eval(&block) if block_given?
+          @__elasticsearch__
+        end
       end
 
       # @overload dup
@@ -130,6 +138,11 @@ def inspect
       #
       class ClassMethodsProxy
         include Base
+        include Elasticsearch::Model::Client::ClassMethods
+        include Elasticsearch::Model::Naming::ClassMethods
+        include Elasticsearch::Model::Indexing::ClassMethods
+        include Elasticsearch::Model::Searching::ClassMethods
+        include Elasticsearch::Model::Importing::ClassMethods
       end
 
       # A proxy interfacing between Elasticsearch::Model instance methods and model instance methods
@@ -138,6 +151,10 @@ class ClassMethodsProxy
       #
       class InstanceMethodsProxy
         include Base
+        include Elasticsearch::Model::Client::InstanceMethods
+        include Elasticsearch::Model::Naming::InstanceMethods
+        include Elasticsearch::Model::Indexing::InstanceMethods
+        include Elasticsearch::Model::Serializing::InstanceMethods
 
         def klass
           target.class
@@ -153,8 +170,11 @@ def class
         def as_json(options={})
           target.as_json(options)
         end
-      end
 
+        def as_indexed_json(options={})
+          target.respond_to?(:as_indexed_json) ? target.__send__(:as_indexed_json, options) : super
+        end
+      end
     end
   end
 end