[MODEL] Add warning and documentation about STI support being deprecated (#898)

* [MODEL] Add warning and documentation about STI support being deprecated (#895)

* [MODEL] Add warning and documentation about STI support being deprecated

* [MODEL] Minor change to STI deprecation warning

* [MODEL] Freeze string constant depreaction warning

* [MODEL] Update Readme text about STI deprecation

* [Model] Fix naming with inheritance when using Proxy (#887)

* Add naming inheritance tests when using a proxy

* Skip circular call to index_name/document_type when Proxy is used and inheritance is enabled

* [MODEL] Adjust previous cherry-picked commit for 6.x branch

* [MODEL] Only warn if inheritance_enabled is set to true

Author: estolfo

elasticsearch-model/README.md

@@ -724,13 +724,8 @@ module and its submodules for technical information.
 
 The module provides a common `settings` method to customize various features.
 
-At the moment, the only supported setting is `:inheritance_enabled`, which makes the class receiving the module
-respect index names and document types of a super-class, eg. in case you're using "single table inheritance" (STI)
-in Rails:
-
-```ruby
-Elasticsearch::Model.settings[:inheritance_enabled] = true
-```
+Before version 7.0.0 of the gem, the only supported setting was `:inheritance_enabled`. This setting has been deprecated
+and removed.
 
 ## Development and Community
 
@@ -748,6 +743,16 @@ curl -# https://download.elasticsearch.org/elasticsearch/elasticsearch/elasticse
 SERVER=start TEST_CLUSTER_COMMAND=$PWD/tmp/elasticsearch-1.0.0.RC1/bin/elasticsearch bundle exec rake test:all
 ```
 
+### Single Table Inheritance deprecation
+
+`Single Table Inheritance` has been supported until the 6.x series of this gem. With this feature,
+settings on a parent model could be inherited by a child model leading to different model documents being indexed
+into the same Elasticsearch index. This feature depended on the ability to set a `type` for a document in Elasticsearch.
+The Elasticsearch team has deprecated support for `types`, as is described [here](https://www.elastic.co/guide/en/elasticsearch/reference/current/removal-of-types.html)
+This gem will also remove support for types and Single Table Inheritance in version 7.0 as it encourages an anti-pattern. 
+Please save different model documents in separate indices or implement an artificial `type` field manually in each 
+document.
+
 ## License
 
 This software is licensed under the Apache 2 license, quoted below.

elasticsearch-model/lib/elasticsearch/model.rb

@@ -131,12 +131,6 @@ class << self
       end
     end
 
-    # Access the module settings
-    #
-    def self.settings
-      @settings ||= {}
-    end
-
     module ClassMethods
       # Get the client common for all models
       #
@@ -193,7 +187,7 @@ def search(query_or_payload, models=[], options={})
       # @note Inheritance is disabled by default.
       #
       def inheritance_enabled
-        @inheritance_enabled ||= false
+        @settings[:inheritance_enabled] ||= false
       end
 
       # Enable inheritance of index_name and document_type
@@ -203,8 +197,21 @@ def inheritance_enabled
       #     Elasticsearch::Model.inheritance_enabled = true
       #
       def inheritance_enabled=(inheritance_enabled)
-        @inheritance_enabled = inheritance_enabled
+        warn STI_DEPRECATION_WARNING if inheritance_enabled
+        @settings[:inheritance_enabled] = inheritance_enabled
+      end
+
+      # Access the module settings
+      #
+      def settings
+        @settings ||= {}
       end
+
+      private
+
+      STI_DEPRECATION_WARNING = "DEPRECATION WARNING: Support for Single Table Inheritance (STI) is deprecated " +
+        "and will be removed in version 7.0.0.\nPlease save different model documents in separate indices and refer " +
+        "to the Elasticsearch documentation for more information.".freeze
     end
     extend ClassMethods
 

elasticsearch-model/lib/elasticsearch/model/naming.rb

@@ -79,7 +79,12 @@ def implicit(prop)
 
             if Elasticsearch::Model.settings[:inheritance_enabled]
               self.ancestors.each do |klass|
-                next if klass == self
+                # When Naming is included in Proxy::ClassMethods the actual model
+                # is among its ancestors. We don't want to call the actual model
+                # since it will result in the same call to the same instance of
+                # Proxy::ClassMethods. To prevent this we also skip the ancestor
+                # that is the target.
+                next if klass == self || self.respond_to?(:target) && klass == self.target
                 break if value = klass.respond_to?(prop) && klass.send(prop)
               end
             end

elasticsearch-model/spec/elasticsearch/model/module_spec.rb

@@ -72,5 +72,30 @@ def self.search(query, options={})
         expect(Elasticsearch::Model.settings[:foo]).to eq('bar')
       end
     end
+
+    context 'when \'inheritance_enabled\' is set' do
+
+      around do |example|
+        original_value = Elasticsearch::Model.settings[:inheritance_enabled]
+        example.run
+        Elasticsearch::Model.settings[:inheritance_enabled] = original_value
+      end
+
+      context 'when \'inheritance_enabled\' is true' do
+
+        it 'warns with a deprecation message' do
+          expect(Elasticsearch::Model).to receive(:warn)
+          Elasticsearch::Model.inheritance_enabled = true
+        end
+      end
+
+      context 'when \'inheritance_enabled\' is false' do
+
+        it 'does not warn' do
+          expect(Elasticsearch::Model).not_to receive(:warn)
+          Elasticsearch::Model.inheritance_enabled = false
+        end
+      end
+    end
   end
 end

elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb

@@ -2,105 +2,183 @@
 
 describe 'naming inheritance' do
 
-  before(:all) do
-    class ::TestBase
-      extend ActiveModel::Naming
+  context 'without using proxy' do
+    before(:all) do
+      TestBase = Class.new do
+        extend ActiveModel::Naming
 
-      extend  Elasticsearch::Model::Naming::ClassMethods
-      include Elasticsearch::Model::Naming::InstanceMethods
-    end
+        extend  Elasticsearch::Model::Naming::ClassMethods
+        include Elasticsearch::Model::Naming::InstanceMethods
+      end
 
-    class ::Animal < ::TestBase
-      extend ActiveModel::Naming
+      Animal = Class.new TestBase do
+        extend ActiveModel::Naming
 
-      extend  Elasticsearch::Model::Naming::ClassMethods
-      include Elasticsearch::Model::Naming::InstanceMethods
+        extend  Elasticsearch::Model::Naming::ClassMethods
+        include Elasticsearch::Model::Naming::InstanceMethods
+
+        index_name "mammals"
+        document_type "mammal"
+      end
+
+      Dog = Class.new Animal
+
+      module ::MyNamespace
+        Dog = Class.new Animal
+      end
+
+      Cat = Class.new Animal do
+        extend ActiveModel::Naming
+
+        extend  Elasticsearch::Model::Naming::ClassMethods
+        include Elasticsearch::Model::Naming::InstanceMethods
+
+        index_name "cats"
+        document_type "cat"
+      end
 
-      index_name "mammals"
-      document_type "mammal"
     end
 
-    class ::Dog < ::Animal
+    after(:all) do
+      remove_classes(TestBase, Animal, MyNamespace, Cat)
     end
 
-    module ::MyNamespace
-      class Dog < ::Animal
-      end
+    around(:all) do |example|
+      original_value = Elasticsearch::Model.inheritance_enabled
+      Elasticsearch::Model.inheritance_enabled = true
+      example.run
+      Elasticsearch::Model.inheritance_enabled = original_value
     end
 
-    class ::Cat < ::Animal
-      extend ActiveModel::Naming
+    describe '#index_name' do
+
+      it 'returns the default index name' do
+        expect(TestBase.index_name).to eq('test_bases')
+        expect(TestBase.new.index_name).to eq('test_bases')
+      end
+
+      it 'returns the explicit index name' do
+        expect(Animal.index_name).to eq('mammals')
+        expect(Animal.new.index_name).to eq('mammals')
 
-      extend  Elasticsearch::Model::Naming::ClassMethods
-      include Elasticsearch::Model::Naming::InstanceMethods
+        expect(Cat.index_name).to eq('cats')
+        expect(Cat.new.index_name).to eq('cats')
+      end
 
-      index_name "cats"
-      document_type "cat"
+      it 'returns the ancestor index name' do
+        expect(Dog.index_name).to eq('mammals')
+        expect(Dog.new.index_name).to eq('mammals')
+      end
+
+      it 'returns the ancestor index name for namespaced models' do
+        expect(::MyNamespace::Dog.index_name).to eq('mammals')
+        expect(::MyNamespace::Dog.new.index_name).to eq('mammals')
+      end
     end
 
-  end
+    describe '#document_type' do
 
-  after(:all) do
-    remove_classes(TestBase, Animal, MyNamespace, Cat)
-  end
+      it 'returns nil' do
+        expect(TestBase.document_type).to eq('_doc')
+        expect(TestBase.new.document_type).to eq('_doc')
+      end
 
-  around(:all) do |example|
-    original_value = Elasticsearch::Model.settings[:inheritance_enabled]
-    Elasticsearch::Model.settings[:inheritance_enabled] = true
-    example.run
-    Elasticsearch::Model.settings[:inheritance_enabled] = original_value
-  end
+      it 'returns the explicit document type' do
+        expect(Animal.document_type).to eq('mammal')
+        expect(Animal.new.document_type).to eq('mammal')
 
+        expect(Cat.document_type).to eq('cat')
+        expect(Cat.new.document_type).to eq('cat')
+      end
 
-  describe '#index_name' do
+      it 'returns the ancestor document type' do
+        expect(Dog.document_type).to eq('mammal')
+        expect(Dog.new.document_type).to eq('mammal')
+      end
 
-    it 'returns the default index name' do
-      expect(TestBase.index_name).to eq('test_bases')
-      expect(TestBase.new.index_name).to eq('test_bases')
+      it 'returns the ancestor document type for namespaced models' do
+        expect(::MyNamespace::Dog.document_type).to eq('mammal')
+        expect(::MyNamespace::Dog.new.document_type).to eq('mammal')
+      end
     end
+  end
+
+  context 'when using proxy' do
+    before(:all) do
+      TestBase = Class.new do
+        extend ActiveModel::Naming
 
-    it 'returns the explicit index name' do
-      expect(Animal.index_name).to eq('mammals')
-      expect(Animal.new.index_name).to eq('mammals')
+        include Elasticsearch::Model
+      end
 
-      expect(Cat.index_name).to eq('cats')
-      expect(Cat.new.index_name).to eq('cats')
+      Animal = Class.new TestBase do
+        index_name "mammals"
+        document_type "mammal"
+      end
+
+      Dog = Class.new Animal
+
+      module MyNamespace
+        Dog = Class.new Animal
+      end
+
+      Cat = Class.new Animal do
+        index_name "cats"
+        document_type "cat"
+      end
     end
 
-    it 'returns the ancestor index name' do
-      expect(Dog.index_name).to eq('mammals')
-      expect(Dog.new.index_name).to eq('mammals')
+    after(:all) do
+      remove_classes(TestBase, Animal, MyNamespace, Cat)
     end
 
-    it 'returns the ancestor index name for namespaced models' do
-      expect(::MyNamespace::Dog.index_name).to eq('mammals')
-      expect(::MyNamespace::Dog.new.index_name).to eq('mammals')
+    around(:all) do |example|
+      original_value = Elasticsearch::Model.settings[:inheritance_enabled]
+      Elasticsearch::Model.settings[:inheritance_enabled] = true
+      example.run
+      Elasticsearch::Model.settings[:inheritance_enabled] = original_value
     end
-  end
 
-  describe '#document_type' do
+    describe '#index_name' do
 
-    it 'returns the default document type' do
-      expect(TestBase.document_type).to eq('_doc')
-      expect(TestBase.new.document_type).to eq('_doc')
-    end
+      it 'returns the default index name' do
+        expect(TestBase.index_name).to eq('test_bases')
+      end
 
-    it 'returns the explicit document type' do
-      expect(Animal.document_type).to eq('mammal')
-      expect(Animal.new.document_type).to eq('mammal')
+      it 'returns the explicit index name' do
+        expect(Animal.index_name).to eq('mammals')
 
-      expect(Cat.document_type).to eq('cat')
-      expect(Cat.new.document_type).to eq('cat')
-    end
+        expect(Cat.index_name).to eq('cats')
+      end
+
+      it 'returns the ancestor index name' do
+        expect(Dog.index_name).to eq('mammals')
+      end
 
-    it 'returns the ancestor document type' do
-      expect(Dog.document_type).to eq('mammal')
-      expect(Dog.new.document_type).to eq('mammal')
+      it 'returns the ancestor index name for namespaced models' do
+        expect(::MyNamespace::Dog.index_name).to eq('mammals')
+      end
     end
 
-    it 'returns the ancestor document type for namespaced models' do
-      expect(::MyNamespace::Dog.document_type).to eq('mammal')
-      expect(::MyNamespace::Dog.new.document_type).to eq('mammal')
+    describe '#document_type' do
+
+      it 'returns nil' do
+        expect(TestBase.document_type).to eq('_doc')
+      end
+
+      it 'returns the explicit document type' do
+        expect(Animal.document_type).to eq('mammal')
+
+        expect(Cat.document_type).to eq('cat')
+      end
+
+      it 'returns the ancestor document type' do
+        expect(Dog.document_type).to eq('mammal')
+      end
+
+      it 'returns the ancestor document type for namespaced models' do
+        expect(::MyNamespace::Dog.document_type).to eq('mammal')
+      end
     end
   end
 end