From 17c02c92105b8c1c34df507c1ba84f24069da5c2 Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
Date: Thu, 5 Dec 2024 09:34:14 -0500
Subject: [PATCH] Remove `thing_type` and `thing_id` columns from settings
 table (#31971)

---
 app/models/setting.rb                         | 10 +++----
 ...4195405_migrate_hide_network_preference.rb |  7 +++++
 ...135901_remove_legacy_user_settings_data.rb | 16 ++++++++++
 ...925_remove_legacy_user_settings_columns.rb | 30 +++++++++++++++++++
 db/schema.rb                                  |  6 ++--
 spec/fabricators/setting_fabricator.rb        |  2 +-
 6 files changed, 60 insertions(+), 11 deletions(-)
 create mode 100644 db/migrate/20241205135901_remove_legacy_user_settings_data.rb
 create mode 100644 db/post_migrate/20241205135925_remove_legacy_user_settings_columns.rb

diff --git a/app/models/setting.rb b/app/models/setting.rb
index 6af7a98c6d..12ff32f00a 100644
--- a/app/models/setting.rb
+++ b/app/models/setting.rb
@@ -7,10 +7,8 @@
 #  id         :bigint(8)        not null, primary key
 #  var        :string           not null
 #  value      :text
-#  thing_type :string
 #  created_at :datetime
 #  updated_at :datetime
-#  thing_id   :bigint(8)
 #
 
 # This file is derived from a fork of the `rails-settings-cached` gem available at
@@ -46,10 +44,10 @@ class Setting < ApplicationRecord
   after_commit :rewrite_cache, on: %i(create update)
   after_commit :expire_cache, on: %i(destroy)
 
-  # Settings are server-wide settings only, but they were previously
-  # used for users too. This can be dropped later with a database
-  # migration dropping any scoped setting.
-  default_scope { where(thing_type: nil, thing_id: nil) }
+  self.ignored_columns += %w(
+    thing_id
+    thing_type
+  )
 
   class << self
     # get or set a variable with the variable as the called method
diff --git a/db/migrate/20220304195405_migrate_hide_network_preference.rb b/db/migrate/20220304195405_migrate_hide_network_preference.rb
index 0083e0422a..996b7d78ab 100644
--- a/db/migrate/20220304195405_migrate_hide_network_preference.rb
+++ b/db/migrate/20220304195405_migrate_hide_network_preference.rb
@@ -13,6 +13,13 @@ class MigrateHideNetworkPreference < ActiveRecord::Migration[6.1]
     belongs_to :account
   end
 
+  class Setting < ApplicationRecord
+    # Mirror the behavior of the `Setting` model at this point in db history
+    def value
+      YAML.safe_load(self[:value], permitted_classes: [ActiveSupport::HashWithIndifferentAccess, Symbol]) if self[:value].present?
+    end
+  end
+
   def up
     Account.reset_column_information
 
diff --git a/db/migrate/20241205135901_remove_legacy_user_settings_data.rb b/db/migrate/20241205135901_remove_legacy_user_settings_data.rb
new file mode 100644
index 0000000000..09cc80842c
--- /dev/null
+++ b/db/migrate/20241205135901_remove_legacy_user_settings_data.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class RemoveLegacyUserSettingsData < ActiveRecord::Migration[7.2]
+  def up
+    connection.execute(<<~SQL.squish)
+      DELETE FROM settings
+      WHERE
+        thing_type IS NOT NULL
+        AND thing_id IS NOT NULL
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/db/post_migrate/20241205135925_remove_legacy_user_settings_columns.rb b/db/post_migrate/20241205135925_remove_legacy_user_settings_columns.rb
new file mode 100644
index 0000000000..f713f18f25
--- /dev/null
+++ b/db/post_migrate/20241205135925_remove_legacy_user_settings_columns.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class RemoveLegacyUserSettingsColumns < ActiveRecord::Migration[7.2]
+  disable_ddl_transaction!
+
+  def up
+    # In normal usage this should not find anything to delete
+    # Deletion here is already done in RemoveLegacyUserSettingsData migration
+    # and no data like this should be created from app at this point
+    # Deleting again out of caution
+    connection.execute(<<~SQL.squish)
+      DELETE FROM settings
+      WHERE
+        thing_type IS NOT NULL
+        AND thing_id IS NOT NULL
+    SQL
+
+    add_index :settings, :var, unique: true, algorithm: :concurrently
+    remove_index :settings, [:thing_type, :thing_id, :var], name: :index_settings_on_thing_type_and_thing_id_and_var, unique: true
+
+    safety_assured do
+      remove_column :settings, :thing_type, :string
+      remove_column :settings, :thing_id, :bigint
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index e6ee0f55d9..0cbf40b0da 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.2].define(version: 2024_11_23_160722) do
+ActiveRecord::Schema[7.2].define(version: 2024_12_05_135925) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -939,11 +939,9 @@ ActiveRecord::Schema[7.2].define(version: 2024_11_23_160722) do
   create_table "settings", force: :cascade do |t|
     t.string "var", null: false
     t.text "value"
-    t.string "thing_type"
     t.datetime "created_at", precision: nil
     t.datetime "updated_at", precision: nil
-    t.bigint "thing_id"
-    t.index ["thing_type", "thing_id", "var"], name: "index_settings_on_thing_type_and_thing_id_and_var", unique: true
+    t.index ["var"], name: "index_settings_on_var", unique: true
   end
 
   create_table "severed_relationships", force: :cascade do |t|
diff --git a/spec/fabricators/setting_fabricator.rb b/spec/fabricators/setting_fabricator.rb
index ce9a48e901..689a0de002 100644
--- a/spec/fabricators/setting_fabricator.rb
+++ b/spec/fabricators/setting_fabricator.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 Fabricator(:setting) do
-  var 'var'
+  var { sequence(:var) { |n| "var_#{n}" } }
 end