Add run_cmd class method to ActiveRecord::Tasks::DatabaseTasks

  -   Added run_cmd() class method to dry up Kernel.system() messages within
      this namespace and avoid shell expansion by passing a list of
      arguments instead of a string

  -   Update structure_dump, structure_load, and related tests units to
      pass a list of params instead of using a string to
      avoid shell expansion

4 files changed, 39 insertions, 17 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 42b8515133..5c76319f47 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Added run_cmd class method to ActiveRecord::Tasks::DatabaseTasks for
+    drying up Kernel.system() calls within this namespace and to avoid
+    shell expansion by using a paramter list instead of string as arguments
+    for Kernel.system(). Thanks to Nate Berkopec for supply patch to get
+    test units passing.
+
+    *Bryan Paxton*
+
 *   Properly allow uniqueness validations on primary keys.
 
     Fixes #20966.
diff --git a/activerecord/lib/active_record/tasks/database_tasks.rb b/activerecord/lib/active_record/tasks/database_tasks.rb
index 683741768b..94a8116b8b 100644
--- a/activerecord/lib/active_record/tasks/database_tasks.rb
+++ b/activerecord/lib/active_record/tasks/database_tasks.rb
@@ -252,6 +252,19 @@ module ActiveRecord
         end
       end
 
+      def self.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 that `#{cmd}` is :\n"
+        msg << " - present on this system\n"
+        msg << " - in your PATH\n"
+        msg << " - has proper permissions\n\n"
+      end
+
       private
 
       def class_for_adapter(adapter)
diff --git a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb
index d7da95c8a9..d406f24c31 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
-        unless search_path.blank?
-          search_path = search_path.split(",").map{|search_path_part| "--schema=#{Shellwords.escape(search_path_part.strip)}" }.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 = ['-i', '-s', '-x', '-O', '-f', filename]
+        unless search_path.blank?
+          args << search_path.split(',').map do |part|
+            "--schema=#{part.strip}"
+          end.join(' ')
+        end 
+        args << configuration['database']
+        ActiveRecord::Tasks::DatabaseTasks.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']}")
+      def structure_load(filename)      
+        set_psql_env 
+        args = [ '-q', '-f', filename, configuration['database'] ]
+        ActiveRecord::Tasks::DatabaseTasks.run_cmd('psql', args, 'loading' )
       end
 
       private
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