generate dirty tracking method for each field

[giovannibonetti]

I was working with the gem in my project when I noticed all my specs that relied on methods "field_changed?" were failing. Then I realized that, although these methods are defined, they are completely wrong since their implementation comes from ActiveRecord instead of attr_json.

Hence I added one possible implementation to overwrite ActiveRecord's implementation with a correct one based on lib/attr_json/record/dirty.rb. There is probably an easier way to implement it, but at least it seems to be working now.

[jrochkind]

Thanks for checking out attr_json and contributing!

So I spent some time with that, and wasn't able to come up with a "built in" solution that I was happy with in thinking it was reliable and not too risky forwards compatibile. This may be in part because I was originally writing for Rails 5.0, it may have become more possible in 5.1, 5.2.

I haven't looked at your code yet, and will try to find time to. But what I originally did instead was provide an additional API for dirty tracking the attr_json stuff. Did you notice that? I'm wondering if that can work for you? Documented here

[jrochkind]

OK, looking at your code i see you did see it and are using it!

So one thing making this even more confusing is that Rails changed the way dirty tracking worked recently, because there were some confusing/ambiguous parts. See here and here and here.

I think maybe _changed? methods are soft-deprecated in favor of either saved_change_to_X? or... will_save_change_to_X?, depending on which semantics you want, maybe. I get confused, it is confusing!

But I'm not totally sure attr_json_changes.changes_to_save[name.to_s].present? in your PR is the right implementation for X_changed?. There are different ways to do this in Rails now, and it's important to get it right, and I'm confused! Actually, for an attr called foo can you actually call attr_json_changes.foo_changed? now already? I think that may work already -- along with attr_json_changes.will_save_change_to_foo? and attr_json_changes.saved_change_to_foo?. if I'm right those work already, docs may need to be improved.

I'm not sure we should add foo_changed? directly without also adding saved_changes_to_foo? and will_save_change_to_foo? directly -- I feel like Rails is encouraging developers to use those latter ones rather than foo_changed?.

Also rather than put the _changed? methods in with a check that raises if you don't have AttrJson::Dirty, it would be better to have the methods only added if you do have AttrJson::Dirty. Either added by AttrJson::Dirty (if feasible), or only added if AttrJson::Dirty is already there.

But the important thing is figuring out the right API to use for the right semantics... and I'm confused about it myself, with regard to Rails 5.1 changes to dirty tracking methods.

[giovannibonetti]

Also rather than put the _changed? methods in with a check that raises if you don't have AttrJson::Dirty, it would be better to have the methods only added if you do have AttrJson::Dirty. Either added by AttrJson::Dirty (if feasible), or only added if AttrJson::Dirty is already there.

There is just one problem with that approach: currently the gem already implements these methods "foo_changed?" through ActiveRecord. However, the implementation is totally wrong. It took me a while to figure this out, because my specs were failing, but not because of a clear message "undefined method 'foo_changed?'". I personally think that a wrong implementation is worse than no implementation.

That's why I chose the approach to overwrite the method. At least there is an exception now that prevents anyone from using the wrong implementation.

[giovannibonetti]

Here is the output of the current implementation. It seems to work, but it doesn't.

class Klass < ActiveRecord::Base do
  include AttrJson::Record

  self.table_name = "products"
  attr_json :value, :date
end

instance = Klass.new
instance.value_changed? # false
instance.value = Date.current
instance.value_changed? # false
instance.value = nil
instance.value_changed? # false

[jrochkind]

Hi, I found some more time to look into this. Thanks for the contribution and discussion! And thanks for bearing with me, I want to make sure we have the big picture, and understand the best possibility to address this -- cause that approach is what got attr_json to where it is!

*_changed? methods aren't there by default...

Your code example had a typo do at the end of the class decleration line (but thanks for the sample code!) I try a variation:

class MyModel < ActiveRecord::Base
  include AttrJson::Record
  self.table_name = "products"

   attr_json :value, :date
end

For me, there in fact no value_changed? method by default.

instance = MyModel.new
instance.value_changed?
# raised: NoMethodError: undefined method `value_changed?' for #<MyModel:0x00007f9ef626f420>

So bare bones AttrJson, the broken "_changed?" methods aren't actually there. They ARE there if you tell attr_json to make a Rails attribute, like so:

attr_json :value, :date, rails_attribute: true

I am guessing you were using rails_attribute: true in your code? I am going to assume so.

I am now remembering noticing this when I considered making rails_attribute: true the default, and decided not to for now due to this, indeed.

Still, even if not default, there are reasons you might want rails_attribute: true (like to work well with simple_form), and it is annoying that you now get _changed? attributes that are broken (I believe they will always return false, indeed).

There are more broken methods that dirty provides, too...

It's not just *_changed?. There are a bunch of other "magic" methods provided, including but probably not limited to:

• instance.value_was
• instance.value_change
• instance.value_previously_changed?
• instance.value_previous_change
• instance.will_save_change_to_value?
• instance.saved_change_to_value?
• instance.saved_change_to_value

These are all also going to be broken. Also the "base" methods like instance.saved_change_to_attribute(:value) I think all of those "dynamic" ones also have a version that takes an arg like this, also all broken.

And then there's things like instance.changes which you might expect to include your attr_json's, but won't.

I am not sure it makes sense to only fix *_changed?, without all those other ones. It's one (unfortunate) thing to document that none of em are gonna work; it's another to make some of em work but some of em not work, I think it would make things less predictable. and more confusing.

Ideally would be actually integrating properly with ActiveModel::Dirty to make it really work right, not just trying to catch all the methods to override.

