feat: ensure saved configuration is loaded appropriately into the Run…

…timeConfiguration

config.ru

@@ -6,7 +6,6 @@ app = PactBroker::App.new do | config |
   config.log_stream = :stdout
   config.base_urls = ["http://localhost:9292"]
   config.database_url = "sqlite:////tmp/pact_broker_database.sqlite3"
-  config.log_configuration # do this last so the logger is configured correctly
 end
 
 run app

lib/pact_broker/app.rb

@@ -125,8 +125,7 @@ def prepare_database
     end
 
     def load_configuration_from_database
-      require "pact_broker/config/load"
-      PactBroker::Config::Load.call(configuration)
+      configuration.load_from_database!
     end
 
     def configure_database_connection
@@ -156,9 +155,8 @@ def seed_example_data
         logger.info "Seeding example data"
         configuration.example_data_seeder.call
         logger.info "Marking seed as done"
-        configuration.seed_example_data = false
-        require "pact_broker/config/save"
-        PactBroker::Config::Save.call(configuration, [:seed_example_data])
+        require "pact_broker/config/repository"
+        PactBroker::Config::Repository.new.create_or_update_setting(:seed_example_data, false)
       else
         logger.info "Not seeding example data"
       end
@@ -285,6 +283,7 @@ def running_app
     end
 
     def print_startup_message
+      configuration.log_configuration if configuration.log_configuration_on_startup
       unless configuration.hide_pactflow_messages
         logger.info "\n\n#{'*' * 80}\n\nWant someone to manage your Pact Broker for you? Check out https://pactflow.io/oss for a hardened, fully supported SaaS version of the Pact Broker with an improved UI + more.\n\n#{'*' * 80}\n"
       end

lib/pact_broker/config/load.rb

@@ -9,12 +9,12 @@ class Load
 
       include PactBroker::Logging
 
-      def self.call configuration
-        new(configuration).call
+      def self.call runtime_configuration
+        new(runtime_configuration).call
       end
 
-      def initialize configuration
-        @configuration = configuration
+      def initialize runtime_configuration
+        @runtime_configuration = runtime_configuration
       end
 
       def call
@@ -25,18 +25,29 @@ def call
 
       private
 
-      attr_reader :configuration
+      attr_reader :runtime_configuration
 
       def configuration_attribute_exists? setting
-        configuration.respond_to?("#{setting.name}=")
+        runtime_configuration.respond_to?("#{setting.name}=")
+      end
+
+      def unset_or_value_from_default? setting
+        setting_source(setting).nil? || setting_source(setting)[:type] == :defaults
+      end
+
+      def setting_source(setting)
+        runtime_configuration.to_source_trace.dig(setting.name, :source)
       end
 
       def set_value_on_configuration setting
-        if configuration_attribute_exists? setting
-          logger.debug("Loading #{setting.name} configuration from database.")
-          configuration.send("#{setting.name}=", setting.value_object)
+        if configuration_attribute_exists?(setting)
+          if unset_or_value_from_default?(setting)
+            runtime_configuration.send("#{setting.name}=", setting.value_object)
+          else
+            logger.debug("Ignoring #{setting.name} configuration from database, as it has been set by another source #{setting_source(setting)}")
+          end
         else
-          logger.warn("Could not load configuration setting \"#{setting.name}\" as there is no matching attribute on the Configuration class")
+          logger.warn("Could not load configuration setting \"#{setting.name}\" as there is no matching attribute on the #{runtime_configuration.class} class")
         end
       end
     end

lib/pact_broker/config/runtime_configuration.rb

@@ -24,7 +24,8 @@ class RuntimeConfiguration < Anyway::Config
         log_level: :info,
         log_format: nil,
         warning_error_class_names: ["Sequel::ForeignKeyConstraintViolation"],
-        hide_pactflow_messages: false
+        hide_pactflow_messages: false,
+        log_configuration_on_startup: true
       )
 
       on_load :validate_logging_attributes!
