algolia / algoliasearch-rails

AlgoliaSearch integration to your favorite ORM
MIT License
410 stars 118 forks source link

fix: settings changes detection issue for replica indexes with inherit opt #426

Closed kiyohara closed 2 years ago

kiyohara commented 2 years ago
Q A
Bug fix? yes
New feature? no
BC breaks? no
Related Issue -
Need Doc update no

Describe your change

If you use inherit option for replica following this document, the settings changes check function ( algoliasearch_settings_changed? ) falsely detects the changes. This function always detects that there is a change, even if there is no change.

The reason is simple. The setting Hash building process, that should be done before the diff check process, is handled after the checking currently. Therefore, when comparing replica index settings that use the inherit option, the comparison process is being performed with the replicas settings remaining that must be deleted in advance. The replica index's settings do not include replicas. It's available in the primary index's settings only, so it has not to be inherited.

So this PR move the process of removing the replicas settings from the replica index's settings to before the comparison logic.

What problem is this fixing?

If you have a replica index with the inherit option, your app calls useless Algolia setting update API frequently. It's heavy and blocking(synchronous) API calling, so your search function might be timed out.

kiyohara commented 2 years ago

Hi @DevinCodes

I've added the tests. Please check them. An added replica index name may be a little confusing to read.

I add ForwardToReplicas_replica_inherited to compare the replica settings inherit: true and inherit: true. On the other hand, In one test, it changes the replica settings from inherit: false to inherit: true to compare them. Therefore, the following similar codes exist nearby.

I think the intent of this replica name is clear to anyone who reads this test section all. However, if you have a better name suggestion, please let me know.


BTW, I have created some PRs in algolia gem repo. Could you review them too, please? If it's out of line, I'm sorry. Please let me know who is appropriate to ask.

These PRs have no test. However, I think these PRs are very difficult to create tests. Are they required?

DevinCodes commented 2 years ago

Thank you again @kiyohara ! I will review this PR this week, and I'll try to take a look at the other PRs as well 🙂

kiyohara commented 2 years ago

@DevinCodes Thank you so much for your review! I'm very glad.