diff --git a/app/controllers/settings/providers_controller.rb b/app/controllers/settings/providers_controller.rb index 69a52c87c..d31f34a3e 100644 --- a/app/controllers/settings/providers_controller.rb +++ b/app/controllers/settings/providers_controller.rb @@ -28,9 +28,6 @@ class Settings::ProvidersController < ApplicationController updated_fields = [] - # This hash will store only the updates for dynamic (non-declared) fields - dynamic_updates = {} - # Perform all updates within a transaction for consistency Setting.transaction do provider_params.each do |param_key, param_value| @@ -57,32 +54,13 @@ class Settings::ProvidersController < ApplicationController # This is safe and uses the proper setter. Setting.public_send("#{key_str}=", value) else - # If it's a dynamic field, add it to our batch hash - # to avoid the Read-Modify-Write conflict. - dynamic_updates[key_str] = value + # If it's a dynamic field, set it as an individual entry + # Each field is stored independently, preventing race conditions + Setting[key_str] = value end updated_fields << param_key end - - # Now, if we have any dynamic updates, apply them all at once - if dynamic_updates.any? - # 1. READ the current hash once - current_dynamic = Setting.dynamic_fields.dup - - # 2. MODIFY by merging changes - # Treat nil values as deletions to keep the hash clean - dynamic_updates.each do |key, value| - if value.nil? - current_dynamic.delete(key) - else - current_dynamic[key] = value - end - end - - # 3. WRITE the complete, merged hash back once - Setting.dynamic_fields = current_dynamic - end end if updated_fields.any? diff --git a/app/models/provider/configurable.rb b/app/models/provider/configurable.rb index 680d2cc2e..104ed107a 100644 --- a/app/models/provider/configurable.rb +++ b/app/models/provider/configurable.rb @@ -1,8 +1,8 @@ # Module for providers to declare their configuration requirements # # Providers can declare their own configuration fields without needing to modify -# the Setting model. Settings are stored dynamically using RailsSettings::Base's -# hash-style access (Setting[:key] = value). +# the Setting model. Settings are stored dynamically as individual entries using +# RailsSettings::Base's bracket-style access (Setting[:key] = value). # # Configuration fields are automatically registered and displayed in the UI at # /settings/providers. The system checks Setting storage first, then ENV variables, @@ -186,8 +186,8 @@ module Provider::Configurable # Get the value for this field (Setting -> ENV -> default) def value - # First try Setting using dynamic hash-style access - # This works even without explicit field declarations in Setting model + # First try Setting using dynamic bracket-style access + # Each field is stored as an individual entry without explicit field declarations setting_value = Setting[setting_key] return normalize_value(setting_value) if setting_value.present? diff --git a/app/models/setting.rb b/app/models/setting.rb index dc1b025db..67100b59c 100644 --- a/app/models/setting.rb +++ b/app/models/setting.rb @@ -11,9 +11,8 @@ class Setting < RailsSettings::Base field :openai_model, type: :string, default: ENV["OPENAI_MODEL"] field :brand_fetch_client_id, type: :string, default: ENV["BRAND_FETCH_CLIENT_ID"] - # Single hash field for all dynamic provider credentials and other dynamic settings - # This allows unlimited dynamic fields without declaring them upfront - field :dynamic_fields, type: :hash, default: {} + # Dynamic fields are now stored as individual entries with "dynamic:" prefix + # This prevents race conditions and ensures each field is independently managed # Onboarding and app settings ONBOARDING_STATES = %w[open closed invite_only].freeze @@ -50,7 +49,7 @@ class Setting < RailsSettings::Base end # Support dynamic field access via bracket notation - # First checks if it's a declared field, then falls back to dynamic_fields hash + # First checks if it's a declared field, then falls back to individual dynamic entries def [](key) key_str = key.to_s @@ -58,8 +57,8 @@ class Setting < RailsSettings::Base if respond_to?(key_str) public_send(key_str) else - # Fall back to dynamic_fields hash - dynamic_fields[key_str] + # Fall back to individual dynamic entry lookup + find_by(var: dynamic_key_name(key_str))&.value end end @@ -70,21 +69,26 @@ class Setting < RailsSettings::Base if respond_to?("#{key_str}=") public_send("#{key_str}=", value) else - # Otherwise, manage in dynamic_fields hash - current_dynamic = dynamic_fields.dup + # Store as individual dynamic entry + dynamic_key = dynamic_key_name(key_str) if value.nil? - current_dynamic.delete(key_str) # treat nil as delete + where(var: dynamic_key).destroy_all else - current_dynamic[key_str] = value + # Use upsert to avoid conflicts + record = find_or_initialize_by(var: dynamic_key) + record.value = value + record.save! end - self.dynamic_fields = current_dynamic # persists & busts cache end end # Check if a dynamic field exists (useful to distinguish nil value vs missing key) def key?(key) key_str = key.to_s - respond_to?(key_str) || dynamic_fields.key?(key_str) + return true if respond_to?(key_str) + + # Check if dynamic entry exists + where(var: dynamic_key_name(key_str)).exists? end # Delete a dynamic field @@ -92,15 +96,21 @@ class Setting < RailsSettings::Base key_str = key.to_s return nil if respond_to?(key_str) # Can't delete declared fields - current_dynamic = dynamic_fields.dup - value = current_dynamic.delete(key_str) - self.dynamic_fields = current_dynamic + dynamic_key = dynamic_key_name(key_str) + value = self[key_str] + where(var: dynamic_key).destroy_all value end # List all dynamic field keys (excludes declared fields) def dynamic_keys - dynamic_fields.keys + where("var LIKE ?", "dynamic:%").pluck(:var).map { |var| var.sub(/^dynamic:/, "") } + end + + private + + def dynamic_key_name(key_str) + "dynamic:#{key_str}" end end diff --git a/db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb b/db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb new file mode 100644 index 000000000..2e4931859 --- /dev/null +++ b/db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb @@ -0,0 +1,35 @@ +class MigrateDynamicFieldsToIndividualEntries < ActiveRecord::Migration[7.2] + def up + # Find the dynamic_fields setting record + dynamic_fields_record = Setting.find_by(var: "dynamic_fields") + return unless dynamic_fields_record + + # Parse the hash and create individual entries + dynamic_fields_hash = dynamic_fields_record.value || {} + dynamic_fields_hash.each do |key, value| + Setting.create!( + var: "dynamic:#{key}", + value: value + ) + end + + # Delete the old dynamic_fields record + dynamic_fields_record.destroy + end + + def down + # Collect all dynamic: entries back into a hash + dynamic_fields_hash = {} + Setting.where("var LIKE ?", "dynamic:%").find_each do |record| + key = record.var.sub(/^dynamic:/, "") + dynamic_fields_hash[key] = record.value + record.destroy + end + + # Recreate the dynamic_fields record with the hash + Setting.create!( + var: "dynamic_fields", + value: dynamic_fields_hash + ) if dynamic_fields_hash.any? + end +end diff --git a/db/schema.rb b/db/schema.rb index 86a1ec71d..4743b4f02 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: 2025_11_10_104411) do +ActiveRecord::Schema[7.2].define(version: 2025_11_11_094448) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -687,6 +687,10 @@ ActiveRecord::Schema[7.2].define(version: 2025_11_10_104411) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "name" + t.boolean "manual", default: false, null: false + t.decimal "expected_amount_min", precision: 19, scale: 4 + t.decimal "expected_amount_max", precision: 19, scale: 4 + t.decimal "expected_amount_avg", precision: 19, scale: 4 t.index ["family_id", "merchant_id", "amount", "currency"], name: "idx_recurring_txns_merchant", unique: true, where: "(merchant_id IS NOT NULL)" t.index ["family_id", "name", "amount", "currency"], name: "idx_recurring_txns_name", unique: true, where: "((name IS NOT NULL) AND (merchant_id IS NULL))" t.index ["family_id", "status"], name: "index_recurring_transactions_on_family_id_and_status" diff --git a/test/controllers/settings/providers_controller_test.rb b/test/controllers/settings/providers_controller_test.rb index 7f97d6de0..9443a5032 100644 --- a/test/controllers/settings/providers_controller_test.rb +++ b/test/controllers/settings/providers_controller_test.rb @@ -41,10 +41,10 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest end test "updates dynamic provider fields using batch update" do - # plaid_client_id is a dynamic field, so it should go through dynamic_fields hash + # plaid_client_id is a dynamic field, stored as an individual entry with_self_hosting do # Clear any existing plaid settings - Setting.dynamic_fields = {} + Setting["plaid_client_id"] = nil patch settings_providers_url, params: { setting: { plaid_client_id: "test_client_id" } @@ -52,14 +52,16 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest assert_redirected_to settings_providers_url assert_equal "test_client_id", Setting["plaid_client_id"] - assert_equal "test_client_id", Setting.dynamic_fields["plaid_client_id"] end end test "batches multiple dynamic fields from same provider atomically" do - # Test that multiple fields from Plaid are updated together in one write operation + # Test that multiple fields from Plaid are updated as individual entries with_self_hosting do - Setting.dynamic_fields = {} + # Clear existing fields + Setting["plaid_client_id"] = nil + Setting["plaid_secret"] = nil + Setting["plaid_environment"] = nil patch settings_providers_url, params: { setting: { @@ -71,7 +73,7 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest assert_redirected_to settings_providers_url - # All three should be present in dynamic_fields + # All three should be present as individual entries assert_equal "new_client_id", Setting["plaid_client_id"] assert_equal "new_secret", Setting["plaid_secret"] assert_equal "production", Setting["plaid_environment"] @@ -79,9 +81,14 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest end test "batches dynamic fields from multiple providers atomically" do - # Test that fields from different providers are all batched together + # Test that fields from different providers are stored as individual entries with_self_hosting do - Setting.dynamic_fields = {} + # Clear existing fields + Setting["plaid_client_id"] = nil + Setting["plaid_secret"] = nil + Setting["plaid_eu_client_id"] = nil + Setting["plaid_eu_secret"] = nil + Setting["simplefin_setup_token"] = nil patch settings_providers_url, params: { setting: { @@ -108,10 +115,8 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest # Test that updating some fields doesn't overwrite other existing fields with_self_hosting do # Set initial fields - Setting.dynamic_fields = { - "existing_field_1" => "value1", - "plaid_client_id" => "old_client_id" - } + Setting["existing_field_1"] = "value1" + Setting["plaid_client_id"] = "old_client_id" # Update one field and add a new one patch settings_providers_url, params: { @@ -162,7 +167,7 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest # Set initial values Setting["plaid_client_id"] = "old_value" assert_equal "old_value", Setting["plaid_client_id"] - assert Setting.dynamic_fields.key?("plaid_client_id") + assert Setting.key?("plaid_client_id") patch settings_providers_url, params: { setting: { plaid_client_id: " " } # Blank string with spaces @@ -170,18 +175,18 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest assert_redirected_to settings_providers_url assert_nil Setting["plaid_client_id"] - # Key should be removed from hash, not just set to nil - refute Setting.dynamic_fields.key?("plaid_client_id"), - "nil values should delete the key from dynamic_fields" + # Entry should be removed, not just set to nil + refute Setting.key?("plaid_client_id"), + "nil values should delete the entry" end end test "handles sequential updates to different dynamic fields safely" do # This test simulates what would happen if two requests tried to update - # different dynamic fields sequentially. With the batch update approach, - # all changes should be preserved. + # different dynamic fields sequentially. With individual entries, + # all changes should be preserved without conflicts. with_self_hosting do - Setting.dynamic_fields = { "existing_field" => "existing_value" } + Setting["existing_field"] = "existing_value" # Simulate first request updating plaid fields patch settings_providers_url, params: { @@ -266,8 +271,8 @@ class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest test "logs errors when update fails" do with_self_hosting do # Test that errors during update are properly logged and handled gracefully - # We'll force an error by making the dynamic_fields= setter raise - Setting.expects(:dynamic_fields=).raises(StandardError.new("Database error")).once + # We'll force an error by making the []= method raise + Setting.expects(:[]=).with("plaid_client_id", "test").raises(StandardError.new("Database error")).once # Mock logger to verify error is logged Rails.logger.expects(:error).with(regexp_matches(/Failed to update provider settings.*Database error/)).once diff --git a/test/models/setting_test.rb b/test/models/setting_test.rb index bb0ffe8df..cdadb0458 100644 --- a/test/models/setting_test.rb +++ b/test/models/setting_test.rb @@ -7,6 +7,11 @@ class SettingTest < ActiveSupport::TestCase Setting.openai_model = nil end + teardown do + # Clean up dynamic fields after each test + Setting.where("var LIKE ?", "dynamic:%").destroy_all + end + test "validate_openai_config! passes when both uri base and model are set" do assert_nothing_raised do Setting.validate_openai_config!(uri_base: "https://api.example.com", model: "gpt-4") @@ -61,4 +66,69 @@ class SettingTest < ActiveSupport::TestCase Setting.validate_openai_config!(uri_base: "", model: nil) end end + + # Dynamic field tests + test "can set and get dynamic fields" do + Setting["custom_key"] = "custom_value" + assert_equal "custom_value", Setting["custom_key"] + end + + test "can set and get multiple dynamic fields independently" do + Setting["key1"] = "value1" + Setting["key2"] = "value2" + Setting["key3"] = "value3" + + assert_equal "value1", Setting["key1"] + assert_equal "value2", Setting["key2"] + assert_equal "value3", Setting["key3"] + end + + test "setting nil value deletes dynamic field" do + Setting["temp_key"] = "temp_value" + assert_equal "temp_value", Setting["temp_key"] + + Setting["temp_key"] = nil + assert_nil Setting["temp_key"] + end + + test "can delete dynamic field" do + Setting["delete_key"] = "delete_value" + assert_equal "delete_value", Setting["delete_key"] + + value = Setting.delete("delete_key") + assert_equal "delete_value", value + assert_nil Setting["delete_key"] + end + + test "key? returns true for existing dynamic field" do + Setting["exists_key"] = "exists_value" + assert Setting.key?("exists_key") + end + + test "key? returns false for non-existing dynamic field" do + assert_not Setting.key?("nonexistent_key") + end + + test "dynamic_keys returns all dynamic field keys" do + Setting["dynamic1"] = "value1" + Setting["dynamic2"] = "value2" + + keys = Setting.dynamic_keys + assert_includes keys, "dynamic1" + assert_includes keys, "dynamic2" + end + + test "declared fields take precedence over dynamic fields" do + # Try to set a declared field using bracket notation + Setting["openai_model"] = "custom-model" + assert_equal "custom-model", Setting["openai_model"] + assert_equal "custom-model", Setting.openai_model + end + + test "cannot delete declared fields" do + Setting.openai_model = "test-model" + result = Setting.delete("openai_model") + assert_nil result + assert_equal "test-model", Setting.openai_model + end end