From 40664444eef2432fc564d8eecb647854bd06e5c3 Mon Sep 17 00:00:00 2001
From: Steve Mitchell <smitchell@groupon.com>
Date: Fri, 18 Sep 2015 14:16:15 -0500
Subject: Check response of structure_load for mysql_database_tasks and make
 structure_dump consistent

---
 .../active_record/tasks/mysql_database_tasks.rb    | 28 +++++++++++++++-------
 activerecord/test/cases/tasks/mysql_rake_test.rb   | 19 ++++++++++++---
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/activerecord/lib/active_record/tasks/mysql_database_tasks.rb b/activerecord/lib/active_record/tasks/mysql_database_tasks.rb
index ba5dd0a0d9..5b07773d60 100644
--- a/activerecord/lib/active_record/tasks/mysql_database_tasks.rb
+++ b/activerecord/lib/active_record/tasks/mysql_database_tasks.rb
@@ -56,22 +56,21 @@ module ActiveRecord
       end
 
       def structure_dump(filename)
-        args = prepare_command_options('mysqldump')
+        args = prepare_command_options
         args.concat(["--result-file", "#{filename}"])
         args.concat(["--no-data"])
         args.concat(["--routines"])
         args.concat(["#{configuration['database']}"])
-        unless Kernel.system(*args)
-          $stderr.puts "Could not dump the database structure. "\
-                       "Make sure `mysqldump` is in your PATH and check the command output for warnings."
-        end
+        
+        run_cmd('mysqldump', args, 'dumping')
       end
 
       def structure_load(filename)
-        args = prepare_command_options('mysql')
+        args = prepare_command_options
         args.concat(['--execute', %{SET FOREIGN_KEY_CHECKS = 0; SOURCE #{filename}; SET FOREIGN_KEY_CHECKS = 1}])
         args.concat(["--database", "#{configuration['database']}"])
-        Kernel.system(*args)
+        
+        run_cmd('mysql', args, 'loading')
       end
 
       private
@@ -130,7 +129,7 @@ IDENTIFIED BY '#{configuration['password']}' WITH GRANT OPTION;
         $stdin.gets.strip
       end
 
-      def prepare_command_options(command)
+      def prepare_command_options
         args = {
           'host'      => '--host',
           'port'      => '--port',
@@ -145,7 +144,18 @@ IDENTIFIED BY '#{configuration['password']}' WITH GRANT OPTION;
           'sslkey'    => '--ssl-key'
         }.map { |opt, arg| "#{arg}=#{configuration[opt]}" if configuration[opt] }.compact
 
-        [command, *args]
+        args
+      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}"
+        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
diff --git a/activerecord/test/cases/tasks/mysql_rake_test.rb b/activerecord/test/cases/tasks/mysql_rake_test.rb
index d0deb4c273..edca42dfc0 100644
--- a/activerecord/test/cases/tasks/mysql_rake_test.rb
+++ b/activerecord/test/cases/tasks/mysql_rake_test.rb
@@ -274,11 +274,23 @@ module ActiveRecord
       filename = "awesome-file.sql"
       Kernel.expects(:system).with("mysqldump", "--result-file", filename, "--no-data", "--routines", "test-db").returns(false)
 
-      warnings = capture(:stderr) do
+      # There doesn't seem to be a good way to get a handle on a Process::Status object without actually
+      # creating a child process, hence this to populate $?
+      system("not_a_real_program_#{SecureRandom.hex}")
+      assert_raise(RuntimeError) { 
         ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, filename)
-      end
+      }
+    end
+    
+    def test_warn_when_external_structure_dump_command_execution_fails
+      filename = "awesome-file.sql"
+      Kernel.expects(:system)
+        .with("mysqldump", "--result-file", filename, "--no-data", "--routines", "test-db")
+        .returns(nil)
 
-      assert_match(/Could not dump the database structure/, warnings)
+      assert_raise(RuntimeError) { 
+        ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, filename)
+      }
     end
 
     def test_structure_dump_with_port_number
@@ -311,6 +323,7 @@ module ActiveRecord
     def test_structure_load
       filename = "awesome-file.sql"
       Kernel.expects(:system).with('mysql', '--execute', %{SET FOREIGN_KEY_CHECKS = 0; SOURCE #{filename}; SET FOREIGN_KEY_CHECKS = 1}, "--database", "test-db")
+        .returns(true)
 
       ActiveRecord::Tasks::DatabaseTasks.structure_load(@configuration, filename)
     end
-- 
cgit v1.2.3