Clean up PHPStan errors and refactor functionality

Kitware · Jan 3, 2025 · ad4e155 · ad4e155
1 parent 77d00c6
commit ad4e155
Show file tree

Hide file tree

Showing 23 changed files with 100 additions and 221 deletions.
diff --git a/app/Console/Commands/ValidateXml.php b/app/Console/Commands/ValidateXml.php
@@ -2,9 +2,10 @@
 
 namespace App\Console\Commands;
 
+use App\Exceptions\BadSubmissionException;
 use App\Utils\SubmissionUtils;
+use BadMethodCallException;
 use Illuminate\Console\Command;
-use App\Exceptions\XMLValidationException;
 
 class ValidateXml extends Command
 {
@@ -43,41 +44,32 @@ public function handle(): int
                 $has_skipped = true;
                 continue;
             }
-
             try {
                 $xml_info = SubmissionUtils::get_xml_type($xml_file_handle, $input_xml_file);
-            } catch (XMLValidationException $e) {
-                foreach ($e->getDecodedMessage() as $error) {
-                    $this->error($error);
-                }
+            } catch (BadSubmissionException $e) {
+                $this->error($e->getMessage());
                 $has_errors = true;
                 continue;
             } finally {
                 fclose($xml_file_handle);
             }
-            $file = $xml_info['xml_type'];
-
-            // If validation is enabled and if this file has a corresponding schema, validate it
-            if (null === $xml_info['xml_handler']::$schema_file) {
-                $this->warn("WARNING: Skipped input file '{$input_xml_file}' as validation"
-                               ." of this file format is currently not supported. Validator was"
-                               ."{$xml_info['xml_handler']::$schema_file}");
-                $has_skipped = true;
-                continue;
-            }
 
             // run the validator and collect errors if there are any
             try {
-                $xml_info['xml_handler']::validate_xml($input_xml_file);
-            } catch (XMLValidationException $e) {
-                foreach ($e->getDecodedMessage() as $error) {
-                    $this->error($error);
+                $errors = $xml_info['xml_handler']::validate($input_xml_file);
+                if (count($errors) > 0) {
+                    foreach ($errors as $error) {
+                        $this->error($error);
+                    }
+                    $has_errors = true;
+                } else {
+                    $this->line("Validated file: {$input_xml_file}.");
                 }
-                $has_errors = true;
-                continue;
+            } catch (BadMethodCallException $e) {
+                $this->warn("WARNING: Skipped input file '{$input_xml_file}' as validation"
+                    ." of this file format is currently not supported.");
+                $has_skipped = true;
             }
-
-            $this->line("Validated file: {$input_xml_file}.");
         }
 
         // finally, report the results

diff --git a/app/Exceptions/BadSubmissionException.php b/app/Exceptions/BadSubmissionException.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace App\Exceptions;
+
+use RuntimeException;
+
+class BadSubmissionException extends RuntimeException
+{
+}
diff --git a/app/Exceptions/XMLValidationException.php b/app/Exceptions/XMLValidationException.php
diff --git a/app/Http/Controllers/SubmissionController.php b/app/Http/Controllers/SubmissionController.php
@@ -2,12 +2,12 @@
 
 namespace App\Http\Controllers;
 
+use App\Exceptions\BadSubmissionException;
 use App\Jobs\ProcessSubmission;
 use App\Models\Site;
 use App\Utils\AuthTokenUtil;
 use App\Utils\UnparsedSubmissionProcessor;
 use App\Utils\SubmissionUtils;
-use App\Exceptions\XMLValidationException;
 use CDash\Model\Build;
 use CDash\Model\PendingSubmissions;
 use CDash\Model\Project;
