Modify how deletion service works (actually fixes #2085); cover changes with test

app/Http/Controllers/Admin/ServersController.php

@@ -409,7 +409,7 @@ class ServersController extends Controller
             ['id', '=', $database],
         ]);
 
-        $this->databaseManagementService->delete($database->id);
+        $this->databaseManagementService->delete($database);
 
         return response('', 204);
     }

app/Http/Controllers/Api/Application/Servers/DatabaseController.php

@@ -133,7 +133,7 @@ class DatabaseController extends ApplicationApiController
      */
     public function delete(ServerDatabaseWriteRequest $request): Response
     {
-        $this->databaseManagementService->delete($request->getModel(Database::class)->id);
+        $this->databaseManagementService->delete($request->getModel(Database::class));
 
         return response('', 204);
     }

app/Http/Controllers/Api/Client/Servers/DatabaseController.php

@@ -129,7 +129,7 @@ class DatabaseController extends ClientApiController
      */
     public function delete(DeleteDatabaseRequest $request, Server $server, Database $database): Response
     {
-        $this->managementService->delete($database->id);
+        $this->managementService->delete($database);
 
         return Response::create('', Response::HTTP_NO_CONTENT);
     }

app/Services/Databases/DatabaseManagementService.php

@@ -151,20 +151,19 @@ class DatabaseManagementService
     /**
      * Delete a database from the given host server.
      *
-     * @param int $id
+     * @param \Pterodactyl\Models\Database $database
      * @return bool|null
      *
-     * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException
+     * @throws \Exception
      */
-    public function delete($id)
+    public function delete(Database $database)
     {
-        $database = $this->repository->find($id);
         $this->dynamic->set('dynamic', $database->database_host_id);
 
         $this->repository->dropDatabase($database->database);
         $this->repository->dropUser($database->username, $database->remote);
         $this->repository->flush();
 
-        return $this->repository->delete($id);
+        return $database->delete();
     }
 }

app/Services/Servers/ServerDeletionService.php

@@ -3,11 +3,10 @@
 namespace Pterodactyl\Services\Servers;
 
 use Exception;
-use Psr\Log\LoggerInterface;
+use Illuminate\Http\Response;
 use Pterodactyl\Models\Server;
+use Illuminate\Support\Facades\Log;
 use Illuminate\Database\ConnectionInterface;
-use Pterodactyl\Repositories\Eloquent\ServerRepository;
-use Pterodactyl\Repositories\Eloquent\DatabaseRepository;
 use Pterodactyl\Repositories\Wings\DaemonServerRepository;
 use Pterodactyl\Services\Databases\DatabaseManagementService;
 use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException;
@@ -29,50 +28,26 @@ class ServerDeletionService
      */
     private $daemonServerRepository;
 
-    /**
-     * @var \Pterodactyl\Repositories\Eloquent\DatabaseRepository
-     */
-    private $databaseRepository;
-
     /**
      * @var \Pterodactyl\Services\Databases\DatabaseManagementService
      */
     private $databaseManagementService;
 
