Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for validation of XML submission files #2437

Merged
merged 9 commits into from
Jan 13, 2025

Conversation

sbelsk
Copy link
Collaborator

@sbelsk sbelsk commented Sep 10, 2024

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.

@sbelsk sbelsk added this to the v3.6 milestone Sep 10, 2024
@sbelsk sbelsk force-pushed the submission-xml-files-validation branch 3 times, most recently from 6a60f38 to c5431c2 Compare September 10, 2024 17:36
@josephsnyder josephsnyder self-assigned this Sep 23, 2024
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from c5431c2 to d94a42b Compare September 25, 2024 20:29
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from d94a42b to 4602d25 Compare October 9, 2024 16:58
@williamjallen williamjallen modified the milestones: v3.6, v3.7 Oct 15, 2024
@williamjallen
Copy link
Collaborator

Moving this to the 3.7 release since it isn't quite finished.

Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@williamjallen williamjallen marked this pull request as draft November 15, 2024 21:52
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch 4 times, most recently from f02158b to 309f06e Compare November 25, 2024 18:17
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from 309f06e to 24c388e Compare December 3, 2024 20:25
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from 24c388e to 1114898 Compare December 19, 2024 20:07
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from 1114898 to 9941aea Compare December 24, 2024 18:14
@williamjallen williamjallen modified the milestones: v3.7, v3.8 Jan 2, 2025
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch 2 times, most recently from aedfeb1 to 77d00c6 Compare January 2, 2025 20:01
@williamjallen
Copy link
Collaborator

@josephsnyder I just pushed an update which makes the code cleaner and hopefully gets rid of some of the complexity that might've been tripping you up. I also resolved all of the PHPStan errors. I didn't take a detailed look into why the submission validation test is failing, but I can dig into that a bit more if you'd like.

@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from ad4e155 to 4ba9d80 Compare January 6, 2025 14:39
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from 4ba9d80 to 035378a Compare January 6, 2025 19:03
@josephsnyder josephsnyder marked this pull request as ready for review January 6, 2025 19:24
Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be on the right track overall. A few more changes, and then this should be good to merge.

@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch 2 times, most recently from 43d10bd to aff80fe Compare January 7, 2025 20:48
Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this also needs a rebase to meet the new style guidelines.

@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from aff80fe to 815ec93 Compare January 9, 2025 14:21
Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple quick changes, and then this should be good to go.

sbelsk and others added 9 commits January 13, 2025 11:10
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.
Switch the dependency property for the submissionValidation test to a
later php_test.  This should prevent the test from moving far up in
test number and throwing off the expected bolus of submission values
Add a new test location for the submission validation test
Update the paths and objects needed.
Add in further test cases for when the environment variable is in
different states: not set, false, true
@josephsnyder josephsnyder force-pushed the submission-xml-files-validation branch from 24b69b6 to c17dcdd Compare January 13, 2025 16:21
Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work on this!

@williamjallen williamjallen added this pull request to the merge queue Jan 13, 2025
Merged via the queue into Kitware:master with commit 684a762 Jan 13, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants