Made #changed_in_place? to compare deserialized values

[lenkton]

The Aim

This PR addresses the issue mentioned in https://github.com/byroot/activerecord-typedstore/issues/58.

In short: currentrly in some scenarios a freshly loaded record could be considered as changed by rails. So it could result in the following:

user = User.first
user.changed? # => true

The Idea

Now #changed_in_place? compares not the serialized versions of fields but deserialized versions instead.

Drawbacks

The new algorythm can cause decrease in performance, because now it has to decerialize the original value every time the changes are checked. However, prior to this change the new value had to be serialized, so overall overhead should not increase too much.

[byroot]

Thank you but this change isn't desired for several reasons:

• Nothing tell us the deserialized object is comparable
• If the serialization method changed it might be best to update the record.

Overall this is consistent with how Active Record serialized fields work. It isn't perfect, but I don't think there is a perfect solution so I'd rather stick to a behavior consistent with vanilla Active Record.

[dwillett]

Hey @byroot - I wanted to resurface this issue after running into the original issue in #58 and comparing it to the behavior of our other serialize attributes. Looking at ActiveRecord::Type::Json it looks like it does deserialize before comparing the subtype:

def changed_in_place?(raw_old_value, new_value)
  deserialize(raw_old_value) != new_value
end

So with our other models that serialize JSON it does not show the model as changed from simple accesses. Wouldn't it be more consistent with Vanilla AR to do this?

[byroot]

ActiveRecord::Type::Json can know only the JSON primitive types are used, so it's safe to compare with ==.

But as soon as the deserialized payload could contain a more complex Ruby object, this assumption no longer holds.

[dwillett]

Got it. Is there any current means or avenue to allow configuration when the type is known to be JSON?

[byroot]

It could be made a configuration yes, but that would be a sharp one.

[dwillett]

Ah I was misunderstanding the root of the problem a little, but think I've got it now.

For all our usages we are strictly using JSON primitive types, so we might need to patch this for now to be able to use AR locking.