From b3074d9df4a83ecc9ee53d99391b24d136086cd4 Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Fri, 22 Feb 2019 10:51:43 +0100 Subject: [PATCH 1/7] Add tests to detect change on not index property --- spec/integration_spec.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb index 64ab27de..573e04ea 100644 --- a/spec/integration_spec.rb +++ b/spec/integration_spec.rb @@ -155,6 +155,7 @@ class Camera < Product class Color < ActiveRecord::Base include AlgoliaSearch + attr_accessor :not_indexed algoliasearch :synchronous => true, :index_name => safe_index_name("Color"), :per_environment => true do attributesToIndex [:name] @@ -532,6 +533,24 @@ class SerializedObject < ActiveRecord::Base end +describe 'Change detection' do + + it "should detect settings changes" do + color = Color.new name: "dark-blue", short_name: "blue" + + Color.algolia_must_reindex?(color).should == true + color.save + Color.algolia_must_reindex?(color).should == false + + color.not_indexed = "strstr" + Color.algolia_must_reindex?(color).should == false + color.name = "red" + Color.algolia_must_reindex?(color).should == true + end + +end + + describe 'Namespaced::Model' do it "should have an index name without :: hierarchy" do Namespaced::Model.index_name.should == "Namespaced_Model" From 20ad9d2ba4e7ae09e0e03279d4b465500ba727a6 Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Fri, 22 Feb 2019 10:53:25 +0100 Subject: [PATCH 2/7] Fix attribute_changed? detection --- lib/algoliasearch-rails.rb | 26 ++++++++++++++------------ spec/integration_spec.rb | 10 +++++++++- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/lib/algoliasearch-rails.rb b/lib/algoliasearch-rails.rb index dc3051a3..437b77e6 100644 --- a/lib/algoliasearch-rails.rb +++ b/lib/algoliasearch-rails.rb @@ -743,15 +743,14 @@ def algolia_must_reindex?(object) next if options[:slave] || options[:replica] return true if algolia_object_id_changed?(object, options) settings.get_attribute_names(object).each do |k| - changed_method = attribute_changed_method(k) - return true if !object.respond_to?(changed_method) || object.send(changed_method) + return true if algolia_attribute_changed?(object, k) + # return true if !object.respond_to?(changed_method) || object.send(changed_method) end [options[:if], options[:unless]].each do |condition| case condition when nil when String, Symbol - changed_method = attribute_changed_method(condition) - return true if !object.respond_to?(changed_method) || object.send(changed_method) + return true if algolia_attribute_changed?(object, condition) else # if the :if, :unless condition is a anything else, # we have no idea whether we should reindex or not @@ -825,8 +824,8 @@ def algolia_object_id_of(o, options = nil) end def algolia_object_id_changed?(o, options = nil) - m = attribute_changed_method(algolia_object_id_method(options)) - o.respond_to?(m) ? o.send(m) : false + changed = algolia_attribute_changed?(o, algolia_object_id_method(options)) + changed.nil? ? false : changed end def algoliasearch_settings_changed?(prev, current) @@ -920,13 +919,16 @@ def algolia_find_in_batches(batch_size, &block) end end - def attribute_changed_method(attr) - if defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1 || - (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR > 5) - "will_save_change_to_#{attr}?" - else - "#{attr}_changed?" + def algolia_attribute_changed?(object, attr_name) + # if one of two method is implemented, we return its result + # true/false means whether it has changed or not + ["#{attr_name}_changed?", "will_save_change_to_#{attr_name}?"].each do |method_name| + if object.respond_to?(method_name) + return object.send(method_name) + end end + # Nil means we don't know if the attribute has changed + nil end end diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb index 573e04ea..92727226 100644 --- a/spec/integration_spec.rb +++ b/spec/integration_spec.rb @@ -167,6 +167,10 @@ class Color < ActiveRecord::Base # we're using all attributes of the Color class + the _tag "extra" attribute end + + def hex_changed? + false + end end class DisabledBoolean < ActiveRecord::Base @@ -542,15 +546,19 @@ class SerializedObject < ActiveRecord::Base color.save Color.algolia_must_reindex?(color).should == false + color.hex = 123456 + Color.algolia_must_reindex?(color).should == false + color.not_indexed = "strstr" Color.algolia_must_reindex?(color).should == false color.name = "red" Color.algolia_must_reindex?(color).should == true + + color.delete end end - describe 'Namespaced::Model' do it "should have an index name without :: hierarchy" do Namespaced::Model.index_name.should == "Namespaced_Model" From 9293d654d588e7dfca77ecbdc14c3921000c8be6 Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Fri, 22 Feb 2019 16:42:49 +0100 Subject: [PATCH 3/7] Introduce algolia_dirty? instance method --- lib/algoliasearch-rails.rb | 4 ++++ spec/integration_spec.rb | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/lib/algoliasearch-rails.rb b/lib/algoliasearch-rails.rb index 437b77e6..54aae26d 100644 --- a/lib/algoliasearch-rails.rb +++ b/lib/algoliasearch-rails.rb @@ -739,6 +739,9 @@ def algolia_index_name(options = nil) end def algolia_must_reindex?(object) + # use +algolia_dirty?+ method if implemented + return object.send(:algolia_dirty?) if (object.respond_to?(:algolia_dirty?)) + # Loop over each index to see if a attribute used in records has changed algolia_configurations.each do |options, settings| next if options[:slave] || options[:replica] return true if algolia_object_id_changed?(object, options) @@ -759,6 +762,7 @@ def algolia_must_reindex?(object) end end end + # By default, we don't reindex return false end diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb index 92727226..db6cc1ee 100644 --- a/spec/integration_spec.rb +++ b/spec/integration_spec.rb @@ -83,6 +83,12 @@ t.boolean :premium t.boolean :released end + create_table :ebooks do |t| + t.string :name + t.string :author + t.boolean :premium + t.boolean :released + end create_table :disabled_booleans do |t| t.string :name end @@ -377,6 +383,22 @@ def public? end end +class Ebook < ActiveRecord::Base + include AlgoliaSearch + attr_accessor :current_time, :published_at + + algoliasearch :synchronous => true, :index_name => safe_index_name("eBooks")do + attributesToIndex [:name] + end + + def algolia_dirty? + return true if self.published_at.nil? || self.current_time.nil? + # Consider dirty if published date is in the past + # This doesn't make so much business sense but it's easy to test. + self.published_at < self.current_time + end +end + class EncodedString < ActiveRecord::Base include AlgoliaSearch @@ -539,8 +561,8 @@ class SerializedObject < ActiveRecord::Base describe 'Change detection' do - it "should detect settings changes" do - color = Color.new name: "dark-blue", short_name: "blue" + it "should detect attribute changes" do + color = Color.new :name => "dark-blue", :short_name => "blue" Color.algolia_must_reindex?(color).should == true color.save @@ -557,6 +579,17 @@ class SerializedObject < ActiveRecord::Base color.delete end + it "should detect change with algolia_dirty? method" do + ebook = Ebook.new :name => "My life", :author => "Myself", :premium => false, :released => true + + Ebook.algolia_must_reindex?(ebook).should == true # Because it's defined in algolia_dirty? method + ebook.current_time = 10 + ebook.published_at = 8 + Ebook.algolia_must_reindex?(ebook).should == true + ebook.published_at = 12 + Ebook.algolia_must_reindex?(ebook).should == false + end + end describe 'Namespaced::Model' do From b7c05bbfece6c0ea1e38d1a432826d567ac98fd0 Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Thu, 28 Feb 2019 18:26:20 +0100 Subject: [PATCH 4/7] Check if _changed? is user-defined before calling it in recent Rails version --- lib/algoliasearch-rails.rb | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/lib/algoliasearch-rails.rb b/lib/algoliasearch-rails.rb index 54aae26d..c59c03d2 100644 --- a/lib/algoliasearch-rails.rb +++ b/lib/algoliasearch-rails.rb @@ -926,14 +926,36 @@ def algolia_find_in_batches(batch_size, &block) def algolia_attribute_changed?(object, attr_name) # if one of two method is implemented, we return its result # true/false means whether it has changed or not - ["#{attr_name}_changed?", "will_save_change_to_#{attr_name}?"].each do |method_name| - if object.respond_to?(method_name) + # +#{attr_name}_changed?+ always defined for automatic attributes but deprecated after Rails 5.2 + # +will_save_change_to_#{attr_name}?+ should be use instead for Rails 5.2+, also defined for automatic attributes. + # If none of the method are defined, it's a dynamic attribute + + method_name = "#{attr_name}_changed?" + if object.respond_to?(method_name) + # If +#{attr_name}_changed?+ respond we want to see if the method is user defined or if it's automatically + # defined by Rails. + # If it's user-defined, we call it. + # If it's automatic we check ActiveRecord version to see if this method is deprecated + # and try to call +will_save_change_to_#{attr_name}?+ instead. + # See: https://github.com/algolia/algoliasearch-rails/pull/338 + rails_magic_def = object.method(method_name).source_location[0].end_with?("active_model/attribute_methods.rb") + unless rails_magic_def && automatic_changed_method_deprecated? return object.send(method_name) end end + + if object.respond_to?("will_save_change_to_#{attr_name}?") + return object.send("will_save_change_to_#{attr_name}?") + end + # Nil means we don't know if the attribute has changed nil end + + def automatic_changed_method_deprecated? + (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1) || + (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR > 5) + end end # these are the instance methods included From 1840b6a55e5d9b2f5fe3cb1177e4516b9eecaa10 Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Fri, 1 Mar 2019 18:25:54 +0100 Subject: [PATCH 5/7] Support Ruby 1.8 for user-defined _changed? method --- lib/algoliasearch-rails.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/algoliasearch-rails.rb b/lib/algoliasearch-rails.rb index c59c03d2..74d4409f 100644 --- a/lib/algoliasearch-rails.rb +++ b/lib/algoliasearch-rails.rb @@ -938,8 +938,7 @@ def algolia_attribute_changed?(object, attr_name) # If it's automatic we check ActiveRecord version to see if this method is deprecated # and try to call +will_save_change_to_#{attr_name}?+ instead. # See: https://github.com/algolia/algoliasearch-rails/pull/338 - rails_magic_def = object.method(method_name).source_location[0].end_with?("active_model/attribute_methods.rb") - unless rails_magic_def && automatic_changed_method_deprecated? + unless automatic_changed_method?(object, method_name) && automatic_changed_method_deprecated? return object.send(method_name) end end @@ -952,6 +951,16 @@ def algolia_attribute_changed?(object, attr_name) nil end + def automatic_changed_method?(object, method_name) + raise ArgumentError.new("Method #{method_name} doesn't exist on #{object.class.name}") unless object.respond_to?(method_name) + if defined?(:RUBY_VERSION) && RUBY_VERSION.to_f < 1.9 + file = object.method(method_name).__file__ + else + file = object.method(method_name).source_location[0] + end + file.end_with?("active_model/attribute_methods.rb") + end + def automatic_changed_method_deprecated? (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1) || (defined?(::ActiveRecord) && ActiveRecord::VERSION::MAJOR > 5) From 40865e256030007af84f42c4f6a5e1a229268f85 Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Fri, 1 Mar 2019 19:01:49 +0100 Subject: [PATCH 6/7] Add more tests for change detection --- spec/integration_spec.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb index db6cc1ee..2fafd879 100644 --- a/spec/integration_spec.rb +++ b/spec/integration_spec.rb @@ -177,6 +177,10 @@ class Color < ActiveRecord::Base def hex_changed? false end + + def will_save_change_to_short_name? + false + end end class DisabledBoolean < ActiveRecord::Base @@ -590,6 +594,23 @@ class SerializedObject < ActiveRecord::Base Ebook.algolia_must_reindex?(ebook).should == false end + it "should know if the _changed? method is user-defined" do + color = Color.new :name => "dark-blue", :short_name => "blue" + + expect { Color.send(:automatic_changed_method?, color, :something_that_doesnt_exist) }.to raise_error(ArgumentError) + + Color.send(:automatic_changed_method?, color, :name_changed?).should == true + Color.send(:automatic_changed_method?, color, :hex_changed?).should == false + + Color.send(:automatic_changed_method?, color, :will_save_change_to_short_name?).should == false + + if Color.send(:automatic_changed_method_deprecated?) + Color.send(:automatic_changed_method?, color, :will_save_change_to_name?).should == true + Color.send(:automatic_changed_method?, color, :will_save_change_to_hex?).should == true + end + + end + end describe 'Namespaced::Model' do From bc07d06c723261363ea0490e554f58dfdf91529b Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Mon, 4 Mar 2019 11:28:56 +0100 Subject: [PATCH 7/7] dont call will_save_change_ method for Ruby 1.8 source_location method were added in Ruby 1.9 we cannot figure out if the _changed? method was user-defined or if it's the automatic Rails method. --- lib/algoliasearch-rails.rb | 11 ++++++----- spec/integration_spec.rb | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/algoliasearch-rails.rb b/lib/algoliasearch-rails.rb index 74d4409f..2f315c7f 100644 --- a/lib/algoliasearch-rails.rb +++ b/lib/algoliasearch-rails.rb @@ -938,6 +938,11 @@ def algolia_attribute_changed?(object, attr_name) # If it's automatic we check ActiveRecord version to see if this method is deprecated # and try to call +will_save_change_to_#{attr_name}?+ instead. # See: https://github.com/algolia/algoliasearch-rails/pull/338 + # This feature is not compatible with Ruby 1.8 + # In this case, we always call #{attr_name}_changed? + if Object.const_defined?(:RUBY_VERSION) && RUBY_VERSION.to_f < 1.9 + return object.send(method_name) + end unless automatic_changed_method?(object, method_name) && automatic_changed_method_deprecated? return object.send(method_name) end @@ -953,11 +958,7 @@ def algolia_attribute_changed?(object, attr_name) def automatic_changed_method?(object, method_name) raise ArgumentError.new("Method #{method_name} doesn't exist on #{object.class.name}") unless object.respond_to?(method_name) - if defined?(:RUBY_VERSION) && RUBY_VERSION.to_f < 1.9 - file = object.method(method_name).__file__ - else - file = object.method(method_name).source_location[0] - end + file = object.method(method_name).source_location[0] file.end_with?("active_model/attribute_methods.rb") end diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb index 2fafd879..915d6232 100644 --- a/spec/integration_spec.rb +++ b/spec/integration_spec.rb @@ -594,7 +594,7 @@ class SerializedObject < ActiveRecord::Base Ebook.algolia_must_reindex?(ebook).should == false end - it "should know if the _changed? method is user-defined" do + it "should know if the _changed? method is user-defined", :skip => Object.const_defined?(:RUBY_VERSION) && RUBY_VERSION.to_f < 1.9 do color = Color.new :name => "dark-blue", :short_name => "blue" expect { Color.send(:automatic_changed_method?, color, :something_that_doesnt_exist) }.to raise_error(ArgumentError)