byroot / activerecord-typedstore

ActiveRecord::Store but with type definition
MIT License
439 stars 57 forks source link

previous_changes don't work with 1.1.1 and Rails 5.1 #55

Open rivsc opened 7 years ago

rivsc commented 7 years ago

ActiveModel::Dirty#previous_changes (http://api.rubyonrails.org/classes/ActiveModel/Dirty.html#method-i-previous_changes) don't return attributes out of ar_typed_store field.

Model :

class Product < ActiveRecord::Base
  typed_store :ar_typed_store do |s|
    s.string :attr_in_ar_typed_store
  end
end

To reproduce that issue :

a = Product.find(x)
a.attr_in_ar_typed_store = 'test'
a.save #=> true
puts a.previous_changes

Work on 1.1.1 with Rails 4.2 (ar_typed_store appears and attr_in_ar_typed_store too) :

{"ar_typed_store"=>[{"attr_in_ar_typed_store"...},{"attr_in_ar_typed_store"...}], "attr_in_ar_typed_store"=>[nil, 'test'], updated_at"=>[Tue, 27 Jun 2017 09:53:32 UTC +00:00, Tue, 27 Jun 2017 09:56:28 UTC +00:00]}

Don't work on 1.1.1 with Rails 5.1.1 (ar_typed_store appears but not attr_in_ar_typed_store)

# {"ar_typed_store"=>[{"attr_in_ar_typed_store"...},{"attr_in_ar_typed_store"...}], updated_at"=>[Tue, 27 Jun 2017 09:53:32 UTC +00:00, Tue, 27 Jun 2017 09:56:28 UTC +00:00]}

EDIT : works in rails 4.2 with Activerecord-Typedstore 1.1.1

Edouard-chin commented 7 years ago

Thanks for opening the issue. I dug on this, and it's not going to be simple to put back the original behaviour.

The behaviour changed because of https://github.com/rails/rails/commit/136fc65c9b8b66e1fb56f3a17f0d1fddff9b4bd0 which was introduce in rails 5.0 Model#previous_changes was previously defined in ActiveModel and was just returning the @previously_changed hash. This gem injects store attributes inside that hash.

On rails >= 5.0, previous_changes is delegated to AttributeMutationTracker#changes, the implementation returns only attributes that AR is aware of.

Vanilla rails store method never behave that way, and I was asking myself if this could be something we could patch upstream, as it's pretty useful.

Otherwise the only way I could think of would be to monkey patch AttributesMutationTracker#changes

FYI @rafaelfranca

rafaelfranca commented 7 years ago

Yes, we can try to implement this upstream but consider it a new feature not a bug fix. Not sure if it is possible

rivsc commented 7 years ago

Thanks for replies.

I had looked at the typed_store and rails code for a few days. But I don't think it's possible without monkey patch rails. And I'm not sure it's a good idea... :(