Fixes #45868 by Using #to_hash to serialize AS::HWIA for stored attributes

[the-spectator]

Summary

Fixes #45868

Originally we were calling #to_h to serialize ActiveSupport::HashWithIndifferentAccess but it was serializing only top-level hash keys.
By Using #to_hash to serialize ActiveSupport::HashWithIndifferentAccess it will also handle/convert nested objects.

[tkalliom]

Maybe this is too contrived to consider, but technically you could have a Hash with a HashWithIndifferentAccess nested inside

[the-spectator]

Great point @tkalliom. Looks like it would have to be a recursive solution.

[tkalliom]

Functionally LGTM

[tkalliom]

This duplicated block could be extracted, but maybe this is short enough that duplication is preferrable to indirection. Matter of taste.

[byroot]

I don't think we should do this. Active Record Store is responsible for the top level hash, anything inside is the responsability of the caller.

Some people might be serializing some HWIA in stores on purpose, and casting this would break their code.

You can pass a custom coder when registering the store that allows you to do all the casting that makes sense in your app, but Rails as a framework cannot assume which casting make sense.

[byroot]

Re-opening because I oversaw that HashWithIndifferentAccess#[]= will convert hashes into HWIA, so we do indeed need to convert that back.

However I think this does too much, just calling .to_hash should be enough to cast the hash that were converted automatically.

[byroot]

We don't need that part of the change. Just the to_h -> to_hash is enough.

[the-spectator]

@byroot Thank you for reviewing. If I am understanding it correctly, we should only call HWIA#to_hash and drop nested handling for Hash & Array. i.e

def as_regular_hash(obj)
  case obj
  when ActiveSupport::HashWithIndifferentAccess
    obj.to_hash
  when Hash
     obj            
  else
    {}
   end
end

[byroot]

Yes. to_hash will take care of the recursive behavior. If the top level object is already a regular hash, there is no point recursively checking the object graph.

[byroot]

Actually, even simpler: obj ? obj.to_hash : {}, since Hash#to_hash returns self.

[the-spectator]

👍

[the-spectator]

I opted for obj.respond_to?(:to_hash) ? obj.to_hash : {} as we have a test that fails when the passed object doesn't implement #to_hash.

TEST "convert store attributes from any format other than Hash or HashWithIndifferentAccess losing the data" => activerecord/test/cases/store_test.rb:208

[byroot]

Backported to 7-0-stable. Thanks @the-spectator for the fix, and thanks @tkalliom for insisting when I misunderstood the issue.

[tkalliom]

Since the original fix #45591 was merged into 6-1-stable, should this fix of the fix be as well?

[byroot]

Done.

[dominikklein]

@byroot Any thoughts that this leads to a "changed" state when you fetch one object which has for example a store column. Afterward, the with_lock complains about an already existing lock (Locking a record with unpersisted changes is not supported. ). Do you have an idea, about a solution?

[byroot]

You mean the first time you fetch an old record that used to be serialized as HWIA?

Ideally you can go over you old records and update them. But yes, that's problematic. I don't see how we could handle that.

[dominikklein]

You mean the first time you fetch an old record that used to be serialized as HWIA?

Yes, correct.

The problem is that it could be a lot of records and will take a lot of time to migrate this.
But why is this marked as changed in this situation in the end it's the same data for the "outside".

[byroot]

But why is this marked as changed in this situation in the end it's the same data for the "outside".

Because for checking if a serialized field is changed, Active Record compare their serialized forms, e.g:

>> puts({"foo" => "bar"}.to_yaml)
---
foo: bar                                                                                  
=> nil                                                                                    
>> puts({"foo" => "bar"}.with_indifferent_access.to_yaml)
--- !ruby/hash:ActiveSupport::HashWithIndifferentAccess
foo: bar                                                                                  
=> nil   

So yeah, these columns effectively changed.

[mgruner]

In case anyone is interested, we implemented a workaround that will solve issues with failing with_lock calls because of the internally changed records:

# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/

module ActiveRecord::Locking::Pessimistic

  # https://github.com/zammad/zammad/issues/3664
  #
  # With Zammad 5.2 and Rails update from 6.1.6.2 to 6.1.7, internal database storage format
  #   of preferences columns changed from serialized `ActiveSupport::HashWithIndifferentAccess` to just serialized `Hash``.
  # The downside is that 'old' values still have the old format, and show up as changed, which prevents
  #   `with_lock` from working correctly - it would throw errors on previously modified records,
  #   making tickets/users non-updateable.
  # We work around this by suppressing the exception in just this case.
  if !method_defined?(:orig_lock!)

    alias orig_lock! lock!

    def lock!(lock = true) # rubocop:disable Style/OptionalBooleanParameter
      if persisted? && has_changes_to_save?

        # We will skip the exception in case if the changes only contain columns which are store-type and have idential value.
        skip_exception = changes.all? do |key, value|
          send(key.to_sym).instance_of?(ActiveSupport::HashWithIndifferentAccess) && Marshal.dump(value[0]) == Marshal.dump(value[1])
        end

        if skip_exception
          reload(lock: lock)
          return self
        end
      end

      orig_lock!(lock)
    end
  end
end