Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In after_touch, incorrect saved_changes #33429

Closed
jaredbeck opened this issue Jul 24, 2018 · 11 comments
Closed

In after_touch, incorrect saved_changes #33429

jaredbeck opened this issue Jul 24, 2018 · 11 comments

Comments

@jaredbeck
Copy link
Contributor

jaredbeck commented Jul 24, 2018

touch only updates timestamps. Therefore, during the after_touch callback, I'd expect saved_changes to only contain those timestamps, but ..

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "5.2.0"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.timestamps null: false
  end
end

class Post < ActiveRecord::Base
  after_touch :inspect_saved_changes

  def inspect_saved_changes
    $after_touch_saved_changes = saved_changes.deep_dup
  end
end

class BugTest < Minitest::Test
  def test_saved_changes
    post = Post.create!
    post.touch
    assert_equal %w[updated_at], $after_touch_saved_changes.keys
    #   1) Failure:
    # --- expected
    # +++ actual
    # @@ -1 +1 @@
    # -["updated_at"]
    # +["id", "created_at", "updated_at"]
  end
end
@y-yagi
Copy link
Member

y-yagi commented Aug 2, 2018

I do not know what this behavior was guaranteed, Dirty uses after_create / after_update callbacks to track changes.

after_create { changes_applied }
after_update { changes_applied }

But touch does not execute after_create / after_update callbacks. Therefore, it seems like specifications that can not get changed attributes.
https://edgeapi.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-touch

Was this working with the old version of Rails?

@jaredbeck
Copy link
Contributor Author

Hello Yaginuma-san, thank you for your response.

Was this working with the old version of Rails?

I think saved_changes was introduced in rails 5.1, which seems to have the same behavior as 5.2.

But, touch only updates timestamps, so it seems wrong to claim that other changes occurred.

1) Failure:
--- expected
+++ actual
@@ -1 +1 @@
-["updated_at"]
+["id", "created_at", "updated_at"]

For example, the id has not changed, so it seems wrong to claim that it has changed.

@sgrif what do you think?

@y-yagi
Copy link
Member

y-yagi commented Aug 2, 2018

Thank you for your comment.

Since saved_changes is not affected by touch, changes made at create remain as they are.
Therefore, if do reload before touch, the result will be empty.

diff --git a/33429.rb b/33429.rb
index 422bafd..753b54e 100644
--- a/33429.rb
+++ b/33429.rb
@@ -45,6 +45,7 @@ end
 class BugTest < Minitest::Test
   def test_saved_changes
     post = Post.create!
+    post.reload
     post.touch
     assert_equal %w[updated_at], $after_touch_saved_changes.keys
$ ruby 33429.rb
Finished in 0.009886s, 101.1511 runs/s, 101.1511 assertions/s.

  1) Failure:
BugTest#test_saved_changes [33429.rb:50]:
Expected: ["updated_at"]
  Actual: []

1 runs, 1 assertions, 1 failures, 0 errors, 0 skips 

(I also want to wait for the other maintainer's comment ;)

@jaredbeck
Copy link
Contributor Author

Since saved_changes is not affected by touch, changes made at create remain as they are.
Therefore, if do reload before touch, the result will be empty.

Oh! I did not realize the changes were coming from the create. Thank you, that is helpful.

Maybe we just need to mention in the documentation, something like:

     # Saves the record with the updated_at/on attributes set to the current time
     # or the time specified.
     # Please note that no validation is performed and only the +after_touch+,
-    # +after_commit+ and +after_rollback+ callbacks are executed.
+    # +after_commit+ and +after_rollback+ callbacks are executed. No dirty-tracking
+    # occurs, meaning that the `Dirty` methods, like `changed?`, `previous_changes`,
+    # and `changes` return the same values before and after `touch`.
     #

@TylerRick
Copy link
Contributor

TylerRick commented Oct 24, 2018

I guess that makes some sense...

But we still need some way to tell which attributes were changed by the touch.

Could we introduce another mutation tracker that gets reset before the touch and modified by the touch so that it can be inspected to determine what was changed by the touch ... Maybe add something like changes_from_touch to the complement saved_changes and friends? Then, just like you can use saved_changes within an after_save, you could use touched_changes (or whatever we'd name it) inside an after_touch.

Or, if that seems like overkill (though with the existing complexity surrounding touch already like NoTouching and TouchLater how could it be? 😉), should we at least pass a hash of changes to the after_touch callback(s)?

@rails-bot
Copy link

rails-bot bot commented Jan 22, 2019

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Jan 22, 2019
@jaredbeck
Copy link
Contributor Author

Not stale. Waiting for #34306 (improves docs) to be merged.

@rails-bot rails-bot bot removed the stale label Jan 22, 2019
@kamipo kamipo closed this as completed in d1107f4 Apr 15, 2019
@jaredbeck
Copy link
Contributor Author

kamipo closed this in d1107f4 13 hours ago

Thanks!

@jgigault
Copy link

Dear contributors,

Reading carefully this issue and the related Pull Requests, I am still a bit confused with the behavior of Dirty with touch when it is triggered by association belongs_to with touch: true option.

It looks like touching objects "manually" versus "in cascade through belongs_to association" does not result in the same behavior.

Given the following models:

class Project < ActiveRecord::Base
  has_many :documents

  after_touch :my_after_touch_callback

  def my_after_touch_callback
    puts "updated_at_before_last_save:#{updated_at_before_last_save}"
  end
end

class Document < ActiveRecord::Base
  belongs_to :project, touch: true
end

I can observe different behaviors with Dirty methods:

Project.last.touch
# updated_at_before_last_save:2019-11-24 12:58:24 UTC

Project.last.documents.last.touch
# updated_at_before_last_save:

Am I wrong somewhere with my investigation? Is that the expected behavior regarding this issue resolution?

I was not able to find documentation about that, please apologize if my question is kind of newbie :-)

@jaredbeck
Copy link
Contributor Author

Hi Jean-Michel.

It looks like touching objects "manually" versus "in cascade through belongs_to association" does not result in the same behavior.

Not sure if it's a bug, but I can reproduce this in rails latest (6.0.1) with a script

However, saved_changes seems to work ..

  def my_after_touch_callback
    puts saved_changes[:updated_at] #=> [nil, 2019-11-25 ...
  end

Perhaps saved_changes works and updated_at_before_last_save does not, because the later only works after a Project save, not after a Project create?

.. [I] apologize if my question is kind of newbie ..

I think this is quite a complicated question, actually 😄 très difficile

@jgigault
Copy link

Thank you for your answer @jaredbeck :-)
Do you think that I should open an issue to describe the discrepancy?

zammad-sync pushed a commit to zammad/zammad that referenced this issue Jun 14, 2021
…as expected in Rails < 6 because of these bugs:

- rails/rails#33429
- rails/rails#30466

This lead to that `saved_changes` always contained all attributes (for the test!) and therefore `saved_change_to_attribute?` always returned true in `app/models/concerns/has_ticket_create_screen_impact.rb`.

This also means that `#touch` actually never triggered the queueing of the `TicketCreateScreenJob` before the update to Rails 6.
That's because `saved_changes` was empty for `#touch` saves. With Rails 6 `saved_changes` contains `updated_at` for `#touch` saves. Therefore we have to add `updated_at` to our `saved_change_to_attribute?` check list.

TODO: We need to check the impact of this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants