-
Notifications
You must be signed in to change notification settings - Fork 2
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
TDR-3414 - Use metadata validation library to validate metadata #723
Conversation
…/deleteing additional metadata Add feature access block for the library
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.
Looks good, just a few comments
src/main/scala/uk/gov/nationalarchives/tdr/api/service/DisplayPropertiesService.scala
Show resolved
Hide resolved
src/main/scala/uk/gov/nationalarchives/tdr/api/service/ValidateFileMetadataService.scala
Outdated
Show resolved
Hide resolved
val descriptiveFields = fields.filter(f => f.propertyGroup.contains("OptionalMetadata")) | ||
|
||
val closureMetadataCriteria = createCriteria(closureFields, fields).head | ||
val descriptiveMetadataCriteria = createCriteria(descriptiveFields, fields) |
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.
Sorry for my lack of understanding. How come we only need to return the head for the closureMetadataCriteria but we return the whole descriptiveMetadataCriteria?
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.
Actually, ClosureType is the only metadata at the top level rest of them are dependencies. So, we are returning only head
src/main/scala/uk/gov/nationalarchives/tdr/api/service/ValidateFileMetadataService.scala
Outdated
Show resolved
Hide resolved
new MetadataValidation(closureMetadataCriteria, descriptiveMetadataCriteria) | ||
} | ||
|
||
private def createCriteria(customMetadata: List[CustomMetadataField], allCustomMetadata: List[CustomMetadataField]): List[MetadataCriteria] = { |
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.
This function is not optimised for tail recursion. Should probably look into making it tail recursive as potentially could get stack overflow issues.
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 don't think it is possible with the metadata criteria because we are recalling the same function over values and settings as dependencies. Also, the custom metadata is coming from the DB which is properly structured. So, I don't think there will be any stack overflow issues.
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.
Fair point.
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.
Looks good. Thanks
Use metadata validation library to validate while updating/deleting additional metadata
Add a feature access block for the library