Clean up submission handler public interface (#2572)

This PR accomplishes two main goals: formalizing the interface exposed by the submission handler constructor, and hiding implementation details about how child builds are handled. Getting the parent build for a given submission is now possible via the `getBuild()` public method, rather than the previous convoluted logic necessary to parse the parent build from a set containing both parent and child builds.
Kitware · Nov 21, 2024 · be99b14 · be99b14
1 parent ec97bbe
commit be99b14
Show file tree

Hide file tree

Showing 39 changed files with 235 additions and 480 deletions.
diff --git a/app/Jobs/ProcessSubmission.php b/app/Jobs/ProcessSubmission.php
@@ -3,6 +3,7 @@
 namespace App\Jobs;
 
 use AbstractSubmissionHandler;
+use ActionableBuildInterface;
 use App\Utils\UnparsedSubmissionProcessor;
 use App\Models\SuccessfulJob;
 
@@ -141,8 +142,7 @@ public function handle()
 
         // Resubmit the file if necessary.
         if (is_a($handler, 'DoneHandler') && $handler->shouldRequeue()) {
-            $build = $this->getBuildFromHandler($handler);
-            $this->requeueSubmissionFile($build->Id);
+            $this->requeueSubmissionFile($handler->getBuild()->Id);
         }
 
         if ((int) config('cdash.backup_timeframe') === 0) {
@@ -171,7 +171,7 @@ public function handle()
     /**
      * The job failed to process.
      */
-    public function failed(\Throwable $exception) : void
+    public function failed(\Throwable $exception): void
     {
         Log::warning("Failed to process {$this->filename} with message: {$exception}");
         $this->renameSubmissionFile("inprogress/{$this->filename}", "failed/{$this->filename}");
@@ -185,8 +185,7 @@ public function failed(\Throwable $exception) : void
      * This method could be running on a worker that is either remote or local, so it accepts
      * a file handle or a filename that it can query the CDash API for.
      **/
-    private function doSubmit($filename, $projectid, $buildid = null,
-        $expected_md5 = '')
+    private function doSubmit($filename, $projectid, $buildid = null, $expected_md5 = ''): AbstractSubmissionHandler|UnparsedSubmissionProcessor|false
     {
         $filehandle = $this->getSubmissionFileHandle($filename);
         if ($filehandle === false) {
@@ -215,13 +214,11 @@ private function doSubmit($filename, $projectid, $buildid = null,
             return false;
         }
 
-        $build = $this->getBuildFromHandler($handler);
-        if (!is_null($build)) {
-            $pendingSubmissions = new PendingSubmissions();
-            $pendingSubmissions->Build = $build;
-            if ($pendingSubmissions->Exists()) {
-                $pendingSubmissions->Decrement();
-            }
+        $build = $handler->getBuild();
+        $pendingSubmissions = new PendingSubmissions();
+        $pendingSubmissions->Build = $build;
+        if ($pendingSubmissions->Exists()) {
+            $pendingSubmissions->Decrement();
         }
 
         // Set status on repository.
@@ -237,12 +234,7 @@ private function doSubmit($filename, $projectid, $buildid = null,
         }
 
         // Send more general build emails.
-        if (is_a($handler, 'TestingHandler') ||
-            is_a($handler, 'BuildHandler') ||
-            is_a($handler, 'ConfigureHandler') ||
-            is_a($handler, 'DynamicAnalysisHandler') ||
-            is_a($handler, 'UpdateHandler')
-        ) {
+        if ($handler instanceof ActionableBuildInterface) {
             sendemail($handler, intval($projectid));
         }
 
@@ -292,19 +284,4 @@ private function getSubmissionFileHandle($filename)
             return false;
         }
     }
-
-    private function getBuildFromHandler(AbstractSubmissionHandler $handler)
-    {
-        $build = null;
-        $builds = $handler->getBuilds();
-        if (count($builds) > 1) {
-            // More than one build referenced by the handler.
-            // Return the parent build.
-            $build = new Build();
-            $build->Id = $builds[0]->GetParentId();
-        } elseif (count($builds) === 1 && $builds[0] instanceof Build) {
-            $build = $builds[0];
-        }
-        return $build;
-    }
 }
diff --git a/app/cdash/include/Test/UseCase/BuildUseCase.php b/app/cdash/include/Test/UseCase/BuildUseCase.php
@@ -1,6 +1,7 @@
 <?php
 namespace CDash\Test\UseCase;
 
+use CDash\Model\Project;
 use DOMDocument;
 use DOMElement;
 use DOMText;
@@ -51,7 +52,10 @@ public function build(): AbstractXmlHandler
         if ($xml_str === false) {
             throw new \Exception('Invalid XML.');
         }
-        $handler = new BuildHandler($this->projectId);
+
+        $project = new Project();
+        $project->Id = $this->projectId;
+        $handler = new BuildHandler($project);
         return $this->getXmlHandler($handler, $xml_str);
     }
 

diff --git a/app/cdash/include/Test/UseCase/ConfigUseCase.php b/app/cdash/include/Test/UseCase/ConfigUseCase.php
@@ -1,6 +1,7 @@
 <?php
 namespace CDash\Test\UseCase;
 
+use CDash\Model\Project;
 use DOMDocument;
 use DOMElement;
 use DOMText;
@@ -62,7 +63,10 @@ public function build(): AbstractXmlHandler
         if ($xml_str === false) {
             throw new \Exception('Invalid XML.');
         }
-        $handler = new ConfigureHandler($this->projectId);
+
+        $project = new Project();
+        $project->Id = $this->projectId;
+        $handler = new ConfigureHandler($project);
         return $this->getXmlHandler($handler, $xml_str);
     }
 }
diff --git a/app/cdash/include/Test/UseCase/DynamicAnalysisUseCase.php b/app/cdash/include/Test/UseCase/DynamicAnalysisUseCase.php
@@ -16,6 +16,7 @@
 
 namespace CDash\Test\UseCase;
 
+use CDash\Model\Project;
 use DOMDocument;
 use DOMElement;
 use DOMNode;
@@ -77,7 +78,9 @@ public function build(): AbstractXmlHandler
         if ($xml_str === false) {
             throw new \Exception('Invalid XML.');
         }
-        $handler = new DynamicAnalysisHandler($this->projectId);
+        $project = new Project();
+        $project->Id = $this->projectId;
+        $handler = new DynamicAnalysisHandler($project);
         return $this->getXmlHandler($handler, $xml_str);
     }
 

