Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Still touch associations when theres no timestamp #14390

Merged
merged 1 commit into from Mar 25, 2014

Conversation

Projects
None yet
7 participants
Contributor

huoxito commented Mar 15, 2014

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
Still touch associations when theres no timestamp
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

@robin850 robin850 added this to the 4.0.5 milestone Mar 15, 2014

@robin850 robin850 added the regression label Mar 15, 2014

@rafaelfranca rafaelfranca commented on the diff Mar 15, 2014

...st/cases/associations/belongs_to_associations_test.rb
@@ -340,6 +340,17 @@ def test_belongs_to_with_touch_option_on_touch
assert_queries(1) { line_item.touch }
end
+ def test_belongs_to_with_touch_option_on_touch_without_updated_at_attributes
+ assert !LineItem.column_names.include?("updated_at")
@rafaelfranca

rafaelfranca Mar 15, 2014

Owner

Could you use assert_not?

@rafaelfranca rafaelfranca commented on the diff Mar 15, 2014

...st/cases/associations/belongs_to_associations_test.rb
@@ -340,6 +340,17 @@ def test_belongs_to_with_touch_option_on_touch
assert_queries(1) { line_item.touch }
end
+ def test_belongs_to_with_touch_option_on_touch_without_updated_at_attributes
+ assert !LineItem.column_names.include?("updated_at")
+
+ line_item = LineItem.create!
+ invoice = Invoice.create!(line_items: [line_item])
+ initial = invoice.updated_at
+ line_item.touch
+
+ refute_equal initial, invoice.reload.updated_at
@rafaelfranca

rafaelfranca Mar 15, 2014

Owner

Could you use assert_not_equal?

@rafaelfranca rafaelfranca commented on the diff Mar 15, 2014

activerecord/lib/active_record/persistence.rb
@@ -452,6 +452,8 @@ def touch(name = nil)
changed_attributes.except!(*changes.keys)
primary_key = self.class.primary_key
self.class.unscoped.where(primary_key => self[primary_key]).update_all(changes) == 1
+ else
+ true
@rafaelfranca

rafaelfranca Mar 15, 2014

Owner

Returning true seems wrong to me. We are telling the touch operation was successful even if it was not. I'm think this is more an usage problem than a Rails bug.

Owner

rafaelfranca commented Mar 15, 2014

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.
Member

arthurnn commented Mar 16, 2014

Member

arthurnn commented Mar 16, 2014

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.

Owner

rafaelfranca commented Mar 16, 2014

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.

Owner

jeremy commented Mar 16, 2014

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

Contributor

huoxito commented Mar 16, 2014

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.

Contributor

huoxito commented Mar 16, 2014

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.

Contributor

airhorns commented Mar 17, 2014

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?

Owner

rafaelfranca commented Mar 17, 2014

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.

Member

arthurnn commented Mar 25, 2014

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

Owner

rafaelfranca commented Mar 25, 2014

I'll merge this today.

Contributor

huoxito commented Mar 25, 2014

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

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 added a commit that referenced this pull request Mar 25, 2014

Merge pull request #14390 from huoxito/true-touch
Still touch associations when theres no timestamp

@rafaelfranca rafaelfranca merged commit ea3a73e into rails:master Mar 25, 2014

1 check passed

default The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Mar 25, 2014

Merge pull request #14390 from huoxito/true-touch
Still touch associations when theres no timestamp

rafaelfranca added a commit that referenced this pull request Mar 25, 2014

Merge pull request #14390 from huoxito/true-touch
Still touch associations when theres no timestamp

rafaelfranca added a commit that referenced this pull request Mar 25, 2014

Merge pull request #14390 from huoxito/true-touch
Still touch associations when theres no timestamp
Owner

rafaelfranca commented Mar 25, 2014

Done

Member

arthurnn commented Mar 25, 2014

thanks all ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment