Please document criteria for smart _changed? behavior better

[vincentwoo]

it goes unsaid, but if your setup uses tags or an :if or :unless option, algoliasearch-rails will attempt to do things like examine

_tags_changed? and whatever your :if condition symbol name is + _changed? as well. this is pretty hard to track down if you're wondering why your app is doing a lot of frivolous updates.

please also consider adding a method that can override algolia_must_reindex? entirely.

[vincentwoo]

I also find it super strange that yall do stuff like

alias_method :must_reindex?, :algolia_must_reindex? unless method_defined? :must_reindex?

but then only refer to algolia_must_reindex? internally. Overriding class.must_reindex? doesn't do anything for the user because it's ignored - the aliasing just appears to be a convenience method for accessing algolia's method. If so, why even bother calling it something other than algolia_must_reindex?

[redox]

but then only refer to algolia_must_reindex? internally. Overriding class.must_reindex? doesn't do anything for the user because it's ignored - the aliasing just appears to be a convenience method for accessing algolia's method. If so, why even bother calling it something other than algolia_must_reindex?

The methods we're defining are indeed not designed to be reimplemented. And the algolia_ prefix has been added everywhere to avoid overriding any existing method. The shorter name is just there for convenience purpose.

That being said, what you say about the :if and :unless options impacting the must_reindex? function is definitely correct. The current code playing with the _changed? suffix is not good and we need to change it.

I will rather spend some time improving the doc & behavior of the :if feature; I don't think we want to encourage people to override the must_reindex? method.

@vincentwoo to make sure I get this right; could you share with us what kind of :if / :unless option you are using? I assume it's a Proc, right?

[vincentwoo]

No, it's just a relation. I want to index some models if they have a relation. Could easily be a proc.

I don't think you should expose methods like class.must_reindex? if you do not intend for them to be used or overridden.

[vincentwoo]

You might want to come up with a scheme for algolia being aware of relation-dependencies. Right now I have to hack around updating certain objects if their dependents update. Doing something like before_save { relations.each &:touch } might work but is pretty hacky

[jadercorrea]

@redox Did you update the docs on this? I'm having problems with algolia_must_reindex causing too many transactions as a side effect on a shared index.

[vincentwoo]

Ping

[oyeanuj]

@redox What is the recommended way to tell Algolia when to or not to reindex? (Also could you talk to how to trigger reindexing when relations change?)

@vincentwoo @jadercorrea Curious what workarounds do you use for this?

[vincentwoo]

I implemented my own _changed? functions for some attributes and manually trigger reindexing for others

[oyeanuj]

@vincentwoo Could you share those _changed? functions especially how they trigger reindexing when relations change?

[vincentwoo]

This issue's been open a couple years. Has the situation changed? I'm again running into a model performing way more updates than is necessary because of the limited ability of algoliasearch-rails to understand context.

[vincentwoo]

It looks like the _changed? stuff got even more broken in rails 5 - was trying to figure out our insane volume of updates and stumbled on https://github.com/algolia/algoliasearch-rails/blob/master/lib/algoliasearch-rails.rb#L923. You have to implement will_save_change_to_#{attr}? now, instead of #{attr}_changed? for any virtual algolia attributes you've defined. This is, of course, not communicated in the documentation: https://www.algolia.com/doc/framework-integration/rails/options/?language=ruby#custom-attribute-definition.

This has probably cost us, oh, I don't know, thousands of dollars.

[vincentwoo]

It looks like the original PR that broke this behavior was #325 - an attempt to stay ahead of Rails 5 deprecations that broke everyone with _changed? implementations for virtual attributes. Coupled with the lack of documentation change or release announcements, users were completely in the dark. This was horribly managed.

[redox]

@julienbourdeau could you help us improve the documentation of the _changed? behaviours? We have tests running for Rails 5 but maybe they're not correctly testing the _changed? methods.

[vincentwoo]

Instead of trying to bandaid a docs change, this is a really big opportunity to fixed the change detection system holistically. Right now, to work within the bounds of the system you have setup, users need to be clever enough to realize they have to implement will_save_change_to__tags (notice the double underscore).

This is a pretty rough place to be. Instead, you could try asking the user to pre-declare all column names that are eventually used in the computation of the record, along with a fallback method a user can defined to take control over where the record should be reindexed at all.

[julienbourdeau]

I'm working on the issue, I'll try to find a better solution. Ideally, I'd rather not ask the user to list all used attributes.

[vincentwoo]

You can ask for only the columns that you can't detect - e.g. columns used in computing virtual attributes, if/unless, and tags

[julienbourdeau]

@vincentwoo Sorry for the BC break, I didn't see it coming 😞

I opened this PR #338, any feedback is very welcome.

[julienbourdeau]

@vincentwoo I started refreshing the Rails docs, I'm almost done. You can find the preview here: https://deploy-preview-2711--algolia-doc.netlify.com/doc/framework-integration/rails/indexing/indexing/
I added docs about _changed? and it's new name. What do you think?

I'll also release #338 which should fix your issue and restore BC.