Merge pull request #1021 from acelaya-forks/feature/migrate-command-timeout

Feature/migrate command timeout

CHANGELOG.md

@@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 ### Changed
 * [#977](https://github.com/shlinkio/shlink/issues/977) Migrated from `laminas/laminas-paginator` to `pagerfanta/core` to handle pagination.
 * [#986](https://github.com/shlinkio/shlink/issues/986) Updated official docker image to use PHP 8.
+* [#1010](https://github.com/shlinkio/shlink/issues/1010) Increased timeout for database commands to 10 minutes.
 
 ### Deprecated
 * [#959](https://github.com/shlinkio/shlink/issues/959) Deprecated all command flags using camelCase format (like `--expirationDate`), adding kebab-case replacements for all of them (like `--expiration-date`).

module/CLI/config/dependencies.config.php

@@ -34,6 +34,8 @@ return [
             PhpExecutableFinder::class => InvokableFactory::class,
 
             Util\GeolocationDbUpdater::class => ConfigAbstractFactory::class,
+            Util\ProcessRunner::class => ConfigAbstractFactory::class,
+
             ApiKey\RoleResolver::class => ConfigAbstractFactory::class,
 
             Command\ShortUrl\GenerateShortUrlCommand::class => ConfigAbstractFactory::class,
@@ -62,6 +64,7 @@ return [
 
     ConfigAbstractFactory::class => [
         Util\GeolocationDbUpdater::class => [DbUpdater::class, Reader::class, LOCAL_LOCK_FACTORY],
+        Util\ProcessRunner::class => [SymfonyCli\Helper\ProcessHelper::class],
         ApiKey\RoleResolver::class => [DomainService::class],
 
         Command\ShortUrl\GenerateShortUrlCommand::class => [
@@ -97,14 +100,14 @@ return [
 
         Command\Db\CreateDatabaseCommand::class => [
             LockFactory::class,
-            SymfonyCli\Helper\ProcessHelper::class,
+            Util\ProcessRunner::class,
             PhpExecutableFinder::class,
             Connection::class,
             NoDbNameConnectionFactory::SERVICE_NAME,
         ],
         Command\Db\MigrateDatabaseCommand::class => [
             LockFactory::class,
-            SymfonyCli\Helper\ProcessHelper::class,
+            Util\ProcessRunner::class,
             PhpExecutableFinder::class,
         ],
     ],

module/CLI/src/Command/Db/AbstractDatabaseCommand.php

@@ -6,31 +6,34 @@ namespace Shlinkio\Shlink\CLI\Command\Db;
 
 use Shlinkio\Shlink\CLI\Command\Util\AbstractLockedCommand;
 use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig;
-use Symfony\Component\Console\Helper\ProcessHelper;
+use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Process\PhpExecutableFinder;
 
 abstract class AbstractDatabaseCommand extends AbstractLockedCommand
 {
-    private ProcessHelper $processHelper;
+    private ProcessRunnerInterface $processRunner;
     private string $phpBinary;
 
-    public function __construct(LockFactory $locker, ProcessHelper $processHelper, PhpExecutableFinder $phpFinder)
-    {
+    public function __construct(
+        LockFactory $locker,
+        ProcessRunnerInterface $processRunner,
+        PhpExecutableFinder $phpFinder
+    ) {
         parent::__construct($locker);
-        $this->processHelper = $processHelper;
+        $this->processRunner = $processRunner;
         $this->phpBinary = $phpFinder->find(false) ?: 'php';
     }
 
     protected function runPhpCommand(OutputInterface $output, array $command): void
     {
         $command = [$this->phpBinary, ...$command, '--no-interaction'];
-        $this->processHelper->mustRun($output, $command);
+        $this->processRunner->run($output, $command);
     }
 
     protected function getLockConfig(): LockedCommandConfig
     {
-        return new LockedCommandConfig($this->getName(), true);
+        return LockedCommandConfig::blocking($this->getName());
     }
 }

module/CLI/src/Command/Db/CreateDatabaseCommand.php

@@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\Db;
 
 use Doctrine\DBAL\Connection;
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
-use Symfony\Component\Console\Helper\ProcessHelper;
+use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
@@ -26,12 +26,12 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
 
     public function __construct(
         LockFactory $locker,
-        ProcessHelper $processHelper,
+        ProcessRunnerInterface $processRunner,
         PhpExecutableFinder $phpFinder,
         Connection $conn,
         Connection $noDbNameConn
     ) {
-        parent::__construct($locker, $processHelper, $phpFinder);
+        parent::__construct($locker, $processRunner, $phpFinder);
         $this->regularConn = $conn;
         $this->noDbNameConn = $noDbNameConn;
     }

module/CLI/src/Command/Util/LockedCommandConfig.php

@@ -6,19 +6,29 @@ namespace Shlinkio\Shlink\CLI\Command\Util;
 
 final class LockedCommandConfig
 {
-    private const DEFAULT_TTL = 90.0; // 1.5 minutes
+    public const DEFAULT_TTL = 600.0; // 10 minutes
 
     private string $lockName;
     private bool $isBlocking;
     private float $ttl;
 
-    public function __construct(string $lockName, bool $isBlocking = false, float $ttl = self::DEFAULT_TTL)
+    private function __construct(string $lockName, bool $isBlocking, float $ttl = self::DEFAULT_TTL)
     {
         $this->lockName = $lockName;
         $this->isBlocking = $isBlocking;
         $this->ttl = $ttl;
     }
 
+    public static function blocking(string $lockName): self
+    {
+        return new self($lockName, true);
+    }
+
+    public static function nonBlocking(string $lockName): self
+    {
+        return new self($lockName, false);
+    }
+
     public function lockName(): string
     {
         return $this->lockName;

module/CLI/src/Command/Visit/LocateVisitsCommand.php

@@ -208,6 +208,6 @@ class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocat
 
     protected function getLockConfig(): LockedCommandConfig
     {
-        return new LockedCommandConfig($this->getName());
+        return LockedCommandConfig::nonBlocking($this->getName());
     }
 }

module/CLI/src/Util/ProcessRunner.php

@@ -0,0 +1,60 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\CLI\Util;
+
+use Closure;
+use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig;
+use Symfony\Component\Console\Helper\DebugFormatterHelper;
+use Symfony\Component\Console\Helper\ProcessHelper;
+use Symfony\Component\Console\Output\ConsoleOutputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Process\Process;
+
+use function spl_object_hash;
+use function sprintf;
+use function str_replace;
+
+class ProcessRunner implements ProcessRunnerInterface
+{
+    private ProcessHelper $helper;
+    private Closure $createProcess;
+
+    public function __construct(ProcessHelper $helper, ?callable $createProcess = null)
+    {
+        $this->helper = $helper;
+        $this->createProcess = $createProcess !== null
+            ? Closure::fromCallable($createProcess)
+            : static fn (array $cmd) => new Process($cmd, null, null, null, LockedCommandConfig::DEFAULT_TTL);
+    }
+
+    public function run(OutputInterface $output, array $cmd): void
+    {
+        if ($output instanceof ConsoleOutputInterface) {
+            $output = $output->getErrorOutput();
+        }
+
+        /** @var DebugFormatterHelper $formatter */
+        $formatter = $this->helper->getHelperSet()->get('debug_formatter');
+        /** @var Process $process */
+        $process = ($this->createProcess)($cmd);
+
+        if ($output->isVeryVerbose()) {
+            $output->write(
+                $formatter->start(spl_object_hash($process), str_replace('<', '\\<', $process->getCommandLine())),
+            );
+        }
+
+        $callback = $output->isDebug() ? $this->helper->wrapCallback($output, $process) : null;
+        $process->mustRun($callback);
+
+        if ($output->isVeryVerbose()) {
+            $message = $process->isSuccessful() ? 'Command ran successfully' : sprintf(
+                '%s Command did not run successfully',
+                $process->getExitCode(),
+            );
+            $output->write($formatter->stop(spl_object_hash($process), $message, $process->isSuccessful()));
+        }
+    }
+}

module/CLI/src/Util/ProcessRunnerInterface.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\CLI\Util;
+
+use Symfony\Component\Console\Output\OutputInterface;
+
+interface ProcessRunnerInterface
+{
+    public function run(OutputInterface $output, array $cmd): void;
+}

module/CLI/test/Command/Db/CreateDatabaseCommandTest.php

@@ -12,14 +12,13 @@ use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
 use Prophecy\Prophecy\ObjectProphecy;
 use Shlinkio\Shlink\CLI\Command\Db\CreateDatabaseCommand;
+use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface;
 use Symfony\Component\Console\Application;
-use Symfony\Component\Console\Helper\ProcessHelper;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Tester\CommandTester;
 use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Lock\LockInterface;
 use Symfony\Component\Process\PhpExecutableFinder;
-use Symfony\Component\Process\Process;
 
 class CreateDatabaseCommandTest extends TestCase
 {
@@ -43,7 +42,7 @@ class CreateDatabaseCommandTest extends TestCase
         $phpExecutableFinder = $this->prophesize(PhpExecutableFinder::class);
         $phpExecutableFinder->find(false)->willReturn('/usr/local/bin/php');
 
-        $this->processHelper = $this->prophesize(ProcessHelper::class);
+        $this->processHelper = $this->prophesize(ProcessRunnerInterface::class);
         $this->schemaManager = $this->prophesize(AbstractSchemaManager::class);
         $this->databasePlatform = $this->prophesize(AbstractPlatform::class);
 
@@ -113,12 +112,12 @@ class CreateDatabaseCommandTest extends TestCase
         $createDatabase = $this->schemaManager->createDatabase($shlinkDatabase)->will(function (): void {
         });
         $listTables = $this->schemaManager->listTableNames()->willReturn([]);
-        $runCommand = $this->processHelper->mustRun(Argument::type(OutputInterface::class), [
+        $runCommand = $this->processHelper->run(Argument::type(OutputInterface::class), [
             '/usr/local/bin/php',
             CreateDatabaseCommand::DOCTRINE_SCRIPT,
             CreateDatabaseCommand::DOCTRINE_CREATE_SCHEMA_COMMAND,
             '--no-interaction',
-        ], Argument::cetera())->willReturn(new Process([]));
+        ]);
 
         $this->commandTester->execute([]);
         $output = $this->commandTester->getDisplay();

module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php

@@ -9,14 +9,13 @@ use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
 use Prophecy\Prophecy\ObjectProphecy;
 use Shlinkio\Shlink\CLI\Command\Db\MigrateDatabaseCommand;
+use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface;
 use Symfony\Component\Console\Application;
-use Symfony\Component\Console\Helper\ProcessHelper;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Tester\CommandTester;
 use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Lock\LockInterface;
 use Symfony\Component\Process\PhpExecutableFinder;
-use Symfony\Component\Process\Process;
 
 class MigrateDatabaseCommandTest extends TestCase
 {
@@ -37,7 +36,7 @@ class MigrateDatabaseCommandTest extends TestCase
         $phpExecutableFinder = $this->prophesize(PhpExecutableFinder::class);
         $phpExecutableFinder->find(false)->willReturn('/usr/local/bin/php');
 
-        $this->processHelper = $this->prophesize(ProcessHelper::class);
+        $this->processHelper = $this->prophesize(ProcessRunnerInterface::class);
 
         $command = new MigrateDatabaseCommand(
             $locker->reveal(),
@@ -53,12 +52,12 @@ class MigrateDatabaseCommandTest extends TestCase
     /** @test */
     public function migrationsCommandIsRunWithProperVerbosity(): void
     {
-        $runCommand = $this->processHelper->mustRun(Argument::type(OutputInterface::class), [
+        $runCommand = $this->processHelper->run(Argument::type(OutputInterface::class), [
             '/usr/local/bin/php',
             MigrateDatabaseCommand::DOCTRINE_MIGRATIONS_SCRIPT,
             MigrateDatabaseCommand::DOCTRINE_MIGRATE_COMMAND,
             '--no-interaction',
-        ], Argument::cetera())->willReturn(new Process([]));
+        ]);
 
         $this->commandTester->execute([]);
         $output = $this->commandTester->getDisplay();

module/CLI/test/Command/Visit/LocateVisitsCommandTest.php

@@ -52,7 +52,7 @@ class LocateVisitsCommandTest extends TestCase
         $this->lock->acquire(false)->willReturn(true);
         $this->lock->release()->will(function (): void {
         });
-        $locker->createLock(Argument::type('string'), 90.0, false)->willReturn($this->lock->reveal());
+        $locker->createLock(Argument::type('string'), 600.0, false)->willReturn($this->lock->reveal());
 
         $command = new LocateVisitsCommand(
             $this->visitService->reveal(),

module/CLI/test/Util/ProcessRunnerTest.php

@@ -0,0 +1,106 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\CLI\Util;
+
+use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
+use Prophecy\PhpUnit\ProphecyTrait;
+use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\CLI\Util\ProcessRunner;
+use Symfony\Component\Console\Helper\DebugFormatterHelper;
+use Symfony\Component\Console\Helper\HelperSet;
+use Symfony\Component\Console\Helper\ProcessHelper;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Process\Process;
+
+class ProcessRunnerTest extends TestCase
+{
+    use ProphecyTrait;
+
+    private ProcessRunner $runner;
+    private ObjectProphecy $helper;
+    private ObjectProphecy $formatter;
+    private ObjectProphecy $process;
+    private ObjectProphecy $output;
+
+    protected function setUp(): void
+    {
+        $this->helper = $this->prophesize(ProcessHelper::class);
+        $this->formatter = $this->prophesize(DebugFormatterHelper::class);
+        $helperSet = $this->prophesize(HelperSet::class);
+        $helperSet->get('debug_formatter')->willReturn($this->formatter->reveal());
+        $this->helper->getHelperSet()->willReturn($helperSet->reveal());
+        $this->process = $this->prophesize(Process::class);
+
+        $this->runner = new ProcessRunner($this->helper->reveal(), fn () => $this->process->reveal());
+        $this->output = $this->prophesize(OutputInterface::class);
+    }
+
+    /** @test */
+    public function noMessagesAreWrittenWhenOutputIsNotVerbose(): void
+    {
+        $isVeryVerbose = $this->output->isVeryVerbose()->willReturn(false);
+        $isDebug = $this->output->isDebug()->willReturn(false);
+        $mustRun = $this->process->mustRun(Argument::cetera())->willReturn($this->process->reveal());
+
+        $this->runner->run($this->output->reveal(), []);
+
+        $isVeryVerbose->shouldHaveBeenCalledTimes(2);
+        $isDebug->shouldHaveBeenCalledOnce();
+        $mustRun->shouldHaveBeenCalledOnce();
+        $this->process->isSuccessful()->shouldNotHaveBeenCalled();
+        $this->process->getCommandLine()->shouldNotHaveBeenCalled();
+        $this->output->write(Argument::cetera())->shouldNotHaveBeenCalled();
+        $this->helper->wrapCallback(Argument::cetera())->shouldNotHaveBeenCalled();
+        $this->formatter->start(Argument::cetera())->shouldNotHaveBeenCalled();
+        $this->formatter->stop(Argument::cetera())->shouldNotHaveBeenCalled();
+    }
+
+    /** @test */
+    public function someMessagesAreWrittenWhenOutputIsVerbose(): void
+    {
+        $isVeryVerbose = $this->output->isVeryVerbose()->willReturn(true);
+        $isDebug = $this->output->isDebug()->willReturn(false);
+        $mustRun = $this->process->mustRun(Argument::cetera())->willReturn($this->process->reveal());
+        $isSuccessful = $this->process->isSuccessful()->willReturn(true);
+        $getCommandLine = $this->process->getCommandLine()->willReturn('true');
+        $start = $this->formatter->start(Argument::cetera())->willReturn('');
+        $stop = $this->formatter->stop(Argument::cetera())->willReturn('');
+
+        $this->runner->run($this->output->reveal(), []);
+
+        $isVeryVerbose->shouldHaveBeenCalledTimes(2);
+        $isDebug->shouldHaveBeenCalledOnce();
+        $mustRun->shouldHaveBeenCalledOnce();
+        $this->output->write(Argument::cetera())->shouldHaveBeenCalledTimes(2);
+        $this->helper->wrapCallback(Argument::cetera())->shouldNotHaveBeenCalled();
+        $isSuccessful->shouldHaveBeenCalledTimes(2);
+        $getCommandLine->shouldHaveBeenCalledOnce();
+        $start->shouldHaveBeenCalledOnce();
+        $stop->shouldHaveBeenCalledOnce();
+    }
+
+    /** @test */
+    public function wrapsCallbackWhenOutputIsDebug(): void
+    {
+        $isVeryVerbose = $this->output->isVeryVerbose()->willReturn(false);
+        $isDebug = $this->output->isDebug()->willReturn(true);
+        $mustRun = $this->process->mustRun(Argument::cetera())->willReturn($this->process->reveal());
+        $wrapCallback = $this->helper->wrapCallback(Argument::cetera())->willReturn(function (): void {
+        });
+
+        $this->runner->run($this->output->reveal(), []);
+
+        $isVeryVerbose->shouldHaveBeenCalledTimes(2);
+        $isDebug->shouldHaveBeenCalledOnce();
+        $mustRun->shouldHaveBeenCalledOnce();
+        $wrapCallback->shouldHaveBeenCalledOnce();
+        $this->process->isSuccessful()->shouldNotHaveBeenCalled();
+        $this->process->getCommandLine()->shouldNotHaveBeenCalled();
+        $this->output->write(Argument::cetera())->shouldNotHaveBeenCalled();
+        $this->formatter->start(Argument::cetera())->shouldNotHaveBeenCalled();
+        $this->formatter->stop(Argument::cetera())->shouldNotHaveBeenCalled();
+    }
+}

phpstan.neon

@@ -2,5 +2,4 @@ parameters:
 	checkMissingIterableValueType: false
 	checkGenericClassInNonGenericObjectType: false
 	ignoreErrors:
-		- '#mustRun\(\)#'
 		- '#If condition is always false#'