Fix settings update regression #330
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #330 +/- ##
==========================================
+ Coverage 89.32% 89.47% +0.14%
==========================================
Files 11 11
Lines 684 684
==========================================
+ Hits 611 612 +1
+ Misses 73 72 -1 ☔ View full report in Codecov by Sentry. |
Creating the index synchronously was required to get an up to version of its settings, in order to compare them to the user configuration and decide whether or not to update settings. Unfortunately this made practically every operation have a synchronous component causing a lot of problems down the line. This commit reverts to using create_index asynchronously to avoid said issues, and represents the settings of a nonexistent index as an empty hash, which will queue an update_settings if the user has specified any settings at all.
ellnix
force-pushed
the
fix-settings-update-regression
branch
from
b3d8a47
to
03a8daa
Compare
There was a problem hiding this comment.
@ellnix I made some points that we could discuss, but I don't think we should wait for the answers.
LGTM!
| @@ -843,7 +843,7 @@ def meilisearch_settings_changed?(server_state, user_configuration) | |||
| 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) | |||
| user.map(&:to_s).sort! != server.map(&:to_s).sort! | |||
There was a problem hiding this comment.
Why did you have to use sort! and not just sort here?
There was a problem hiding this comment.
Mainly out of frustration. While I understand objectively that this function is very short and does not deal with a lot of data, the type conversions really bother me and it feels like it performs terribly.
So I looked up performance comparisons for different methods to compare arrays regardless of order and sort! was slightly faster than sort.
Of course these arrays are tiny and the difference does not matter one bit, especially since map already crates new arrays.
| @@ -261,7 +261,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 }) | |||
There was a problem hiding this comment.
Let's trust on the test suite 🙏
There was a problem hiding this comment.
Actually it was asynchronous on 0.10.2 so it should be fine:
meilisearch-rails/lib/meilisearch-rails.rb
Line 263 in e5ad4d1
| @@ -306,7 +306,7 @@ def settings(*args) | |||
| SafeIndex.log_or_throw(:settings, @raise_on_failure) do | |||
| @index.settings(*args) | |||
| rescue ::MeiliSearch::ApiError => e | |||
| return {} if e.code == 404 # not fatal | |||
| return {} if e.code == 'index_not_found' # not fatal | |||
There was a problem hiding this comment.
I'm not sure why we had the 404 in the first place, but index_not_found is just one issue that could happen, shouldn't we make it more generic? Like parsing the error response to grab a "match-all" case?
There was a problem hiding this comment.
I think the logic is that everytime an index is used, we attempt to create it and we also check if its settings are good, immediately afterward.
So
- if
index.settingssays that the index is not found - it means that the index has just been created
- thus we can pass
{}as the settings to compare against the user settings - this way
- if there are any user settings at all
- we know the index has just been created and is therefore on default settings
- we will send an
update_settingsrequest
This logic has a flaw that we should discuss in due time but it's minor.
Anyway from what I can see the only purpose of this rescue block is to handle this one case wherein the index has not been created yet. Other errors are handled by log_or_throw since this method just propagates them.
|
bors merge |
Pull Request
Related issue
Fixes #329
May fix #328
What does this PR do?
SafeIndexbefore I tried to fix Skip indexCreation and settingsUpdate whenever adding new record in production #280 with Correctly check if index settings have been updated #301 did not guarantee that its index exists at all, since@indexwas created asynchronously it was possible thatSafeIndex#settingscould be called on an index that does not exist.This line:
meilisearch-rails/lib/meilisearch-rails.rb
Line 309 in 29f59c8
was supposed return an empty hash when asked to fetch the settings of an index that does not exist, however
ApiError#codeis the meilisearch code ("index_not_found") and not the http code (404). That line was therefore skipped and theindex_not_founderror was being propagated and caught by therescue nilin:meilisearch-rails/lib/meilisearch-rails.rb
Line 750 in e5ad4d1
until I removed it and tried to replace it with ensuring that
SafeIndexhad an index by makingcreate_index!synchronous and all kinds of hell broke loose since now every time an index was used would cause a synchronous wait.