Skip to content

Commit 02ac308

Browse files
DaneEverittDaneEveritt
committed
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.
1 parent 2cda14b commit 02ac308

File tree

6 files changed

+75
-60
lines changed

6 files changed

+75
-60
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ This project follows [Semantic Versioning](http://semver.org) guidelines.
1010
value when showing an error state.
1111
* Mass deleting files now executes properly and doesn't result in a JS console error.
1212
* Scrolling on email settings page now works.
13+
* Database host management will now properly display an error message to the user when there is any type of MySQL related
14+
error encountered during creation or update.
1315

1416
### Added
1517
* Server listing view now displays the total used disk space for each server.

‎app/Http/Controllers/Admin/DatabaseController.php‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace Pterodactyl\Http\Controllers\Admin;
44

5+
use Exception;
56
use PDOException;
67
use Illuminate\View\View;
78
use Pterodactyl\Models\DatabaseHost;
@@ -118,17 +119,22 @@ public function view(int $host): View
118119
* @param \Pterodactyl\Http\Requests\Admin\DatabaseHostFormRequest $request
119120
* @return \Illuminate\Http\RedirectResponse
120121
*
121-
* @throws \Pterodactyl\Exceptions\Model\DataValidationException
122-
* @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException
122+
* @throws \Throwable
123123
*/
124124
public function create(DatabaseHostFormRequest $request): RedirectResponse
125125
{
126126
try {
127127
$host = $this->creationService->handle($request->normalize());
128-
} catch (PDOException $ex) {
129-
$this->alert->danger($ex->getMessage())->flash();
130-
131-
return redirect()->route('admin.databases');
128+
} catch (Exception $exception) {
129+
if ($exception instanceof PDOException || $exception->getPrevious() instanceof PDOException) {
130+
$this->alert->danger(
131+
sprintf('There was an error while trying to connect to the host or while executing a query: "%s"', $exception->getMessage())
132+
)->flash();
133+
134+
redirect()->route('admin.databases')->withInput($request->validated());
135+
} else {
136+
throw $exception;
137+
}
132138
}
133139

134140
$this->alert->success('Successfully created a new database host on the system.')->flash();
@@ -143,8 +149,7 @@ public function create(DatabaseHostFormRequest $request): RedirectResponse
143149
* @param \Pterodactyl\Models\DatabaseHost $host
144150
* @return \Illuminate\Http\RedirectResponse
145151
*
146-
* @throws \Pterodactyl\Exceptions\Model\DataValidationException
147-
* @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException
152+
* @throws \Throwable
148153
*/
149154
public function update(DatabaseHostFormRequest $request, DatabaseHost $host): RedirectResponse
150155
{
@@ -153,9 +158,17 @@ public function update(DatabaseHostFormRequest $request, DatabaseHost $host): Re
153158
try {
154159
$this->updateService->handle($host->id, $request->normalize());
155160
$this->alert->success('Database host was updated successfully.')->flash();
156-
} catch (PDOException $ex) {
157-
$this->alert->danger($ex->getMessage())->flash();
158-
$redirect->withInput($request->normalize());
161+
} catch (Exception $exception) {
162+
// Catch any SQL related exceptions and display them back to the user, otherwise just
163+
// throw the exception like normal and move on with it.
164+
if ($exception instanceof PDOException || $exception->getPrevious() instanceof PDOException) {
165+
$this->alert->danger(
166+
sprintf('There was an error while trying to connect to the host or while executing a query: "%s"', $exception->getMessage())
167+
)->flash();
168+
$redirect->withInput($request->normalize());
169+
} else {
170+
throw $exception;
171+
}
159172
}
160173

161174
return $redirect;

‎app/Services/Databases/Hosts/HostCreationService.php‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -65,28 +65,26 @@ public function __construct(
6565
* @param array $data
6666
* @return \Pterodactyl\Models\DatabaseHost
6767
*
68-
* @throws \Pterodactyl\Exceptions\Model\DataValidationException
69-
* @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException
68+
* @throws \Throwable
7069
*/
7170
public function handle(array $data): DatabaseHost
7271
{
73-
$this->connection->beginTransaction();
72+
return $this->connection->transaction(function () use ($data) {
73+
$host = $this->repository->create([
74+
'password' => $this->encrypter->encrypt(array_get($data, 'password')),
75+
'name' => array_get($data, 'name'),
76+
'host' => array_get($data, 'host'),
77+
'port' => array_get($data, 'port'),
78+
'username' => array_get($data, 'username'),
79+
'max_databases' => null,
80+
'node_id' => array_get($data, 'node_id'),
81+
]);
7482

75-
$host = $this->repository->create([
76-
'password' => $this->encrypter->encrypt(array_get($data, 'password')),
77-
'name' => array_get($data, 'name'),
78-
'host' => array_get($data, 'host'),
79-
'port' => array_get($data, 'port'),
80-
'username' => array_get($data, 'username'),
81-
'max_databases' => null,
82-
'node_id' => array_get($data, 'node_id'),
83-
]);
83+
// Confirm access using the provided credentials before saving data.
84+
$this->dynamic->set('dynamic', $host);
85+
$this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual');
8486

85-
// Confirm access using the provided credentials before saving data.
86-
$this->dynamic->set('dynamic', $host);
87-
$this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual');
88-
$this->connection->commit();
89-
90-
return $host;
87+
return $host;
88+
});
9189
}
9290
}

‎app/Services/Databases/Hosts/HostUpdateService.php‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,9 @@ public function __construct(
7171
*
7272
* @param int $hostId
7373
* @param array $data
74-
* @return mixed
74+
* @return \Pterodactyl\Models\DatabaseHost
7575
*
76-
* @throws \Pterodactyl\Exceptions\Model\DataValidationException
77-
* @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException
76+
* @throws \Throwable
7877
*/
7978
public function handle(int $hostId, array $data): DatabaseHost
8079
{
@@ -84,13 +83,12 @@ public function handle(int $hostId, array $data): DatabaseHost
8483
unset($data['password']);
8584
}
8685

87-
$this->connection->beginTransaction();
88-
$host = $this->repository->update($hostId, $data);
86+
return $this->connection->transaction(function () use ($data, $hostId) {
87+
$host = $this->repository->update($hostId, $data);
88+
$this->dynamic->set('dynamic', $host);
89+
$this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual');
8990

90-
$this->dynamic->set('dynamic', $host);
91-
$this->databaseManager->connection('dynamic')->select('SELECT 1 FROM dual');
92-
$this->connection->commit();
93-
94-
return $host;
91+
return $host;
92+
});
9593
}
9694
}

‎tests/Unit/Services/Databases/Hosts/HostCreationServiceTest.php‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -60,18 +60,20 @@ public function testDatabaseHostIsCreated()
6060
{
6161
$model = factory(DatabaseHost::class)->make();
6262

63-
$this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull();
64-
$this->encrypter->shouldReceive('encrypt')->with('test123')->once()->andReturn('enc123');
65-
$this->repository->shouldReceive('create')->with(m::subset([
63+
$this->connection->expects('transaction')->with(m::on(function ($closure) {
64+
return ! is_null($closure());
65+
}))->andReturn($model);
66+
67+
$this->encrypter->expects('encrypt')->with('test123')->andReturn('enc123');
68+
$this->repository->expects('create')->with(m::subset([
6669
'password' => 'enc123',
6770
'username' => $model->username,
6871
'node_id' => $model->node_id,
69-
]))->once()->andReturn($model);
72+
]))->andReturn($model);
7073

71-
$this->dynamic->shouldReceive('set')->with('dynamic', $model)->once()->andReturnNull();
72-
$this->databaseManager->shouldReceive('connection')->with('dynamic')->once()->andReturnSelf();
73-
$this->databaseManager->shouldReceive('select')->with('SELECT 1 FROM dual')->once()->andReturnNull();
74-
$this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull();
74+
$this->dynamic->expects('set')->with('dynamic', $model)->andReturnNull();
75+
$this->databaseManager->expects('connection')->with('dynamic')->andReturnSelf();
76+
$this->databaseManager->expects('select')->with('SELECT 1 FROM dual')->andReturnNull();
7577

7678
$response = $this->getService()->handle([
7779
'password' => 'test123',

‎tests/Unit/Services/Databases/Hosts/HostUpdateServiceTest.php‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -60,14 +60,15 @@ public function testPasswordIsEncryptedWhenProvided()
6060
{
6161
$model = factory(DatabaseHost::class)->make();
6262

63-
$this->encrypter->shouldReceive('encrypt')->with('test123')->once()->andReturn('enc123');
64-
$this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull();
65-
$this->repository->shouldReceive('update')->with(1234, ['password' => 'enc123'])->once()->andReturn($model);
63+
$this->connection->expects('transaction')->with(m::on(function ($closure) {
64+
return ! is_null($closure());
65+
}))->andReturn($model);
6666

67-
$this->dynamic->shouldReceive('set')->with('dynamic', $model)->once()->andReturnNull();
68-
$this->databaseManager->shouldReceive('connection')->with('dynamic')->once()->andReturnSelf();
69-
$this->databaseManager->shouldReceive('select')->with('SELECT 1 FROM dual')->once()->andReturnNull();
70-
$this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull();
67+
$this->encrypter->expects('encrypt')->with('test123')->andReturn('enc123');
68+
$this->repository->expects('update')->with(1234, ['password' => 'enc123'])->andReturn($model);
69+
$this->dynamic->expects('set')->with('dynamic', $model)->andReturnNull();
70+
$this->databaseManager->expects('connection')->with('dynamic')->andReturnSelf();
71+
$this->databaseManager->expects('select')->with('SELECT 1 FROM dual')->andReturnNull();
7172

7273
$response = $this->getService()->handle(1234, ['password' => 'test123']);
7374
$this->assertNotEmpty($response);
@@ -81,13 +82,14 @@ public function testUpdateOccursWhenNoPasswordIsProvided()
8182
{
8283
$model = factory(DatabaseHost::class)->make();
8384

84-
$this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull();
85-
$this->repository->shouldReceive('update')->with(1234, ['username' => 'test'])->once()->andReturn($model);
85+
$this->connection->expects('transaction')->with(m::on(function ($closure) {
86+
return ! is_null($closure());
87+
}))->andReturn($model);
8688

87-
$this->dynamic->shouldReceive('set')->with('dynamic', $model)->once()->andReturnNull();
88-
$this->databaseManager->shouldReceive('connection')->with('dynamic')->once()->andReturnSelf();
89-
$this->databaseManager->shouldReceive('select')->with('SELECT 1 FROM dual')->once()->andReturnNull();
90-
$this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull();
89+
$this->repository->expects('update')->with(1234, ['username' => 'test'])->andReturn($model);
90+
$this->dynamic->expects('set')->with('dynamic', $model)->andReturnNull();
91+
$this->databaseManager->expects('connection')->with('dynamic')->andReturnSelf();
92+
$this->databaseManager->expects('select')->with('SELECT 1 FROM dual')->andReturnNull();
9193

9294
$response = $this->getService()->handle(1234, ['password' => '', 'username' => 'test']);
9395
$this->assertNotEmpty($response);

0 commit comments

Comments
 (0)