algolia / algoliasearch-rails

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

callbacks should use after_commit, and not after_save #61

Closed iffyuva closed 9 years ago

iffyuva commented 9 years ago

code in question: https://github.com/algolia/algoliasearch-rails/commit/fbdff0d#diff-dfaec67e907148ea8211e17340f0f30cR267

after_save shouldn't be used because it fails in transaction. consider this example

class User < AR::Base
  algoliasearch index_name: 'users' do
    attributes :name
  end
end

user = User.create(name: 'Bob')

# after couple of days.
User.transaction do
  user.update_attributes!(name: 'Marley')
  call_a_method_which_raises_error!
end

since we are using after_save, it will update algolia with user name as 'Marley'. so, its advised to use after_commit. after_commit will only be called if transaction is successful, which is the behavior we want here.

elasticsearch-rails does the same, check this readme for more info: https://github.com/elastic/elasticsearch-rails/blob/738c63ef/elasticsearch-model/README.md#custom-callbacks

nishantmodak commented 9 years ago

:+1:

redox commented 9 years ago

Yeah I agree, we should move to after_commit. I just need to confirm it's compliant with Mongoid and Sequel :)

redox commented 9 years ago

@nishantmodak @iffyuva OK for you guys?

iffyuva commented 9 years ago

@redox i doubt if mongoid has after_commit, http://mongoid.org/en/mongoid/docs/callbacks.html doesn't list it. i suppose we should modify code. i don't understand Sequel :), so can't comment.

this is not a high priority: while we are at it, can we extract code into modules specific to db adapters? i think that makes code more readable.

redox commented 9 years ago

Oh yeah good catch, I was sure I had some tests with Mongoid and that since they were passing I thought everything was OK.

Indeed, since a few releases we now have the Sequel compatibility which make the code hard to read. I'll create a new issue to introduce the Adapter pattern, I agree.