search

pat / thinking-sphinx

Sphinx/Manticore plugin for ActiveRecord/Rails
http://freelancing-gods.com/thinking-sphinx
MIT License
1.63k stars 468 forks source link

Real time indeces dont take update callbacks into consideration #1203

Closed krauselukas closed 3 years ago

krauselukas commented 3 years ago

Recently I realized that the attribute fields of an associated model were not updated after the associated model got deleted. Since the way we use TS differs a bit from the documentation, I started to adjust the code to use TS how its documented, but I ended up having the same issue. After looking a bit through the TS codebase I started wondering where the after_update callbacks are taken into account for the real time indexes.

To me it looks like the the appender class only adds the after_save and after_destroy callbacks for real_time.

https://github.com/pat/thinking-sphinx/blob/2c8d3e7a9509407148786f2f7697580b8e19f21f/lib/thinking_sphinx/callbacks/appender.rb#L42 https://github.com/pat/thinking-sphinx/blob/2c8d3e7a9509407148786f2f7697580b8e19f21f/lib/thinking_sphinx/callbacks/appender.rb#L26

To give a little more detail, here a little snippet of the code I used: https://gist.github.com/krauselukas/1aabfb4ecd3d61b6486916237eabdcfb

The problem I faced here is that after I deleted an Attrib associated to a Package, the attrib_type_ids in the package_index still contained the id of the deleted Attrib.

So Package.search("foo", :with => { :attrib_type_ids => 1 } ) still returned the Package even the associated Attrib was already deleted.

krauselukas commented 3 years ago

This fixed it for me https://github.com/krauselukas/thinking-sphinx/commit/53b6ff1f791112c9b78bda1c9bb8544e2fa5819b

pat commented 3 years ago

Thanks for raising this :)

One thing to note is that after_save is invoked for both creates and updates (so, the callback should have still been firing, but perhaps with incomplete data changes). I think you're right in waiting for the transaction to be committed before updating Sphinx, so I've switched over to after_commit instead - albeit with different approaches for destroys vs creates/updates, as it's invoked in all cases.

It all seemed fine on my machine, and hopefully CI agrees - I'll try to get a new release out today.

pat commented 3 years ago

Just pushed 5.3.0 with this fix :)

krauselukas commented 3 years ago

@pat thanks for the quick response and release!