fix(reindex): merge of settings in reindex method #389
Conversation
| tmp_index = algolia_ensure_init(tmp_options, tmp_settings, master_settings) | ||
| end | ||
| ::Algolia::copy_index!(src_index_name, tmp_index_name, %w(settings synonyms rules)) | ||
| tmp_index = SafeIndex.new(tmp_index_name, !!options[:raise_on_failure]) |
There was a problem hiding this comment.
Did you understand why @julienbourdeau introduced this test in ba94561 ? I'm not sure I get it, even though I think I'm fine with your updated version.
There was a problem hiding this comment.
My understanding was that it was to avoid having to make multiple calls to get_settings, but the problem was that the configuration described in the model, even though it might have changed later, was in that case pulled back and that was what caused the issue I'm trying to fix. But what I'm not not sure I understand indeed was the need to pull back this configuration. I may be changing the behaviour and I do need @julienbourdeau enlighten on this.
There was a problem hiding this comment.
As far as I understand, the new version is not going to apply setting changes anymore. I added the check_settings options to avoid many get/set settings call. Scoped copy indexed feature was just introduced.
Issues about searchableAttributes vs attributeToIndex should be addressed by updating the code in the algoliasearch block to use the new name.
I this this PR is going to create more issue that it solves. We should definitely encourage people to turn off the check_settings feature. Especially, if the "fix" is to ensure people use the code for "check_settings = false".
chloelbn
force-pushed
the
fix/reindex-method
branch
from
e82f123
to
69f68ce
Compare
There was a problem hiding this comment.
I think the issue this PR is trying to fix should be addressed otherwise. Mostly docs and helping people update their config. Many implementation won't be applying new settings automatically with this change.
For sure, we shouldn't be adding more magic to handle searchableAttributes vs attributeToIndex and such
| tmp_index = algolia_ensure_init(tmp_options, tmp_settings, master_settings) | ||
| end | ||
| ::Algolia::copy_index!(src_index_name, tmp_index_name, %w(settings synonyms rules)) | ||
| tmp_index = SafeIndex.new(tmp_index_name, !!options[:raise_on_failure]) |
There was a problem hiding this comment.
As far as I understand, the new version is not going to apply setting changes anymore. I added the check_settings options to avoid many get/set settings call. Scoped copy indexed feature was just introduced.
Issues about searchableAttributes vs attributeToIndex should be addressed by updating the code in the algoliasearch block to use the new name.
I this this PR is going to create more issue that it solves. We should definitely encourage people to turn off the check_settings feature. Especially, if the "fix" is to ensure people use the code for "check_settings = false".
Describe your change
In the
reindexmethod, when using a tmp index, base configuration of the index was copied regardless of changes that could have been made, causing an error especially if the deprecatedattributesToIndexparameter was used. This fixes this issue.