Merge #301

301: Correctly check if index settings have been updated r=brunoocasali a=ellnix # Pull Request ## Related issue Fixes #280 (partly) - In issue #280 there is also the related problem of a redundant request to create an already existing index the first time it is used. However this request should not be significant to performance, so we can deal with it in a better way after some refactoring. ## PR checklist Please check if your PR fulfills the following requirements: - [X] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [X] Have you read the contributing guidelines? - [X] Have you made sure that the title is accurate and descriptive of the changes? # Notes (please read) I believe this fixes issue #280 (I could not reproduce it after this edit). I will add regression tests when I have the time (it took a while trying to figure out what was going wrong). The problem was (to the extent I have determined it): - The first time an index is used `ms_ensure_init` will fetch it from the MeiliSearch instance - Subsequent calls for that same index will have been memoized, so this problem occurs only once (makes it harder to test), however in background jobs this memoization is not relevant - During that first call when the index is first fetched from the MeiliSearch instance, its settings are checked with the current settings to see if they need updating, from here there are 2 separate issues: 1. The previous settings are not fetched correctly: [this line](https://github.com/meilisearch/meilisearch-rails/blob/e7a12569f18ee40684c3c643a3cf573f3465c62e/lib/meilisearch-rails.rb#L744C37-L744C37) provides an argument to `MeiliSearch::Client#settings` which takes no arguments and therefore causes an `ArgumentError` which is then rescued with nil (moral of the story always specify the exception you are `rescue`-ing) ```ruby current_settings = `@ms_indexes[MeiliSearch::Rails.active?][settings].settings(getVersion:` 1) rescue nil ``` 2. The [`meilisearch_settings_changed?`](https://github.com/meilisearch/meilisearch-rails/blob/e7a12569f18ee40684c3c643a3cf573f3465c62e/lib/meilisearch-rails.rb#L797) method has therefore never been run up to this point (as far as I can see) and has several problems that have therefore never been addressed 1. The `prev` keys are not just strings instead of symbols, they are also camelCase-d 2. There is no guaranteed order to `current` and `prev` values if they are arrays, and `==` comparison between Arrays will be false if their elements have different orders Co-authored-by: ellnix <[email protected]>
meilisearch · Dec 15, 2023 · f0ed5d3 · f0ed5d3
2 parents 9c6b5ed + eab5165
commit f0ed5d3
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 44 deletions.
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -1,11 +1,27 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2023-11-01 17:06:53 UTC using RuboCop version 1.27.0.
+# on 2023-11-06 11:54:44 UTC using RuboCop version 1.27.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
+# Offense count: 1
+# This cop supports safe auto-correction (--auto-correct).
+# Configuration parameters: TreatCommentsAsGroupSeparators, ConsiderPunctuation, Include.
+# Include: **/*.gemfile, **/Gemfile, **/gems.rb
+Bundler/OrderedGems:
+  Exclude:
+    - 'Gemfile'
+
+# Offense count: 1
+# This cop supports safe auto-correction (--auto-correct).
+# Configuration parameters: Include.
+# Include: **/*.gemspec
+Gemspec/RequireMFA:
+  Exclude:
+    - 'meilisearch-rails.gemspec'
+
 # Offense count: 1
 # This cop supports safe auto-correction (--auto-correct).
 # Configuration parameters: EnforcedStyle.
@@ -35,7 +51,7 @@ Lint/UnusedMethodArgument:
   Exclude:
     - 'lib/meilisearch-rails.rb'
 
-# Offense count: 10
+# Offense count: 11
 # Configuration parameters: IgnoredMethods, CountRepeatedAttributes.
 Metrics/AbcSize:
   Max: 102
@@ -51,7 +67,7 @@ Metrics/BlockLength:
 Metrics/ClassLength:
   Max: 156
 
-# Offense count: 9
+# Offense count: 8
 # Configuration parameters: IgnoredMethods.
 Metrics/CyclomaticComplexity:
   Max: 27
@@ -64,9 +80,9 @@ Metrics/MethodLength:
 # Offense count: 1
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ModuleLength:
-  Max: 428
+  Max: 435
 
-# Offense count: 9
+# Offense count: 8
 # Configuration parameters: IgnoredMethods.
 Metrics/PerceivedComplexity:
   Max: 33
@@ -102,17 +118,18 @@ RSpec/DescribeClass:
   Exclude:
     - 'spec/integration_spec.rb'
 
-# Offense count: 36
+# Offense count: 37
 # Configuration parameters: CountAsOne.
 RSpec/ExampleLength:
   Max: 19
 
-# Offense count: 2
+# Offense count: 3
 # Configuration parameters: Include, CustomTransform, IgnoreMethods, SpecSuffixOnly.
 # Include: **/*_spec*rb*, **/spec/**/*
 RSpec/FilePath:
   Exclude:
     - 'spec/configuration_spec.rb'
+    - 'spec/settings_spec.rb'
     - 'spec/utilities_spec.rb'
 
 # Offense count: 26
@@ -152,13 +169,13 @@ Style/Alias:
   Exclude:
     - 'lib/meilisearch-rails.rb'
 
-# Offense count: 1
+# Offense count: 2
 # Configuration parameters: MinBodyLength.
 Style/GuardClause:
   Exclude:
     - 'lib/meilisearch-rails.rb'
 
-# Offense count: 8
+# Offense count: 9
 # This cop supports safe auto-correction (--auto-correct).
 Style/IfUnlessModifier:
   Exclude:
@@ -178,13 +195,7 @@ Style/OptionalBooleanParameter:
   Exclude:
     - 'lib/meilisearch-rails.rb'
 
-# Offense count: 1
-# This cop supports safe auto-correction (--auto-correct).
-Style/RescueModifier:
-  Exclude:
-    - 'lib/meilisearch-rails.rb'
-
-# Offense count: 21
+# Offense count: 20
 # This cop supports safe auto-correction (--auto-correct).
 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
 # URISchemes: http, https

diff --git a/lib/meilisearch-rails.rb b/lib/meilisearch-rails.rb
@@ -260,7 +260,7 @@ def initialize(index_uid, raise_on_failure, options)
         @raise_on_failure = raise_on_failure.nil? || raise_on_failure
 
         SafeIndex.log_or_throw(nil, @raise_on_failure) do
-          client.create_index(index_uid, { primary_key: primary_key })
+          client.create_index!(index_uid, { primary_key: primary_key })
         end
 
         @index = client.index(index_uid)
@@ -735,33 +735,38 @@ def ms_must_reindex?(document)
 
       protected
 
-      def ms_ensure_init(options = nil, settings = nil, index_settings = nil)
+      def ms_ensure_init(options = meilisearch_options, settings = meilisearch_settings, user_configuration = settings.to_settings)
         raise ArgumentError, 'No `meilisearch` block found in your model.' if meilisearch_settings.nil?
 
         @ms_indexes ||= { true => {}, false => {} }
 
-        options ||= meilisearch_options
-        settings ||= meilisearch_settings
+        @ms_indexes[MeiliSearch::Rails.active?][settings] ||= SafeIndex.new(ms_index_uid(options), meilisearch_options[:raise_on_failure], meilisearch_options)
 
-        return @ms_indexes[MeiliSearch::Rails.active?][settings] if @ms_indexes[MeiliSearch::Rails.active?][settings]
+        update_settings_if_changed(@ms_indexes[MeiliSearch::Rails.active?][settings], options, user_configuration)
 
-        @ms_indexes[MeiliSearch::Rails.active?][settings] = SafeIndex.new(ms_index_uid(options), meilisearch_options[:raise_on_failure], meilisearch_options)
+        @ms_indexes[MeiliSearch::Rails.active?][settings]
+      end
 
-        current_settings = @ms_indexes[MeiliSearch::Rails.active?][settings].settings(getVersion: 1) rescue nil # if the index doesn't exist
+      private
 
-        index_settings ||= settings.to_settings
-        index_settings = options[:primary_settings].to_settings.merge(index_settings) if options[:inherit]
+      def update_settings_if_changed(index, options, user_configuration)
+        server_state = index.settings
+        user_configuration = options[:primary_settings].to_settings.merge(user_configuration) if options[:inherit]
 
-        options[:check_settings] = true if options[:check_settings].nil?
+        config = user_configuration.except(:attributes_to_highlight, :attributes_to_crop, :crop_length)
 
-        if !ms_indexing_disabled?(options) && options[:check_settings] && meilisearch_settings_changed?(current_settings, index_settings)
-          @ms_indexes[MeiliSearch::Rails.active?][settings].update_settings(index_settings)
+        if !skip_checking_settings?(options) && meilisearch_settings_changed?(server_state, config)
+          index.update_settings(user_configuration)
         end
+      end
 
-        @ms_indexes[MeiliSearch::Rails.active?][settings]
+      def skip_checking_settings?(options)
+        ms_indexing_disabled?(options) || ms_checking_disabled?(options)
       end
 
