diff --git a/elasticsearch-model/README.md b/elasticsearch-model/README.md index 22b9b378c..ea3d75cbe 100644 --- a/elasticsearch-model/README.md +++ b/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. diff --git a/elasticsearch-model/lib/elasticsearch/model.rb b/elasticsearch-model/lib/elasticsearch/model.rb index 2c395bd84..102466916 100644 --- a/elasticsearch-model/lib/elasticsearch/model.rb +++ b/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 diff --git a/elasticsearch-model/lib/elasticsearch/model/naming.rb b/elasticsearch-model/lib/elasticsearch/model/naming.rb index cef23e810..50b396c37 100644 --- a/elasticsearch-model/lib/elasticsearch/model/naming.rb +++ b/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 diff --git a/elasticsearch-model/spec/elasticsearch/model/module_spec.rb b/elasticsearch-model/spec/elasticsearch/model/module_spec.rb index 3162e2ec5..0b8986d69 100644 --- a/elasticsearch-model/spec/elasticsearch/model/module_spec.rb +++ b/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 diff --git a/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb b/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb index 287fc7d42..764bc5295 100644 --- a/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb +++ b/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