diff --git a/app/cdash/include/Test/UseCase/TestUseCase.php b/app/cdash/include/Test/UseCase/TestUseCase.php
@@ -1,6 +1,7 @@
 <?php
 namespace CDash\Test\UseCase;
 
+use CDash\Model\Project;
 use DOMDocument;
 use DOMElement;
 use DOMText;
@@ -51,7 +52,9 @@ public function build(): AbstractXmlHandler
         if ($xml_str === false) {
             throw new \Exception('Invalid XML.');
         }
-        $handler = new TestingHandler($this->projectId);
+        $project = new Project();
+        $project->Id = $this->projectId;
+        $handler = new TestingHandler($project);
         return $this->getXmlHandler($handler, $xml_str);
     }
 

diff --git a/app/cdash/include/Test/UseCase/UpdateUseCase.php b/app/cdash/include/Test/UseCase/UpdateUseCase.php
@@ -2,6 +2,7 @@
 namespace CDash\Test\UseCase;
 
 use AbstractXmlHandler;
+use CDash\Model\Project;
 
 class UpdateUseCase extends UseCase
 {
@@ -86,7 +87,9 @@ public function build(): AbstractXmlHandler
         if ($xml_str === false) {
             throw new \Exception('Invalid XML.');
         }
-        $handler = new \UpdateHandler($this->projectId);
+        $project = new Project();
+        $project->Id = $this->projectId;
+        $handler = new \UpdateHandler($project);
         return $this->getXmlHandler($handler, $xml_str);
     }
 

diff --git a/app/cdash/include/ctestparser.php b/app/cdash/include/ctestparser.php
@@ -62,7 +62,7 @@ function generateBackupFileName($projectname, $subprojectname, $buildname,
 }
 
 /** Function to handle new style submissions via HTTP PUT */
