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

feat(rdf-api): Add a general-purpose SHACL validation utility (DSP-930) #1762

Merged
merged 4 commits into from Dec 1, 2020

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Nov 25, 2020

resolves DSP-930.

  • Add SHACL validation API in ShaclValidator.scala.
  • Add implementations:
    • JenaShaclValidator
    • RDF4JShaclValidator
  • Return these from RdfFeatureFactory.
  • Add tests.
  • Update docs.
@benjamingeer benjamingeer self-assigned this Nov 25, 2020
@benjamingeer benjamingeer marked this pull request as draft Nov 25, 2020
@benjamingeer benjamingeer requested a review from SepidehAlassi Nov 26, 2020
@benjamingeer benjamingeer marked this pull request as ready for review Nov 26, 2020
Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

This looks great! Thanks for implementing this.

* Recursively loads graphs of SHACL shapes from a directory and its subdirectories.
*
* @param baseDir the base directory that SHACL graphs are loaded from.
* @param dir the base directory or a subdirectory.

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Dec 1, 2020
Contributor

It seems like the Shacl shapes are actually loaded from dir directory and its subdirectories, and baseDir is only used to get its path to make the relativePath. Can you please change the comments for baseDir and dir in scaladocs?

I also wonder, if only the path of the baseDir is needed here, why not just using its path as the parameter of this function, I mean

 private def  loadShaclGraphs(baseDirPath: Path, dir: File) : Map[Path, ShaclGraphT] = {...}

two directory parameters are a bit confusing.

This comment has been minimized.

@benjamingeer

benjamingeer Dec 1, 2020
Author Collaborator

It seems like the Shacl shapes are actually loaded from dir directory and its subdirectories, and baseDir is only used to get its path to make the relativePath.... I also wonder, if only the path of the baseDir is needed here, why not just using its path as the parameter of this function

Actually, the SHACL shapes are loaded from baseDir and its subdirectories. File and Path are basically the same thing, but File is the old java.io API which I'm more used to, and Path is the newer java.nio API. The consensus in the Java community seems to be that seems that Path should be used instead of File for everything nowadays (https://stackoverflow.com/a/26658436). I guess we should do a PR sometime to use java.nio instead of java.io everywhere. For now, I've refactored AbstractShaclValidator to use java.nio to walk the directory tree, which seems clearer.

val graphToValidate: jena.graph.Graph = rdfModel.asJenaDataset.asDatasetGraph.getDefaultGraph

// Validate the data and get Jena's validation report.
val validationReport: jena.shacl.ValidationReport = shaclValidator.validate(shaclGraph, graphToValidate)

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Dec 1, 2020
Contributor

Nice! Jena Shacl validate is like pyShacl getting the same parameters and returning report and conforms in the same way.

This comment has been minimized.

@benjamingeer

benjamingeer Dec 1, 2020
Author Collaborator

Standards are nice. :)

@benjamingeer benjamingeer merged commit bfd3192 into main Dec 1, 2020
8 checks passed
8 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Update next release draft
Details
Publish (on release only)
Details
@benjamingeer benjamingeer deleted the wip/DSP-930-shacl branch Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants