Fix callback to only run before update

[seanlinsley]

before_save doesn't accept :on as an argument, so this callback was actually running on all save events, not just on update.

rails/rails#17622

Depending on what reset_timestamp_attrs_for_update_if_needed does, this might need to be backported to older versions.

I've submitted a PR to Rails to raise an exception when unexpected arguments are passed... unsure if an exception is the right way to go when there might be quite a few gems with this bug.

rails/rails#30919

[jaredbeck]

Hi Sean,

Thanks for finding this issue, and for patching it in rails. APIs that do not validate their input are just asking for trouble, aren't they?

Depending on what reset_timestamp_attrs_for_update_if_needed does ..

Basically, it calls restore_updated_at!, but why, I don't know. If I am updating a reified record, or any record, I'd want the updated_at timestamp to reflect the current time. Can you look through some blames and figure out the purpose of reset_timestamp_attrs_for_update_if_needed?

The MySQL tests are failing, e.g.

 1) Widget Callbacks before_save resets value for timestamp attrs for update so that value gets updated properly
     Failure/Error: expect { subject.save! }.to change(subject, :updated_at)
       expected #updated_at to have changed, but is still 2017-10-18 16:15:38.000000000 +0000
     # ./spec/models/widget_spec.rb:62:in `block (5 levels) in <top (required)>'
     # ./spec/models/widget_spec.rb:61:in `block (4 levels) in <top (required)>'

Given that only MySQL is failing, and other RDBMS' are passing, and it's timestamp related, I'm guessing this is due to MySQL's lack of fractional seconds precision. Or, maybe the test is just written poorly.

[seanlinsley]

That test was originally added by eb36d1a to fix #357. So perhaps the code should've always been before_save instead of before_save on: :update.

[jaredbeck]

That test was originally added by eb36d1a to fix #357. So perhaps the code should've always been before_save instead of before_save on: :update.

Hmmm given that we're only talking updates, before_update seems most accurate.

Any insight into why the MySQL tests are failing?

[seanlinsley]

Well the failing test doesn't actually fail on an update, it fails on reifying a deleted version.

Hmm but only MySQL is failing... Perhaps it just needs to be changed to Timecop.travel(2)

[jaredbeck]

Hmm but only MySQL is failing... Perhaps it just needs to be changed to Timecop.travel(2)

Yeah, but that travel has been working for us recently ... maybe start by making sure that the callback is still happening? We expect it to happen, right? The failing test is calling save! but that should still trigger the before_update callback, right?

[seanlinsley]

I'd expect that before_update wouldn't be called for that test case, because it's recreating a deleted version. So before_create and before_save would run, but before_update wouldn't.

Which is doubly confusing because the test passes for SQLite and Postgres.

[jaredbeck]

I'd expect that before_update wouldn't be called for that test case, because it's recreating a deleted version. So before_create and before_save would run, but before_update wouldn't.

Yeah, it's just calling save! on something with no changes.

Which is doubly confusing because the test passes for SQLite and Postgres.

😆 Yeah. Well those two RDBMS have fraction-seconds precision, so maybe their timestamps are changing by a fraction of a second when we call save!?

[jaredbeck]

I wonder if the order of callbacks matters? before_save comes before before_update.

The simplest/safest thing we can do is to stick with before_save and just drop the on.

[seanlinsley]

I switched it back to before_save. Now to see if the tests pass...

[seanlinsley]

They did ¯\(ツ)/¯

[jaredbeck]

Works for me, thanks Sean.

[seanlinsley]

Happy to help. Here's hoping the Rails PR gets merged; it's such an unfortunate bug.