JSONB + empty default bug

[nashbridges]

When a JSONB column has empty default ({}) as suggested, and it is wrapped with Mobility translates, reading from that column messes up ActiveRecord's dirty tracking. After you read from that column, changes starts reporting very confusing diff (no writes were performed):

a_record.changes # => {"a_column_i18n" => [{}, {}]}

This is not related to the Mobility's dirty tracking plugin as removing/disabling it does not fix the behavior.

Context

https://gist.github.com/nashbridges/764f5da35d1a4f13eb98fccf647ef8b2

# 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", "1.2.2"
end

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

Mobility.configure do
  plugins do
    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 :name_i18n, default: {}, null: false
    t.jsonb :long_name_i18n
  end
end

class PropertyWithoutMobility < ActiveRecord::Base
  self.table_name = "properties"
end

class PropertyWithMobility < ActiveRecord::Base
  self.table_name = "properties"
  extend Mobility
  translates :name, backend: :jsonb, column_suffix: "_i18n"
  translates :long_name, backend: :jsonb, column_suffix: "_i18n"
end

class BugTest < Minitest::Test
  def test_without_mobility_reading_from_not_null_column
    property = PropertyWithoutMobility.create!
    property.reload
    assert_equal({}, property.name_i18n)
    assert_equal({}, property.changes)
  end

  def test_without_mobility_reading_from_null_column
    property = PropertyWithoutMobility.create!
    property.reload
    assert_nil property.long_name_i18n
    assert_equal({}, property.changes)
  end

  def test_with_mobility_reading_from_not_null_column
    property = PropertyWithMobility.create!
    property.reload
    assert_equal({}, property.name_i18n)
    assert_equal({}, property.changes)
  end

  def test_with_mobility_reading_from_null_column
    property = PropertyWithMobility.create!
    property.reload
    assert_nil property.long_name_i18n
    assert_equal({}, property.changes)
  end
end

We use Postgres 13.2.

Expected Behavior

All tests should pass.

Actual Behavior

I have two failing tests:

Failure:
BugTest#test_with_mobility_reading_from_not_null_column [jsonb_blank.rb:63]:
Expected: {}
  Actual: {"name_i18n"=>[{}, {}]}

Failure:
BugTest#test_with_mobility_reading_from_null_column [jsonb_blank.rb:69]:
Expected {} to be nil

The second failure is interesting, because it shows that by not providing the {} default for the DB column we can get the desired behavior: there's no nil related errors + changes is not mutated. For now this is what we do in our application (dropping the default) as a workaround.

Possible Fix

I was able to track down the source of the issue to different column type.

When there's no translates the JSONB column is handled by the JSONB type, which treats the default correctly.
When the column is wrapped with translates it is handled by the Serialized type, which somehow returns nil for a default value case.

It's not clear though what would be the fix.

[shioyama]

Thanks for the fantastic write up! 👏👏👏 This is the issue I will point to when people ask what they should do to document a bug. Thank you so much.

Also it seems like you found the source of the problem, which helps. I'm not sure what the fix is and don't have much (any?) context on AR internals around this stuff, but hey this is a learning opportunity and seems like a real bug so I'll see if I can figure out what's going on.

[shioyama]

So the problem is this line:

mobility/lib/mobility/backends/active_record/pg_hash.rb

Line 28 in 68ead91

attributes.each { |attribute| store (options[:column_affix] % attribute), coder: Coder }

Removing it "fixes" the bug (but of course also renders the backend unusable).

[shioyama]

I think #536 fixes the issue, and also simplifies the code quite a lot. Give it a try.

[nashbridges]

@shioyama thanks for the immediate response! The branch passed on our CI and I found no issues when playing with it in console. Trying it required to revert our workaround with dropping the default and not null, but that was expected.

[shioyama]

I need to finally release 1.3.0 which includes this, but based on what I've seen in other PRs on master it seems like this will cause some breakage. People make PRs to master which includes this change and find their tests fail on CI.

So far I've been releases patches on 1.2 but this is getting difficult so I need to finally ship 1.3...

[nashbridges]

I'd release 2.x and add a notice for devs to double check JSONB columns has a default {} value.

One should be careful though, because in between running a migration which adds the default value and reloading your application with the fix, the bug with changes will be present.

Even if deploying as one commit, you can't run the migration and reload your application at the very same moment. But I guess it's still better to maintain "migration + gem update" order (and deal with changes), rather to do it in the reverse order (and deal with the undefined method '[]' for nil:NilClass exception).

[shioyama]

Yeah, I'm thinking of releasing a 1.3.0.alpha.

[shioyama]

Ok this has finally been released as 1.3.0.rc1. Sorry about the delay! Hopefully it won't cause too many issues updating for folks using jsonb/hstore backends.

[madhums]

Any update on releasing 1.3 stable?

[shioyama]

1.3.0 has been released