diff --git a/app/Http/Controllers/AdminController.php b/app/Http/Controllers/AdminController.php index 48145005b8..85042906fb 100644 --- a/app/Http/Controllers/AdminController.php +++ b/app/Http/Controllers/AdminController.php @@ -423,7 +423,6 @@ public function upgrade() self::delete_unused_rows('dailyupdatefile', 'dailyupdateid', 'dailyupdate'); self::delete_unused_rows('test2image', 'outputid', 'testoutput'); - self::delete_unused_rows('label2test', 'outputid', 'testoutput'); $xml .= add_XML_value('alert', 'Database cleanup complete.'); } diff --git a/app/Models/Label.php b/app/Models/Label.php index d388e84129..c0e77309de 100644 --- a/app/Models/Label.php +++ b/app/Models/Label.php @@ -4,6 +4,7 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Relations\BelongsToMany; /** * @property int $id @@ -25,4 +26,12 @@ class Label extends Model protected $casts = [ 'id' => 'integer', ]; + + /** + * @return BelongsToMany + */ + public function tests(): BelongsToMany + { + return $this->belongsToMany(Test::class, 'label2test', 'labelid', 'testid'); + } } diff --git a/app/Models/Test.php b/app/Models/Test.php index 5f63f6459d..d7c8b3a7e9 100644 --- a/app/Models/Test.php +++ b/app/Models/Test.php @@ -7,6 +7,7 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Database\Eloquent\Relations\BelongsToMany; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Support\Facades\Config; @@ -32,6 +33,10 @@ class Test extends Model public $timestamps = false; protected $table = 'build2test'; + + /** + * @deprecated 08/24/2024 This member variable is deprecated. Use the labels() Eloquent relationship instead. + */ protected $labels = null; // TODO: Put these in an enum somewhere @@ -94,6 +99,14 @@ public function testMeasurements(): HasMany return $this->hasMany(TestMeasurement::class, 'testid'); } + /** + * @return BelongsToMany<\App\Models\Label> + */ + public function labels(): BelongsToMany + { + return $this->belongsToMany(\App\Models\Label::class, 'label2test', 'testid', 'labelid'); + } + /** * Add a label to this buildtest. **/ @@ -107,20 +120,17 @@ public function addLabel(Label $label): void /** * Get the collection of labels for this buildtest. + * + * @deprecated 08/24/2024 The legacy Label class is deprecated. Use the labels() Eloquent relationship instead. **/ public function getLabels() { if (is_null($this->labels)) { $this->labels = collect(); - $testlabel_models = TestLabel::where([ - ['buildid', '=', $this->buildid], - ['outputid', '=', $this->outputid], - ])->get(); - foreach ($testlabel_models as $testlabel_model) { + foreach ($this->labels()->get() as $eloquent_label) { $label = new Label(); - $label->Id = $testlabel_model->labelid; - $text = $label->GetText(); - $this->labels->put($text, $label); + $label->Id = $eloquent_label->id; + $this->labels->put($eloquent_label->text, $label); } } return $this->labels; @@ -210,10 +220,10 @@ public static function marshal($data, $buildid, $projectid, $projectshowtesttime } } - if (config('database.default') == 'pgsql' && $marshaledData['buildtestid']) { - $buildtest = Test::where('id', '=', $data['buildtestid'])->first(); - if ($buildtest) { - $marshaledData['labels'] = $buildtest->getLabels()->keys()->all(); + if ($marshaledData['buildtestid'] ?? false) { + $test = Test::find((int) $data['buildtestid']); + if ($test !== null) { + $marshaledData['labels'] = $test->labels()->get(['text']); } } else { if (!empty($data['labels'])) { diff --git a/app/Models/TestLabel.php b/app/Models/TestLabel.php deleted file mode 100644 index 74566014d8..0000000000 --- a/app/Models/TestLabel.php +++ /dev/null @@ -1,17 +0,0 @@ -AddTest($buildtest); foreach ($this->labels as $label) { - $label->TestId = $outputid; - $label->TestBuildId = (int) $build->Id; + $label->Test = $buildtest; $label->Insert(); $buildtest->addLabel($label); } diff --git a/app/cdash/app/Controller/Api/Index.php b/app/cdash/app/Controller/Api/Index.php index 099061099d..bb977982c6 100644 --- a/app/cdash/app/Controller/Api/Index.php +++ b/app/cdash/app/Controller/Api/Index.php @@ -1015,10 +1015,7 @@ public function generateBuildResponseFromRow(array $build_array): array|false b2t.status, b2t.newstatus FROM build2test AS b2t - INNER JOIN label2test AS l2t ON ( - l2t.outputid=b2t.outputid - AND l2t.buildid=b2t.buildid - ) + INNER JOIN label2test AS l2t ON l2t.testid = b2t.id WHERE b2t.buildid = ? AND l2t.labelid IN $placeholders diff --git a/app/cdash/app/Controller/Api/QueryTests.php b/app/cdash/app/Controller/Api/QueryTests.php index b147bfe1b4..4417ca7c72 100644 --- a/app/cdash/app/Controller/Api/QueryTests.php +++ b/app/cdash/app/Controller/Api/QueryTests.php @@ -367,8 +367,7 @@ public function getResponse() label2test WHERE label.id=label2test.labelid - AND label2test.outputid=build2test.outputid - AND label2test.buildid=b.id + AND label2test.testid=build2test.id ) AS labelstring $output_select FROM build AS b diff --git a/app/cdash/app/Controller/Api/ViewTest.php b/app/cdash/app/Controller/Api/ViewTest.php index dee00441ba..89f8e79499 100644 --- a/app/cdash/app/Controller/Api/ViewTest.php +++ b/app/cdash/app/Controller/Api/ViewTest.php @@ -226,7 +226,7 @@ public function getResponse() $groupby_sql = ''; if ($this->project->DisplayLabels && config('database.default') != 'pgsql') { $labeljoin_sql = ' - LEFT JOIN label2test AS l2t ON (l2t.outputid=bt.outputid) + LEFT JOIN label2test AS l2t ON (l2t.testid=bt.id) LEFT JOIN label AS l ON (l.id=l2t.labelid)'; $label_sql = ", GROUP_CONCAT(DISTINCT l.text SEPARATOR ', ') AS labels"; $groupby_sql = ' GROUP BY bt.id'; @@ -392,7 +392,7 @@ public function getResponse() $numTimeFailed++; } - $labels_found = (config('database.default') != 'pgsql' && !empty($marshaledTest['labels'])); + $labels_found = !empty($marshaledTest['labels']); $marshaledTest['measurements'] = $test_measurements[$marshaledTest['buildtestid']]; if ($response['hasprocessors']) { diff --git a/app/cdash/app/Model/Build.php b/app/cdash/app/Model/Build.php index 64426c507c..6ce709ddad 100644 --- a/app/cdash/app/Model/Build.php +++ b/app/cdash/app/Model/Build.php @@ -1634,9 +1634,12 @@ public function GetLabels($labelarray = []): array|false if (empty($labelarray) || isset($labelarray['test']['errors'])) { $sql .= - ' OR label.id IN - (SELECT labelid AS id FROM label2test - WHERE label2test.buildid = :buildid)'; + ' OR label.id IN ( + SELECT labelid AS id + FROM label2test + INNER JOIN build2test ON build2test.id = label2test.testid + WHERE build2test.buildid = :buildid + )'; } if (empty($labelarray) || isset($labelarray['coverage']['errors'])) { $sql .= diff --git a/app/cdash/app/Model/Label.php b/app/cdash/app/Model/Label.php index beeb9e2f5e..eba836fc8b 100644 --- a/app/cdash/app/Model/Label.php +++ b/app/cdash/app/Model/Label.php @@ -15,6 +15,7 @@ =========================================================================*/ namespace CDash\Model; +use App\Models\Test; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use App\Models\Label as EloquentLabel; @@ -30,8 +31,7 @@ class Label public $CoverageFileId; public int $CoverageFileBuildId = 0; public $DynamicAnalysisId; - public $TestId; - public int $TestBuildId = 0; + public ?Test $Test = null; public function SetText(?string $text): void { @@ -135,7 +135,7 @@ public function Insert() $this->InsertAssociation('label2dynamicanalysis', 'dynamicanalysisid', intval($this->DynamicAnalysisId)); - $this->InsertAssociation('label2test', 'buildid', $this->TestBuildId, 'outputid', intval($this->TestId)); + $this->Test?->labels()->syncWithoutDetaching([$this->Id]); // TODO: Implement this: // diff --git a/app/cdash/app/Model/Project.php b/app/cdash/app/Model/Project.php index cd1fad560d..c658e96bd6 100644 --- a/app/cdash/app/Model/Project.php +++ b/app/cdash/app/Model/Project.php @@ -975,9 +975,10 @@ public function GetLabels($days): array|false AND build.starttime>? ) UNION ( SELECT labelid AS id - FROM label2test, build + FROM label2test, build, build2test WHERE - label2test.buildid=build.id + build2test.buildid=build.id + AND build2test.id=label2test.testid AND build.projectid=? AND build.starttime>? ) UNION ( diff --git a/app/cdash/include/filterdataFunctions.php b/app/cdash/include/filterdataFunctions.php index 6209a50f42..6971005e4a 100644 --- a/app/cdash/include/filterdataFunctions.php +++ b/app/cdash/include/filterdataFunctions.php @@ -427,7 +427,7 @@ public function getSqlField($field) break; case 'label': { - $sql_field = "(SELECT $this->TextConcat FROM label, label2test WHERE label2test.outputid = build2test.outputid AND label2test.labelid = label.id AND label2test.buildid = b.id)"; + $sql_field = "(SELECT $this->TextConcat FROM label, label2test WHERE label2test.testid = build2test.id AND label2test.labelid = label.id)"; } break; @@ -593,7 +593,7 @@ public function getSqlField($field) break; case 'label': { - $sql_field = "(SELECT $this->TextConcat FROM label, label2test WHERE label.id=label2test.labelid AND label2test.outputid=bt.outputid)"; + $sql_field = "(SELECT $this->TextConcat FROM label, label2test WHERE label.id=label2test.labelid AND label2test.testid=bt.id)"; } break; diff --git a/app/cdash/tests/test_multiplesubprojects.php b/app/cdash/tests/test_multiplesubprojects.php index 7402318504..f229eeb7df 100644 --- a/app/cdash/tests/test_multiplesubprojects.php +++ b/app/cdash/tests/test_multiplesubprojects.php @@ -539,12 +539,11 @@ public function testMultipleSubprojects() // Adding tests to ensure that labels associated with subprojects and tests were saved $sql = " - SELECT text + SELECT label.text FROM label2test - JOIN label - ON - id=labelid - WHERE buildid=:buildid; + INNER JOIN label ON label.id=label2test.labelid + INNER JOIN build2test ON build2test.id = label2test.testid + WHERE build2test.buildid=:buildid; "; $stmt = $pdo->prepare($sql); $stmt->bindParam(':buildid', $build['id'], PDO::PARAM_INT); diff --git a/app/cdash/tests/test_querytests.php b/app/cdash/tests/test_querytests.php index 9b04da769b..09e934f9a7 100644 --- a/app/cdash/tests/test_querytests.php +++ b/app/cdash/tests/test_querytests.php @@ -53,26 +53,23 @@ public function testQueryTests() // Make sure cases with more than one label are handled appropriately. $query_result = DB::select(" SELECT - t.id AS outputid, - l2t.buildid AS buildid + l2t.testid AS testid FROM label l, - label2test l2t, - testoutput t + label2test l2t WHERE l.id = l2t.labelid - AND l2t.outputid = t.id AND l.text = 'Claps' LIMIT 1 ")[0]; DB::insert("INSERT INTO label (text) VALUES ('TestLabel')"); $labelid = (int) DB::select("SELECT id FROM label WHERE text = 'TestLabel'")[0]->id; - DB::insert('INSERT INTO label2test (labelid, buildid, outputid) VALUES (?, ?, ?)', [$labelid, $query_result->buildid, $query_result->outputid]); + DB::insert('INSERT INTO label2test (labelid, testid) VALUES (?, ?)', [$labelid, $query_result->testid]); $this->get($this->url . '/api/v1/queryTests.php?project=Trilinos&filtercount=1&showfilters=1&field1=label&compare1=63&value1=Claps'); $content = $this->getBrowser()->getContent(); $jsonobj = json_decode($content, true); $this->assertEqual($jsonobj['builds'][0]['labels'], 'Claps, TestLabel'); - DB::insert('DELETE FROM label2test WHERE labelid = ? AND buildid = ? AND outputid = ?', [$labelid, $query_result->buildid, $query_result->outputid]); + DB::insert('DELETE FROM label2test WHERE labelid = ? AND testid = ?', [$labelid, $query_result->testid]); DB::delete("DELETE FROM label WHERE text = 'TestLabel'"); diff --git a/database/migrations/2024_08_24_160326_label2test_relationship_refactor.php b/database/migrations/2024_08_24_160326_label2test_relationship_refactor.php new file mode 100644 index 0000000000..2872fde9f4 --- /dev/null +++ b/database/migrations/2024_08_24_160326_label2test_relationship_refactor.php @@ -0,0 +1,102 @@ +dropPrimary(); + $table->dropUnique(['outputid', 'buildid', 'labelid']); + $table->unsignedInteger('testid') + ->nullable(); + }); + + if (config('database.default') === 'pgsql') { + DB::update(' + UPDATE label2test + SET testid = build2test.id + FROM build2test + WHERE + build2test.buildid = label2test.buildid + AND build2test.outputid = label2test.outputid + '); + } else { + DB::update(' + UPDATE label2test, build2test + SET label2test.testid = build2test.id + WHERE + build2test.buildid = label2test.buildid + AND build2test.outputid = label2test.outputid + '); + } + + $rows_deleted = DB::delete('DELETE FROM label2test WHERE testid IS NULL'); + if ($rows_deleted > 0) { + echo "Deleted $rows_deleted invalid rows from label2test."; + } + + Schema::table('label2test', function (Blueprint $table) { + $table->dropForeign(['buildid']); + $table->dropColumn(['buildid', 'outputid']); + $table->unsignedInteger('testid') + ->nullable(false) + ->change(); + $table->foreign('testid') + ->references('id') + ->on('build2test') + ->cascadeOnDelete(); + $table->foreign('labelid') + ->references('id') + ->on('label') + ->cascadeOnDelete(); + $table->unique(['labelid', 'testid']); + $table->unique(['testid', 'labelid']); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('label2test', function (Blueprint $table) { + $table->integer('buildid') + ->nullable(); + $table->bigInteger('outputid') + ->nullable(); + }); + + if (config('database.default') === 'pgsql') { + DB::update(' + UPDATE label2test + SET + buildid = build2test.buildid, + outputid = build2test.outputid + FROM build2test + WHERE build2test.id = label2test.testid + '); + } else { + DB::update(' + UPDATE label2test, build2test + SET + label2test.buildid = build2test.buildid, + label2test.outputid = build2test.outputid + WHERE build2test.id = label2test.testid + '); + } + + Schema::table('label2test', function (Blueprint $table) { + $table->dropForeign(['testid']); + $table->dropColumn('testid'); + $table->primary(['labelid', 'buildid', 'outputid']); + $table->dropForeign(['labelid']); + }); + } +}; diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 4c5abb4bc0..85380242d4 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -2910,6 +2910,14 @@ parameters: count: 2 path: app/Models/SubProject.php + - + message: """ + #^Access to deprecated property \\$labels of class App\\\\Models\\\\Test\\: + 08/24/2024 This member variable is deprecated\\. Use the labels\\(\\) Eloquent relationship instead\\.$# + """ + count: 3 + path: app/Models/Test.php + - message: "#^Construct empty\\(\\) is not allowed\\. Use more strict comparison\\.$#" count: 1 @@ -2917,7 +2925,7 @@ parameters: - message: "#^Loose comparison via \"\\=\\=\" is not allowed\\.$#" - count: 2 + count: 1 path: app/Models/Test.php - @@ -3005,11 +3013,6 @@ parameters: count: 1 path: app/Models/Test.php - - - message: "#^Only booleans are allowed in an if condition, App\\\\Models\\\\Test\\|null given\\.$#" - count: 1 - path: app/Models/Test.php - - message: "#^Property App\\\\Models\\\\Test\\:\\:\\$attributes type has no value type specified in iterable type array\\.$#" count: 1 @@ -5914,7 +5917,7 @@ parameters: - message: "#^Loose comparison via \"\\!\\=\" is not allowed\\.$#" - count: 8 + count: 7 path: app/cdash/app/Controller/Api/ViewTest.php - @@ -9174,11 +9177,6 @@ parameters: count: 1 path: app/cdash/app/Model/Label.php - - - message: "#^Property CDash\\\\Model\\\\Label\\:\\:\\$TestId has no type specified\\.$#" - count: 1 - path: app/cdash/app/Model/Label.php - - message: "#^Variable property access on mixed\\.$#" count: 2 @@ -11580,6 +11578,14 @@ parameters: count: 3 path: app/cdash/include/Messaging/Topic/TestFailureTopic.php + - + message: """ + #^Call to deprecated method getLabels\\(\\) of class App\\\\Models\\\\Test\\: + 08/24/2024 The legacy Label class is deprecated\\. Use the labels\\(\\) Eloquent relationship instead\\.$# + """ + count: 1 + path: app/cdash/include/Messaging/Topic/TestFailureTopic.php + - message: "#^Iterating over an object of an unknown class CDash\\\\Model\\\\TestCollection\\.$#" count: 3 diff --git a/tests/Feature/TestSchemaMigration.php b/tests/Feature/TestSchemaMigration.php index 36a5ab489f..667b261be0 100644 --- a/tests/Feature/TestSchemaMigration.php +++ b/tests/Feature/TestSchemaMigration.php @@ -20,6 +20,9 @@ public function testMigrationOfTestTables() '--force' => 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]);