@@ -56,8 +56,8 @@ public function submit(): Response|JsonResponse
     }
 
     /**
-     * @throws XMLValidationException
-     **/
+     * @throws BadSubmissionException
+     */
     private function submitProcess(): Response
     {
         @set_time_limit(0);
@@ -136,35 +136,17 @@ private function submitProcess(): Response
 
         // Figure out what type of XML file this is.
         $stored_filename  = "inbox/".$filename;
-        try {
-            $xml_info = SubmissionUtils::get_xml_type(fopen(Storage::path($stored_filename), 'r'), $stored_filename);
-        } catch (XMLValidationException $e) {
-            foreach ($e->getDecodedMessage() as $error) {
-                Log::error($error);
-            }
-            abort(Response::HTTP_NOT_FOUND, 'Unable to determine the Type of the submission file.');
-        }
-        $filehandle = $xml_info['file_handle'];
-        $handler_ref = $xml_info['xml_handler'];
-        $file = $xml_info['xml_type'];
+        $xml_info = SubmissionUtils::get_xml_type(fopen(Storage::path($stored_filename), 'r'), $stored_filename);
 
         // If validation is enabled and if this file has a corresponding schema, validate it
-
-        if (isset($handler_ref::$schema_file) && !file_exists(base_path().$handler_ref::$schema_file)) {
-            throw new XMLValidationException(["ERROR: Could not find schema file '{$handler_ref::$schema_file}'"
-                                                  ." to validate input file: '{$filename}'"]);
-        } else {
-            try {
-                $handler_ref::validate_xml(storage_path("app/".$stored_filename));
-            } catch (XMLValidationException $e) {
-                $errorlist = '';
-                foreach ($e->getDecodedMessage() as $error) {
-                    Log::error("Validating $filename: ".$error);
-                    $errorlist = $errorlist."\n\r".$error;
-                }
-                if ((bool) config('cdash.validate_xml_submissions') === true) {
-                    abort(400, "Xml validation failed: rejected file $filename: $errorlist");
-                }
+        $validation_errors = $xml_info['xml_handler']::validate(storage_path("app/" . $stored_filename));
+        if (count($validation_errors) > 0) {
+            $error_string = implode(PHP_EOL, $validation_errors);
+
+            // We always log validation failures, but we only send messages back to the client if configured to do so
+            Log::warning("Submission validation failed for file '$filename':" . PHP_EOL);
+            if ((bool) config('cdash.validate_xml_submissions') === true) {
+                abort(400, "XML validation failed: rejected file $filename:" . PHP_EOL . $error_string);
             }
         }
 

diff --git a/app/Jobs/ProcessSubmission.php b/app/Jobs/ProcessSubmission.php
@@ -4,12 +4,11 @@
 
 use AbstractSubmissionHandler;
 use ActionableBuildInterface;
-use App\Exceptions\XMLValidationException;
+use App\Exceptions\BadSubmissionException;
 use App\Utils\UnparsedSubmissionProcessor;
 use App\Models\SuccessfulJob;
 
 use BuildPropertiesJSONHandler;
-use CDash\Model\Build;
 use CDash\Model\PendingSubmissions;
 use CDash\Model\Repository;
 
@@ -124,7 +123,7 @@ private function requeueSubmissionFile($buildid): bool
      * Execute the job.
      *
      * @return void
-     * @throws XMLValidationException
+     * @throws BadSubmissionException
      */
     public function handle()
     {
@@ -187,7 +186,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.
      *
-     * @throws XMLValidationException
+     * @throws BadSubmissionException
      **/
     private function doSubmit($filename, $projectid, $buildid = null, $expected_md5 = ''): AbstractSubmissionHandler|UnparsedSubmissionProcessor|false
     {

diff --git a/app/Utils/SubmissionUtils.php b/app/Utils/SubmissionUtils.php
@@ -4,17 +4,21 @@
 
 namespace App\Utils;
 
+use App\Exceptions\BadSubmissionException;
 use CDash\Database;
 use CDash\Model\Build;
 use CDash\Model\BuildUpdate;
-use App\Exceptions\XMLValidationException;
 
 class SubmissionUtils
 {
     /**
      * Figure out what type of XML file this is
-     * @return array<string,mixed>
-     * @throws XMLValidationException
+     * @return array{
+     *     file_handle: mixed,
+     *     xml_handler: 'BuildHandler'|'ConfigureHandler'|'CoverageHandler'|'CoverageJUnitHandler'|'CoverageLogHandler'|'DoneHandler'|'DynamicAnalysisHandler'|'NoteHandler'|'ProjectHandler'|'TestingHandler'|'TestingJUnitHandler'|'UpdateHandler'|'UploadHandler',
+     *     xml_type: 'Build'|'Configure'|'Coverage'|'CoverageJUnit'|'CoverageLog'|'Done'|'DynamicAnalysis'|'Notes'|'Project'|'Test'|'TestJUnit'|'Update'|'Upload',
+     * }
+     * @throws BadSubmissionException
      */
     public static function get_xml_type(mixed $filehandle, string $xml_file): array
     {
@@ -75,9 +79,8 @@ public static function get_xml_type(mixed $filehandle, string $xml_file): array
         rewind($filehandle);
 
         // perform minimal error checking as a sanity check
-        if ($file === '') {
-            throw new XMLValidationException(["ERROR: Could not determine submission"
-                                                  ." file type for: '{$xml_file}'"]);
+        if ($file === '' || $handler === null) {
+            throw new BadSubmissionException("Could not determine submission file type for: '{$xml_file}'");
         }
 
         return [

diff --git a/app/cdash/include/ctestparser.php b/app/cdash/include/ctestparser.php
@@ -14,6 +14,7 @@
   PURPOSE. See the above copyright notices for more information.
   =========================================================================*/
 
+use App\Exceptions\BadSubmissionException;
 use CDash\Database;
 use App\Models\BuildFile;
 use App\Utils\SubmissionUtils;
@@ -154,10 +155,11 @@ function parse_put_submission($filehandler, $projectid, $expected_md5, int|null
     return $handler;
 }
 
-/** Main function to parse the incoming xml from ctest */
 /**
- * @throws App\Exceptions\XMLValidationException
- **/
+ * Main function to parse the incoming xml from ctest
+ *
+ * @throws BadSubmissionException
+ */
 function ctest_parse($filehandle, string $filename, $projectid, $expected_md5 = '', int|null $buildid=null): AbstractSubmissionHandler|false
 {
     // Check if this is a new style PUT submission.

diff --git a/app/cdash/tests/test_submissionvalidation.php b/app/cdash/tests/test_submissionvalidation.php
@@ -1,6 +1,5 @@
 <?php
 
-use App\Exceptions\XMLValidationException;
 use Tests\Traits\CreatesSubmissions;
 
 class SubmissionValidationTestCase extends KWWebTestCase
@@ -22,19 +21,15 @@ public function submit(string $fileName) : bool
         return true;
     }
 
-    /**
-     *
-     * @throws XMLValidationException
-    */
     public function testSubmissionValidation() : void
     {
 
         $this->ConfigFile = dirname(__FILE__) . '/../../../.env';
         $this->Original = file_get_contents($this->ConfigFile);
         file_put_contents($this->ConfigFile, "VALIDATE_XML_SUBMISSIONS=true\n", FILE_APPEND | LOCK_EX);
 
-        $this->assertFalse($this->submit("invalid_Configure.xml"), "Submission of invalid_syntax_Build.xml was succeessful when it should have failed.");
-        $this->assertFalse($this->submit("invalid_syntax_Build.xml"), "Submission of invalid_syntax_Build.xml was succeessful when it should have failed.");
+        $this->assertFalse($this->submit("invalid_Configure.xml"), "Submission of invalid_syntax_Build.xml was successful when it should have failed.");
+        $this->assertFalse($this->submit("invalid_syntax_Build.xml"), "Submission of invalid_syntax_Build.xml was successful when it should have failed.");
         $this->assertTrue($this->submit("valid_Build.xml"), "Submission of valid_Build.xml was not successful when it should have passed.");
 
         file_put_contents($this->ConfigFile, $this->Original);

diff --git a/app/cdash/xml_handlers/abstract_submission_handler.php b/app/cdash/xml_handlers/abstract_submission_handler.php
@@ -32,4 +32,17 @@ public function getBuild(): Build
     {
         return $this->Build;
     }
+
+    /**
+     * Accepts an absolute path to an input submission file.
+     *
+     * Returns an array of validation messages (if applicable).  An empty return array indicates
+     * that validation was successful.  Returns an empty array if no validation was defined.
+     *
+     * @return array<string>
+     */
+    public static function validate(string $path): array
+    {
+        return [];
+    }
 }
diff --git a/app/cdash/xml_handlers/abstract_xml_handler.php b/app/cdash/xml_handlers/abstract_xml_handler.php
@@ -18,7 +18,6 @@
 use App\Utils\Stack;
 use CDash\Model\Build;
 use CDash\Model\Project;
-use App\Exceptions\XMLValidationException;
 
 abstract class AbstractXmlHandler extends AbstractSubmissionHandler
 {
@@ -29,6 +28,8 @@ abstract class AbstractXmlHandler extends AbstractSubmissionHandler
 
     private $ModelFactory;
 
+    protected static ?string $schema_file = null;
+
     public function __construct(Build|Project $init)
     {
         parent::__construct($init);
@@ -39,34 +40,36 @@ public function __construct(Build|Project $init)
 
     /**
      * Validate the given XML file based on its type
-     * @throws XMLValidationException
+     *
+     * @return array<string>
      */
-    public static function validate_xml(string $xml_file): void
+    public static function validate(string $path): array
     {
+        if (static::$schema_file === null) {
+            return [];
+        }
+
         $errors = [];
         // let us control the failures so we can continue
         // parsing files instead of crashing midway
         libxml_use_internal_errors(true);
 
         // load the input file to be validated
         $xml = new DOMDocument();
-        $xml->load($xml_file, LIBXML_PARSEHUGE);
+        $xml->load($path, LIBXML_PARSEHUGE);
 
-        // run the validator and collect errors if there are any
-        // schema_file is defined in each child class
-        if (!$xml->schemaValidate(base_path().static::$schema_file)) {
+        // run the validator and collect errors if there are any.
+        if (!$xml->schemaValidate(base_path(static::$schema_file))) {
             $validation_errors = libxml_get_errors();
             foreach ($validation_errors as $error) {
                 if ($error->level === LIBXML_ERR_ERROR || $error->level === LIBXML_ERR_FATAL) {
-                    $errors[] = "ERROR: {$error->message} in {$error->file},"
-                                ." line: {$error->line}, column: {$error->column}";
+                    $errors[] = "ERROR: {$error->message} in {$error->file}, line: {$error->line}, column: {$error->column}";
                 }
             }
             libxml_clear_errors();
         }
-        if (count($errors) !== 0) {
-            throw new XMLValidationException($errors);
-        }
+
+        return $errors;
     }
 
 

diff --git a/app/cdash/xml_handlers/build_handler.php b/app/cdash/xml_handlers/build_handler.php
@@ -55,7 +55,7 @@ class BuildHandler extends AbstractXmlHandler implements ActionableBuildInterfac
     private $Generator;
     private $PullRequest;
     private $BuildErrorFilter;
-    public static string $schema_file = "/app/Validators/Schemas/Build.xsd";
+    protected static ?string $schema_file = '/app/Validators/Schemas/Build.xsd';
 
     public function __construct(Project $project)
     {