Merge pull request #20569 from theSteveMitchell/master

Check mysql structure_load for errors

activerecord/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   `ActiveRecord::Tasks::MySQLDatabaseTasks` fails if shellout to
+    mysql commands (like `mysqldump`) is not successful.
+
+    *Steve Mitchell*
+
 *   Ensure `select` quotes aliased attributes, even when using `from`.
 
     Fixes #21488

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,17 @@ 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: `#{cmd}`\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

activerecord/test/cases/tasks/mysql_rake_test.rb

@@ -270,15 +270,16 @@ module ActiveRecord
       ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, filename)
     end
 
-    def test_warn_when_external_structure_dump_fails
+    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(false)
+      Kernel.expects(:system)
+        .with("mysqldump", "--result-file", filename, "--no-data", "--routines", "test-db")
+        .returns(false)
 
-      warnings = capture(:stderr) do
+      e = assert_raise(RuntimeError) {
         ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, filename)
-      end
-
-      assert_match(/Could not dump the database structure/, warnings)
+      }
+      assert_match(/^failed to execute: `mysqldump`$/, e.message)
     end
 
     def test_structure_dump_with_port_number
@@ -311,6 +312,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