-    /**
-     * @var \Pterodactyl\Repositories\Eloquent\ServerRepository
-     */
-    private $repository;
-
-    /**
-     * @var \Psr\Log\LoggerInterface
-     */
-    private $writer;
-
     /**
      * DeletionService constructor.
      *
      * @param \Illuminate\Database\ConnectionInterface $connection
      * @param \Pterodactyl\Repositories\Wings\DaemonServerRepository $daemonServerRepository
-     * @param \Pterodactyl\Repositories\Eloquent\DatabaseRepository $databaseRepository
      * @param \Pterodactyl\Services\Databases\DatabaseManagementService $databaseManagementService
-     * @param \Pterodactyl\Repositories\Eloquent\ServerRepository $repository
-     * @param \Psr\Log\LoggerInterface $writer
      */
     public function __construct(
         ConnectionInterface $connection,
         DaemonServerRepository $daemonServerRepository,
-        DatabaseRepository $databaseRepository,
+        DatabaseManagementService $databaseManagementService
-        DatabaseManagementService $databaseManagementService,
-        ServerRepository $repository,
-        LoggerInterface $writer
     ) {
         $this->connection = $connection;
         $this->daemonServerRepository = $daemonServerRepository;
-        $this->databaseRepository = $databaseRepository;
         $this->databaseManagementService = $databaseManagementService;
-        $this->repository = $repository;
-        $this->writer = $writer;
     }
 
     /**
@@ -101,27 +76,39 @@ class ServerDeletionService
         try {
             $this->daemonServerRepository->setServer($server)->delete();
         } catch (DaemonConnectionException $exception) {
-            if ($this->force) {
+            // If there is an error not caused a 404 error and this isn't a forced delete,
-                $this->writer->warning($exception);
+            // go ahead and bail out. We specifically ignore a 404 since that can be assumed
-            } else {
+            // to be a safe error, meaning the server doesn't exist at all on Wings so there
+            // is no reason we need to bail out from that.
+            if (! $this->force && $exception->getStatusCode() !== Response::HTTP_NOT_FOUND) {
                 throw $exception;
             }
+
+            Log::warning($exception);
         }
 
         $this->connection->transaction(function () use ($server) {
-            $this->databaseRepository->setColumns('id')->findWhere([['server_id', '=', $server->id]])->each(function ($item) {
+            foreach ($server->databases as $database) {
                 try {
-                    $this->databaseManagementService->delete($item->id);
+                    $this->databaseManagementService->delete($database);
                 } catch (Exception $exception) {
-                    if ($this->force) {
+                    if (!$this->force) {
-                        $this->writer->warning($exception);
-                    } else {
                         throw $exception;
                     }
-                }
-            });
 
-            $this->repository->delete($server->id);
+                    // Oh well, just try to delete the database entry we have from the database
+                    // so that the server itself can be deleted. This will leave it dangling on
+                    // the host instance, but we couldn't delete it anyways so not sure how we would
+                    // handle this better anyways.
+                    //
+                    // @see https://github.com/pterodactyl/panel/issues/2085
+                    $database->delete();
+
+                    Log::warning($exception);
+                }
+            }
+
+            $server->delete();
         });
     }
 }

config/logging.php

@@ -1,5 +1,6 @@
 <?php
 
+use Monolog\Handler\NullHandler;
 use Monolog\Handler\StreamHandler;
 
 return [
@@ -75,5 +76,10 @@ return [
             'driver' => 'errorlog',
             'level' => 'debug',
         ],
+
+        'null' => [
+            'driver' => 'monolog',
+            'handler' => NullHandler::class,
+        ],
     ],
 ];

tests/Integration/Services/Servers/ServerDeletionServiceTest.php

@@ -0,0 +1,166 @@
+<?php
+
+namespace Pterodactyl\Tests\Integration\Services\Servers;
+
+use Mockery;
+use Exception;
+use GuzzleHttp\Psr7\Request;
+use GuzzleHttp\Psr7\Response;
+use Pterodactyl\Models\Database;
+use Pterodactyl\Models\DatabaseHost;
+use GuzzleHttp\Exception\BadResponseException;
+use Pterodactyl\Tests\Integration\IntegrationTestCase;
+use Pterodactyl\Services\Servers\ServerDeletionService;
+use Pterodactyl\Repositories\Wings\DaemonServerRepository;
+use Pterodactyl\Services\Databases\DatabaseManagementService;
+use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException;
+
+class ServerDeletionServiceTest extends IntegrationTestCase
+{
+    /** @var \Mockery\MockInterface */
+    private $daemonServerRepository;
+
+    /** @var \Mockery\MockInterface */
+    private $databaseManagementService;
+
+    private static $defaultLogger;
+
+    /**
+     * Stub out services that we don't want to test in here.
+     */
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        self::$defaultLogger = config('logging.default');
+        // There will be some log calls during this test, don't actually write to the disk.
+        config()->set('logging.default', 'null');
+
+        $this->daemonServerRepository = Mockery::mock(DaemonServerRepository::class);
+        $this->databaseManagementService = Mockery::mock(DatabaseManagementService::class);
+
+        $this->app->instance(DaemonServerRepository::class, $this->daemonServerRepository);
+        $this->app->instance(DatabaseManagementService::class, $this->databaseManagementService);
+    }
+
+    /**
+     * Reset the log driver.
+     */
+    protected function tearDown(): void
+    {
+        config()->set('logging.default', self::$defaultLogger);
+        self::$defaultLogger = null;
+
+        parent::tearDown();
+    }
+
+    /**
+     * Test that a server is not deleted if the force option is not set and an error
+     * is returned by wings.
+     */
+    public function testRegularDeleteFailsIfWingsReturnsError()
+    {
+        $server = $this->createServerModel();
+
+        $this->expectException(DaemonConnectionException::class);
+
+        $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andThrows(
+            new DaemonConnectionException(new BadResponseException('Bad request', new Request('GET', '/test')))
+        );
+
+        $this->getService()->handle($server);
+
+        $this->assertDatabaseHas('servers', ['id' => $server->id]);
+    }
+
+    /**
+     * Test that a 404 from Wings while deleting a server does not cause the deletion to fail.
+     */
+    public function testRegularDeleteIgnores404FromWings()
+    {
+        $server = $this->createServerModel();
+
+        $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andThrows(
+            new DaemonConnectionException(new BadResponseException('Bad request', new Request('GET', '/test'), new Response(404)))
+        );
+
+        $this->getService()->handle($server);
+
+        $this->assertDatabaseMissing('servers', ['id' => $server->id]);
+    }
+
+    /**
+     * Test that an error from Wings does not cause the deletion to fail if the server is being
+     * force deleted.
+     */
+    public function testForceDeleteIgnoresExceptionFromWings()
+    {
+        $server = $this->createServerModel();
+
+        $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andThrows(
+            new DaemonConnectionException(new BadResponseException('Bad request', new Request('GET', '/test'), new Response(500)))
+        );
+
+        $this->getService()->withForce(true)->handle($server);
+
+        $this->assertDatabaseMissing('servers', ['id' => $server->id]);
+    }
+
+    /**
+     * Test that a non-force-delete call does not delete the server if one of the databases
+     * cannot be deleted from the host.
+     */
+    public function testExceptionWhileDeletingStopsProcess()
+    {
+        $server = $this->createServerModel();
+        $host = factory(DatabaseHost::class)->create();
+
+        /** @var \Pterodactyl\Models\Database $db */
+        $db = factory(Database::class)->create(['database_host_id' => $host->id, 'server_id' => $server->id]);
+
+        $server->refresh();
+
+        $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andReturnUndefined();
+        $this->databaseManagementService->expects('delete')->with(Mockery::on(function ($value) use ($db) {
+            return $value instanceof Database && $value->id === $db->id;
+        }))->andThrows(new Exception);
+
+        $this->expectException(Exception::class);
+        $this->getService()->handle($server);
+
+        $this->assertDatabaseHas('servers', ['id' => $server->id]);
+        $this->assertDatabaseHas('databases', ['id' => $db->id]);
+    }
+
+    /**
+     * Test that a server is deleted even if the server databases cannot be deleted from the host.
+     */
+    public function testExceptionWhileDeletingDatabasesDoesNotAbortIfForceDeleted()
+    {
+        $server = $this->createServerModel();
+        $host = factory(DatabaseHost::class)->create();
+
+        /** @var \Pterodactyl\Models\Database $db */
+        $db = factory(Database::class)->create(['database_host_id' => $host->id, 'server_id' => $server->id]);
+
+        $server->refresh();
+
+        $this->daemonServerRepository->expects('setServer->delete')->withNoArgs()->andReturnUndefined();
+        $this->databaseManagementService->expects('delete')->with(Mockery::on(function ($value) use ($db) {
+            return $value instanceof Database && $value->id === $db->id;
+        }))->andThrows(new Exception);
+
+        $this->getService()->withForce(true)->handle($server);
+
+        $this->assertDatabaseMissing('servers', ['id' => $server->id]);
+        $this->assertDatabaseMissing('databases', ['id' => $db->id]);
+    }
+
+    /**
+     * @return \Pterodactyl\Services\Servers\ServerDeletionService
+     */
+    private function getService()
+    {
+        return $this->app->make(ServerDeletionService::class);
+    }
+}