-      private
+      def ms_checking_disabled?(options)
+        options[:check_settings] == false
+      end
 
       def ms_configurations
         raise ArgumentError, 'No `meilisearch` block found in your model.' if meilisearch_settings.nil?
@@ -800,19 +805,22 @@ def ms_pk(options = nil)
         options[:primary_key] || MeiliSearch::Rails::IndexSettings::DEFAULT_PRIMARY_KEY
       end
 
-      def meilisearch_settings_changed?(prev, current)
-        return true if prev.nil?
+      def meilisearch_settings_changed?(server_state, user_configuration)
+        return true if server_state.nil?
+
+        user_configuration.transform_keys! { |key| key.to_s.camelize(:lower) }
 
-        current.each do |k, v|
-          prev_v = prev[k.to_s]
-          if v.is_a?(Array) && prev_v.is_a?(Array)
-            # compare array of strings, avoiding symbols VS strings comparison
-            return true if v.map(&:to_s) != prev_v.map(&:to_s)
-          elsif prev_v != v
-            return true
+        user_configuration.any? do |key, user|
+          server = server_state[key]
+
+          if user.is_a?(Hash) && server.is_a?(Hash)
+            meilisearch_settings_changed?(server, user)
+          elsif user.is_a?(Array) && server.is_a?(Array)
+            user.map(&:to_s) != server.map(&:to_s)
+          else
+            user.to_s != server.to_s
           end
         end
-        false
       end
 
       def ms_conditional_index?(options = nil)

diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb
@@ -58,10 +58,10 @@
 
   it 'does not detect settings changes' do
     expect(Color.send(:meilisearch_settings_changed?, {}, {})).to be(false)
-    expect(Color.send(:meilisearch_settings_changed?, { 'searchable_attributes' => ['name'] },
+    expect(Color.send(:meilisearch_settings_changed?, { 'searchableAttributes' => ['name'] },
                       { searchable_attributes: ['name'] })).to be(false)
     expect(Color.send(:meilisearch_settings_changed?,
-                      { 'searchable_attributes' => ['name'], 'ranking_rules' => ['words', 'typo', 'proximity', 'attribute', 'sort', 'exactness', 'hex:asc'] },
+                      { 'searchableAttributes' => ['name'], 'rankingRules' => ['words', 'typo', 'proximity', 'attribute', 'sort', 'exactness', 'hex:asc'] },
                       { 'ranking_rules' => ['words', 'typo', 'proximity', 'attribute', 'sort', 'exactness', 'hex:asc'] })).to be(false)
   end
 end
@@ -1079,7 +1079,7 @@
 
     it 'does not raise error timeouts on reindex' do
       allow(index_instance).to receive(:add_documents).and_raise(MeiliSearch::TimeoutError)
-      allow(slow_client).to receive(:create_index).and_return(index_instance)
+      allow(slow_client).to receive(:create_index!).and_return(index_instance)
 
       allow(MeiliSearch::Rails).to receive(:client).and_return(slow_client)
 
@@ -1089,7 +1089,7 @@
     end
 
     it 'does not raise error timeouts on data addition' do
-      allow(slow_client).to receive(:create_index).and_raise(MeiliSearch::TimeoutError)
+      allow(slow_client).to receive(:create_index!).and_raise(MeiliSearch::TimeoutError)
       allow(index_instance).to receive(:add_documents).and_return(nil)
 
       allow(MeiliSearch::Rails).to receive(:client).and_return(slow_client)

diff --git a/spec/settings_spec.rb b/spec/settings_spec.rb
@@ -0,0 +1,33 @@
+require 'spec_helper'
+
+describe MeiliSearch::Rails::IndexSettings do
+  describe 'settings change detection' do
+    let(:record) { Color.create name: 'dark-blue', short_name: 'blue' }
+
+    context 'without changing settings' do
+      it 'does not call update settings' do
+        allow(Color.index).to receive(:update_settings).and_call_original
+
+        record.ms_index!
+
+        expect(Color.index).not_to have_received(:update_settings)
+      end
+    end
+
+    context 'when settings have been changed' do
+      it 'makes a request to update settings' do
+        idx = Color.index
+        task = idx.update_settings(
+          filterable_attributes: ['none']
+        )
+        idx.wait_for_task task['taskUid']
+
+        allow(idx).to receive(:update_settings).and_call_original
+
+        record.ms_index!
+
+        expect(Color.index).to have_received(:update_settings).once
+      end
+    end
+  end
+end