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

Update dirty before callbacks #667

Merged
merged 2 commits into from Jan 19, 2021
Merged

Conversation

ujihisa
Copy link
Contributor

@ujihisa ujihisa commented Nov 25, 2020

  • ActiveRecord updates its changes/previous_changes before
    after_create/after_update callbacks
  • However, MongoMapper does that after the callbacks, therefore the
    changes/previous_changes values don't match with equivalent code in
    ActiveRecord.

Comparisons

ActiveRecord:

require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  gem 'activerecord'
  gem 'sqlite3'
end
require 'active_record'

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

ActiveRecord::Schema.define do
  create_table :docs, force: true do |t|
    t.text :x
  end
end

class Doc < ActiveRecord::Base
  after_save do
    p changes #=>          {}
    p previous_changes #=> {"id"=>[nil, 1], "x"=>[nil, "hello"]} at create
                       #=> {"x"=>["hello", "world"]} at save!
  end

  after_create do
    p changes #=>          {}
    p previous_changes #=> {"id"=>[nil, 1], "x"=>[nil, "hello"]}
  end

  after_update do
    p changes #=>          {}
    p previous_changes #=> {"x"=>["hello", "world"]}
  end
end

d = Doc.create(x: 'hello')
d.x = 'world'
d.save!

MongoMapper:

document = Doc {
  key :x, String

  after_save do
    p changes #=>          {}
    p previous_changes #=> {"id"=>[nil, 1], "x"=>[nil, "hello"]} at create
                       #=> {"x"=>["hello", "world"]} at save!
  end
  after_create {
    p changes #=>          {"x"=>[nil, "hello"]}
    p previous_changes #=> {}
  }
  after_update {
    p changes #=>          {"x"=>["hello", "world"]}
    p previous_changes #=> {"x"=>[nil, "hello"]}
  }
}

d = document.create(x: 'hello')
d.x = 'world'
d.save!

MongoMapper with this changeset:

document = Doc {
  key :x, String

  after_save {
    p changes #=>          {}
    p previous_changes #=> {"x"=>[nil, "hello"]} at create
                       #=> {"x"=>["hello", "world"]} at save
  }
  after_create {
    p changes #=>          {}
    p previous_changes #=> {"x"=>[nil, "hello"]}
  }
  after_update {
    p changes #=>          {}
    p previous_changes #=> {"x"=>["hello", "world"]}
  }
}

d = document.new(x: 'hello')
d.save
d.x = 'world'
d.save!

The only difference between the new behaviour and ActiveRecord is to show id in previous_changes or not. Since the behaviour for IDs are already different between MongoMapper and ActiveRecord, I kept it as is, but if it makes more sense I'm also happy to work on adding _id into the previous_changes.

* ActiveRecord updates its changes/previous_changes before
  after_create/after_update callbacks
* However, MongoMapper does that after the callbacks, therefore the
  changes/previous_changes values don't match with equivalent code in
  ActiveRecord.
@smtlaissezfaire
Copy link
Member

Need to review / think more about the implementation but like the idea overall. This is great!

I think this will have to be a major version release since it's a major change in how the software works. So just heads up it might be a little bit before this is released.

FYI, this is how AR used to work so MM was just copying ActiveRecord from back then. But agreed it should work the same way to abide by the principle of least surprise.

@smtlaissezfaire
Copy link
Member

For my own reference (as I'm in the middle of upgrading a rails app) - I ran into this:

https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Dirty.html#method-i-saved_change_to_attribute

and the error that rails 5.1 generates:

DEPRECATION WARNING: The behavior of attribute_changed?inside of after callbacks will be changing in the next version of Rails. The new return value will reflect the behavior of calling the method aftersave returned (e.g. the opposite of what it returns now).

@ujihisa
Copy link
Contributor Author

ujihisa commented Nov 26, 2020

Hmm, I believe this is not a breaking change but rather a bugfix, since the behaviour with after_save has already been same to ActiveRecord, and that's causing an inconsistent changes between after_save and after_update.

Doc.create(x: 'hello').changes:

ActiveRecord MongoMapper MongoMapper with this changeset
after_save {} {} {}
after_create {} {"x"=>[nil, "hello"]} {}

d.x = 'world'; d.changes:

ActiveRecord MongoMapper MongoMapper with this changeset
after_save {} {} {}
after_update {} {"x"=>["hello", "world"]} {}

^ See these examples; I would consider changes at these after callbacks to be always empty when they succeeded.

Sorry, the original issue description I posted has been kinda misleading. I updated the comparison codes in the description now too.

* Also cover the case when some callbacks didn't get called
@ujihisa
Copy link
Contributor Author

ujihisa commented Nov 26, 2020

I added ad9e819 to cover the after_save in the test too.

While adding after_save test, since after_save callback is called twice in the test, I tweaked the test code structure to assert using a strategy to use "history" var like existing callback test :)

