Refactor BaseDatabase class for simplicity

lib/foreman_maintain/concerns/base_database.rb

@@ -16,11 +16,12 @@ def deb_postgresql_data_dir
       end
 
       def deb_postgresql_versions
-        installed_pkgs = package_manager.list_installed_packages('${binary:Package}\n')
-        @deb_postgresql_versions ||= installed_pkgs.grep(/^postgresql-\d+$/).map do |name|
-          name.split('-').last
+        @deb_postgresql_versions ||= begin
+          installed_pkgs = package_manager.list_installed_packages('${binary:Package}\n')
+          installed_pkgs.grep(/^postgresql-\d+$/).map do |name|
+            name.split('-').last
+          end
         end
-        @deb_postgresql_versions
       end
 
       def postgresql_conf
@@ -41,49 +42,47 @@ def configuration
         raise NotImplementedError
       end
 
-      def local?(config = configuration)
-        ['localhost', '127.0.0.1', `hostname`.strip].include?(config['host'])
+      def local?
+        ['localhost', '127.0.0.1', `hostname`.strip].include?(configuration['host'])
       end
 
-      def query(sql, config = configuration)
-        parse_csv(query_csv(sql, config))
+      def query(sql)
+        parse_csv(query_csv(sql))
       end
 
-      def query_csv(sql, config = configuration)
-        psql(%{COPY (#{sql}) TO STDOUT WITH CSV HEADER}, config)
+      def query_csv(sql)
+        psql(%{COPY (#{sql}) TO STDOUT WITH CSV HEADER})
       end
 
-      def psql(query, config = configuration)
-        if ping(config)
-          execute(psql_command(config),
-            :stdin => query,
-            :hidden_patterns => [config['password']])
-        else
-          raise_service_error
-        end
+      def psql(query)
+        ping!
+
+        execute('psql',
+          :stdin => query,
+          :env => base_env)
       end
 
-      def ping(config = configuration)
-        execute?(psql_command(config),
+      def ping
+        execute?('psql',
           :stdin => 'SELECT 1 as ping',
-          :hidden_patterns => [config['password']])
+          :env => base_env)
       end
 
-      def dump_db(file, config = configuration)
-        execute!(dump_command(config) + " > #{file}", :hidden_patterns => [config['password']])
+      def dump_db(file)
+        dump_command = "pg_dump -Fc -f #{file}"
+        execute!(dump_command, :env => base_env)
       end
 
-      def restore_dump(file, localdb, config = configuration)
+      def restore_dump(file, localdb)
         if localdb
           dump_cmd = "runuser - postgres -c 'pg_restore -C -d postgres #{file}'"
           execute!(dump_cmd)
         else
           # TODO: figure out how to completely ignore errors. Currently this
           # sometimes exits with 1 even though errors are ignored by pg_restore
-          dump_cmd = base_command(config, 'pg_restore') +
-                     ' --no-privileges --clean --disable-triggers -n public ' \
-                     "-d #{config['database']} #{file}"
-          execute!(dump_cmd, :hidden_patterns => [config['password']],
+          dump_cmd = 'pg_restore --no-privileges --clean --disable-triggers -n public ' \
+                     "-d #{configuration['database']} #{file}"
+          execute!(dump_cmd, :env => base_env,
             :valid_exit_statuses => [0, 1])
         end
       end
@@ -109,11 +108,11 @@ def backup_dir
         @backup_dir ||= File.expand_path(ForemanMaintain.config.db_backup_dir)
       end
 
-      def dropdb(config = configuration)
+      def dropdb
         if local?
-          execute!("runuser - postgres -c 'dropdb #{config['database']}'")
+          execute!("runuser - postgres -c 'dropdb #{configuration['database']}'")
         else
-          delete_statement = psql(<<-SQL)
+          delete_statement = psql(<<~SQL)
             select string_agg('drop table if exists \"' || tablename || '\" cascade;', '')
             from pg_tables
             where schemaname = 'public';
@@ -122,15 +121,13 @@ def dropdb(config = configuration)
         end
       end
 
-      def db_version(config = configuration)
-        if ping(config)
-          # Note - t removes headers, -A removes alignment whitespace
-          server_version_cmd = psql_command(config) + ' -c "SHOW server_version" -t -A'
-          version_string = execute!(server_version_cmd, :hidden_patterns => [config['password']])
-          version(version_string)
-        else
-          raise_service_error
-        end
+      def db_version
+        ping!
+
+        query = 'SHOW server_version'
+        server_version_cmd = 'psql --tuples-only --no-align'
+        version_string = execute!(server_version_cmd, :stdin => query, :env => base_env)
+        version(version_string)
       end
 
       def psql_cmd_available?
@@ -145,22 +142,20 @@ def raise_psql_missing_error
 
       private
 
-      def base_command(config, command = 'psql')
-        "PGPASSWORD='#{config[%(password)]}' "\
-        "#{command} -h #{config['host'] || 'localhost'} "\
-        " -p #{config['port'] || '5432'} -U #{config['username']}"
+      def base_env
+        {
+          'PGHOST' => configuration.fetch('host', 'localhost'),
+          'PGPORT' => configuration['port']&.to_s,
+          'PGUSER' => configuration['username'],
+          'PGPASSWORD' => configuration['password'],
+          'PGDATABASE' => configuration['database'],
+        }
       end
 
-      def psql_command(config)
-        base_command(config, 'psql') + " -d #{config['database']}"
-      end
-
-      def dump_command(config)
-        base_command(config, 'pg_dump') + " -Fc #{config['database']}"
-      end
-
-      def raise_service_error
-        raise Error::Fail, 'Please check whether database service is up & running state.'
+      def ping!
+        unless ping
+          raise Error::Fail, 'Please check whether database service is up & running state.'
+        end
       end
     end
   end

lib/foreman_maintain/utils/command_runner.rb

@@ -8,26 +8,27 @@ class CommandRunner
       attr_reader :logger, :command
 
       def initialize(logger, command, options)
-        options.validate_options!(:stdin, :hidden_patterns, :interactive, :valid_exit_statuses)
+        options.validate_options!(:stdin, :interactive, :valid_exit_statuses, :env)
         options[:valid_exit_statuses] ||= [0]
+        options[:env] ||= {}
         @logger = logger
         @command = command
         @stdin = options[:stdin]
-        @hidden_patterns = Array(options[:hidden_patterns]).compact
         @interactive = options[:interactive]
         @options = options
         @valid_exit_statuses = options[:valid_exit_statuses]
+        @env = options[:env]
         raise ArgumentError, 'Can not pass stdin for interactive command' if @interactive && @stdin
       end
 
       def run
-        logger&.debug(hide_strings("Running command #{@command} with stdin #{@stdin.inspect}"))
+        logger&.debug("Running command #{@command} with stdin #{@stdin.inspect}")
         if @interactive
           run_interactively
         else
           run_non_interactively
         end
-        logger&.debug("output of the command:\n #{hide_strings(output)}")
+        logger&.debug("output of the command:\n #{output}")
       end
 
       def interactive?
@@ -49,10 +50,10 @@ def success?
       end
 
       def execution_error
-        raise Error::ExecutionError.new(hide_strings(@command),
+        raise Error::ExecutionError.new(@command,
           exit_status,
-          hide_strings(@stdin),
-          @interactive ? nil : hide_strings(@output))
+          @stdin,
+          @interactive ? nil : @output)
       end
 
       private
@@ -81,7 +82,7 @@ def run_interactively
       end
 
       def run_non_interactively
-        IO.popen(full_command, 'r+') do |f|
+        IO.popen(@env, full_command, 'r+') do |f|
           if @stdin
             f.puts(@stdin)
             f.close_write
@@ -94,13 +95,6 @@ def run_non_interactively
       def full_command
         "#{@command} 2>&1"
       end
-
-      def hide_strings(string)
-        return unless string
-        @hidden_patterns.reduce(string) do |result, hidden_pattern|
-          result.gsub(hidden_pattern, '[FILTERED]')
-        end
-      end
     end
   end
 end

test/lib/concerns/base_database_test.rb

@@ -0,0 +1,163 @@
+require 'test_helper'
+
+class FakeDatabase
+  include ForemanMaintain::Concerns::BaseDatabase
+  include ForemanMaintain::Concerns::SystemHelpers
+
+  attr_reader :configuration
+
+  def initialize(config)
+    @configuration = config
+  end
+end
+
+module ForemanMaintain
+  describe Concerns::BaseDatabase do
+    describe 'with local db' do
+      let(:db) { FakeDatabase.new(config) }
+      let(:config) do
+        {
+          'host' => 'localhost',
+          'database' => 'fakedb',
+          'username' => 'fakeuser',
+          'password' => 'fakepassword',
+        }
+      end
+      let(:expected_env) do
+        {
+          'PGHOST' => 'localhost',
+          'PGPORT' => nil,
+          'PGDATABASE' => 'fakedb',
+          'PGUSER' => 'fakeuser',
+          'PGPASSWORD' => 'fakepassword',
+        }
+      end
+
+      it 'accepts localhost as local' do
+        assert db.local?
+      end
+
+      it 'fetches server version' do
+        db.expects(:ping!)
+        db.expects(:execute!).with(
+          'psql --tuples-only --no-align',
+          stdin: 'SHOW server_version',
+          env: expected_env
+        ).returns('13.16')
+
+        assert db.db_version
+      end
+
+      it 'drops db' do
+        db.expects(:execute!).with("runuser - postgres -c 'dropdb fakedb'").returns('')
+
+        assert db.dropdb
+      end
+
+      it 'restores db' do
+        file = '/backup/fake.dump'
+
+        db.expects(:execute!).with("runuser - postgres -c 'pg_restore -C -d postgres #{file}'").
+          returns('')
+
+        assert db.restore_dump(file, true)
+      end
+
+      it 'dumps db' do
+        file = '/backup/fake.dump'
+
+        db.expects(:execute!).with(
+          "pg_dump -Fc -f /backup/fake.dump",
+          env: expected_env
+        ).returns('')
+
+        assert db.dump_db(file)
+      end
+
+      it 'pings db' do
+        db.expects(:execute?).with("psql",
+          stdin: "SELECT 1 as ping", env: expected_env).returns(true)
+
+        assert db.ping
+      end
+
+      it 'runs db queries' do
+        psql_return = <<~PSQL
+           test
+          ------
+             42
+          (1 row)
+        PSQL
+
+        db.expects(:ping!)
+        db.expects(:execute).with("psql",
+          stdin: "SELECT 42 as test", env: expected_env).returns(psql_return)
+
+        assert db.psql('SELECT 42 as test')
+      end
+    end
+
+    describe 'with remote db' do
+      let(:db) { FakeDatabase.new(config) }
+      let(:config) do
+        {
+          'host' => 'db.example.com',
+          'database' => 'fakedb',
+          'username' => 'fakeuser',
+          'password' => 'fakepassword',
+        }
+      end
+      let(:expected_env) do
+        {
+          'PGHOST' => 'db.example.com',
+          'PGPORT' => nil,
+          'PGDATABASE' => 'fakedb',
+          'PGUSER' => 'fakeuser',
+          'PGPASSWORD' => 'fakepassword',
+        }
+      end
+
+      it 'accepts db.example.com as remote' do
+        refute db.local?
+      end
+
+      it 'drops db' do
+        select_statement = <<~SQL
+          select string_agg('drop table if exists \"' || tablename || '\" cascade;', '')
+          from pg_tables
+          where schemaname = 'public';
+        SQL
+        delete_statement = 'drop table if exists \"faketable\"'
+        db.expects(:psql).with(select_statement).returns(delete_statement)
+        db.expects(:psql).with(delete_statement).returns('')
+        assert db.dropdb
+      end
+
+      it 'restores db' do
+        file = '/backup/fake.dump'
+        restore_cmd = <<~CMD.strip
+          pg_restore --no-privileges --clean --disable-triggers -n public -d fakedb #{file}
+        CMD
+
+        db.expects(:execute!).with(
+          restore_cmd,
+          valid_exit_statuses: [0, 1],
+          env: expected_env
+        ).returns('')
+
+        assert db.restore_dump(file, false)
+      end
+
+      it 'dumps remote db' do
+        file = '/backup/fake.dump'
+
+        db.expects(:execute!).with(
+          "pg_dump -Fc -f /backup/fake.dump",
+          env: expected_env
+        ).returns('')
+
+        assert db.dump_db(file)
+      end
+    end
+  end
+end