From f2780237420573a77841673302177d705d0c68de Mon Sep 17 00:00:00 2001 From: Emily S Date: Mon, 5 Aug 2019 15:43:49 +0200 Subject: [PATCH 1/5] [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 --- elasticsearch-model/README.md | 18 +++++++++------ .../lib/elasticsearch/model.rb | 23 ++++++++++++------- .../model/naming_inheritance_spec.rb | 6 ++++- 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/elasticsearch-model/README.md b/elasticsearch-model/README.md index 22b9b378c..d94054c1e 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,15 @@ 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 support + +Versions < 7.0.0 of this gem supported inheritance-- more specifically, `Single Table Inheritance`. 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) +so this gem has also removed support 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..ea7227fb4 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 + @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/spec/elasticsearch/model/naming_inheritance_spec.rb b/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb index 287fc7d42..16d79f394 100644 --- a/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb +++ b/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb @@ -20,7 +20,11 @@ class ::Animal < ::TestBase document_type "mammal" end - class ::Dog < ::Animal + around(:all) do |example| + original_value = Elasticsearch::Model.inheritance_enabled + Elasticsearch::Model.inheritance_enabled = true + example.run + Elasticsearch::Model.inheritance_enabled = original_value end module ::MyNamespace From b16871cc7d788ab8b2951b9df8eb679f1b0fbe86 Mon Sep 17 00:00:00 2001 From: Emily Stolfo Date: Mon, 5 Aug 2019 15:49:55 +0200 Subject: [PATCH 2/5] [MODEL] Update Readme text about STI deprecation --- elasticsearch-model/README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/elasticsearch-model/README.md b/elasticsearch-model/README.md index d94054c1e..ea3d75cbe 100644 --- a/elasticsearch-model/README.md +++ b/elasticsearch-model/README.md @@ -743,14 +743,15 @@ 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 support +### Single Table Inheritance deprecation -Versions < 7.0.0 of this gem supported inheritance-- more specifically, `Single Table Inheritance`. With this feature, +`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) -so this gem has also removed support as it encourages an anti-pattern. Please save different model documents in -separate indices or implement an artificial `type` field manually in each document. +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 From 127f95f32846c815739a1412e35168df05aaa649 Mon Sep 17 00:00:00 2001 From: "Tejas R. Mandke" Date: Thu, 4 Jul 2019 07:26:30 -0700 Subject: [PATCH 3/5] [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 --- .../lib/elasticsearch/model/naming.rb | 7 +- .../model/naming_inheritance_spec.rb | 200 ++++++++++++------ 2 files changed, 146 insertions(+), 61 deletions(-) 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/naming_inheritance_spec.rb b/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb index 16d79f394..6f56f627c 100644 --- a/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb +++ b/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb @@ -2,22 +2,45 @@ 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 + + Animal = Class.new TestBase do + extend ActiveModel::Naming + + extend Elasticsearch::Model::Naming::ClassMethods + include Elasticsearch::Model::Naming::InstanceMethods + + index_name "mammals" + document_type "mammal" + end + + Dog = Class.new Animal - class ::Animal < ::TestBase - extend ActiveModel::Naming + 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 + extend Elasticsearch::Model::Naming::ClassMethods + include Elasticsearch::Model::Naming::InstanceMethods + + index_name "cats" + document_type "cat" + end - index_name "mammals" - document_type "mammal" + end + + after(:all) do + remove_classes(TestBase, Animal, MyNamespace, Cat) end around(:all) do |example| @@ -27,84 +50,141 @@ class ::Animal < ::TestBase Elasticsearch::Model.inheritance_enabled = original_value end - module ::MyNamespace - class Dog < ::Animal + 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 - end - class ::Cat < ::Animal - 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') + + expect(Cat.index_name).to eq('cats') + expect(Cat.new.index_name).to eq('cats') + end - extend Elasticsearch::Model::Naming::ClassMethods - include Elasticsearch::Model::Naming::InstanceMethods + it 'returns the ancestor index name' do + expect(Dog.index_name).to eq('mammals') + expect(Dog.new.index_name).to eq('mammals') + end - index_name "cats" - document_type "cat" + 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 be_nil + expect(TestBase.new.document_type).to be_nil + 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 - it 'returns the explicit index name' do - expect(Animal.index_name).to eq('mammals') - expect(Animal.new.index_name).to eq('mammals') + context 'when using proxy' do + before(:all) do + TestBase = Class.new do + extend ActiveModel::Naming - expect(Cat.index_name).to eq('cats') - expect(Cat.new.index_name).to eq('cats') + include Elasticsearch::Model + end + + 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 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 explicit document type' do - expect(Animal.document_type).to eq('mammal') - expect(Animal.new.document_type).to eq('mammal') + describe '#index_name' do - expect(Cat.document_type).to eq('cat') - expect(Cat.new.document_type).to eq('cat') - end + it 'returns the default index name' do + expect(TestBase.index_name).to eq('test_bases') + end + + it 'returns the explicit index name' do + expect(Animal.index_name).to eq('mammals') - it 'returns the ancestor document type' do - expect(Dog.document_type).to eq('mammal') - expect(Dog.new.document_type).to eq('mammal') + 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 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 be_nil + 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 From 50428e583b2a0e6dfab08aa6315b70c84927a4cf Mon Sep 17 00:00:00 2001 From: Emily Stolfo Date: Thu, 8 Aug 2019 16:26:35 +0200 Subject: [PATCH 4/5] [MODEL] Adjust previous cherry-picked commit for 6.x branch --- .../elasticsearch/model/naming_inheritance_spec.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb b/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb index 6f56f627c..764bc5295 100644 --- a/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb +++ b/elasticsearch-model/spec/elasticsearch/model/naming_inheritance_spec.rb @@ -79,8 +79,8 @@ module ::MyNamespace describe '#document_type' do it 'returns nil' do - expect(TestBase.document_type).to be_nil - expect(TestBase.new.document_type).to be_nil + expect(TestBase.document_type).to eq('_doc') + expect(TestBase.new.document_type).to eq('_doc') end it 'returns the explicit document type' do @@ -139,12 +139,6 @@ module MyNamespace Elasticsearch::Model.settings[:inheritance_enabled] = original_value end - - it 'returns the default document type' do - expect(TestBase.document_type).to eq('_doc') - expect(TestBase.new.document_type).to eq('_doc') - end - describe '#index_name' do it 'returns the default index name' do @@ -169,7 +163,7 @@ module MyNamespace describe '#document_type' do it 'returns nil' do - expect(TestBase.document_type).to be_nil + expect(TestBase.document_type).to eq('_doc') end it 'returns the explicit document type' do From 7c4f07f2dac81d56f342071df58acff14a8755a2 Mon Sep 17 00:00:00 2001 From: Emily Stolfo Date: Thu, 8 Aug 2019 16:55:06 +0200 Subject: [PATCH 5/5] [MODEL] Only warn if inheritance_enabled is set to true --- .../lib/elasticsearch/model.rb | 2 +- .../spec/elasticsearch/model/module_spec.rb | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/elasticsearch-model/lib/elasticsearch/model.rb b/elasticsearch-model/lib/elasticsearch/model.rb index ea7227fb4..102466916 100644 --- a/elasticsearch-model/lib/elasticsearch/model.rb +++ b/elasticsearch-model/lib/elasticsearch/model.rb @@ -197,7 +197,7 @@ def inheritance_enabled # Elasticsearch::Model.inheritance_enabled = true # def inheritance_enabled=(inheritance_enabled) - warn STI_DEPRECATION_WARNING + warn STI_DEPRECATION_WARNING if inheritance_enabled @settings[:inheritance_enabled] = inheritance_enabled 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