-function parse_put_submission($filehandler, $projectid, $expected_md5, int|null $buildid)
+function parse_put_submission($filehandler, $projectid, $expected_md5, int|null $buildid): AbstractSubmissionHandler|false
 {
     $db = Database::getInstance();
 
@@ -118,7 +118,7 @@ function parse_put_submission($filehandler, $projectid, $expected_md5, int|null
     if (stream_resolve_include_path($include_file) === false || !in_array($buildfile->type, $valid_types, true)) {
         Log::error("Project: $projectid.  No handler include file for {$buildfile->type} (tried $include_file)");
         $buildfile->delete();
-        return true;
+        return false;
     }
     require_once $include_file;
 
@@ -127,9 +127,12 @@ function parse_put_submission($filehandler, $projectid, $expected_md5, int|null
     if (!class_exists($className)) {
         Log::error("Project: $projectid.  No handler class for {$buildfile->type}");
         $buildfile->delete();
-        return true;
+        return false;
     }
-    $handler = new $className($buildfile->buildid);
+
+    $build = new \CDash\Model\Build();
+    $build->Id = $buildfile->buildid;
+    $handler = new $className($build);
 
     // Parse the file.
     if (file_exists($filename)) {
@@ -152,12 +155,12 @@ function parse_put_submission($filehandler, $projectid, $expected_md5, int|null
 }
 
 /** Main function to parse the incoming xml from ctest */
-function ctest_parse($filehandle, $projectid, $expected_md5 = '', int|null $buildid=null)
+function ctest_parse($filehandle, $projectid, $expected_md5 = '', int|null $buildid=null): AbstractSubmissionHandler|false
 {
     // Check if this is a new style PUT submission.
     try {
         $handler = parse_put_submission($filehandle, $projectid, $expected_md5, $buildid);
-        if ($handler) {
+        if ($handler !== false) {
             return $handler;
         }
     } catch (CDashParseException $e) {
@@ -171,21 +174,22 @@ function ctest_parse($filehandle, $projectid, $expected_md5 = '', int|null $buil
         $ip = $_SERVER['REMOTE_ADDR'];
     }
 
+    $Project = new Project();
+    $Project->Id = $projectid;
+
     // Figure out what type of XML file this is.
     $xml_info = SubmissionUtils::get_xml_type($filehandle);
     $filehandle = $xml_info['file_handle'];
     $handler_ref = $xml_info['xml_handler'];
     $file = $xml_info['xml_type'];
 
-    $handler = isset($handler_ref) ? new $handler_ref($projectid) : null;
+    $handler = isset($handler_ref) ? new $handler_ref($Project) : null;
 
     rewind($filehandle);
     $content = fread($filehandle, 8192);
 
     if ($handler == null) {
         add_log('error: could not create handler based on xml content', 'ctest_parse', LOG_ERR);
-        $Project = new Project();
-        $Project->Id = $projectid;
 
         $Project->SendEmailToAdmin('Cannot create handler based on XML content',
             'An XML submission from ' . $ip . ' to the project ' . get_project_name($projectid) . ' cannot be parsed. The content of the file is as follows: ' . $content);

diff --git a/app/cdash/tests/CMakeLists.txt b/app/cdash/tests/CMakeLists.txt
@@ -171,11 +171,8 @@ set_tests_properties(/CDash/Service/RepositoryService PROPERTIES DEPENDS /CDash/
 add_unit_test(/CDash/ServiceContainer)
 set_tests_properties(/CDash/ServiceContainer PROPERTIES DEPENDS /CDash/Service/RepositoryService)
 
-add_unit_test(/CDash/Submission/CommitAuthorHandlerTrait)
-set_tests_properties(/CDash/Submission/CommitAuthorHandlerTrait PROPERTIES DEPENDS /CDash/ServiceContainer)
-
 add_unit_test(/CDash/TestUseCase)
-set_tests_properties(/CDash/TestUseCase PROPERTIES DEPENDS /CDash/Submission/CommitAuthorHandlerTrait)
+set_tests_properties(/CDash/TestUseCase PROPERTIES DEPENDS /CDash/ServiceContainer)
 
 add_unit_test(/CDash/UpdateUseCase)
 set_tests_properties(/CDash/UpdateUseCase PROPERTIES DEPENDS /CDash/TestUseCase)

diff --git a/app/cdash/tests/case/CDash/Messaging/Subscription/CommitAuthorSubscriptionBuilderTest.php b/app/cdash/tests/case/CDash/Messaging/Subscription/CommitAuthorSubscriptionBuilderTest.php
@@ -38,7 +38,9 @@ public function testBuildGivenSubmissionWithBuildErrors()
         $mock_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new BuildHandler(0);
+                $project = new Project();
+                $project->Id = 0;
+                $handler = new BuildHandler($project);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 
@@ -60,7 +62,9 @@ public function testBuildGivenSubmissionWithBuildWarnings()
         $mock_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new BuildHandler(0);
+                $project = new Project();
+                $project->Id = 0;
+                $handler = new BuildHandler($project);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 
@@ -82,7 +86,9 @@ public function testBuildGivenSubmissionWithTestFailures()
         $mock_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new TestingHandler(0);
+                $project = new Project();
+                $project->Id = 0;
+                $handler = new TestingHandler($project);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 

diff --git a/app/cdash/tests/case/CDash/Messaging/Subscription/UserSubscriptionBuilderTest.php b/app/cdash/tests/case/CDash/Messaging/Subscription/UserSubscriptionBuilderTest.php
@@ -50,7 +50,9 @@ public function testBuildGivenBuildSubmission()
         $mock_build_submission->expects($this->any())
             ->method('GetTopicCollectionForSubscriber')
             ->willReturnCallback(function ($subscriber) {
-                $handler = new BuildHandler(0);
+                $project = new Project();
+                $project->Id = 0;
+                $handler = new BuildHandler($project);
                 return $handler->GetTopicCollectionForSubscriber($subscriber);
             });
 

diff --git a/app/cdash/tests/case/CDash/Submission/CommitAuthorHandlerTraitTest.php b/app/cdash/tests/case/CDash/Submission/CommitAuthorHandlerTraitTest.php