Migrate versions.object and versions.object_changes to JSON

Why these changes are being introduced:

The versions table, used by paper_trail, has two columns that use
YAML. PaperTrail prefers that these tables use JSON or JSONB.
In order to update paper_trail to the latest version, we need
either to update our config to use YAML.safe_load or migrate
the data to JSON.

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/ETD-626
* https://mitlibraries.atlassian.net/browse/ETD-615

How this addresses that need:

This converts the columns to JSON as described here:
https://github.com/paper-trail-gem/paper_trail/blob/v14.0.0/README.md#6b-custom-serializer

Our migration uses `PaperTrail::Serializers::YAML.load`. This is
effectively the same as `YAML.safe_load`. `YAML.load`,
awhich is uggested in the readme, will no longer work due to a
long-running bug in Psych:
collectiveidea/audited#631

We are using JSON instead of JSONB for two reasons:

1. SQLite does not support JSONB, so our local environments will
differ from Heroku; and
2. We are not actively leveraging JSONB's performance advantages in
the codebase, so there is likely no functional difference.

Long-term, it's worth considering whether we should use postgres
locally, as this would support better dev/prod parity and allow us
to use psql features like JSONB with more confidence. We discussed
this briefly in a team meeting on 3/7/23 and decided that such a
change could bring value, but is not necessary at this time.

Side effects of this change:

* One test has been updated to accommodate a value that change from
a date to a string.
* We are adding some `yaml_column_permitted_classes` to our ActiveRecord
config. This is required to run the db migration because of the
aforementioned bug in Psych. I've added a comment to remove it once
this commit is deployed.
* The migration is relatively inefficient. However, we tested it
against a clone of our prod db and found that it completed in 83.12s,
which we find acceptable.
* Running db migrations sometimes incorrectly parses the
`null: false` param into its own column. This bug occurs despite
PaperTrail's claiming to have fixed it, and it's inconsistent
(it occurs when Adam runs the migrations, but not when Jeremy does).
There's not much we can do about this except to keep an eye on it
in the future.

co-authored-by: Jeremy Prevost <jprevost@mit.edu>

Author: 2 people

Gemfile

@@ -24,7 +24,7 @@ gem 'net-pop', require: false
 gem 'net-smtp', require: false
 gem 'omniauth-rails_csrf_protection'
 gem 'omniauth-saml'
-gem 'paper_trail', '~> 12.3'
+gem 'paper_trail'
 gem 'puma'
 gem 'rails', '~> 7.0'
 gem 'rubyzip'

Gemfile.lock

@@ -257,9 +257,9 @@ GEM
       omniauth (~> 2.0)
       ruby-saml (~> 1.12)
     orm_adapter (0.5.0)
-    paper_trail (12.3.0)
-      activerecord (>= 5.2)
-      request_store (~> 1.1)
+    paper_trail (14.0.0)
+      activerecord (>= 6.0)
+      request_store (~> 1.4)
     parallel (1.22.1)
     parser (3.2.1.0)
       ast (~> 2.4.1)
@@ -436,7 +436,7 @@ DEPENDENCIES
   net-smtp
   omniauth-rails_csrf_protection
   omniauth-saml
-  paper_trail (~> 12.3)
+  paper_trail
   pg
   puma
   rails (~> 7.0)

config/application.rb

@@ -19,5 +19,15 @@ class Application < Rails::Application
     # config.time_zone = "Central Time (US & Canada)"
     # config.eager_load_paths << Rails.root.join("extras")
     config.active_job.queue_adapter = :delayed_job
+
+    # This is required only for the db migration to convert YAML to JSON. We should remove it once we've run that
+    # migration in all environments.
+    config.active_record.yaml_column_permitted_classes = [
+      ActiveSupport::HashWithIndifferentAccess,
+      ActiveSupport::TimeWithZone,
+      ActiveSupport::TimeZone,
+      Date,
+      Time
+    ]
   end
 end

config/initializers/paper_trail.rb

@@ -0,0 +1,2 @@
+PaperTrail.config.enabled = true
+PaperTrail.serializer = PaperTrail::Serializers::JSON

db/migrate/20230307141544_convert_version_columns_to_json.rb

@@ -0,0 +1,22 @@
+class ConvertVersionColumnsToJson < ActiveRecord::Migration[7.0]
+  def change
+    add_column :versions, :new_object, :json
+    add_column :versions, :new_object_changes, :json
+
+    PaperTrail::Version.find_each do |version|
+      version.update_column(:new_object, PaperTrail::Serializers::YAML.load(version.object)) if version.object
+
+      if version.object_changes
+        version.update_column(
+          :new_object_changes,
+          PaperTrail::Serializers::YAML.load(version.object_changes)
+        )
+      end
+    end
+
+    remove_column :versions, :object
+    remove_column :versions, :object_changes
+    rename_column :versions, :new_object, :object
+    rename_column :versions, :new_object_changes, :object_changes
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2022_12_09_161518) do
+ActiveRecord::Schema[7.0].define(version: 2023_03_07_141544) do
   create_table "active_storage_attachments", force: :cascade do |t|
     t.string "name", null: false
     t.string "record_type", null: false
@@ -260,12 +260,12 @@
 
   create_table "versions", force: :cascade do |t|
     t.string "item_type", null: false
-    t.bigint "item_id", null: false
+    t.integer "item_id", null: false
     t.string "event", null: false
     t.string "whodunnit"
-    t.text "object", limit: 1073741823
-    t.text "object_changes", limit: 1073741823
     t.datetime "created_at", precision: nil
+    t.json "object"
+    t.json "object_changes"
     t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id"
   end
 

test/integration/admin/admin_hold_test.rb

@@ -137,7 +137,7 @@ def teardown
     patch admin_hold_path(hold), params: { hold: { status: 'released' } }
     hold.reload
     assert_equal 'released', hold.status
-    assert_equal Date.today.strftime('%Y-%m-%d'), hold.date_released.strftime('%Y-%m-%d')
+    assert_equal Date.today.strftime('%Y-%m-%d'), Date.parse(hold.date_released).strftime('%Y-%m-%d')
   end
 
   test 'hold release date is the most recent released status change' do