pat / ts-delayed-delta

Manage delta indexes via Delayed Job for Thinking Sphinx
http://ts.freelancing-gods.com
MIT License
73 stars 33 forks source link

Delta index runs even if object was not persisted #19

Closed enrico closed 11 years ago

enrico commented 12 years ago

I just exposed some APIs to my app, and discovered this problem. When I invoke my API to create a new record, i.e.

business.customers.create(:first_name => 'foo', ...)

and there is a validation error (i.e. email is already taken). I get an exception on my log:

Started POST "/api/customers" for 127.0.0.1 at 2012-01-29 14:10:49 -0600
  User Load (0.7ms)  SELECT `users`.* FROM `users` WHERE `users`.`authentication_token` = 'vZSDZumq7doM4tgxoTdSHHXR3m1Pqs8C' AND `users`.`current_state` IS NULL LIMIT 1
  Business Load (0.6ms)  SELECT `businesses`.* FROM `businesses` WHERE `businesses`.`id` = 1 LIMIT 1
   (0.1ms)  BEGIN
   (0.3ms)  SELECT 1 FROM `customers` WHERE (LOWER(`customers`.`email`) = LOWER('foo@bar.com') AND `customers`.`business_id` = 1) LIMIT 1
   (0.1ms)  COMMIT
     (0.4ms)  SELECT COUNT(*) FROM `delayed_jobs` WHERE `delayed_jobs`.`handler` = '--- !ruby/object:ThinkingSphinx::Deltas::DeltaJob \nindices: \n- customer_delta\n' AND `delayed_jobs`.`locked_at` IS NULL
You have a nil object when you didn't expect it!
You might have expected an instance of Array.
The error occurred while evaluating nil.*

I believe this is due to the fact that delayed delta checks whether the delta column has been toggled(which in my case was true), but not whether the object has been persisted.

I wanted to provide a patch, but I'm not sure where to add the check: I found 2 places that look like they need it:

delta.rb: line 45

delayed_delta.rb: line 63-65

am I wrong? any feedback?

pat commented 12 years ago

Hi Enrico

Not sure if you're right - it's certainly possible, but it would mean a bug elsewhere. The delta indexing is fired by the after_commit callback on the model - so it means that this callback is being fired even when validations fail, I guess? Which versions of Thinking Sphinx and Rails are you using?

enrico commented 12 years ago

I'm using rails 3.1.3. I guess you're right, not really a bug here, since your code relies on an after-commit hook... I tried to create an invalid user, first invoking User.create(), then by using the parent: business.users.create().

Even if the user doesn't get created in either case, there's a commit (not a rollback) being performed when I invoke create from the parent model, which I guess is triggering ts-delayed-delta to do its thing...

Look at my log differences when I create a user normally and 'thru the 'parent' business:

ruby-1.9.3-p0 :012 > u=User.create(:email=>'foo@bar', :first_name=>'foo', :last_name => 'bar')
   (0.2ms)  BEGIN
   (0.4ms)  SELECT 1 FROM `users` WHERE `users`.`email` = BINARY 'foo@bar' LIMIT 1
   (0.1ms)  ROLLBACK
 => #<User id: nil, business_id: nil, email: "foo@bar", encrypted_password: "", first_name: "foo", last_name: "bar", phone: nil, current_state: "signup", abilities_mask: 0, failed_attempts: 0, unlock_token: nil, locked_at: nil, confirmation_token: nil, confirmed_at: nil, confirmation_sent_at: nil, unconfirmed_email: nil, reset_password_token: nil, reset_password_sent_at: nil, sign_in_count: 0, current_sign_in_at: nil, last_sign_in_at: nil, current_sign_in_ip: nil, last_sign_in_ip: nil, authentication_token: nil, google_auth_secret: nil, created_at: nil, updated_at: nil, deleted_at: nil> 
ruby-1.9.3-p0 :013 > u.errors.messages
 => {:email=>["is invalid"]} 
ruby-1.9.3-p0 :014 >
ruby-1.9.3-p0 :014 >
ruby-1.9.3-p0 :014 > u=Business.first.users.create(:email=>'foo@bar', :first_name=>'foo', :last_name => 'bar')
  Business Load (0.6ms)  SELECT `businesses`.* FROM `businesses` LIMIT 1
   (0.1ms)  BEGIN
   (0.2ms)  SELECT 1 FROM `users` WHERE `users`.`email` = BINARY 'foo@bar' LIMIT 1
   (0.1ms)  COMMIT
 => #<User id: nil, business_id: 1, email: "foo@bar", encrypted_password: "", first_name: "foo", last_name: "bar", phone: nil, current_state: "signup", abilities_mask: 0, failed_attempts: 0, unlock_token: nil, locked_at: nil, confirmation_token: nil, confirmed_at: nil, confirmation_sent_at: nil, unconfirmed_email: nil, reset_password_token: nil, reset_password_sent_at: nil, sign_in_count: 0, current_sign_in_at: nil, last_sign_in_at: nil, current_sign_in_ip: nil, last_sign_in_ip: nil, authentication_token: nil, google_auth_secret: nil, created_at: nil, updated_at: nil, deleted_at: nil> 
ruby-1.9.3-p0 :015 > u.errors.messages
 => {:email=>["is invalid"]} 
ruby-1.9.3-p0 :016 > 
pat commented 12 years ago

Perhaps this is a bug in Rails? after_commit should only apply to the object in question, surely... although, not seeing the delta get fired in the output you've just supplied.

enrico commented 12 years ago

Pat, the reason you're not seeing the delayed delta fire is because delayed delta runs on the customers (and not the users) object off of business. I purposely tried to run this on a non-sphinx-indexed model to see if it was related to ts-delayed-delta or not...Looks like a rails issue to me.... I'll retry with 3.2 and see what I get...