Still touch associations when theres no timestamp

[huoxito]

Prior to Rails 4.0.4 when touching a object which doesn't have timestamp
attributes (updated_at / updated_on) rails would still touch all
associations. After 73ba2c1 it updates
associations but rollsback because touch would return nil since
there's no timestamp attribute.

This sounds a bit insane (touching an object without timestamps) but I came through it today while upgrading and just felt like reporting anyway due to being a change of behaviour between minor releases.

The following would pass on 4.0.3 but fails on 4.0.4

unless File.exist?('Gemfile')
  File.write('Gemfile', <<-GEMFILE)
    source 'https://rubygems.org'
    gem 'rails', path: '../rails'
    gem 'sqlite3'
  GEMFILE

  system 'bundle'
end

require 'bundler'
Bundler.setup(:default)

require 'active_record'
require 'minitest/autorun'
require 'logger'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :products do |t|
    t.timestamps
  end

  create_table :assets do |t|
    t.integer :product_id
  end
end

class Product < ActiveRecord::Base
  has_many :assets
end

class Asset < ActiveRecord::Base
  belongs_to :product, touch: true
end

class BugTest < MiniTest::Unit::TestCase
  def test_association_stuff
    product = Product.create!
    image = Asset.create! product: product
    initial = product.reload.updated_at
    image.touch

    refute_equal initial, product.reload.updated_at
  end
end

[rafaelfranca]

Could you use assert_not?

[rafaelfranca]

I'm more to revert that commit than changing this.

@carlosantoniodasilva are you sure touch should fire after_commit/after_rollback callbacks. The documentation is very clear about firing only after_touch.

# Saves the record with the updated_at/on attributes set to the current time.
# Please note that no validation is performed and only the +after_touch+
# callback is executed.

[arthurnn]

cc @airhorns

[arthurnn]

my two cents on this: If this were catch before releasing 4.0.4 I would revert it, as indeed it should not be in a minor version. However as this is part of 4.0.4 already, and we can fix it without impacting too much, I would go with the fix for version 4.0.5.

[rafaelfranca]

This fix is wrong in my opinion and could cause more regression. But of course we could provide another fix we can keep this change.

[jeremy]

No need to ask "what if"—we can revert now, then revert the revert when a fix is ready.

[huoxito]

I'm certainly not the best person to tell it but fixing this some other way could add a lot of complexity which might not be worth it. So maybe we could just live with it.

I only opened this because I noticed this failing spec on spree build and thought I should report but I don't think the other change should be reverted. It didn't actually cause us any real trouble so far. Possibly easy fix for everyone would be to add the missing timestamps fields to get rid of the rollbacks.

[huoxito]

Hey guys this other approach just came to mind:

def touch(*) #:nodoc:
  if timestamp_attributes_for_update_in_model.empty?
    super
  else
    with_transaction_returning_status { super }
  end
end

Wdyt? build is still happy.

[airhorns]

Original author of the commit which introduced this bug here - sorry! Just wanted to throw in my two cents however.

If we make after_commit and after_rollback not fire upon touch I think the API ends up more broken than it is right now. after_commit is the only mechanism users of Rails have to know when things have actually successfully changed in the database to update other systems. after_save, and after_touch and company work well if you are messing with the database inside the transaction because a save or touch has happened, but neither mechanism works if you want to say only update redis with some info or bust a cache after the transaction has actually committed. With my change, it's actually possible to find out when a touch operation has been committed, and without, it is impossible unless you monkeypatch Rails, which we did at Shopify for a while.

So, I'd rather leave it to the user to properly call touch on models instead of changing the above subtle behaviour, but in all fairness this kind of violates the Tell Don't Ask principle... Would it be so bad for touch to return true if there were no timestamp columns?

[rafaelfranca]

Would it be so bad for touch to return true if there were no timestamp columns?

I was thinking about this. I guess it may be fine since the touch operation will still occurs in the associated models if the model don't have timestamp columns.

Also reverting your patch also brings a undesirable behavior that touch doesn't run inside a transaction, what is bad in my opinion.

I'll check the patch again later.

[arthurnn]

I am 👍 on this patch.. @rafaelfranca any final remarks about it? anything else we need to get done before 🚢 this?

[rafaelfranca]

I'll merge this today.

[huoxito]

nice thanks @rafaelfranca I'll make the changes you requested above later today

[carlosantoniodasilva]

Hey guys, sorry I somehow missed the thread here. I do think after_commit/rollback callbacks should be triggered on touch, so I guess fixing the bug in question here is the way to go (with this or a different approach). Thanks for handling this ❤️

[rafaelfranca]

Done

[arthurnn]

thanks all ❤️