Repository: linkml/linkml

Issue:
Make closed state of SHACL shapes configurable
**Is your feature request related to a problem? Please describe.**
linkML generated all SHACL shapes as closed shapes, except of Mixin shapes and shapes, that have a parent class. See [shaclgen.py#L66](https://github.com/linkml/linkml/blob/main/linkml/generators/shaclgen.py#L66). Closed shapes do not allow additional attributes. However, there are uses cases, which want to allow additional attributes in shapes. 

**Describe the solution you'd like**
Extend CLI with an additional optional parameter to configure ```closed``` state of SHACL shapes.

**How important is this feature?** 
• Medium - can do work without it, by e.g. adding a post-generating procedure to remove ```sh:closed true``` from SHACL shapes
• Furthermore making closed state of SHACL shapes is a simple task

**When will use cases depending on this become relevant?**
• Short-term - 2-4 weeks

**Additional context**
I already provided a fix for this feature.


Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/linkml/linkml/pull/1593?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml) Report
> Merging [#1593](https://app.codecov.io/gh/linkml/linkml/pull/1593?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml) (123442f) into [main](https://app.codecov.io/gh/linkml/linkml/commit/20da3a9a2c3e398e316049addabacb4af0e0d4bd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml) (20da3a9) will **decrease** coverage by `0.11%`.
> Report is 25 commits behind head on main.
> The diff coverage is `87.14%`.

```diff
@@            Coverage Diff             @@
##             main    #1593      +/-   ##
==========================================
- Coverage   79.95%   79.84%   -0.11%     
==========================================
  Files          93       94       +1     
  Lines        9711     9844     +133     
  Branches     2382     2389       +7     
==========================================
+ Hits         7764     7860      +96     
- Misses       1502     1527      +25     
- Partials      445      457      +12     
```


| [Files Changed](https://app.codecov.io/gh/linkml/linkml/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml) | Coverage Δ | |
|---|---|---|
| [linkml/generators/\_\_init\_\_.py](https://app.codecov.io/gh/linkml/linkml/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml#diff-bGlua21sL2dlbmVyYXRvcnMvX19pbml0X18ucHk=) | `100.00% <ø> (ø)` | |
| [linkml/generators/pydanticgen.py](https://app.codecov.io/gh/linkml/linkml/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml#diff-bGlua21sL2dlbmVyYXRvcnMvcHlkYW50aWNnZW4ucHk=) | `82.17% <73.33%> (ø)` | |
| [linkml/generators/shaclgen.py](https://app.codecov.io/gh/linkml/linkml/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml#diff-bGlua21sL2dlbmVyYXRvcnMvc2hhY2xnZW4ucHk=) | `81.06% <87.50%> (-0.05%)` | :arrow_down: |
| [linkml/generators/plantumlgen.py](https://app.codecov.io/gh/linkml/linkml/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml#diff-bGlua21sL2dlbmVyYXRvcnMvcGxhbnR1bWxnZW4ucHk=) | `88.23% <88.23%> (ø)` | |

... and [19 files with indirect coverage changes](https://app.codecov.io/gh/linkml/linkml/pull/1593/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml)


anjastrunk: > Thank you!
> 
> Normally we require a test case - note we are switching from unittest to pytest (and making it easier to contribute tests). I see you added a CLI base for tests, great. I would recommend that we add a test in test_generators that tests specifically this behavior independent of the CLI.
> 
> Let us know if you want help writing a test

I added an additional test case to test configuration of closed parameter. Code coverage still decreased by this PR. However ```shaclgen.py``` (the only class I touched) is now again by 100 %. 
cmungall: @sujaypatil96 can you rebase the branch to get the fixes for codecov?

Patch:
diff --git a/linkml/generators/jsonschemagen.py b/linkml/generators/jsonschemagen.py
index 5043414b..9cf8aabc 100644
--- a/linkml/generators/jsonschemagen.py
+++ b/linkml/generators/jsonschemagen.py
@@ -15,6 +15,7 @@ from linkml_runtime.linkml_model.meta import (
     EnumDefinition,
     PermissibleValue,
     PermissibleValueText,
+    PresenceEnum,
     SlotDefinition,
     metamodel_version,
 )
@@ -275,7 +276,18 @@ class JsonSchemaGenerator(Generator):
         subschema = JsonSchema()
         for slot in cls.slot_conditions.values():
             prop = self.get_subschema_for_slot(slot, omit_type=True)
-            subschema.add_property(self.aliased_slot_name(slot), prop, properties_required)
+            if slot.value_presence:
+                if slot.value_presence == PresenceEnum(PresenceEnum.PRESENT):
+                    this_properties_required = True
+                elif slot.value_presence == PresenceEnum(PresenceEnum.ABSENT):
+                    this_properties_required = False
+                    # make the slot unsatisfiable
+                    prop["enum"] = []
+                else:
+                    this_properties_required = False
+            else:
+                this_properties_required = properties_required
+            subschema.add_property(self.aliased_slot_name(slot), prop, this_properties_required)
 
         if cls.any_of is not None and len(cls.any_of) > 0:
             subschema["anyOf"] = [
@@ -380,6 +392,11 @@ class JsonSchemaGenerator(Generator):
         constraints.add_keyword("maximum", slot.maximum_value)
         constraints.add_keyword("const", slot.equals_string)
         constraints.add_keyword("const", slot.equals_number)
+        if slot.value_presence:
+            if slot.value_presence == PresenceEnum(PresenceEnum.PRESENT):
+                constraints.add_keyword("required", True)
+            elif slot.value_presence == PresenceEnum(PresenceEnum.ABSENT):
+                constraints.add_keyword("enum", [])
         return constraints
 
     def get_subschema_for_slot(self, slot: SlotDefinition, omit_type: bool = False) -> JsonSchema:
diff --git a/linkml/generators/shaclgen.py b/linkml/generators/shaclgen.py
index 656aaf66..b847739c 100644
--- a/linkml/generators/shaclgen.py
+++ b/linkml/generators/shaclgen.py
@@ -1,5 +1,6 @@
 import logging
 import os
+from dataclasses import dataclass, field
 
 import click
 from linkml_runtime.utils.formatutils import underscore
@@ -21,8 +22,12 @@ LINK_ML_TYPES_DATE = URIRef("http://www.w3.org/2001/XMLSchema#date")
 LINK_ML_TYPES_TIME = URIRef("http://www.w3.org/2001/XMLSchema#time")
 
 
+@dataclass
 class ShaclGenerator(Generator):
     # ClassVars
+    closed: bool = field(default_factory=lambda: True)
+    """True means add 'sh:closed=true' to all shapes, except of mixin shapes and shapes, that have parents"""
+
     generatorname = os.path.basename(__file__)
     generatorversion = "0.0.1"
     valid_formats = ["ttl"]
@@ -63,10 +68,13 @@ class ShaclGenerator(Generator):
             class_uri = URIRef(sv.get_uri(c, expand=True))
             shape_pv(RDF.type, SH.NodeShape)
             shape_pv(SH.targetClass, class_uri)  # TODO
-            if c.mixin or c.abstract:
-                shape_pv(SH.closed, Literal(False))
+            if self.closed:
+                if c.mixin or c.abstract:
+                    shape_pv(SH.closed, Literal(False))
+                else:
+                    shape_pv(SH.closed, Literal(True))
             else:
-                shape_pv(SH.closed, Literal(True))
+                shape_pv(SH.closed, Literal(False))
             if c.title is not None:
                 shape_pv(SH.name, Literal(c.title))
             if c.description is not None:
@@ -154,6 +162,12 @@ class ShaclGenerator(Generator):
 
 
 @shared_arguments(ShaclGenerator)
+@click.option(
+    "--closed/--non-closed",
+    default=True,
+    show_default=True,
+    help="Use '--closed' to generate closed SHACL shapes. Use '--non-closed' to generate open SHACL shapes.",
+)
 @click.version_option(__version__, "-V", "--version")
 @click.command()
 def cli(yamlfile, **args):


Test patch:
diff --git a/tests/test_compliance/test_boolean_slot_compliance.py b/tests/test_compliance/test_boolean_slot_compliance.py
index a5e2419a..8dc57cf6 100644
--- a/tests/test_compliance/test_boolean_slot_compliance.py
+++ b/tests/test_compliance/test_boolean_slot_compliance.py
@@ -315,6 +315,71 @@ def test_none_of(framework, data_name, value, is_valid):
     )
 
 
+@pytest.mark.parametrize(
+    "data_name,instance,is_valid",
+    [
+        ("v0", {}, False),
+        ("v1", {SLOT_S1: "x"}, True),
+        ("v2", {SLOT_S2: "x"}, True),
+        ("v3", {SLOT_S1: "x", SLOT_S2: "x"}, False),
+    ],
+)
+@pytest.mark.parametrize("framework", CORE_FRAMEWORKS)
+@pytest.mark.skip("requires metamodel changes")
+def test_cardinality_in_exactly_one_of(framework, data_name, instance, is_valid):
+    """
+    Tests intersection of cardinality and exactly_one_of.
+
+    TODO: unskip this test when metamodel allows attributes or slot usage on anon expressions.
+
+    :param framework:
+    :param data_name:
+    :param instance:
+    :param is_valid:
+    :return:
+    """
+    classes = {
+        CLASS_C: {
+            "exactly_one_of": [
+                {
+                    "attributes": {
+                        SLOT_S1: {
+                            "required": True,
+                        },
+                    },
+                },
+                {
+                    "attributes": {
+                        SLOT_S2: {
+                            "required": True,
+                        },
+                    },
+                },
+            ],
+        },
+    }
+    schema = validated_schema(
+        test_cardinality_in_exactly_one_of,
+        "default",
+        framework,
+        classes=classes,
+        core_elements=["exactly_one_of", "minimum_value", "maximum_value"],
+    )
+    expected_behavior = ValidationBehavior.IMPLEMENTS
+    if framework != JSON_SCHEMA:
+        expected_behavior = ValidationBehavior.INCOMPLETE
+    check_data(
+        schema,
+        data_name,
+        framework,
+        instance,
+        is_valid,
+        target_class=CLASS_C,
+        expected_behavior=expected_behavior,
+        description=f"validity {is_valid} check for value {instance}",
+    )
+
+
 @pytest.mark.parametrize(
     "value",
     [1, 10, 15, 20, 21],
@@ -542,3 +607,99 @@ def test_union_of(framework, data_name, value, is_valid):
         expected_behavior=expected_behavior,
         description=f"validity {is_valid} check for value {value}",
     )
+
+
+@pytest.mark.parametrize(
+    "data_name,instance,is_valid",
+    [
+        ("v0", {}, False),
+        ("v1", {SLOT_S1: "x"}, True),
+        ("v2", {SLOT_S2: "x"}, True),
+        ("v3", {SLOT_S1: "x", SLOT_S2: "x"}, False),
+    ],
+)
+@pytest.mark.parametrize("multivalued", [False, True])
+@pytest.mark.parametrize("framework", CORE_FRAMEWORKS)
+def test_value_presence_in_rules(framework, multivalued, data_name, instance, is_valid):
+    """
+    Tests intersection of value_presence and rules.
+
+    Uses a class with two slots, s1 and s2, where either s1 or s2 is set,
+    but not both.
+
+    :param framework:
+    :param multivalued:
+    :param data_name:
+    :param instance:
+    :param is_valid:
+    :return:
+    """
+    classes = {
+        CLASS_C: {
+            "attributes": {
+                SLOT_S1: {
+                    "multivalued": multivalued,
+                },
+                SLOT_S2: {
+                    "multivalued": multivalued,
+                },
+            },
+            "rules": [
+                {
+                    "preconditions": {
+                        "slot_conditions": {
+                            SLOT_S1: {
+                                "value_presence": "PRESENT",
+                            },
+                        },
+                    },
+                    "postconditions": {
+                        "slot_conditions": {
+                            SLOT_S2: {
+                                "value_presence": "ABSENT",
+                            },
+                        },
+                    },
+                },
+                {
+                    "preconditions": {
+                        "slot_conditions": {
+                            SLOT_S1: {
+                                "value_presence": "ABSENT",
+                            },
+                        },
+                    },
+                    "postconditions": {
+                        "slot_conditions": {
+                            SLOT_S2: {
+                                "value_presence": "PRESENT",
+                            },
+                        },
+                    },
+                },
+            ],
+        },
+    }
+    schema = validated_schema(
+        test_value_presence_in_rules,
+        f"MV{multivalued}",
+        framework,
+        classes=classes,
+        core_elements=["preconditions", "postconditions", "slot_conditions", "value_presence"],
+    )
+    expected_behavior = ValidationBehavior.IMPLEMENTS
+    if framework != JSON_SCHEMA:
+        if not is_valid:
+            expected_behavior = ValidationBehavior.INCOMPLETE
+    if multivalued:
+        instance = {k: [v] for k, v in instance.items()}
+    check_data(
+        schema,
+        data_name,
+        framework,
+        instance,
+        is_valid,
+        target_class=CLASS_C,
+        expected_behavior=expected_behavior,
+        description=f"validity {is_valid} check for value {instance}",
+    )
diff --git a/tests/test_generators/test_shaclgen.py b/tests/test_generators/test_shaclgen.py
index e5f156bd..8e584111 100644
--- a/tests/test_generators/test_shaclgen.py
+++ b/tests/test_generators/test_shaclgen.py
@@ -17,13 +17,33 @@ EXPECTED = [
     ),
 ]
 
+EXPECTED_closed = [
+    (
+        rdflib.term.URIRef("https://w3id.org/linkml/tests/kitchen_sink/Person"),
+        rdflib.term.URIRef("http://www.w3.org/ns/shacl#closed"),
+        rdflib.term.Literal(
+            "false", datatype=rdflib.term.URIRef("http://www.w3.org/2001/XMLSchema#boolean")
+        ),
+    ),
+]
+
 
 def test_shacl(kitchen_sink_path):
     """tests shacl generation"""
     shaclstr = ShaclGenerator(kitchen_sink_path, mergeimports=True).serialize()
+    do_test(shaclstr, EXPECTED)
+
+
+def test_shacl_closed(kitchen_sink_path):
+    """tests shacl generation"""
+    shaclstr = ShaclGenerator(kitchen_sink_path, mergeimports=True, closed=False).serialize()
+    do_test(shaclstr, EXPECTED_closed)
+
+
+def do_test(shaclstr, expected):
     g = rdflib.Graph()
     g.parse(data=shaclstr)
     triples = list(g.triples((None, None, None)))
-    for et in EXPECTED:
+    for et in expected:
         assert et in triples
     # TODO: test shacl validation; pyshacl requires rdflib6
diff --git a/tests/test_scripts/test_gen_shacl.py b/tests/test_scripts/test_gen_shacl.py
new file mode 100644
index 00000000..41ce3aea
--- /dev/null
+++ b/tests/test_scripts/test_gen_shacl.py
@@ -0,0 +1,19 @@
+import unittest
+
+from linkml.generators import shaclgen
+from tests.test_scripts.environment import env
+from tests.utils.clicktestcase import ClickTestCase
+
+
+class GenShaclTestCase(ClickTestCase):
+    testdir = "genshacl"
+    click_ep = shaclgen.cli
+    prog_name = "gen-shacl"
+    env = env
+
+    def test_help(self):
+        self.do_test("--help", "help")
+
+
+if __name__ == "__main__":
+    unittest.main()