From f88bd4431c93b693cb52cc463ab1ecd93cf84a84 Mon Sep 17 00:00:00 2001 From: Zack Galbreath Date: Wed, 20 Nov 2024 15:54:07 -0500 Subject: [PATCH] Use more foreign keys for cascading deletions (#2577) Use foreign keys to automatically delete records from the following tables: * buildfailure2argument * test2image * updatefile --- app/Utils/DatabaseCleanupUtils.php | 13 - app/cdash/tests/CMakeLists.txt | 5 +- ...4306_buildfailure2argument_foreign_key.php | 31 ++ ...24_11_20_144307_updatefile_foreign_key.php | 31 ++ ...24_11_20_144308_test2image_foreign_key.php | 31 ++ tests/Feature/TestSchemaMigration.php | 274 ------------------ 6 files changed, 94 insertions(+), 291 deletions(-) create mode 100644 database/migrations/2024_11_20_144306_buildfailure2argument_foreign_key.php create mode 100644 database/migrations/2024_11_20_144307_updatefile_foreign_key.php create mode 100644 database/migrations/2024_11_20_144308_test2image_foreign_key.php delete mode 100644 tests/Feature/TestSchemaMigration.php diff --git a/app/Utils/DatabaseCleanupUtils.php b/app/Utils/DatabaseCleanupUtils.php index 5026c7d7f..b12dc4d93 100644 --- a/app/Utils/DatabaseCleanupUtils.php +++ b/app/Utils/DatabaseCleanupUtils.php @@ -120,17 +120,6 @@ public static function removeBuild($buildid) : void $db = Database::getInstance(); $buildid_prepare_array = $db->createPreparedArray(count($buildids)); - // Remove the buildfailureargument - $buildfailureids = []; - $buildfailure = DB::select("SELECT id FROM buildfailure WHERE buildid IN $buildid_prepare_array", $buildids); - foreach ($buildfailure as $buildfailure_array) { - $buildfailureids[] = intval($buildfailure_array->id); - } - if (count($buildfailureids) > 0) { - $buildfailure_prepare_array = $db->createPreparedArray(count($buildfailureids)); - DB::delete("DELETE FROM buildfailure2argument WHERE buildfailureid IN $buildfailure_prepare_array", $buildfailureids); - } - // Delete buildfailuredetails that are only used by builds that are being // deleted. DB::delete(" @@ -242,7 +231,6 @@ public static function removeBuild($buildid) : void if (count($updateids) > 0) { $updateids_prepare_array = $db->createPreparedArray(count($updateids)); DB::delete("DELETE FROM buildupdate WHERE id IN $updateids_prepare_array", $updateids); - DB::delete("DELETE FROM updatefile WHERE updateid IN $updateids_prepare_array", $updateids); } // Delete tests and testoutputs that are not shared. @@ -303,7 +291,6 @@ public static function removeBuild($buildid) : void $imgids_prepare_array = $db->createPreparedArray(count($imgids)); DB::delete("DELETE FROM image WHERE id IN $imgids_prepare_array", $imgids); } - self::deleteRowsChunked('DELETE FROM test2image WHERE outputid IN ', $testoutputs_to_delete); } } diff --git a/app/cdash/tests/CMakeLists.txt b/app/cdash/tests/CMakeLists.txt index 627f0dc27..a9e349593 100644 --- a/app/cdash/tests/CMakeLists.txt +++ b/app/cdash/tests/CMakeLists.txt @@ -225,11 +225,8 @@ set_tests_properties(/Feature/GraphQL/CoverageTypeTest PROPERTIES DEPENDS /Featu add_laravel_test(/Feature/PurgeUnusedProjectsCommand) set_tests_properties(/Feature/PurgeUnusedProjectsCommand PROPERTIES DEPENDS "/Feature/GraphQL/TestTypeTest;/Feature/GraphQL/TestMeasurementTypeTest;/Feature/GraphQL/NoteTypeTest;/Feature/GraphQL/BuildMeasurementTypeTest;/Feature/GraphQL/CoverageTypeTest") -add_laravel_test(/Feature/TestSchemaMigration) -set_tests_properties(/Feature/TestSchemaMigration PROPERTIES DEPENDS /Feature/PurgeUnusedProjectsCommand) - add_laravel_test(/Feature/MeasurementPositionMigration) -set_tests_properties(/Feature/MeasurementPositionMigration PROPERTIES DEPENDS /Feature/TestSchemaMigration) +set_tests_properties(/Feature/MeasurementPositionMigration PROPERTIES DEPENDS /Feature/PurgeUnusedProjectsCommand) add_laravel_test(/Feature/RemoveMeasurementCheckboxesMigration) set_tests_properties(/Feature/RemoveMeasurementCheckboxesMigration PROPERTIES DEPENDS /Feature/MeasurementPositionMigration) diff --git a/database/migrations/2024_11_20_144306_buildfailure2argument_foreign_key.php b/database/migrations/2024_11_20_144306_buildfailure2argument_foreign_key.php new file mode 100644 index 000000000..777ba2256 --- /dev/null +++ b/database/migrations/2024_11_20_144306_buildfailure2argument_foreign_key.php @@ -0,0 +1,31 @@ +buildfailure(id)..."; + $num_deleted = DB::delete("DELETE FROM buildfailure2argument WHERE buildfailureid NOT IN (SELECT id FROM buildfailure)"); + echo $num_deleted . ' invalid rows deleted' . PHP_EOL; + Schema::table('buildfailure2argument', function (Blueprint $table) { + $table->foreign('buildfailureid')->references('id')->on('buildfailure')->cascadeOnDelete(); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('buildfailure2argument', function (Blueprint $table) { + $table->dropForeign(['buildfailureid']); + }); + } +}; diff --git a/database/migrations/2024_11_20_144307_updatefile_foreign_key.php b/database/migrations/2024_11_20_144307_updatefile_foreign_key.php new file mode 100644 index 000000000..78cb57d47 --- /dev/null +++ b/database/migrations/2024_11_20_144307_updatefile_foreign_key.php @@ -0,0 +1,31 @@ +buildupdate(id)..."; + $num_deleted = DB::delete("DELETE FROM updatefile WHERE updateid NOT IN (SELECT id FROM buildupdate)"); + echo $num_deleted . ' invalid rows deleted' . PHP_EOL; + Schema::table('updatefile', function (Blueprint $table) { + $table->foreign('updateid')->references('id')->on('buildupdate')->cascadeOnDelete(); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('updatefile', function (Blueprint $table) { + $table->dropForeign(['updateid']); + }); + } +}; diff --git a/database/migrations/2024_11_20_144308_test2image_foreign_key.php b/database/migrations/2024_11_20_144308_test2image_foreign_key.php new file mode 100644 index 000000000..93f6781db --- /dev/null +++ b/database/migrations/2024_11_20_144308_test2image_foreign_key.php @@ -0,0 +1,31 @@ +testoutput(id)..."; + $num_deleted = DB::delete("DELETE FROM test2image WHERE outputid NOT IN (SELECT id FROM testoutput)"); + echo $num_deleted . ' invalid rows deleted' . PHP_EOL; + Schema::table('test2image', function (Blueprint $table) { + $table->foreign('outputid')->references('id')->on('testoutput')->cascadeOnDelete(); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('test2image', function (Blueprint $table) { + $table->dropForeign(['outputid']); + }); + } +}; diff --git a/tests/Feature/TestSchemaMigration.php b/tests/Feature/TestSchemaMigration.php deleted file mode 100644 index 667b261be..000000000 --- a/tests/Feature/TestSchemaMigration.php +++ /dev/null @@ -1,274 +0,0 @@ - true]); - - // Rollback some migrations to drop the relevant tables. - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2024_08_24_160326_label2test_relationship_refactor.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2024_07_09_025240_find_test_measurements_by_testid.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2024_04_17_183212_remove_test_table.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2020_02_17_112005_reformat_test_data.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2020_02_17_111951_add_test_output_table.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2019_09_30_111055_create_build2test_table.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2019_09_30_111055_create_label2test_table.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2019_09_30_111055_create_test_table.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2019_09_30_111055_create_test2image_table.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2019_09_30_111055_create_testmeasurement_table.php', - '--force' => true]); - - // Make sure they're really gone. - $this::assertFalse(Schema::hasTable('build2test')); - $this::assertFalse(Schema::hasTable('label2test')); - $this::assertFalse(Schema::hasTable('test')); - $this::assertFalse(Schema::hasTable('test2image')); - $this::assertFalse(Schema::hasTable('testmeasurement')); - $this::assertFalse(Schema::hasTable('testoutput')); - - // Recreate testing tables with original schema using migrations. - Artisan::call('migrate', [ - '--path' => 'database/migrations/2019_09_30_111055_create_build2test_table.php', - '--force' => true]); - Artisan::call('migrate', [ - '--path' => 'database/migrations/2019_09_30_111055_create_label2test_table.php', - '--force' => true]); - Artisan::call('migrate', [ - '--path' => 'database/migrations/2019_09_30_111055_create_test_table.php', - '--force' => true]); - Artisan::call('migrate', [ - '--path' => 'database/migrations/2019_09_30_111055_create_test2image_table.php', - '--force' => true]); - Artisan::call('migrate', [ - '--path' => 'database/migrations/2019_09_30_111055_create_testmeasurement_table.php', - '--force' => true]); - - // Make sure they exist now. - $this::assertTrue(Schema::hasTable('build2test')); - $this::assertTrue(Schema::hasTable('label2test')); - $this::assertTrue(Schema::hasTable('test')); - $this::assertTrue(Schema::hasTable('test2image')); - $this::assertTrue(Schema::hasTable('testmeasurement')); - - // Populate some data. - $base_test = [ - 'projectid' => 1, - 'crc32' => 123, - 'name' => 'a test', - 'path' => '/tmp', - 'command' => 'ls', - 'details' => 'Completed', - 'output' => '0', - ]; - $test1 = $base_test; - - $test2 = $base_test; - $test2['projectid'] = 2; - - $test3 = $base_test; - $test3['crc32'] = 456; - $test3['output'] = '0 0 0'; - - $test4 = $base_test; - $test4['crc32'] = '789'; - $test4['name'] = 'another test'; - $test4['output'] = 'something else'; - - DB::table('test')->insert([$test1, $test2, $test3, $test4]); - - $base_buildtest = [ - 'buildid' => 1, - 'testid' => 1, - 'status' => 'passed', - 'time' => 1.23, - 'timemean' => 0.00, - 'timestd' => 0.00, - 'timestatus' => 0, - 'newstatus' => 1, - ]; - - $buildtest1 = $base_buildtest; - $buildtest2 = $base_buildtest; - $buildtest2['testid'] = 2; - $buildtest3 = $base_buildtest; - $buildtest3['testid'] = 3; - $buildtest4 = $base_buildtest; - $buildtest4['testid'] = 4; - DB::table('build2test')->insert([$buildtest1, $buildtest2, $buildtest3, $buildtest4]); - - $base_testlabel = [ - 'labelid' => 1, - 'buildid' => 1, - 'testid' => 1, - ]; - $testlabel1 = $base_testlabel; - $testlabel2 = $base_testlabel; - $testlabel2['testid'] = 2; - $testlabel3 = $base_testlabel; - $testlabel3['testid'] = 3; - $testlabel4 = $base_testlabel; - $testlabel4['testid'] = 4; - DB::table('label2test')->insert([$testlabel1, $testlabel2, $testlabel3, $testlabel4]); - - $base_testimage = [ - 'imgid' => 1, - 'testid' => 1, - 'role' => 'BaseImage', - ]; - $testimage1 = $base_testimage; - $testimage2 = $base_testimage; - $testimage2['testid'] = 2; - $testimage2['role'] = 'DifferenceImage'; - $testimage3 = $base_testimage; - $testimage3['testid'] = 3; - $testimage3['role'] = 'TestImage'; - $testimage4 = $base_testimage; - $testimage4['testid'] = 4; - DB::table('test2image')->insert([$testimage1, $testimage2, $testimage3, $testimage4]); - - $base_testmeasurement = [ - 'testid' => 1, - 'name' => 'WallTime', - 'type' => 'numeric/double', - 'value' => 0.1, - ]; - $testmeasurement1 = $base_testmeasurement; - $testmeasurement2 = $base_testmeasurement; - $testmeasurement2['testid'] = 2; - $testmeasurement3 = $base_testmeasurement; - $testmeasurement3['testid'] = 3; - $testmeasurement4 = $base_testmeasurement; - $testmeasurement4['testid'] = 4; - DB::table('testmeasurement')->insert([$testmeasurement1, $testmeasurement2, $testmeasurement3, $testmeasurement4]); - - // Run the migrations under test. - Artisan::call('migrate', [ - '--path' => 'database/migrations/2020_02_17_111951_add_test_output_table.php', - '--force' => true]); - Artisan::call('migrate', [ - '--path' => 'database/migrations/2020_02_17_112005_reformat_test_data.php', - '--force' => true]); - - // Verify results. - $this::assertEquals(3, DB::table('test')->count()); - $expected_tests = [ - [ - 'name' => 'a test', - 'projectid' => 1, - ], - [ - 'name' => 'a test', - 'projectid' => 2, - ], - [ - 'name' => 'another test', - 'projectid' => 1, - ], - ]; - foreach ($expected_tests as $expected_test) { - $this->assertDatabaseHas('test', $expected_test); - } - - $this::assertEquals(4, DB::table('testoutput')->count()); - $expected_testoutputs = [ - [ - 'crc32' => 123, - 'path' => '/tmp', - 'command' => 'ls', - 'output' => '0', - ], - [ - 'crc32' => 456, - 'path' => '/tmp', - 'command' => 'ls', - 'output' => '0 0 0', - ], - [ - 'crc32' => 789, - 'path' => '/tmp', - 'command' => 'ls', - 'output' => 'something else', - ], - ]; - foreach ($expected_testoutputs as $expected_testoutput) { - $this->assertDatabaseHas('testoutput', $expected_testoutput); - } - - foreach (['build2test', 'label2test', 'test2image', 'testmeasurement'] as $table) { - $this::assertEquals(4, DB::table($table)->count()); - $this->assertDatabaseHas($table, ['outputid' => 1]); - $this->assertDatabaseHas($table, ['outputid' => 2]); - $this->assertDatabaseHas($table, ['outputid' => 3]); - $this->assertDatabaseHas($table, ['outputid' => 4]); - } - - // Test rollback too. - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2020_02_17_112005_reformat_test_data.php', - '--force' => true]); - Artisan::call('migrate:rollback', [ - '--path' => 'database/migrations/2020_02_17_111951_add_test_output_table.php', - '--force' => true]); - $this::assertFalse(Schema::hasTable('testoutput')); - $this->assertDatabaseHas('test', $test1); - $this->assertDatabaseHas('test', $test2); - $this->assertDatabaseHas('test', $test3); - $this->assertDatabaseHas('test', $test4); - - $this->assertDatabaseHas('build2test', $buildtest1); - $this->assertDatabaseHas('build2test', $buildtest2); - $this->assertDatabaseHas('build2test', $buildtest3); - $this->assertDatabaseHas('build2test', $buildtest4); - - $this->assertDatabaseHas('label2test', $testlabel1); - $this->assertDatabaseHas('label2test', $testlabel2); - $this->assertDatabaseHas('label2test', $testlabel3); - $this->assertDatabaseHas('label2test', $testlabel4); - - $this->assertDatabaseHas('test2image', $testimage1); - $this->assertDatabaseHas('test2image', $testimage2); - $this->assertDatabaseHas('test2image', $testimage3); - $this->assertDatabaseHas('test2image', $testimage4); - - $this->assertDatabaseHas('testmeasurement', $testmeasurement1); - $this->assertDatabaseHas('testmeasurement', $testmeasurement2); - $this->assertDatabaseHas('testmeasurement', $testmeasurement3); - $this->assertDatabaseHas('testmeasurement', $testmeasurement4); - - Artisan::call('migrate:fresh', [ - '--force' => true]); - } -}