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

Fix issues in SHACL #526

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

mhrimaz
Copy link

@mhrimaz mhrimaz commented Sep 9, 2024

This pull request fixes some critical issues in the current SHACL #519 and also some of the mentioned issues in admin-shell-io/aas-specs#421

Regarding #519

We had:

aas:LangStringDefinitionTypeIec61360Shape a sh:NodeShape ;
    sh:targetClass aas:LangStringDefinitionTypeIec61360 ;
    rdfs:subClassOf aas:AbstractLangStringShape ;
    sh:property [
        a sh:PropertyShape ;
        # *********************************************PROBLEM IS HERE
        sh:path <https://admin-shell.io/aas/3/0/LangStringDefinitionTypeIec61360/text> ;
        sh:datatype xs:string ;
        sh:minCount 1 ;
        sh:maxCount 1 ;
        sh:maxLength 1023 ;
    ] ;
.

Now we have:

aas:LangStringDefinitionTypeIec61360Shape a sh:NodeShape ;
    sh:targetClass aas:LangStringDefinitionTypeIec61360 ;
    sh:node aas:AbstractLangStringShape ; 
    sh:property [
        a sh:PropertyShape ;
        # **********************************************CHANGE IS HERE
        sh:path <https://admin-shell.io/aas/3/0/AbstractLangString/text> ; 
        sh:datatype xs:string ;
        sh:minCount 1 ;
        sh:maxCount 1 ;
        sh:maxLength 1023 ;
    ] ;
.

Which means: previously we had to have <https://admin-shell.io/aas/3/0/LangStringDefinitionTypeIec61360/text> as predicate (DatatypeProperty) for instances of aas:LangStringDefinitionTypeIec61360. However, in the generated sample data, and also in the ontology this predicate is missing. So instead of using concrete class in the predicate we should use the abstract class name. IMO, data and ontology are fine and we don't need to change them, SHACL is actually wrong and we can fix it with less side effects (I hope).


  • Remove this first pattern wherever possible. It says "only printable Unicode chars" and most of the time is redundant (eg digits are printable chars)

Having multiple pattern in a PropertyShape is not allowed. Two duplicate property path created. Of course this is redundant, and having generic printable regex when other patterns are much more restrictive is unnecessary. People can remove that extra PropertyShape for performance reasons.

We had:

aas:ReferableShape a sh:NodeShape ;
    sh:targetClass aas:Referable ;
    rdfs:subClassOf aas:HasExtensionsShape ;
    sh:sparql [
        a sh:SPARQLConstraint ;
        sh:message "(ReferableShape): An aas:Referable is an abstract class. Please use one of the subclasses for the generation of instances."@en ;
        sh:prefixes aas: ;
        sh:select """
            SELECT ?this ?type
            WHERE {
                ?this rdf:type ?type .
                FILTER (?type = aas:Referable)
            }
        """ ;
    ] ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/category> ;
        sh:datatype xs:string ;
        sh:minCount 0 ;
        sh:maxCount 1 ;
        sh:minLength 1 ;
        sh:maxLength 128 ;
        sh:pattern "^[\\x09\\x0A\\x0D\\x20-\\uD7FF\\uE000-\\uFFFD\\U00010000-\\U0010FFFF]*$" ;
    ] ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/idShort> ;
        sh:datatype xs:string ;
        sh:minCount 0 ;
        sh:maxCount 1 ;
        sh:minLength 1 ;
        sh:maxLength 128 ;
        sh:pattern "^[\\x09\\x0A\\x0D\\x20-\\uD7FF\\uE000-\\uFFFD\\U00010000-\\U0010FFFF]*$" ;
        sh:pattern "^[a-zA-Z][a-zA-Z0-9_]*$" ;
        #<<---- **********************************************TWO sh:pattern IS NOT ALLOWED!
    ] ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/displayName> ;
        sh:class aas:LangStringNameType ;
        sh:minCount 0 ;
    ] ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/description> ;
        sh:class aas:LangStringTextType ;
        sh:minCount 0 ;
    ] ;
.

Now we have:

aas:ReferableShape a sh:NodeShape ;
    sh:targetClass aas:Referable ;
    sh:node aas:HasExtensionsShape ;
    sh:sparql [
        a sh:SPARQLConstraint ;
        sh:message "(ReferableShape): An aas:Referable is an abstract class. Please use one of the subclasses for the generation of instances."@en ;
        sh:prefixes aas: ;
        sh:select """
            SELECT ?this ?type
            WHERE {
                ?this <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> ?type .
                FILTER (?type = aas:Referable)
            }
        """ ;
    ] ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/category> ;
        sh:datatype xs:string ;
        sh:minCount 0 ;
        sh:maxCount 1 ;
        sh:minLength 1 ;
        sh:maxLength 128 ;
        sh:pattern "^([\\x09\\x0a\\x0d\\x20-\\ud7ff\\ue000-\\ufffd]|\\ud800[\\udc00-\\udfff]|[\\ud801-\\udbfe][\\udc00-\\udfff]|\\udbff[\\udc00-\\udfff])*$" ;
    ] ; 
    #<<---- **********************************************CHANGE BEGINS HERE
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/idShort> ;
        sh:datatype xs:string ;
        sh:minCount 0 ;
        sh:maxCount 1 ;
        sh:minLength 1 ;
        sh:maxLength 128 ;
        sh:pattern "^([\\x09\\x0a\\x0d\\x20-\\ud7ff\\ue000-\\ufffd]|\\ud800[\\udc00-\\udfff]|[\\ud801-\\udbfe][\\udc00-\\udfff]|\\udbff[\\udc00-\\udfff])*$" ; 
    ] ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/idShort> ; 
        sh:datatype xs:string ;
        sh:minCount 0 ;
        sh:maxCount 1 ;
        sh:minLength 1 ;
        sh:maxLength 128 ;
        sh:pattern "^[a-zA-Z][a-zA-Z0-9_]*$" ; 
    ] ;
    #<<---- **********************************************CHANGE ENDS HERE
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/displayName> ;
        sh:class aas:LangStringNameType ;
        sh:minCount 0 ;
    ] ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/Referable/description> ;
        sh:class aas:LangStringTextType ;
        sh:minCount 0 ;
    ] ;
.

Which means: previously it wasn't possible to validate the shacl against the shacl-shacl (shacl metamodel or shacl-shacl) for example using pyshacl -m flag throws an error when you have two sh:pattern.


  • Declaring a shape subClassOf another shape is questionable at best (a Shape is not a Class, so it cannot be a subClass). What is the purpose of this? (shapes are not inherited this way)

We had:

aas:LangStringDefinitionTypeIec61360Shape a sh:NodeShape ;
    sh:targetClass aas:LangStringDefinitionTypeIec61360 ;
    rdfs:subClassOf aas:AbstractLangStringShape ;
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/LangStringDefinitionTypeIec61360/text> ;
        sh:datatype xs:string ;
        sh:minCount 1 ;
        sh:maxCount 1 ;
        sh:maxLength 1023 ;
    ] ;
.

Now we have:

aas:LangStringDefinitionTypeIec61360Shape a sh:NodeShape ;
    sh:targetClass aas:LangStringDefinitionTypeIec61360 ;
    sh:node aas:AbstractLangStringShape ; #<<---- **********************************************CHANGE IS HERE
    sh:property [
        a sh:PropertyShape ;
        sh:path <https://admin-shell.io/aas/3/0/AbstractLangString/text> ;
        sh:datatype xs:string ;
        sh:minCount 1 ;
        sh:maxCount 1 ;
        sh:maxLength 1023 ;
    ] ;
.

Which means: previously rdfs:subClassOf aas:AbstractLangStringShape ; had no effect, since we don't have sub classing in SHACL, and rather we should use sh:node to state that another NodeShape should hold.


They are removed.


@mristin I am not aware of all aspects, feel free to modify, change, refactor the code. I haven't added the generated shacl.

A SHACL node shape cannot be a subclass of another shape. Intention of rdfs:subClassOf is to enforce that the child should be valid against the shape of parents too. For this purpose we should use sh:node https://www.w3.org/TR/shacl/#NodeConstraintComponent
In SHACL it is not allowed to have multiple sh:pattern in a single PropertyShape. For this reason, for each regular expression pattern, we need to create a separate PropertyShape.
rdf:type without declaring prefix would cause issue in some validators. Using URI wihout prefix would solve this.
@mhrimaz
Copy link
Author

mhrimaz commented Sep 16, 2024

shacl-schema.ttl.txt
Here is the generated shacl-schema as a reference.

@sebbader-sap
Copy link

I can not approve the PR as the changes itself target the python code - and I have simply no clue what is going on. However, on the level of RDF and the improvements that you, @mhrimaz, propose, I am completely aligned and support these changes.

@JaFeKl
Copy link

JaFeKl commented Oct 2, 2024

shacl-schema.ttl.txt Here is the generated shacl-schema as a reference.

This is highly needed, was just going to open an issue regarding the LangString errors in the shacl file but found that it has already been found here....

@mristin
Copy link
Contributor

mristin commented Oct 3, 2024

@mhrimaz @sebbader-sap @JaFeKl please apologize the radio silence; it's a lot going on.

We should have pretty soon (1-4 months) dedicated maintainers for aas-core RDF and SHACL schemas. Till then, the main focus is on V3.1 and testing.

Pull requests are welcome & I volunteer to mentor anybody who would like to make the necessary changes.

@mhrimaz
Copy link
Author

mhrimaz commented Oct 3, 2024

@mristin great to hear that!!
This is a PR anyways and the changes are in the SHACL generator not the SHACL itself.
But feel free to apply the changes whenever it is appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants