Backend container reader mixes up dirty tracking and creates empty locale hash

[doits]

This is in a similar vein to #535: The container backend does some weird things to the dirty tracking after a reload or after accessing a reader.

One is about .changes where it states that the column changed like this: {"translations"=>[{}, {}]} simply after reload.

property.translations
# => {}

property.changes
# => {}

property.reload

property.changes
# => {"translations"=>[{}, {}]}
# Expected: {}

Second one is that when a reader is accessed it adds the locale key to the translations, but with empty hash:

property.translations
# => {}

property.changes
# => {}

property.name
# => nil

property.translations
# => {"en"=>{}}
# Expected: {}

property.changes
# => {"translations"=>[{}, {"en"=>{}}]}
# Expected: {}

Bug template

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", "6.1.4.1"
  gem "pg", "1.2.3"
  gem "mobility", github: 'shioyama/mobility'
end

require "active_record"
require "minitest/autorun"
require "logger"
require "mobility"

Mobility.configure do
  plugins do
    backend :container

    reader

    active_record
  end
end

ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "mobility_test")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :properties, force: true do |t|
    t.jsonb :translations, default: {}, null: false
  end
end

class PropertyWithMobility < ActiveRecord::Base
  self.table_name = "properties"
  extend Mobility
  translates :name
end

class BugTest < Minitest::Test
  def test_translations_and_changes_are_blank_after_create_reload
    property = PropertyWithMobility.create!
    property.reload

    assert_equal({}, property.translations, "translations should be blank")
    assert_equal({}, property.changes, "changes should be blank")
    assert_equal(false, property.changed?, "changed should be false")
  end

  def test_translations_and_changes_are_blank_after_accessor
    property = PropertyWithMobility.create!

    property.name

    assert_equal({}, property.translations, "translations should be blank")
    assert_equal({}, property.changes, "changes should be blank")
    assert_equal(false, property.changed?, "changed should be false")
  end
end

[doits]

I think one culprit is this:

mobility/lib/mobility/backends/active_record/container.rb

Lines 109 to 111 in 14d2ab9

	def model_translations(locale)
	model[column_name][locale] ||= {}
	end

Another one might be here:

mobility/lib/mobility/backends/active_record/container.rb

Lines 113 to 118 in 14d2ab9

	def set_attribute_translation(locale, value)
	translations = model[column_name] || {}
	translations[locale.to_s] ||= {}
	translations[locale.to_s][attribute] = value
	model[column_name] = translations
	end

Maybe something like this could fix it?

--- a/lib/mobility/backends/active_record/container.rb
+++ b/lib/mobility/backends/active_record/container.rb
@@ -107,14 +107,15 @@ Implements the {Mobility::Backends::Container} backend for ActiveRecord models.
       private

       def model_translations(locale)
-        model[column_name][locale] ||= {}
+        model[column_name][locale] || {}
       end

       def set_attribute_translation(locale, value)
         translations = model[column_name] || {}
         translations[locale.to_s] ||= {}
         translations[locale.to_s][attribute] = value
-        model[column_name] = translations
+        translations[locale.to_s].compact!
+        model[column_name] = translations.compact
       end

       class Coder

[shioyama]

Thanks! You're right we should do the same thing on all AR backends that use store with a coder. I think the fix would be to entirely remove the store line here:

mobility/lib/mobility/backends/active_record/container.rb

Line 87 in 14d2ab9

store options[:column_name], coder: Coder

maybe the changes you made above would help make that possible, I haven't looked at the code for quite a while.

[doits]

I played a little bit with it and could fix the errors I reported but some existing specs were failing whenever I touched something. Looks like a deeper investigation is needed here, I'll see if I can find some time the next days.

[shioyama]

Thanks! 🙇‍♂️

[doits]

I gave it a try in #543! specs should pass and issues are fixed. Lets discuss the code there.