From 241f7d0125d778dfb21893e9900965aa58aa4aa6 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sat, 17 Feb 2018 13:37:53 -0600 Subject: [PATCH] Fix some data integrity issues --- CHANGELOG.md | 1 + app/Models/EggVariable.php | 2 +- .../Variables/VariableCreationService.php | 9 +++-- .../Eggs/Variables/VariableUpdateService.php | 12 ++++--- .../Variables/VariableCreationServiceTest.php | 34 ++++++++----------- .../Variables/VariableUpdateServiceTest.php | 20 +++++------ 6 files changed, 41 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd777f36b..6d9ff8e42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ This project follows [Semantic Versioning](http://semver.org) guidelines. * `[rc.2]` — Fix data integrity exception that could occur when an email containing non-username characters was passed. * `[rc.2]` — Fix data integrity exception occurring when no default value is provided for an egg variable. * `[rc.2]` — Fixes a bug that would cause non-editable variables on the front-end to throw a validation error. +* `[rc.2]` — Fixes a data integrity exception occurring when saving egg variables with no value. ### Added * Added ability to search the following API endpoints: list users, list servers, and list locations. diff --git a/app/Models/EggVariable.php b/app/Models/EggVariable.php index 44b074bd1..293b86c4b 100644 --- a/app/Models/EggVariable.php +++ b/app/Models/EggVariable.php @@ -65,7 +65,7 @@ class EggVariable extends Model implements CleansAttributes, ValidableContract protected static $dataIntegrityRules = [ 'egg_id' => 'exists:eggs,id', 'name' => 'string|between:1,255', - 'description' => 'nullable|string', + 'description' => 'string', 'env_variable' => 'regex:/^[\w]{1,255}$/|notIn:' . self::RESERVED_ENV_NAMES, 'default_value' => 'string', 'user_viewable' => 'boolean', diff --git a/app/Services/Eggs/Variables/VariableCreationService.php b/app/Services/Eggs/Variables/VariableCreationService.php index 47d6a57ab..35fc399a0 100644 --- a/app/Services/Eggs/Variables/VariableCreationService.php +++ b/app/Services/Eggs/Variables/VariableCreationService.php @@ -44,10 +44,15 @@ class VariableCreationService $options = array_get($data, 'options') ?? []; - return $this->repository->create(array_merge($data, [ + return $this->repository->create([ 'egg_id' => $egg, + 'name' => $data['name'] ?? '', + 'description' => $data['description'] ?? '', + 'env_variable' => $data['env_variable'] ?? '', + 'default_value' => $data['default_value'] ?? '', 'user_viewable' => in_array('user_viewable', $options), 'user_editable' => in_array('user_editable', $options), - ])); + 'rules' => $data['rules'] ?? '', + ]); } } diff --git a/app/Services/Eggs/Variables/VariableUpdateService.php b/app/Services/Eggs/Variables/VariableUpdateService.php index 7234b47fe..9960e78c9 100644 --- a/app/Services/Eggs/Variables/VariableUpdateService.php +++ b/app/Services/Eggs/Variables/VariableUpdateService.php @@ -46,7 +46,7 @@ class VariableUpdateService } $search = $this->repository->setColumns('id')->findCountWhere([ - ['env_variable', '=', array_get($data, 'env_variable')], + ['env_variable', '=', $data['env_variable']], ['egg_id', '=', $variable->egg_id], ['id', '!=', $variable->id], ]); @@ -60,10 +60,14 @@ class VariableUpdateService $options = array_get($data, 'options') ?? []; - return $this->repository->withoutFreshModel()->update($variable->id, array_merge($data, [ - 'default_value' => array_get($data, 'default_value') ?? '', + return $this->repository->withoutFreshModel()->update($variable->id, [ + 'name' => $data['name'] ?? '', + 'description' => $data['description'] ?? '', + 'env_variable' => $data['env_variable'] ?? '', + 'default_value' => $data['default_value'] ?? '', 'user_viewable' => in_array('user_viewable', $options), 'user_editable' => in_array('user_editable', $options), - ])); + 'rules' => $data['rules'] ?? '', + ]); } } diff --git a/tests/Unit/Services/Eggs/Variables/VariableCreationServiceTest.php b/tests/Unit/Services/Eggs/Variables/VariableCreationServiceTest.php index 968a76743..a34c5777a 100644 --- a/tests/Unit/Services/Eggs/Variables/VariableCreationServiceTest.php +++ b/tests/Unit/Services/Eggs/Variables/VariableCreationServiceTest.php @@ -37,13 +37,14 @@ class VariableCreationServiceTest extends TestCase */ public function testVariableIsCreatedAndStored() { - $data = ['env_variable' => 'TEST_VAR_123']; - $this->repository->shouldReceive('create')->with([ + $data = ['env_variable' => 'TEST_VAR_123', 'default_value' => 'test']; + $this->repository->shouldReceive('create')->with(m::subset([ 'egg_id' => 1, + 'default_value' => 'test', 'user_viewable' => false, 'user_editable' => false, 'env_variable' => 'TEST_VAR_123', - ])->once()->andReturn(new EggVariable); + ]))->once()->andReturn(new EggVariable); $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data)); } @@ -54,33 +55,31 @@ class VariableCreationServiceTest extends TestCase public function testOptionsPassedInArrayKeyAreParsedProperly() { $data = ['env_variable' => 'TEST_VAR_123', 'options' => ['user_viewable', 'user_editable']]; - $this->repository->shouldReceive('create')->with([ - 'egg_id' => 1, + $this->repository->shouldReceive('create')->with(m::subset([ + 'default_value' => '', 'user_viewable' => true, 'user_editable' => true, 'env_variable' => 'TEST_VAR_123', - 'options' => ['user_viewable', 'user_editable'], - ])->once()->andReturn(new EggVariable); + ]))->once()->andReturn(new EggVariable); $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data)); } /** * Test that an empty (null) value passed in the option key is handled - * properly as an array. + * properly as an array. Also tests the same case aganist the default_value. * * @see https://github.com/Pterodactyl/Panel/issues/841 + * @see https://github.com/Pterodactyl/Panel/issues/943 */ public function testNullOptionValueIsPassedAsArray() { - $data = ['env_variable' => 'TEST_VAR_123', 'options' => null]; - $this->repository->shouldReceive('create')->with([ - 'egg_id' => 1, + $data = ['env_variable' => 'TEST_VAR_123', 'options' => null, 'default_value' => null]; + $this->repository->shouldReceive('create')->with(m::subset([ + 'default_value' => '', 'user_viewable' => false, 'user_editable' => false, - 'env_variable' => 'TEST_VAR_123', - 'options' => null, - ])->once()->andReturn(new EggVariable); + ]))->once()->andReturn(new EggVariable); $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data)); } @@ -103,12 +102,9 @@ class VariableCreationServiceTest extends TestCase public function testEggIdPassedInDataIsNotApplied() { $data = ['egg_id' => 123456, 'env_variable' => 'TEST_VAR_123']; - $this->repository->shouldReceive('create')->with([ + $this->repository->shouldReceive('create')->with(m::subset([ 'egg_id' => 1, - 'user_viewable' => false, - 'user_editable' => false, - 'env_variable' => 'TEST_VAR_123', - ])->once()->andReturn(new EggVariable); + ]))->once()->andReturn(new EggVariable); $this->assertInstanceOf(EggVariable::class, $this->service->handle(1, $data)); } diff --git a/tests/Unit/Services/Eggs/Variables/VariableUpdateServiceTest.php b/tests/Unit/Services/Eggs/Variables/VariableUpdateServiceTest.php index 52201d6bb..b6d7456d0 100644 --- a/tests/Unit/Services/Eggs/Variables/VariableUpdateServiceTest.php +++ b/tests/Unit/Services/Eggs/Variables/VariableUpdateServiceTest.php @@ -49,10 +49,9 @@ class VariableUpdateServiceTest extends TestCase ->shouldReceive('update')->with($this->model->id, m::subset([ 'user_viewable' => false, 'user_editable' => false, - 'test-data' => 'test-value', ]))->once()->andReturn(true); - $this->assertTrue($this->service->handle($this->model, ['test-data' => 'test-value'])); + $this->assertTrue($this->service->handle($this->model, [])); } /** @@ -62,12 +61,11 @@ class VariableUpdateServiceTest extends TestCase */ public function testNullDefaultValue() { - $this->repository->shouldReceive('withoutFreshModel')->withNoArgs()->once()->andReturnSelf() - ->shouldReceive('update')->with($this->model->id, [ - 'user_viewable' => false, - 'user_editable' => false, - 'default_value' => '', - ])->once()->andReturn(true); + $this->repository->shouldReceive('withoutFreshModel->update')->with($this->model->id, m::subset([ + 'user_viewable' => false, + 'user_editable' => false, + 'default_value' => '', + ]))->once()->andReturn(true); $this->assertTrue($this->service->handle($this->model, ['default_value' => null])); } @@ -96,7 +94,7 @@ class VariableUpdateServiceTest extends TestCase /** * Test that an empty (null) value passed in the option key is handled - * properly as an array. + * properly as an array. Also tests that a null description is handled. * * @see https://github.com/Pterodactyl/Panel/issues/841 */ @@ -106,10 +104,10 @@ class VariableUpdateServiceTest extends TestCase ->shouldReceive('update')->with($this->model->id, m::subset([ 'user_viewable' => false, 'user_editable' => false, - 'options' => null, + 'description' => '', ]))->once()->andReturn(true); - $this->assertTrue($this->service->handle($this->model, ['options' => null])); + $this->assertTrue($this->service->handle($this->model, ['options' => null, 'description' => null])); } /**