@ujihisa
Copy link
Contributor Author

ujihisa commented Dec 7, 2020

Please let me know if I need further descriptions or changes for this; I'm more than happy to work on them.

@smtlaissezfaire
Copy link
Member

Thanks @ujihisa. Yeah, I need to look at the pending patches! Will do soon.

@a2ikm
Copy link
Contributor

a2ikm commented Dec 14, 2020

JFYI, this PR will fix #673.
Thanks!

@smtlaissezfaire
Copy link
Member

@ujihisa So FYI I do want to get this in, but first would like to release a point version with pending changes.

Then I'm also reworking a lot of the proxy code. It's sort of a cleanup project but I'd like to get that in before taking this on. This would probably be a minor release version alone.

But just a heads up in still on my radar - just waiting a bit to merge it. Thanks for your patience.

@ujihisa
Copy link
Contributor Author

ujihisa commented Jan 18, 2021

Thanks for the update! Yes, it makes sense to release a minor version first with the changes already in the master branch before this.

However, I still believe this fix should be included not too late, even before the rework, as I mentioned at #667 (comment). It's attracting to include proxy code reworks first to clean things up without changing behaviour, but it will also need to take time finding out new bugs before release.

@mtsmfm
Copy link
Contributor

mtsmfm commented Jan 18, 2021

@smtlaissezfaire cc @ujihisa

I think this change should be shipped as bugfix because the behavior for changes already changed since 0.15 and behaves like latest active record.

MongoMapper::Version 0.14.0
after_save
{"x"=>[nil, "hello"]}
{}
after_save
{"x"=>["hello", "world"]}
{"x"=>[nil, "hello"]}
MongoMapper::Version 0.15.1
after_save
{}
{"x"=>[nil, "hello"]}
after_save
{}
{"x"=>["hello", "world"]}
code
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'pry-byebug'
  gem 'activemodel', '5.0.7.2'

  case ENV['MM_VERSION']
  when '0.15', 'pr-667'
    gem 'mongo_mapper', '0.15.1'
  when '0.14'
    gem 'mongo_mapper', '0.14.0'
    gem 'bson', '1.9.2'
  else
    raise 'MM_VERSION must be 0.14, 0.15 or pr-667'
  end
  gem 'bson_ext'
  gem 'activemodel-serializers-xml'
end

MongoMapper.setup({'development' => {'uri' => 'mongodb://mongo/test'}}, 'development')

if ENV['MM_VERSION'] == 'pr-667'
  module MongoMapper
    module Plugins
      module Dirty
        def create_or_update(*)
          super
        end

        def save_to_collection(*)
          super.tap do
            changes_applied
          end
        end
      end
    end
  end
end

class Doc
  include MongoMapper::Document

  key :x, String

  after_save do
    puts "after_save"
    p changes
    p previous_changes
  end
end

if ENV['MM_VERSION'] == 'pr-667'
  puts "MongoMapper::Version #{MongoMapper::Version} (with pr-667 patch)"
else
  puts "MongoMapper::Version #{MongoMapper::Version}"
end

d = Doc.create(x: 'hello')
d.x = 'world'
d.save!

@smtlaissezfaire
Copy link
Member

Ah! Didn't understand all the context.

Merging now + will release a new point version.

@smtlaissezfaire smtlaissezfaire merged commit 85cc344 into mongomapper:master Jan 19, 2021
@ujihisa
Copy link
Contributor Author

ujihisa commented Jan 21, 2021

Thank you! 👍🎉👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants