active_record_upsert
active_record_upsert copied to clipboard
jesjos
Updating does not fire callbacks with updated attributes
When updating a record via upsert!, callbacks do not use the latest attribute values. See example below:
class Car < ActiveRecord::Base
upsert_keys [:internal_id]
before_save :calc_sum
def calc_sum
self.sum = first_val + second_val
end
end
Car.upsert!(internal_id: 1, first_val: 1, second_val: 2) # sets sum to 3
car = Car.upsert!(internal_id: 1, first_val: 2, second_val: 2) # *should* set sum to 4
car.sum == 4 # returns false, sum is still 3
I think its related to this commit.
Hi @abinoda!
I can confirm that this doesn't work as expected and was able to trace it down to this line: https://github.com/jesjos/active_record_upsert/blob/24d7faaaf6ca63f987a870c0f6c47e02e8d50b48/lib/active_record_upsert/active_record/persistence.rb#L47
When using .upsert!, the attributes used in the ON CONFLICT get limited to whatever is passed to the method in the first place. Any changes happening in the callbacks are ignored as a result.
Maybe it's save to remove that parameter? I don't know to be honest 😄
@benedikt Would adding a run_callbacks(:before_save) to #upsert! work? Before line 8 of https://github.com/jesjos/active_record_upsert/blob/24d7faaaf6ca63f987a870c0f6c47e02e8d50b48/lib/active_record_upsert/active_record/persistence.rb#L8
I'm not familiar with this code but just an idea.
No, run_callbacks(:save) runs all save related callbacks (before, around, after).
@benedikt Should the README be updated? See related – https://github.com/zdennis/activerecord-import#callbacks
Or I wonder if all callback support should just be removed since they don't work predictably or consistently? Instead of there being ambiguous partial support? That'd be easier and less error-prone for developers, avoiding scenarios like what occurred here for me.
I have this vague memory that we added callback support in order to have timestamps work like you expect, but I'm not sure. I'm all for removing callback support if we can just make sure timestamps work.
@jesjos The timestamps don't rely on callbacks right now: https://github.com/jesjos/active_record_upsert/blob/master/lib/active_record_upsert/active_record/timestamp.rb
However, I still think this isn't a problem with callbacks, but with the way .upsert! handles the attributes that will be saved to the database. It will only update the given attributes in the ON CONFLICT clause.
The initial problem can be easily bypassed by explicitly passing sum: nil as an attribute.
Car.upsert!(internal_id: 1, first_val: 1, second_val: 2)
car = Car.upsert!(internal_id: 1, first_val: 2, second_val: 2, sum: nil)
car.sum #=> 4
Just to provide some more color to this, I'm currently looking at using https://github.com/attr-encrypted/attr_encrypted. I'm thinking through whether or not it works out of the box with upsert.
It would be nice to be able to say "oh, upsert" doesnt handle callbacks, I need to take care of them manually" versus trying to figure out what callbacks do and don't fire, and how to only address the ones that dont. The former is simpler.
So personally, I'd vote for removing all callback support in a breaking change to simplify this library.