But I had trouble integrating with ActiveModel/ActiveRecord::Dirty to make it really work right

It was tricky and I couldn't get it to work. It's possible it's gotten easier in Rails 5.2, since Rails 5.0 when I started, not sure if ActiveModel::Dirty has been cleaned up in there.

But one of my problems is that I really want to allow direct editing of the 'master' Hash too. Right now in AttrJson, you can actually edit json_attributes (the default container attribute) inline, and everything just works.

instance = SomeModel.new
instance.json_attributes["value"] = Date.new

# comes back fine even though we set it on the container!
instance.value  # => a date object

# AttrJson's own dirty tracking catches it even though we set it on the container!
instance.instance.attr_json_changes.will_save_change_to_value? # => true

# and the other way too
instance.value = Date.tomorrow
instance.json_attributes["value"] #=> tomorrow

When I first wrote attr_json, I decided it was important that if you write directly to the container, everything should still work seamlessly/interchangeably with writes/reads to the attribute value. I'm not sure why I decided this, it's possible it isn't really necessary -- although I have some memory that it makes the "nested/compound" model stuff more clearly reliable -- you can set a bunch of em at once from a hash from a form submit, rather than use the individual accessors, and it just works.

This is part of what made it infeasible to get built-in ActiveModel::Dirty working this way, built-in Dirty just doesn't work like that, and assumes it has control of the storage of the value, rather than in our case really just storing in the container hash.

So wait, I wonder how Rails store accessor work?

There's a similar more limited feature built into Rails, the store accessor. Which with a pg supported JSON column, actually becomes simply store_attribute.

I wonder how that works with dirty, and with direct access to the container attribute? Let's find out.

class MyModel < ActiveRecord::Base
  self.table_name = "products"

  store_accessor :json_attributes, :stored_value
end

instance = MyModel.new
instance.stored_value = "foo"
# We don't get dirty tracking at all!
instance.stored_value_changed? # NoMethodError: undefined method `stored_value_changed?'
instance.stored_value_previously_changed? # NoMethodError

# Cause it's not actually in the Rails attribute API
instance.attributes.keys.include?(:stored_value?) # => false

I'm guessing this means it won't work well with simple_form either. This is now pretty much similar to attr_json without rails_attribute: true.

And incidentally, store_accessor works similarly with regard to the "real" value being on the container, like attr_json -- I may have used it as a model in implementation, since it is a similar use case.

instance.stored_value = "foo"
instance.json_attributes # => {"stored_value"=>"foo"}
instance.json_attributes["stored_value"] = "bar"
instance.stored_value # => "bar"

It goes both ways -- but no dirty tracking. So attr_json out of the box is just like this, but if you add rails_attribute: true, it does have a gotcha.

So...

You only get the built-in Rails dirty methods if you do rails_attribute: true, but then you do get them in a broken state. :(

• At the least we should document this better. I will try to do that sooner than later, because the other things are all kinda hard...

• our attr_json_changes should probably support *_changed? not just _will_save_change_to_*? and saved_change_to_X?? changed? is the most widely used one -- but it's also the most confusing one, with Rails changing the implementation Rails 5.1. it was confusing me, as to how to get it to work as expected in both Rails 5.0 and Rails 5.1 -- and I sort of expected that Rails was going to deprecate, or at least soft deprecate, just the _changed?, but that message didn't get out.

  • But I can spend more time investigating how to support .attr_json_changes.X_changed? in a way compatible with Rails. I would say this is next priority.

Now, it is still unfortunate that you get a broken *_changed? method (and a bunch of other broken methods) when you have rails_attribute: true, I agree. Even with better documentation.

But I don't think providing a cover method just on individual *_changed? is the way to go, when there are still so many other broken methods. (Also if we were going to do that, I wonder if the Rails attribute_method_affix macro is the way to, rather than manual method definition, to be consistent with how Rails does it's own methods). And trying to fix all the broken methods seems challenging too; there are so many, and I'm not even sure how to make sure we've caught em all.

The best thing to do would be to actually integrate better with the built in Dirty tracking to make it just work right. I vaguely remember there being something about a plan from @sgrif to rework the Rails "Dirty" stuff in a way to make it more flexible and clean, which I was hoping might make this easier -- but alas @sgrif left Rails, and I doubt anyone else will do that, I think we'll be stuck with what we've got for a while.

Still, it might be possible... although it's not encouraging that current Rails store_accessor doesn't pull it off either. (I think maybe that was part of the @sgrif plan, to fix that). Maybe we could store an extra reference to the value inside the "rails dirty" place... I dunno, it gets tricky, and I probably won't get to it soon, honestly. But better docs, and maybe implementing _changed? on attr_json_changes seem more realistic on a quicker timeframe.

(Alternately, we could try to just make all those broken methods raise instead of just returning the wrong thing... not sure if that's really any easier although it might be. Still not thinking this is a short-term thing).

Sorry for so many words, but this stuff gets complicated. Thank you for engaging! I'm sorry there doesn't at the moment seem to be an easy fix I'm happy with.

[sgrif]

FYI when you at-mention someone you email them and subscribe them to the issue. May want to keep that in mind. :)

[jrochkind]

OK sorry... scared to type your name again! We miss you!

[giovannibonetti]

Thanks for the explanation, @jrochkind! It makes a lot of sense.

At the least we should document this better. I will try to do that sooner than later, because the other things are all kinda hard...

That seems like a fair compromise! 👍

[sgrif]

No worries 😄 ❤️