pat / thinking-sphinx

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

Real Time indices: after_save_commit #1193

Closed kalsan closed 3 years ago

kalsan commented 3 years ago

Hi Pat!

I've recently tried to combine the power of ThinkingSphinx with the glory of ActiveStorage. Turns out, ActiveStorage only completes uploaded content after the transaction was committed, as discussed e.g. here: https://github.com/rails/rails/issues/37304

This is a problem when trying to combine it with ThinkingSphinx real-time hooks, because ThinkingSphinx::RealTime::Callbacks::RealTimeCallbacks currently only supports the after_save hook. If you want to real-time index contents from uploaded files, the upload completes way after the TS callback is invoked. This means that methods such as my_attachment.download will fail at the time of the callback.

For this reason, it might be interesting to have an after_save_commit available for ThinkingSphinx::RealTime::Callbacks::RealTimeCallbacks. This is only an idea, I'm looking forward to hearing you opinion on this topic.

Cheers! Kalsan

pat commented 3 years ago

Hi Kalsan :)

Thanks for the suggestion. At this point, I think this situation can be handled by calling the standard callback within your own custom callback - such as in the third code sample in the documentation:

after_save :populate_to_sphinx

# ...

def populate_to_sphinx
  return unless indexing?

  ThinkingSphinx::RealTime::Callbacks::RealTimeCallbacks.new(
    :article
  ).after_save self
end

Changing it to use after_commit instead (but still use the after_save method in your use of ThinkingSphinx::RealTime::Callbacks::RealTimeCallbacks) should, I think, do the trick? Certainly if it doesn't, then we'll need to look into another way, like perhaps what you've suggested.

Let me know how this goes! :)

kalsan commented 3 years ago

Hi pat!

Ah, I get it - after_save does not refer to a Rails hook but is to be interpreted as a call back (equivalent to on_record_saved)

Thanks for the tip and have a great week!

Cheers Kalsan

pat commented 3 years ago

Yeah, it's because when you give a Rails callback an object, it'll call a method with the same name as a callback with the record instance. i.e.

after_save ThinkingSphinx::RealTime::Callbacks::RealTimeCallbacks.new(:article)

But you can still use that object in other callbacks, provided you call that after_save method on it.

I would prefer that Rails would just use call instead - thus making it more generic and obviously useful across different callbacks - but I guess there's also good reasons for their current approach.

kalsan commented 3 years ago

Awesome, thanks!