Add support for validation of XML submission files #2437
Conversation
sbelsk
force-pushed
the
submission-xml-files-validation
branch
3 times, most recently
from
6a60f38
to
c5431c2
Compare
josephsnyder
force-pushed
the
submission-xml-files-validation
branch
from
c5431c2
to
d94a42b
Compare
josephsnyder
force-pushed
the
submission-xml-files-validation
branch
from
d94a42b
to
4602d25
Compare
|
Moving this to the 3.7 release since it isn't quite finished. |
There was a problem hiding this comment.
After digging into the submission handler logic a bit more, I am no longer certain that this is the correct approach. Instead of validating the XML files when submissions are processed, I think it would be better to validate them at submission time. This approach was initially rejected due to the hypothesis that doing so would slow down submission acceptance. After gaining a better understanding of the code, I am reasonably confident that doing this validation at submission time will have a negligible performance impact.
The main benefit of doing this validation at submission time is that error messages will always be returned to the client, regardless of the queueing system used.
josephsnyder
force-pushed
the
submission-xml-files-validation
branch
3 times, most recently
from
b707518
to
f02158b
Compare
When attempting to validate an HTTP submission use the `storage_path` helper to turn the filename which starts at the inproess folder, `inprocess/<>.xml`, to a full path. This should not affect the path given directly to the submission:validate artisan command.
Add a test which utilizes the submission function of KWTester to submit the same data as the manual validation steps.
josephsnyder
force-pushed
the
submission-xml-files-validation
branch
from
f02158b
to
309f06e
Compare
| @@ -128,6 +130,35 @@ private function submitProcess(): Response | |||
| } elseif (intval($this->project->Id) < 1) { | |||
| abort(Response::HTTP_NOT_FOUND, 'The requested project does not exist.'); | |||
| } | |||
| if ((bool) config('cdash.validate_xml_submissions') === true) { | |||
There was a problem hiding this comment.
What do you think about running all submissions through the validator? I think it would be useful to log the result of submission validation for all submissions, and instead use the config setting to control whether mis-formatted submissions are entirely rejected or not.
There was a problem hiding this comment.
I like that idea. It seems like it would be useful information about malformed submissions that aren't "rejected"
| * Validate the given XML file based on its type | ||
| * @throws CDashXMLValidationException | ||
| */ | ||
| public static function validate_xml(string $xml_file, string $schema_file): void |
There was a problem hiding this comment.
What do you think about moving this to app/cdash/xml_handlers/abstract_xml_handler.php? That way, the $schema_file argument could be stored as a class member and users of this method won't need to know about the exact details of how the validation actually occurs.
|
|
||
|
|
||
|
|
||
| class SubmissionValidationTestCase extends KWWebTestCase |
There was a problem hiding this comment.
It would be better to make this a feature test under /tests/Feature. Getting a submission method in a feature test will require #2578 though, which of course requires us to figure out what is causing errors on your system. What do you think about delaying this until #2578 is merged and then converting this to a feature test which uses the new submission method?
This is a follow up to the submission XML schemas along with the validation artisan command (see #2335) that were recently introduced. The changes in this PR add the option to use this validation process on incoming submission files, although this option is disabled by default.
A test has been added to verify these changes, and the docs have been updated to reflect the new configuration option.