Fix buggy site information deduplication behavior (#2711)

Site information is currently mistakenly saved on every submission, regardless of whether any information actually changed. This PR fixes the issue and adds tests to prevent it from occurring again.
Kitware · Feb 14, 2025 · 5f0a637 · 5f0a637
1 parent 3e3798d
commit 5f0a637
Show file tree

Hide file tree

Showing 17 changed files with 283 additions and 71 deletions.
diff --git a/app/Http/Submission/Traits/UpdatesSiteInformation.php b/app/Http/Submission/Traits/UpdatesSiteInformation.php
@@ -0,0 +1,40 @@
+<?php
+
+namespace App\Http\Submission\Traits;
+
+use App\Models\Site;
+use App\Models\SiteInformation;
+
+trait UpdatesSiteInformation
+{
+    /**
+     * Saves a new site information record if $newInformation is different from the most recent
+     * information recorded for the site.
+     */
+    protected function updateSiteInfoIfChanged(Site $site, SiteInformation $newInformation): void
+    {
+        if ($site->information()->count() === 0) {
+            // No existing information, so save whatever we're given, regardless of whether it's all null.
+            $site->information()->save($newInformation);
+            return;
+        }
+
+        $all_attributes_null = true;
+        foreach ($newInformation->getFillable() as $attribute) {
+            $all_attributes_null = $all_attributes_null && $newInformation->getAttribute($attribute) === null;
+        }
+
+        if ($all_attributes_null) {
+            // Don't save information which is all null unless this is the first information we've received.
+            return;
+        }
+
+        foreach ($newInformation->getFillable() as $attribute) {
+            if ($site->mostRecentInformation?->getAttribute($attribute) !== $newInformation->getAttribute($attribute)) {
+                // Something changed, so we can go ahead and save the new information.
+                $site->information()->save($newInformation);
+                return;
+            }
+        }
+    }
+}
diff --git a/app/Models/SiteInformation.php b/app/Models/SiteInformation.php
@@ -33,7 +33,6 @@ class SiteInformation extends Model
     public $timestamps = false;
 
     protected $fillable = [
-        'timestamp',
         'processoris64bits',
         'processorvendor',
         'processorvendorid',

diff --git a/app/cdash/tests/CMakeLists.txt b/app/cdash/tests/CMakeLists.txt
@@ -278,6 +278,9 @@ set_tests_properties(/Feature/Mail/AuthTokenExpiredTest PROPERTIES DEPENDS /CDas
 add_laravel_test(/Feature/GraphQL/Mutations/UpdateSiteDescriptionTest)
 set_tests_properties(/Feature/GraphQL/Mutations/UpdateSiteDescriptionTest PROPERTIES DEPENDS /CDash/XmlHandler/UpdateHandler)
 
+add_laravel_test(/Feature/Traits/UpdatesSiteInformationTest)
+set_tests_properties(/Feature/Traits/UpdatesSiteInformationTest PROPERTIES DEPENDS /CDash/XmlHandler/UpdateHandler)
+
 ###################################################################################################
 
 cdash_install()
@@ -317,6 +320,7 @@ set_property(TEST install_3 PROPERTY DEPENDS
   /Feature/Mail/AuthTokenExpiringTest
   /Feature/Mail/AuthTokenExpiredTest
   /Feature/GraphQL/Mutations/UpdateSiteDescriptionTest
+  /Feature/Traits/UpdatesSiteInformationTest
 )
 
 

diff --git a/app/cdash/tests/case/CDash/TestUseCaseTest.php b/app/cdash/tests/case/CDash/TestUseCaseTest.php
@@ -103,7 +103,7 @@ public function testTestUseCaseCreatesBuildAndSiteInformation()
         $builds = $handler->GetBuildCollection();
         /** @var Build $build */
         $build = $builds->current();
-        $information = $build->GetSite()->mostRecentInformation;
+        $information = $build->GetSite()?->refresh()->mostRecentInformation;
 
         $this->assertEquals($siteInformation['Description'], $information?->description);
         $this->assertEquals($siteInformation['Is64Bits'], $information?->processoris64bits);

diff --git a/app/cdash/xml_handlers/build_handler.php b/app/cdash/xml_handlers/build_handler.php
@@ -15,6 +15,7 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\SubmissionUtils;
@@ -38,6 +39,7 @@
 class BuildHandler extends AbstractXmlHandler implements ActionableBuildInterface, CommitAuthorHandlerInterface
 {
     use CommitAuthorHandlerTrait;
+    use UpdatesSiteInformation;
 
     private $StartTimeStamp;
     private $EndTimeStamp;
@@ -124,7 +126,7 @@ public function startElement($parser, $name, $attributes): void
                 $this->BuildName = '(empty)';
             }
 
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
         } elseif ($name == 'SUBPROJECT') {
             $this->SubProjectName = $attributes['NAME'];
             if (!array_key_exists($this->SubProjectName, $this->SubProjects)) {

diff --git a/app/cdash/xml_handlers/configure_handler.php b/app/cdash/xml_handlers/configure_handler.php
@@ -15,6 +15,7 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\SubmissionUtils;
@@ -33,6 +34,8 @@
 
 class ConfigureHandler extends AbstractXmlHandler implements ActionableBuildInterface
 {
+    use UpdatesSiteInformation;
+
     private $StartTimeStamp;
     private $EndTimeStamp;
     private $Builds;
@@ -117,7 +120,7 @@ public function startElement($parser, $name, $attributes): void
                 $this->BuildName = '(empty)';
             }
 
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
         } elseif ($name == 'SUBPROJECT') {
             $this->SubProjectName = $attributes['NAME'];
             if (!array_key_exists($this->SubProjectName, $this->SubProjects)) {

diff --git a/app/cdash/xml_handlers/coverage_handler.php b/app/cdash/xml_handlers/coverage_handler.php
@@ -15,6 +15,7 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\SubmissionUtils;
@@ -28,6 +29,8 @@
 
 class CoverageHandler extends AbstractXmlHandler
 {
+    use UpdatesSiteInformation;
+
     private $StartTimeStamp;
     private $EndTimeStamp;
 
@@ -83,7 +86,7 @@ public function startElement($parser, $name, $attributes): void
                 }
             }
 
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
 
             $this->Build->SiteId = $this->Site->id;
             $this->Build->Name = $attributes['BUILDNAME'];

diff --git a/app/cdash/xml_handlers/coverage_junit_handler.php b/app/cdash/xml_handlers/coverage_junit_handler.php
@@ -15,10 +15,10 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\SubmissionUtils;
-use CDash\Model\Build;
 use CDash\Model\Coverage;
 use CDash\Model\CoverageFile;
 use CDash\Model\CoverageSummary;
@@ -27,6 +27,8 @@
 
 class CoverageJUnitHandler extends AbstractXmlHandler
 {
+    use UpdatesSiteInformation;
+
     private $StartTimeStamp;
     private $EndTimeStamp;
 
@@ -79,7 +81,7 @@ public function startElement($parser, $name, $attributes): void
                 }
             }
 
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
 
             $this->Build->SiteId = $this->Site->id;
             $this->Build->Name = $attributes['BUILDNAME'];

diff --git a/app/cdash/xml_handlers/dynamic_analysis_handler.php b/app/cdash/xml_handlers/dynamic_analysis_handler.php
@@ -15,6 +15,7 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\SubmissionUtils;
@@ -35,6 +36,8 @@
 
 class DynamicAnalysisHandler extends AbstractXmlHandler implements ActionableBuildInterface
 {
+    use UpdatesSiteInformation;
+
     private $StartTimeStamp;
     private $EndTimeStamp;
     private $Checker;
@@ -112,7 +115,7 @@ public function startElement($parser, $name, $attributes): void
             if (empty($this->BuildName)) {
                 $this->BuildName = '(empty)';
             }
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
         } elseif ($name == 'SUBPROJECT') {
             $this->SubProjectName = $attributes['NAME'];
             if (!array_key_exists($this->SubProjectName, $this->SubProjects)) {

diff --git a/app/cdash/xml_handlers/note_handler.php b/app/cdash/xml_handlers/note_handler.php
@@ -15,15 +15,17 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\NoteCreator;
 use App\Utils\SubmissionUtils;
-use CDash\Model\Build;
 use CDash\Model\Project;
 
 class NoteHandler extends AbstractXmlHandler
 {
+    use UpdatesSiteInformation;
+
     private $AdjustStartTime;
     private $NoteCreator;
     protected static ?string $schema_file = '/app/Validators/Schemas/Notes.xsd';
@@ -73,7 +75,7 @@ public function startElement($parser, $name, $attributes): void
                 }
             }
 
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
 
             $this->Build->SiteId = $this->Site->id;
             $this->Build->Name = $attributes['BUILDNAME'];

diff --git a/app/cdash/xml_handlers/testing_handler.php b/app/cdash/xml_handlers/testing_handler.php
@@ -1,5 +1,6 @@
 <?php
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Models\TestMeasurement;
@@ -25,6 +26,7 @@
 class TestingHandler extends AbstractXmlHandler implements ActionableBuildInterface, CommitAuthorHandlerInterface
 {
     use CommitAuthorHandlerTrait;
+    use UpdatesSiteInformation;
 
     protected static ?string $schema_file = '/app/Validators/Schemas/Test.xsd';
     private $StartTimeStamp;
@@ -126,7 +128,7 @@ public function startElement($parser, $name, $attributes): void
             if (empty($this->BuildName)) {
                 $this->BuildName = '(empty)';
             }
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
         } elseif ($name == 'SUBPROJECT') {
             $this->SubProjectName = $attributes['NAME'];
             if (!array_key_exists($this->SubProjectName, $this->SubProjects)) {

diff --git a/app/cdash/xml_handlers/testing_j_unit_handler.php b/app/cdash/xml_handlers/testing_j_unit_handler.php
@@ -15,15 +15,17 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\SubmissionUtils;
 use App\Utils\TestCreator;
-use CDash\Model\Build;
 use CDash\Model\Project;
 
 class TestingJUnitHandler extends AbstractXmlHandler
 {
+    use UpdatesSiteInformation;
+
     private $StartTimeStamp;
     private $EndTimeStamp;
     // Should we update the end time of the build?
@@ -100,7 +102,7 @@ public function startElement($parser, $name, $attributes): void
                 }
             }
 
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
 
             $this->Build->SiteId = $this->Site->id;
             $this->Build->Name = $attributes['BUILDNAME'];

diff --git a/app/cdash/xml_handlers/upload_handler.php b/app/cdash/xml_handlers/upload_handler.php
@@ -15,10 +15,10 @@
   PURPOSE. See the above copyright notices for more information.
 =========================================================================*/
 
+use App\Http\Submission\Traits\UpdatesSiteInformation;
 use App\Models\Site;
 use App\Models\SiteInformation;
 use App\Utils\SubmissionUtils;
-use CDash\Model\Build;
 use CDash\Model\Label;
 use CDash\Model\Project;
 use CDash\Model\UploadFile;
@@ -43,6 +43,8 @@
  */
 class UploadHandler extends AbstractXmlHandler
 {
+    use UpdatesSiteInformation;
+
     private $UploadFile;
     private $TmpFilename;
     private $Base64TmpFileWriteHandle;
@@ -108,7 +110,7 @@ public function startElement($parser, $name, $attributes): void
                 }
             }
 
-            $this->Site->mostRecentInformation()->save($siteInformation);
+            $this->updateSiteInfoIfChanged($this->Site, $siteInformation);
 
             $this->Build->SiteId = $this->Site->id;
             $this->Build->Name = $attributes['BUILDNAME'];