Merge pull request #17885 from starbelly/patch-1

Add method to run command-line db apps

Conflicts:
	activerecord/CHANGELOG.md

3 files changed, 31 insertions, 14 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index c1ea8a3252..c3b2d157f3 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   `ActiveRecord::Tasks::PostgreSQLDatabaseTasks` fail if shellout to
+    postgresql commands (like `pg_dump`) is not successful.
+
+    *Bryan Paxton*, *Nate Berkopec*
+
 *   Add `ActiveRecord::Relation#in_batches` to work with records and relations
     in batches.
 
diff --git a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb
index d7da95c8a9..55f839444b 100644
--- a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb
+++ b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb
@@ -1,5 +1,3 @@
-require 'shellwords'
-
 module ActiveRecord
   module Tasks # :nodoc:
     class PostgreSQLDatabaseTasks # :nodoc:
@@ -55,19 +53,22 @@ module ActiveRecord
         when String
           ActiveRecord::Base.dump_schemas
         end
+
+        args = ['-i', '-s', '-x', '-O', '-f', filename]
         unless search_path.blank?
-          search_path = search_path.split(",").map{|search_path_part| "--schema=#{Shellwords.escape(search_path_part.strip)}" }.join(" ")
+          args << search_path.split(',').map do |part|
+            "--schema=#{part.strip}"
+          end.join(' ')
         end
-
-        command = "pg_dump -i -s -x -O -f #{Shellwords.escape(filename)} #{search_path} #{Shellwords.escape(configuration['database'])}"
-        raise 'Error dumping database' unless Kernel.system(command)
-
+        args << configuration['database']
+        run_cmd('pg_dump', args, 'dumping')
         File.open(filename, "a") { |f| f << "SET search_path TO #{connection.schema_search_path};\n\n" }
       end
 
       def structure_load(filename)
         set_psql_env
-        Kernel.system("psql -X -q -f #{Shellwords.escape(filename)} #{configuration['database']}")
+        args = [ '-q', '-f', filename, configuration['database'] ]
+        run_cmd('psql', args, 'loading' )
       end
 
       private
@@ -93,6 +94,17 @@ module ActiveRecord
         ENV['PGPASSWORD'] = configuration['password'].to_s if configuration['password']
         ENV['PGUSER']     = configuration['username'].to_s if configuration['username']
       end
+
+      def run_cmd(cmd, args, action)
+        fail run_cmd_error(cmd, args, action) unless Kernel.system(cmd, *args)
+      end
+
+      def run_cmd_error(cmd, args, action)
+        msg = "failed to execute:\n"
+        msg << "#{cmd} #{args.join(' ')}\n\n"
+        msg << "Please check the output above for any errors and make sure that `#{cmd}` is installed in your PATH and has proper permissions.\n\n"
+        msg
+      end
     end
   end
 end
diff --git a/activerecord/test/cases/tasks/postgresql_rake_test.rb b/activerecord/test/cases/tasks/postgresql_rake_test.rb
index 084302cde5..184ff7fc63 100644
--- a/activerecord/test/cases/tasks/postgresql_rake_test.rb
+++ b/activerecord/test/cases/tasks/postgresql_rake_test.rb
@@ -204,7 +204,7 @@ module ActiveRecord
     end
 
     def test_structure_dump
-      Kernel.expects(:system).with("pg_dump -i -s -x -O -f #{@filename}  my-app-db").returns(true)
+      Kernel.expects(:system).with('pg_dump', '-i', '-s', '-x', '-O', '-f', @filename, 'my-app-db').returns(true)
 
       ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
     end
@@ -212,7 +212,7 @@ module ActiveRecord
     def test_structure_dump_with_schema_search_path
       @configuration['schema_search_path'] = 'foo,bar'
 
-      Kernel.expects(:system).with("pg_dump -i -s -x -O -f #{@filename} --schema=foo --schema=bar my-app-db").returns(true)
+      Kernel.expects(:system).with('pg_dump', '-i', '-s', '-x', '-O', '-f', @filename, '--schema=foo --schema=bar', 'my-app-db').returns(true)
 
       ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
     end
@@ -220,7 +220,7 @@ module ActiveRecord
     def test_structure_dump_with_schema_search_path_and_dump_schemas_all
       @configuration['schema_search_path'] = 'foo,bar'
 
-      Kernel.expects(:system).with("pg_dump -i -s -x -O -f #{@filename}  my-app-db").returns(true)
+      Kernel.expects(:system).with("pg_dump", '-i', '-s', '-x', '-O', '-f', @filename,  'my-app-db').returns(true)
 
       with_dump_schemas(:all) do
         ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
@@ -228,7 +228,7 @@ module ActiveRecord
     end
 
     def test_structure_dump_with_dump_schemas_string
-      Kernel.expects(:system).with("pg_dump -i -s -x -O -f #{@filename} --schema=foo --schema=bar my-app-db").returns(true)
+      Kernel.expects(:system).with("pg_dump", '-i', '-s', '-x', '-O', '-f', @filename, '--schema=foo --schema=bar', "my-app-db").returns(true)
 
       with_dump_schemas('foo,bar') do
         ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
@@ -261,14 +261,14 @@ module ActiveRecord
 
     def test_structure_load
       filename = "awesome-file.sql"
-      Kernel.expects(:system).with("psql -X -q -f #{filename} my-app-db")
+      Kernel.expects(:system).with('psql', '-q', '-f', filename, @configuration['database']).returns(true)
 
       ActiveRecord::Tasks::DatabaseTasks.structure_load(@configuration, filename)
     end
 
     def test_structure_load_accepts_path_with_spaces
       filename = "awesome file.sql"
-      Kernel.expects(:system).with("psql -X -q -f awesome\\ file.sql my-app-db")
+      Kernel.expects(:system).with('psql', '-q', '-f', filename, @configuration['database']).returns(true)
 
       ActiveRecord::Tasks::DatabaseTasks.structure_load(@configuration, filename)
     end