-
-
Notifications
You must be signed in to change notification settings - Fork 499
Improve validation response #8985
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
base: main
Are you sure you want to change the base?
Improve validation response #8985
Conversation
namespaces.add(Namespace.getNamespace("geonet", "http://www.fao.org/geonetwork")); | ||
namespaces.add(Namespace.getNamespace("svrl", "http://purl.oclc.org/dsdl/svrl")); | ||
|
||
restructureReportToHavePatternRuleHierarchy(schemaTronReport); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method addAllReportsMatchingRequirement
is called 2 times with the schemaTronReport
.
As restructureReportToHavePatternRuleHierarchy
will be applied twice to the schemaTronReport
and changes it's structure. Have you verified that does cause any side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest push I moved restructureReportToHavePatternRuleHierarchy
so that it can be called before the addAllReportsMatchingRequirement
calls to avoid restructuring twice
|
||
List<?> failedAssert = Xml.selectNodes(schemaTronReport, | ||
"geonet:report[@geonet:required = '" + SchematronRequirement.REQUIRED + "']/svrl:schematron-output/svrl:failed-assert", | ||
// Get all the errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file contains some non-used imports, I'm not sure if related to the pull request, but can you update it to remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in latest push
<span data-translate="">batchValidationReport</span> | ||
</strong> | ||
</div> | ||
<div |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in latest push
web-ui/src/main/resources/catalog/components/utility/partials/batchvalidationreport.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments / suggestions, but other than that the code changes look fine and the reporting is much more useful.
To consider if it should be backported to 4.2.x or not.
This looks really good @tylerjmchugh, thanks! Since this contains an API change I would suggest that we keep this in 4.4.x. Would that make sense to you? |
I have removed the 4.2.x backport. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the latest changes. I noticed an issue changing a schematron from required to warning (or the other way around), I need to restart the application so the report shows changed validation errors in the correct section. It seems some kind of cache issue.
Test:
-
Validate the sample metadata with default schematron configuration (all required) --> some errors reported related to Datacite for ISO19139
-
Update the validation rule to be optional
-
Reissue the validation --> Datacite elements are still in the errors section.
-
Restart the application and reissue the validation --> Datacite elements are displayed in the warning section.
I have identified the issue. There seems to be a caching mechanism for the validation reports so the old validation result (where the schematron is REQUIRED) is being used. The validation report cache will also cause issues for validations performed by external APIs. For example we have a schematron which calls an API to validate resources associated with the record. Since the cached validation report is only deleted on an update to the metadata if the resource is updated the validation result would be stale. To resolve these issues I have removed the validation report caching mechanism. |
Currently the report returned for batch validation is missing useful information and includes unused fields. This is problematic for users that want to validate multiple records as they will need to open the editor for each record to see the full schematron validation results.
This PR aims to fix this issue by creating a new
MetadataValidationProcessingReport
class andgnBatchValidationReport
directive.The new report includes missing fields like:
validMetadata
+ countinvalidMetadata
+ countmetadataWithWarnings
+ countvalidationErrors
validationWarnings
The new report excludes unused
ProcessingReport
fields like:numberOfRecordsUnchanged
- Not used in validation processmetadataErrors
+ count - Replaced withvalidationErrors
andvalidationWarnings
metadataInfos
- Replaced withvalidMetadata
noProcessFoundCount
- Not used in validation processFor the below samples consider the following:
The current API response:
The API response after these changes:
The current UI:
After these changes:
Most importantly, without these changes we cannot see any warnings messages and it is unclear where the error messages originate from. It is also easier to determine which / how many records are valid, invalid, and have warnings with the proposed changes.
Checklist
main
branch, backports managed with labelREADME.md
filespom.xml
dependency management. Update build documentation with intended library use and library tutorials or documentation