tests/Unit/Services/Servers/ServerDeletionServiceTest.php

@@ -1,156 +0,0 @@
-<?php
-
-namespace Tests\Unit\Services\Servers;
-
-use Mockery as m;
-use Tests\TestCase;
-use GuzzleHttp\Psr7\Response;
-use Pterodactyl\Models\Server;
-use Psr\Log\LoggerInterface as Writer;
-use Tests\Traits\MocksRequestException;
-use Illuminate\Database\ConnectionInterface;
-use Pterodactyl\Services\Servers\ServerDeletionService;
-use Pterodactyl\Services\Databases\DatabaseManagementService;
-use Pterodactyl\Contracts\Repository\ServerRepositoryInterface;
-use Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface;
-use Pterodactyl\Contracts\Repository\Daemon\ServerRepositoryInterface as DaemonServerRepositoryInterface;
-
-class ServerDeletionServiceTest extends TestCase
-{
-    use MocksRequestException;
-
-    /**
-     * @var \Illuminate\Database\ConnectionInterface|\Mockery\Mock
-     */
-    private $connection;
-
-    /**
-     * @var \Pterodactyl\Contracts\Repository\Daemon\ServerRepositoryInterface|\Mockery\Mock
-     */
-    private $daemonServerRepository;
-
-    /**
-     * @var \Pterodactyl\Services\Databases\DatabaseManagementService|\Mockery\Mock
-     */
-    private $databaseManagementService;
-
-    /**
-     * @var \Pterodactyl\Contracts\Repository\DatabaseRepositoryInterface|\Mockery\Mock
-     */
-    private $databaseRepository;
-
-    /**
-     * @var \Pterodactyl\Contracts\Repository\ServerRepositoryInterface|\Mockery\Mock
-     */
-    private $repository;
-
-    /**
-     * @var \Psr\Log\LoggerInterface|\Mockery\Mock
-     */
-    private $writer;
-
-    /**
-     * Setup tests.
-     */
-    public function setUp(): void
-    {
-        parent::setUp();
-
-        $this->connection = m::mock(ConnectionInterface::class);
-        $this->daemonServerRepository = m::mock(DaemonServerRepositoryInterface::class);
-        $this->databaseRepository = m::mock(DatabaseRepositoryInterface::class);
-        $this->databaseManagementService = m::mock(DatabaseManagementService::class);
-        $this->repository = m::mock(ServerRepositoryInterface::class);
-        $this->writer = m::mock(Writer::class);
-    }
-
-    /**
-     * Test that a server can be force deleted by setting it in a function call.
-     */
-    public function testForceParameterCanBeSet()
-    {
-        $response = $this->getService()->withForce(true);
-
-        $this->assertInstanceOf(ServerDeletionService::class, $response);
-    }
-
-    /**
-     * Test that a server can be deleted when force is not set.
-     */
-    public function testServerCanBeDeletedWithoutForce()
-    {
-        $model = factory(Server::class)->make();
-
-        $this->daemonServerRepository->shouldReceive('setServer')->once()->with($model)->andReturnSelf();
-        $this->daemonServerRepository->shouldReceive('delete')->once()->withNoArgs()->andReturn(new Response);
-
-        $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull();
-        $this->databaseRepository->shouldReceive('setColumns')->once()->with('id')->andReturnSelf();
-        $this->databaseRepository->shouldReceive('findWhere')->once()->with([
-            ['server_id', '=', $model->id],
-        ])->andReturn(collect([(object) ['id' => 50]]));
-
-        $this->databaseManagementService->shouldReceive('delete')->once()->with(50)->andReturnNull();
-        $this->repository->shouldReceive('delete')->once()->with($model->id)->andReturn(1);
-        $this->connection->shouldReceive('commit')->once()->withNoArgs()->andReturnNull();
-
-        $this->getService()->handle($model);
-    }
-
-    /**
-     * Test that a server is deleted when force is set.
-     */
-    public function testServerShouldBeDeletedEvenWhenFailureOccursIfForceIsSet()
-    {
-        $this->configureExceptionMock();
-        $model = factory(Server::class)->make();
-
-        $this->daemonServerRepository->shouldReceive('setServer')->once()->with($model)->andReturnSelf();
-        $this->daemonServerRepository->shouldReceive('delete')->once()->withNoArgs()->andThrow($this->getExceptionMock());
-
-        $this->writer->shouldReceive('warning')->with($this->exception)->once()->andReturnNull();
-        $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull();
-        $this->databaseRepository->shouldReceive('setColumns')->with('id')->once()->andReturnSelf();
-        $this->databaseRepository->shouldReceive('findWhere')->with([
-            ['server_id', '=', $model->id],
-        ])->once()->andReturn(collect([(object) ['id' => 50]]));
-
-        $this->databaseManagementService->shouldReceive('delete')->with(50)->once()->andReturnNull();
-        $this->repository->shouldReceive('delete')->with($model->id)->once()->andReturn(1);
-        $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull();
-
-        $this->getService()->withForce()->handle($model);
-    }
-
-    /**
-     * Test that an exception is thrown if a server cannot be deleted from the node and force is not set.
-     *
-     * @expectedException \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException
-     */
-    public function testExceptionShouldBeThrownIfDaemonReturnsAnErrorAndForceIsNotSet()
-    {
-        $this->configureExceptionMock();
-        $model = factory(Server::class)->make();
-
-        $this->daemonServerRepository->shouldReceive('setServer->delete')->once()->andThrow($this->getExceptionMock());
-
-        $this->getService()->handle($model);
-    }
-
-    /**
-     * Return an instance of the class with mocked dependencies.
-     *
-     * @return \Pterodactyl\Services\Servers\ServerDeletionService
-     */
-    private function getService(): ServerDeletionService
-    {
-        return new ServerDeletionService(
-            $this->connection,
-            $this->daemonServerRepository,
-            $this->databaseRepository,
-            $this->databaseManagementService,
-            $this->repository,
-            $this->writer
-        );
-    }
-}