@@ -65,6 +66,7 @@ class RuntimeConfiguration < Anyway::Config
         auto_detect_main_branch: true,
         main_branch_candidates: ["develop", "main", "master"],
         semver_formats: ["%M.%m.%p%s%d", "%M.%m", "%M"],
+        seed_example_data: true,
         features: []
       )
 

lib/pact_broker/config/runtime_configuration_logging_methods.rb

@@ -20,19 +20,16 @@ def sensitive_value?(value)
 
       module InstanceMethods
         def log_configuration(logger)
-          logger.info "------------------------------------------------------------------------"
-          logger.info "PACT BROKER CONFIGURATION:"
           to_source_trace.sort_by { |key, _| key }.each { |key, value| log_config_inner(key, value, logger) }
-          logger.info "------------------------------------------------------------------------"
         end
 
         def log_config_inner(key, value, logger)
           if !value.has_key? :value
             value.sort_by { |inner_key, _| inner_key }.each { |inner_key, inner_value| log_config_inner("#{key}:#{inner_key}", inner_value) }
           elsif self.class.sensitive_value?(key)
-            logger.info "#{key}=#{redact(key, value[:value])} source=[#{value[:source]}]"
+            logger.info "#{key}=#{redact(key, value[:value])} source=#{value[:source]}"
           else
-            logger.info "#{key}=#{value[:value]} source=[#{value[:source]}]"
+            logger.info "#{key}=#{value[:value]} source=#{value[:source]}"
           end
         end
         private :log_config_inner

lib/pact_broker/configuration.rb

@@ -19,34 +19,8 @@ class Configuration
 
     delegate PactBroker::Config::RuntimeConfiguration.getter_and_setter_method_names => :runtime_configuration
 
-    SAVABLE_SETTING_NAMES = [
-      :order_versions_by_date,
-      :use_case_sensitive_resource_names,
-      :enable_public_badge_access,
-      :shields_io_base_url,
-      :check_for_potential_duplicate_pacticipant_names,
-      :webhook_retry_schedule,
-      :semver_formats,
-      :disable_ssl_verification,
-      :webhook_http_method_whitelist,
-      :webhook_scheme_whitelist,
-      :webhook_host_whitelist,
-      :webhook_http_code_success,
-      :base_equality_only_on_content_that_affects_verification_results,
-      :seed_example_data,
-      :badge_provider_mode,
-      :warning_error_class_names,
-      :base_urls,
-      :log_dir,
-      :allow_missing_migration_files,
-      :auto_migrate_db_data,
-      :use_rack_protection,
-      :create_deployed_versions_for_tags,
-      :metrics_sql_statement_timeout
-    ]
-
     attr_accessor :database_connection
-    attr_accessor :example_data_seeder, :seed_example_data
+    attr_accessor :example_data_seeder
     attr_accessor :html_pact_renderer, :version_parser, :sha_generator
     attr_accessor :content_security_policy, :hal_browser_content_security_policy_overrides
     attr_accessor :api_error_reporters
@@ -75,7 +49,6 @@ def self.default_configuration
       config.html_pact_renderer = default_html_pact_render
       config.version_parser = PactBroker::Versions::ParseSemanticVersion
       config.sha_generator = PactBroker::Pacts::GenerateSha
-      config.seed_example_data = true
       config.example_data_seeder = lambda do
         require "pact_broker/db/seed_example_data"
         PactBroker::DB::SeedExampleData.call
@@ -133,7 +106,10 @@ def logger= logger
     end
 
     def log_configuration
+      logger.info "------------------------------------------------------------------------"
+      logger.info "PACT BROKER CONFIGURATION:"
       runtime_configuration.log_configuration(logger)
+      logger.info "------------------------------------------------------------------------"
     end
 
     def self.default_html_pact_render
@@ -214,16 +190,10 @@ def enable_badge_resources= enable_badge_resources
       self.enable_public_badge_access = enable_badge_resources
     end
 
-    def save_to_database
-      # Can't require a Sequel::Model class before the connection has been set
-      require "pact_broker/config/save"
-      PactBroker::Config::Save.call(self, SAVABLE_SETTING_NAMES)
-    end
-
     def load_from_database!
       # Can't require a Sequel::Model class before the connection has been set
       require "pact_broker/config/load"
-      PactBroker::Config::Load.call(self)
+      PactBroker::Config::Load.call(runtime_configuration)
     end
   end
 end

spec/lib/pact_broker/config/load_spec.rb

@@ -3,11 +3,23 @@
 module PactBroker
   module Config
     describe Load do
-
       describe ".call" do
 
-        class MockConfig
-          attr_accessor :foo, :bar, :nana, :meep, :lalala, :meow, :peebo, :whitelist, :blah
+        class MockConfig < Anyway::Config
+          attr_config(
+            foo: "default",
+            bar: "default",
+            nana: "default",
+            meep: "default",
+            lalala: "default",
+            meow: "default",
+            peebo: "default",
+            whitelist: "default",
+            blah: "default",
+            setting_with_override: "default"
+          )
+          attr_config(:setting_with_no_default)
+          config_name :foo
         end
 
         before do
@@ -21,9 +33,14 @@ class MockConfig
           Setting.create(name: "unknown", type: "string", value: nil)
           Setting.create(name: "whitelist", type: "space_delimited_string_list", value: "foo bar")
           Setting.create(name: "blah", type: "symbol", value: "boop")
+          Setting.create(name: "setting_with_no_default", type: "string", value: "boop")
+          Setting.create(name: "setting_with_override", type: "string", value: "meep")
+
+          allow(Load.logger).to receive(:warn)
+          allow(Load.logger).to receive(:debug)
         end
 
-        let(:configuration) { MockConfig.new }
+        let(:configuration) { MockConfig.new(setting_with_override: "overridden") }
 
         subject { Load.call(configuration) }
 
@@ -72,9 +89,19 @@ class MockConfig
           expect(configuration.whitelist).to eq ["foo", "bar"]
         end
 
+        it "loads settings where there is no default" do
+          subject
+          expect(configuration.setting_with_no_default).to eq "boop"
+        end
+
+        it "does not overwrite settings that did not come from the default class" do
+          expect(Load.logger).to receive(:debug).with(/Ignoring.*setting_with_override/)
+          subject
+          expect(configuration.setting_with_override).to eq "overridden"
+        end
+
         it "does not load a setting where the Configuration object does not have a matching property" do
-          allow(Load.logger).to receive(:warn)
-          expect(Load.logger).to receive(:warn).with("Could not load configuration setting \"unknown\" as there is no matching attribute on the Configuration class")
+          expect(Load.logger).to receive(:warn).with("Could not load configuration setting \"unknown\" as there is no matching attribute on the PactBroker::Config::MockConfig class")
           subject
         end
       end

spec/lib/pact_broker/configuration_spec.rb

@@ -86,26 +86,6 @@ module PactBroker
         end
       end
 
-      describe "SETTING_NAMES" do
-        let(:configuration) { PactBroker::Configuration.new}
-
-        Configuration::SAVABLE_SETTING_NAMES.each do | setting_name |
-          describe setting_name do
-            it "exists as a method on a PactBroker::Configuration instance" do
-              expect(configuration).to respond_to(setting_name)
-            end
-          end
-        end
-      end
-
-      describe "save_to_database" do
-        let(:configuration) { PactBroker::Configuration.default_configuration }
-
-        it "saves the configuration to the database" do
-          expect { configuration.save_to_database }.to change { PactBroker::Config::Setting.count }.by(Configuration::SAVABLE_SETTING_NAMES.size)
-        end
-      end
-
       describe "load_from_database!" do
         let(:configuration) { PactBroker::Configuration.new }