Skip to content

Commit

Permalink
Add foreign key constraints for all projectids (#1655)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
williamjallen authored Aug 17, 2023
1 parent 1b508b7 commit e9272a7
Show file tree
Hide file tree
Showing 9 changed files with 267 additions and 37 deletions.
2 changes: 2 additions & 0 deletions app/cdash/include/Test/UseCase/TestUseCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
19 changes: 14 additions & 5 deletions app/cdash/tests/case/CDash/MultipleSubprojectsEmailTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,24 @@
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;

/** @var ActionableBuildInterface $submission */
private $submission;

/** @var Project|PHPUnit_Framework_MockObject_MockObject $project */
private $project;

/** @var UseCase $useCase */
private $useCase;

Expand Down Expand Up @@ -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();
}

/**
Expand Down Expand Up @@ -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',
Expand Down
26 changes: 26 additions & 0 deletions app/cdash/tests/case/CDash/TestUseCaseTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -157,6 +177,7 @@ public function testTestUseCaseCreatesTestPassed()

/** @var UseCase $sut */
$sut
->setProjectId($this->projectid)
->createSite(['Name' => 'Site.Name'])
->createTestPassed('some.test.name');

Expand All @@ -181,6 +202,7 @@ public function testTestUseCaseCreatesTestFailed()

/** @var UseCase $sut */
$sut
->setProjectId($this->projectid)
->createSite(['Name' => 'Site.Name'])
->createTestFailed('some.test.name');

Expand All @@ -205,6 +227,7 @@ public function testTestUseCaseCreatesTestTimeout()

/** @var UseCase $sut */
$sut
->setProjectId($this->projectid)
->createSite(['Name' => 'Site.Name'])
->createTestTimedout('some.test.name');

Expand All @@ -229,6 +252,7 @@ public function testTestUseCaseCreatesTestNotRun()

/** @var UseCase $sut */
$sut
->setProjectId($this->projectid)
->createSite(['Name' => 'Site.Name'])
->createTestNotRun('some.test.name');

Expand All @@ -253,6 +277,7 @@ public function testTestUseCaseCreatesMultisubprojectTestXMLFile()

/** @var TestUseCase $sut */
$sut
->setProjectId($this->projectid)
->createSite([
'Name' => 'Site.Name',
'BuildName' => 'SomeOS-SomeBuild',
Expand Down Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions app/cdash/tests/test_buildmodel.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 2 additions & 2 deletions app/cdash/tests/test_upgrade.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions app/cdash/xml_handlers/testing_handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
199 changes: 199 additions & 0 deletions database/migrations/2023_07_20_185727_project_foreign_keys.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration {
/**
* Run the migrations.
*
* @return void
*/
public function up()
{
echo 'Adding projectid foreign key to authtoken table...' . PHP_EOL;
Schema::table('authtoken', function (Blueprint $table) {
$table->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']);
});
}
};
Loading

0 comments on commit e9272a7

Please sign in to comment.