From e9272a75fa067742eed576581c2ac86ec51334c3 Mon Sep 17 00:00:00 2001 From: William Allen <16820599+williamjallen@users.noreply.github.com> Date: Thu, 17 Aug 2023 16:27:31 -0400 Subject: [PATCH] Add foreign key constraints for all projectids (#1655) Our database schema is currently a mess. With no foreign keys defined, it is easy to accidentally forget to update a relationship field properly when an entity is created or deleted. Virtually all modern relational databases support foreign key constraints, and there is no reason for us to not add them. In a first step towards #1442, this PR adds foreign key constraints to all of the table columns which reference a project's ID. This should be a relatively low risk operation for production systems, but it creates a bit of a headache when writing tests, because creating any model which references a project necessarily requires a project model to be created as well. --- .../include/Test/UseCase/TestUseCase.php | 2 + .../CDash/MultipleSubprojectsEmailTest.php | 19 +- .../tests/case/CDash/TestUseCaseTest.php | 26 +++ app/cdash/tests/test_buildmodel.php | 1 + app/cdash/tests/test_upgrade.php | 4 +- app/cdash/xml_handlers/testing_handler.php | 1 + ...2023_07_20_185727_project_foreign_keys.php | 199 ++++++++++++++++++ phpstan-baseline.neon | 20 -- .../Feature/MeasurementPositionMigration.php | 32 ++- 9 files changed, 267 insertions(+), 37 deletions(-) create mode 100644 database/migrations/2023_07_20_185727_project_foreign_keys.php diff --git a/app/cdash/include/Test/UseCase/TestUseCase.php b/app/cdash/include/Test/UseCase/TestUseCase.php index 8b229eeb13..fe334f84ce 100644 --- a/app/cdash/include/Test/UseCase/TestUseCase.php +++ b/app/cdash/include/Test/UseCase/TestUseCase.php @@ -211,6 +211,8 @@ public function createTest(array $properties) $properties['FullCommandLine'] = "{$properties['FullName']} --run-test ."; } + $properties['ProjectId'] = $this->projectId; + $this->setModel('Test', $properties); return $this; } diff --git a/app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php b/app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php index acd22b2aa2..e7e92bd092 100644 --- a/app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php +++ b/app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php @@ -25,17 +25,17 @@ use CDash\Messaging\Subscription\SubscriptionCollection; use CDash\Messaging\Subscription\UserSubscriptionBuilder; use CDash\Model\Label; -use CDash\Model\Project; use CDash\Model\Subscriber; use CDash\Test\CDashUseCaseTestCase; use CDash\Test\UseCase\UseCase; +use Illuminate\Support\Facades\DB; class MultipleSubprojectsEmailTest extends CDashUseCaseTestCase { private static $tz; private static $database; - private static $projectId = 2; + private int $projectid = -1; /** @var Database|PHPUnit_Framework_MockObject_MockObject $db */ private $db; @@ -43,9 +43,6 @@ class MultipleSubprojectsEmailTest extends CDashUseCaseTestCase /** @var ActionableBuildInterface $submission */ private $submission; - /** @var Project|PHPUnit_Framework_MockObject_MockObject $project */ - private $project; - /** @var UseCase $useCase */ private $useCase; @@ -105,6 +102,17 @@ public function setUp() : void $this->createApplication(); config(['app.url' => 'http://open.cdash.org']); + + $this->projectid = DB::table('project')->insertGetId([ + 'name' => 'TestProject1', + ]); + } + + public function tearDown(): void + { + DB::delete('DELETE FROM project WHERE id = ?', [$this->projectid]); + + parent::tearDown(); } /** @@ -159,6 +167,7 @@ private function getNotifications(array $subscribers) public function testMultipleSubprojectsTestSubmission() { $this->useCase = UseCase::createBuilder($this, UseCase::TEST) + ->setProjectId($this->projectid) ->createSite([ 'BuildName' => 'CTestTest-Linux-c++-Subprojects', 'BuildStamp' => '20160728-1932-Experimental', diff --git a/app/cdash/tests/case/CDash/TestUseCaseTest.php b/app/cdash/tests/case/CDash/TestUseCaseTest.php index 088cee16b5..1c746c0151 100644 --- a/app/cdash/tests/case/CDash/TestUseCaseTest.php +++ b/app/cdash/tests/case/CDash/TestUseCaseTest.php @@ -5,9 +5,29 @@ use CDash\Test\CDashUseCaseTestCase; use CDash\Test\UseCase\TestUseCase; use CDash\Test\UseCase\UseCase; +use Illuminate\Support\Facades\DB; class TestUseCaseTest extends CDashUseCaseTestCase { + private int $projectid = -1; + + public function setUp(): void + { + $this->createApplication(); + $this->projectid = DB::table('project')->insertGetId([ + 'name' => 'TestProject1', + ]); + + parent::setUp(); + } + + public function tearDown(): void + { + DB::delete('DELETE FROM project WHERE id = ?', [$this->projectid]); + + parent::tearDown(); + } + public function testUseCaseBuildsTestUseCase() { $sut = UseCase::createBuilder($this, UseCase::TEST); @@ -157,6 +177,7 @@ public function testTestUseCaseCreatesTestPassed() /** @var UseCase $sut */ $sut + ->setProjectId($this->projectid) ->createSite(['Name' => 'Site.Name']) ->createTestPassed('some.test.name'); @@ -181,6 +202,7 @@ public function testTestUseCaseCreatesTestFailed() /** @var UseCase $sut */ $sut + ->setProjectId($this->projectid) ->createSite(['Name' => 'Site.Name']) ->createTestFailed('some.test.name'); @@ -205,6 +227,7 @@ public function testTestUseCaseCreatesTestTimeout() /** @var UseCase $sut */ $sut + ->setProjectId($this->projectid) ->createSite(['Name' => 'Site.Name']) ->createTestTimedout('some.test.name'); @@ -229,6 +252,7 @@ public function testTestUseCaseCreatesTestNotRun() /** @var UseCase $sut */ $sut + ->setProjectId($this->projectid) ->createSite(['Name' => 'Site.Name']) ->createTestNotRun('some.test.name'); @@ -253,6 +277,7 @@ public function testTestUseCaseCreatesMultisubprojectTestXMLFile() /** @var TestUseCase $sut */ $sut + ->setProjectId($this->projectid) ->createSite([ 'Name' => 'Site.Name', 'BuildName' => 'SomeOS-SomeBuild', @@ -309,6 +334,7 @@ public function testUseCaseSetsPropertiesByTestName() /** @var TestUseCase $sut */ $sut = UseCase::createBuilder($this, UseCase::TEST); $sut + ->setProjectId($this->projectid) ->createSite([ 'Name' => 'elysium', 'BuildName' => 'test_timing', diff --git a/app/cdash/tests/test_buildmodel.php b/app/cdash/tests/test_buildmodel.php index 7f4fd6a66f..4f415cc1e0 100644 --- a/app/cdash/tests/test_buildmodel.php +++ b/app/cdash/tests/test_buildmodel.php @@ -307,6 +307,7 @@ public function testBuildModelGetsConfigures() public function testBuildModelAddBuild() { $build = new Build(); + $build->ProjectId = 1; $this->assertTrue($build->AddBuild()); $this->assertTrue($build->Id > 0); diff --git a/app/cdash/tests/test_upgrade.php b/app/cdash/tests/test_upgrade.php index 5c7c3c3344..72b0475fc4 100644 --- a/app/cdash/tests/test_upgrade.php +++ b/app/cdash/tests/test_upgrade.php @@ -480,8 +480,8 @@ public function testSiteConstraintUpgrade() if ($table_to_update === 'build') { // Handle unique constraint here. $insert_query = - "INSERT INTO $table_to_update (siteid, uuid) - VALUES ($dupe, '$dupe')"; + "INSERT INTO $table_to_update (projectid, siteid, uuid) + VALUES (1, $dupe, '$dupe')"; } elseif ($table_to_update === 'client_job') { $insert_query = "INSERT INTO $table_to_update (siteid, scheduleid, osid, cmakeid, compilerid) diff --git a/app/cdash/xml_handlers/testing_handler.php b/app/cdash/xml_handlers/testing_handler.php index 84124d6bd6..263050d061 100644 --- a/app/cdash/xml_handlers/testing_handler.php +++ b/app/cdash/xml_handlers/testing_handler.php @@ -171,6 +171,7 @@ public function endElement($parser, $name) if ($this->Labels) { $this->TestCreator->labels = $this->Labels; } + $this->TestCreator->projectid = $this->projectid; $this->TestCreator->create($build); } elseif ($name == 'LABEL' && $parent == 'LABELS') { if (!empty($this->TestSubProjectName)) { diff --git a/database/migrations/2023_07_20_185727_project_foreign_keys.php b/database/migrations/2023_07_20_185727_project_foreign_keys.php new file mode 100644 index 0000000000..e63a3168e5 --- /dev/null +++ b/database/migrations/2023_07_20_185727_project_foreign_keys.php @@ -0,0 +1,199 @@ +integer('projectid')->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to blockbuild table...' . PHP_EOL; + Schema::table('blockbuild', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to build table...' . PHP_EOL; + Schema::table('build', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to build_filters table...' . PHP_EOL; + Schema::table('build_filters', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to buildgroup table...' . PHP_EOL; + Schema::table('buildgroup', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to coveragefilepriority table...' . PHP_EOL; + Schema::table('coveragefilepriority', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to labelemail table...' . PHP_EOL; + Schema::table('labelemail', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to measurement table...' . PHP_EOL; + Schema::table('measurement', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to overview_components table...' . PHP_EOL; + Schema::table('overview_components', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to project2repositories table...' . PHP_EOL; + Schema::table('project2repositories', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to subproject table...' . PHP_EOL; + Schema::table('subproject', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to subprojectgroup table...' . PHP_EOL; + Schema::table('subprojectgroup', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to test table...' . PHP_EOL; + Schema::table('test', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to user2project table...' . PHP_EOL; + Schema::table('user2project', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to user2repository table...' . PHP_EOL; + Schema::table('user2repository', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + + echo 'Adding projectid foreign key to userstatistics table...' . PHP_EOL; + Schema::table('userstatistics', function (Blueprint $table) { + $table->integer('projectid')->nullable(false)->change(); + $table->foreign('projectid')->references('id')->on('project')->cascadeOnDelete(); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('authtoken', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('blockbuild', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('build', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('build_filters', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('buildgroup', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('coveragefilepriority', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('labelemail', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('measurement', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('overview_components', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('project2repositories', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('subproject', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('subprojectgroup', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('test', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('user2project', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('user2repository', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + + Schema::table('userstatistics', function (Blueprint $table) { + $table->integer('projectid')->change(); + $table->dropForeign(['projectid']); + }); + } +}; diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 71272e39cb..54dea9d3b5 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -19292,31 +19292,11 @@ parameters: count: 1 path: app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php - - - message: "#^Property MultipleSubprojectsEmailTest\\:\\:\\$project has unknown class PHPUnit_Framework_MockObject_MockObject as its type\\.$#" - count: 1 - path: app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php - - - - message: "#^Property MultipleSubprojectsEmailTest\\:\\:\\$project is unused\\.$#" - count: 1 - path: app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php - - - - message: "#^Property MultipleSubprojectsEmailTest\\:\\:\\$projectId has no type specified\\.$#" - count: 1 - path: app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php - - message: "#^Property MultipleSubprojectsEmailTest\\:\\:\\$tz has no type specified\\.$#" count: 1 path: app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php - - - message: "#^Static property MultipleSubprojectsEmailTest\\:\\:\\$projectId is never read, only written\\.$#" - count: 1 - path: app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php - - message: "#^Access to an undefined property NightlyTimeTest\\:\\:\\$Project\\.$#" count: 9 diff --git a/tests/Feature/MeasurementPositionMigration.php b/tests/Feature/MeasurementPositionMigration.php index ea6a42682a..84ba7f8f8a 100644 --- a/tests/Feature/MeasurementPositionMigration.php +++ b/tests/Feature/MeasurementPositionMigration.php @@ -2,6 +2,7 @@ namespace Tests\Feature; +use CDash\Model\Project; use Illuminate\Support\Facades\Artisan; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; @@ -28,9 +29,17 @@ public function testMeasurementPositionMigration() // Verify that worked. $this::assertFalse(Schema::hasColumn('measurement', 'position')); + $project1 = new Project(); + $project1->Name = 'testMeasurementPositionMigration1'; + $project1->Save(); + + $project2 = new Project(); + $project2->Name = 'testMeasurementPositionMigration2'; + $project2->Save(); + // Populate some data to migrate. $base_measurement = [ - 'projectid' => 1, + 'projectid' => $project1->Id, 'name' => 'a' ]; $measurement1 = $base_measurement; @@ -42,14 +51,14 @@ public function testMeasurementPositionMigration() $measurement3['name'] = 'b'; $measurement4 = $base_measurement; - $measurement4['projectid'] = '2'; + $measurement4['projectid'] = $project2->Id; $measurement5 = $base_measurement; - $measurement5['projectid'] = '2'; + $measurement5['projectid'] = $project2->Id; $measurement5['name'] = 'c'; $measurement6 = $base_measurement; - $measurement6['projectid'] = '2'; + $measurement6['projectid'] = $project2->Id; $measurement6['name'] = 'b'; DB::table('measurement')->insert( @@ -64,32 +73,32 @@ public function testMeasurementPositionMigration() $this::assertEquals(DB::table('measurement')->count(), 6); $expected_measurements = [ [ - 'projectid' => 1, + 'projectid' => $project1->Id, 'name' => 'a', 'position' => 1, ], [ - 'projectid' => 1, + 'projectid' => $project1->Id, 'name' => 'b', 'position' => 2, ], [ - 'projectid' => 1, + 'projectid' => $project1->Id, 'name' => 'c', 'position' => 3, ], [ - 'projectid' => 2, + 'projectid' => $project2->Id, 'name' => 'a', 'position' => 1, ], [ - 'projectid' => 2, + 'projectid' => $project2->Id, 'name' => 'b', 'position' => 2, ], [ - 'projectid' => 2, + 'projectid' => $project2->Id, 'name' => 'c', 'position' => 3, ], @@ -106,5 +115,8 @@ public function testMeasurementPositionMigration() Artisan::call('migrate:fresh', [ '--force' => true]); + + $project1->Delete(); + $project2->Delete(); } }