From 02ac308042ddd4480754ee2133300c411a31d76b Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sat, 3 Aug 2019 12:33:28 -0700 Subject: [PATCH] Fix database host modification not properly showing SQL errors This is caused by an old bug relating to not rolling back transactions properly causing session data to not be flashed back to the user properly. --- CHANGELOG.md | 2 ++ .../Controllers/Admin/DatabaseController.php | 33 ++++++++++++------ .../Databases/Hosts/HostCreationService.php | 34 +++++++++---------- .../Databases/Hosts/HostUpdateService.php | 18 +++++----- .../Hosts/HostCreationServiceTest.php | 18 +++++----- .../Databases/Hosts/HostUpdateServiceTest.php | 28 ++++++++------- 6 files changed, 74 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bbbd84f0..67f54d40c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ This project follows [Semantic Versioning](http://semver.org) guidelines. value when showing an error state. * Mass deleting files now executes properly and doesn't result in a JS console error. * Scrolling on email settings page now works. +* Database host management will now properly display an error message to the user when there is any type of MySQL related +error encountered during creation or update. ### Added * Server listing view now displays the total used disk space for each server. diff --git a/app/Http/Controllers/Admin/DatabaseController.php b/app/Http/Controllers/Admin/DatabaseController.php index affb0c80e..777b27792 100644 --- a/app/Http/Controllers/Admin/DatabaseController.php +++ b/app/Http/Controllers/Admin/DatabaseController.php @@ -2,6 +2,7 @@ namespace Pterodactyl\Http\Controllers\Admin; +use Exception; use PDOException; use Illuminate\View\View; use Pterodactyl\Models\DatabaseHost; @@ -118,17 +119,22 @@ class DatabaseController extends Controller * @param \Pterodactyl\Http\Requests\Admin\DatabaseHostFormRequest $request * @return \Illuminate\Http\RedirectResponse * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Throwable */ public function create(DatabaseHostFormRequest $request): RedirectResponse { try { $host = $this->creationService->handle($request->normalize()); - } catch (PDOException $ex) { - $this->alert->danger($ex->getMessage())->flash(); + } catch (Exception $exception) { + if ($exception instanceof PDOException || $exception->getPrevious() instanceof PDOException) { + $this->alert->danger( + sprintf('There was an error while trying to connect to the host or while executing a query: "%s"', $exception->getMessage()) + )->flash(); - return redirect()->route('admin.databases'); + redirect()->route('admin.databases')->withInput($request->validated()); + } else { + throw $exception; + } } $this->alert->success('Successfully created a new database host on the system.')->flash(); @@ -143,8 +149,7 @@ class DatabaseController extends Controller * @param \Pterodactyl\Models\DatabaseHost $host * @return \Illuminate\Http\RedirectResponse * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Throwable */ public function update(DatabaseHostFormRequest $request, DatabaseHost $host): RedirectResponse { @@ -153,9 +158,17 @@ class DatabaseController extends Controller try { $this->updateService->handle($host->id, $request->normalize()); $this->alert->success('Database host was updated successfully.')->flash(); - } catch (PDOException $ex) { - $this->alert->danger($ex->getMessage())->flash(); - $redirect->withInput($request->normalize()); + } catch (Exception $exception) { + // Catch any SQL related exceptions and display them back to the user, otherwise just + // throw the exception like normal and move on with it. + if ($exception instanceof PDOException || $exception->getPrevious() instanceof PDOException) { + $this->alert->danger( + sprintf('There was an error while trying to connect to the host or while executing a query: "%s"', $exception->getMessage()) + )->flash(); + $redirect->withInput($request->normalize()); + } else { + throw $exception; + } } return $redirect; diff --git a/app/Services/Databases/Hosts/HostCreationService.php b/app/Services/Databases/Hosts/HostCreationService.php index 15b32ea04..d6ea2f485 100644 --- a/app/Services/Databases/Hosts/HostCreationService.php +++ b/app/Services/Databases/Hosts/HostCreationService.php @@ -65,28 +65,26 @@ class HostCreationService * @param array $data * @return \Pterodactyl\Models\DatabaseHost * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Throwable */ public function handle(array $data): DatabaseHost { - $this->connection->beginTransaction(); + return $this->connection->transaction(function () use ($data) { + $host = $this->repository->create([ + 'password' => $this->encrypter->encrypt(array_get($data, 'password')), + 'name' => array_get($data, 'name'), + 'host' => array_get($data, 'host'), + 'port' => array_get($data, 'port'), + 'username' => array_get($data, 'username'), + 'max_databases' => null, + 'node_id' => array_get($data, 'node_id'), + ]); - $host = $this->repository->create([ - 'password' => $this->encrypter->encrypt(array_get($data, 'password')), - 'name' => array_get($data, 'name'), - 'host' => array_get($data, 'host'), - 'port' => array_get($data, 'port'), - 'username' => array_get($data, 'username'), - 'max_databases' => null, - 'node_id' => array_get($data, 'node_id'), - ]); + // Confirm access using the provided credentials before saving data. + $this->dynamic->set('dynamic', $host); + $this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual'); - // Confirm access using the provided credentials before saving data. - $this->dynamic->set('dynamic', $host); - $this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual'); - $this->connection->commit(); - - return $host; + return $host; + }); } } diff --git a/app/Services/Databases/Hosts/HostUpdateService.php b/app/Services/Databases/Hosts/HostUpdateService.php index 5f4b19b31..cb6312d21 100644 --- a/app/Services/Databases/Hosts/HostUpdateService.php +++ b/app/Services/Databases/Hosts/HostUpdateService.php @@ -71,10 +71,9 @@ class HostUpdateService * * @param int $hostId * @param array $data - * @return mixed + * @return \Pterodactyl\Models\DatabaseHost * - * @throws \Pterodactyl\Exceptions\Model\DataValidationException - * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException + * @throws \Throwable */ public function handle(int $hostId, array $data): DatabaseHost { @@ -84,13 +83,12 @@ class HostUpdateService unset($data['password']); } - $this->connection->beginTransaction(); - $host = $this->repository->update($hostId, $data); + return $this->connection->transaction(function () use ($data, $hostId) { + $host = $this->repository->update($hostId, $data); + $this->dynamic->set('dynamic', $host); + $this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual'); - $this->dynamic->set('dynamic', $host); - $this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual'); - $this->connection->commit(); - - return $host; + return $host; + }); } } diff --git a/tests/Unit/Services/Databases/Hosts/HostCreationServiceTest.php b/tests/Unit/Services/Databases/Hosts/HostCreationServiceTest.php index 603b871a0..ea28637df 100644 --- a/tests/Unit/Services/Databases/Hosts/HostCreationServiceTest.php +++ b/tests/Unit/Services/Databases/Hosts/HostCreationServiceTest.php @@ -60,18 +60,20 @@ class HostCreationServiceTest extends TestCase { $model = factory(DatabaseHost::class)->make(); - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->encrypter->shouldReceive('encrypt')->with('test123')->once()->andReturn('enc123'); - $this->repository->shouldReceive('create')->with(m::subset([ + $this->connection->expects('transaction')->with(m::on(function ($closure) { + return ! is_null($closure()); + }))->andReturn($model); + + $this->encrypter->expects('encrypt')->with('test123')->andReturn('enc123'); + $this->repository->expects('create')->with(m::subset([ 'password' => 'enc123', 'username' => $model->username, 'node_id' => $model->node_id, - ]))->once()->andReturn($model); + ]))->andReturn($model); - $this->dynamic->shouldReceive('set')->with('dynamic', $model)->once()->andReturnNull(); - $this->databaseManager->shouldReceive('connection')->with('dynamic')->once()->andReturnSelf(); - $this->databaseManager->shouldReceive('select')->with('SELECT 1 FROM dual')->once()->andReturnNull(); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + $this->dynamic->expects('set')->with('dynamic', $model)->andReturnNull(); + $this->databaseManager->expects('connection')->with('dynamic')->andReturnSelf(); + $this->databaseManager->expects('select')->with('SELECT 1 FROM dual')->andReturnNull(); $response = $this->getService()->handle([ 'password' => 'test123', diff --git a/tests/Unit/Services/Databases/Hosts/HostUpdateServiceTest.php b/tests/Unit/Services/Databases/Hosts/HostUpdateServiceTest.php index 7e115c000..fa7454ca1 100644 --- a/tests/Unit/Services/Databases/Hosts/HostUpdateServiceTest.php +++ b/tests/Unit/Services/Databases/Hosts/HostUpdateServiceTest.php @@ -60,14 +60,15 @@ class HostUpdateServiceTest extends TestCase { $model = factory(DatabaseHost::class)->make(); - $this->encrypter->shouldReceive('encrypt')->with('test123')->once()->andReturn('enc123'); - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->repository->shouldReceive('update')->with(1234, ['password' => 'enc123'])->once()->andReturn($model); + $this->connection->expects('transaction')->with(m::on(function ($closure) { + return ! is_null($closure()); + }))->andReturn($model); - $this->dynamic->shouldReceive('set')->with('dynamic', $model)->once()->andReturnNull(); - $this->databaseManager->shouldReceive('connection')->with('dynamic')->once()->andReturnSelf(); - $this->databaseManager->shouldReceive('select')->with('SELECT 1 FROM dual')->once()->andReturnNull(); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + $this->encrypter->expects('encrypt')->with('test123')->andReturn('enc123'); + $this->repository->expects('update')->with(1234, ['password' => 'enc123'])->andReturn($model); + $this->dynamic->expects('set')->with('dynamic', $model)->andReturnNull(); + $this->databaseManager->expects('connection')->with('dynamic')->andReturnSelf(); + $this->databaseManager->expects('select')->with('SELECT 1 FROM dual')->andReturnNull(); $response = $this->getService()->handle(1234, ['password' => 'test123']); $this->assertNotEmpty($response); @@ -81,13 +82,14 @@ class HostUpdateServiceTest extends TestCase { $model = factory(DatabaseHost::class)->make(); - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->repository->shouldReceive('update')->with(1234, ['username' => 'test'])->once()->andReturn($model); + $this->connection->expects('transaction')->with(m::on(function ($closure) { + return ! is_null($closure()); + }))->andReturn($model); - $this->dynamic->shouldReceive('set')->with('dynamic', $model)->once()->andReturnNull(); - $this->databaseManager->shouldReceive('connection')->with('dynamic')->once()->andReturnSelf(); - $this->databaseManager->shouldReceive('select')->with('SELECT 1 FROM dual')->once()->andReturnNull(); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + $this->repository->expects('update')->with(1234, ['username' => 'test'])->andReturn($model); + $this->dynamic->expects('set')->with('dynamic', $model)->andReturnNull(); + $this->databaseManager->expects('connection')->with('dynamic')->andReturnSelf(); + $this->databaseManager->expects('select')->with('SELECT 1 FROM dual')->andReturnNull(); $response = $this->getService()->handle(1234, ['password' => '', 'username' => 'test']); $this->assertNotEmpty($response);