From 66e3333e41772e10501c7711583beb288cb65176 Mon Sep 17 00:00:00 2001 From: FABIOTESS Date: Wed, 19 Aug 2026 17:37:07 +0100 Subject: [PATCH] fix(ontology): address review findings on the SHACL namespace fix Four findings from the automated review, all reproduced first. 1. The fix only reached Turtle. `_uri` was the single place I corrected, and JSON-LD and N-Triples build sh:targetClass, sh:path and sh:class straight from graph.base_uri, so two of the three formats went on emitting shapes that match nothing. That is the defect this PR claims to close, still live wherever the output is not Turtle. All three serializers now resolve through one `_term_iri`, and the pySHACL violation test runs against each of them. 2. Classes and properties shared one name-keyed index built with setdefault, so a property named after a class was permanently mapped to the class IRI and its sh:path validated the wrong predicate. The index is now split into class_iris and property_iris, and each call site says which it wants. 3. OntologyEngine.to_shacl forwarded target_namespace and attach_domainless_properties through generate(**options), which never reads them, so both were silently dropped on the public path. They are now named parameters passed to the constructor, and documented. 4. The opt-in attachment logged at debug. It broadens constraint generation, so it warns. 7 further tests, including the target-namespace and real-violation checks parametrised across Turtle, N-Triples and JSON-LD. --- semantica/ontology/engine.py | 12 ++ semantica/ontology/ontology_generator.py | 82 ++++++++----- tests/ontology/test_shacl_target_namespace.py | 114 ++++++++++++++++++ 3 files changed, 177 insertions(+), 31 deletions(-) diff --git a/semantica/ontology/engine.py b/semantica/ontology/engine.py index d698f5e2..8cf7b1d4 100644 --- a/semantica/ontology/engine.py +++ b/semantica/ontology/engine.py @@ -203,6 +203,8 @@ class OntologyEngine: include_inherited: bool = True, severity: str = "Violation", quality_tier: str = "standard", + target_namespace: Optional[str] = None, + attach_domainless_properties: bool = False, validate_output: bool = False, **options, ) -> str: @@ -217,6 +219,12 @@ class OntologyEngine: include_inherited: Propagate parent class property shapes to child shapes. severity: Default severity — "Violation", "Warning", or "Info". quality_tier: Constraint completeness — "basic", "standard" (default), "strict". + target_namespace: Namespace sh:targetClass and sh:path are expanded in, + when the ontology supplies no absolute IRIs. Separate from base_uri, + which says where the shape resources themselves live. + attach_domainless_properties: Attach properties with no declared domain + to every node shape. Off by default: it states a constraint the + ontology does not. validate_output: Syntax-check output via rdflib before returning. Returns: @@ -236,12 +244,16 @@ class OntologyEngine: or (ns.get("base_uri") if isinstance(ns, dict) else None) or "https://semantica.dev/shapes/" ) + # These two are constructor-only on SHACLGenerator, so forwarding + # them through generate(**options) silently dropped them. generator = SHACLGenerator( base_uri=resolved_base, shapes_uri=shapes_uri, include_inherited=include_inherited, severity=severity, quality_tier=quality_tier, + target_namespace=target_namespace, + attach_domainless_properties=attach_domainless_properties, ) graph = generator.generate(ontology, **options) self.progress.update_tracking(tracking_id, message="Serializing SHACL graph") diff --git a/semantica/ontology/ontology_generator.py b/semantica/ontology/ontology_generator.py index 564b94d7..0a29c2fb 100644 --- a/semantica/ontology/ontology_generator.py +++ b/semantica/ontology/ontology_generator.py @@ -752,10 +752,13 @@ class SHACLGraph: shapes_uri: str node_shapes: List[NodeShape] = field(default_factory=list) prefixes: Dict[str, str] = field(default_factory=dict) - # Bare class and property names mapped to the absolute IRI the data uses - # for them. Shapes are indexed internally by name; this is what those names - # expand to at serialisation time (#1104). - term_iris: Dict[str, str] = field(default_factory=dict) + # Bare names mapped to the absolute IRI the data uses for them. Shapes are + # indexed internally by name; this is what those names expand to at + # serialisation time (#1104). Classes and properties are kept apart because + # a property may legitimately share a class's name, and a single map would + # silently give it the class's IRI. + class_iris: Dict[str, str] = field(default_factory=dict) + property_iris: Dict[str, str] = field(default_factory=dict) class SHACLGenerator: @@ -858,7 +861,8 @@ class SHACLGenerator: base_uri=base_uri, shapes_uri=self.shapes_uri, prefixes=prefixes, - term_iris=self._build_term_index(classes, properties, target_ns), + class_iris=self._build_term_index(classes, target_ns), + property_iris=self._build_term_index(properties, target_ns), ) self.progress_tracker.update_tracking(tracking_id, message="Generating node shapes") @@ -979,14 +983,11 @@ class SHACLGenerator: return self._DEFAULT_TARGET_NAMESPACE def _build_term_index( - self, - classes: List[Dict[str, Any]], - properties: List[Dict[str, Any]], - target_ns: str, + self, terms: List[Dict[str, Any]], target_ns: str ) -> Dict[str, str]: - """Map every class and property name to the absolute IRI it expands to.""" + """Map each term's name to the absolute IRI it expands to.""" index: Dict[str, str] = {} - for term in list(classes or []) + list(properties or []): + for term in terms or []: if not isinstance(term, dict): continue name = term.get("name") @@ -1053,9 +1054,10 @@ class SHACLGenerator: f"Property '{pname}' domain '{d}' has no matching node shape — skipped" ) elif self.attach_domainless_properties: - self.logger.debug( - f"Property '{pname}' has no domain, attaching to all node shapes " - "because attach_domainless_properties is set" + self.logger.warning( + f"Property '{pname}' declares no domain and is being attached " + "to every node shape because attach_domainless_properties is " + "set. This states a constraint the ontology does not." ) for node_shape in graph.node_shapes: node_shape.property_shapes.append(self._build_property_shape(prop)) @@ -1147,22 +1149,40 @@ class SHACLGenerator: def _prefix_decls(self, graph: SHACLGraph) -> str: return "\n".join(f"@prefix {p}: <{u}> ." for p, u in sorted(graph.prefixes.items())) - def _uri(self, graph: SHACLGraph, local: str) -> str: + def _term_iri(self, graph: SHACLGraph, local: str, kind: str = "class") -> str: """ - Return a URI reference for a class or property name. + Resolve a class or property name to the absolute IRI the data uses. - Bare names are expanded through the graph's term index, which holds the - IRI the data actually uses, rather than being pasted onto whichever - namespace the shapes happen to live in (#1104). + Every serializer goes through here. Turtle alone was corrected at first, + which left JSON-LD and N-Triples still pasting names onto the shapes + namespace, so the shapes they produced went on matching nothing (#1104). + + `kind` selects the index: a property may share a class's name, and the + two can carry different IRIs. """ if local.startswith("http://") or local.startswith("https://"): - return f"<{local}>" - resolved = graph.term_iris.get(local) + return local + index = graph.property_iris if kind == "property" else graph.class_iris + resolved = index.get(local) if resolved: - return f"<{resolved}>" + return resolved + # Fall back to the other index before giving up: sh:class names a class, + # but a caller may pass a term only registered on the other side. + other = graph.class_iris if kind == "property" else graph.property_iris + resolved = other.get(local) + if resolved: + return resolved if ":" in local: return local - return f"ex:{local}" + separator = "" if graph.base_uri.endswith(("#", "/", ":")) else "#" + return f"{graph.base_uri}{separator}{local}" + + def _uri(self, graph: SHACLGraph, local: str, kind: str = "class") -> str: + """Turtle-facing wrapper: the resolved IRI, in angle brackets.""" + resolved = self._term_iri(graph, local, kind) + if resolved.startswith(("http://", "https://", "urn:")): + return f"<{resolved}>" + return resolved def _serialize_turtle(self, graph: SHACLGraph) -> str: lines = [self._prefix_decls(graph), ""] @@ -1189,11 +1209,11 @@ class SHACLGenerator: is_last = i == len(node_shape.property_shapes) - 1 terminator = " ." if is_last else " ;" parts = [" sh:property ["] - parts.append(f" sh:path {self._uri(graph, ps.path)} ;") + parts.append(f' sh:path {self._uri(graph, ps.path, "property")} ;') if ps.datatype: parts.append(f" sh:datatype {ps.datatype} ;") if ps.class_: - parts.append(f" sh:class {self._uri(graph, ps.class_)} ;") + parts.append(f' sh:class {self._uri(graph, ps.class_, "class")} ;') if ps.min_count is not None: parts.append(f" sh:minCount {ps.min_count} ;") if ps.max_count is not None: @@ -1234,7 +1254,7 @@ class SHACLGenerator: node: Dict[str, Any] = { "@id": shape_id, "@type": "sh:NodeShape", - "sh:targetClass": {"@id": f"{graph.base_uri}{node_shape.target_class}"}, + "sh:targetClass": {"@id": self._term_iri(graph, node_shape.target_class, "class")}, } if node_shape.name: node["sh:name"] = node_shape.name @@ -1247,7 +1267,7 @@ class SHACLGenerator: props = [] for ps in node_shape.property_shapes: p: Dict[str, Any] = { - "sh:path": {"@id": f"{graph.base_uri}{ps.path}"} + "sh:path": {"@id": self._term_iri(graph, ps.path, "property")} } if ps.datatype: dt = ps.datatype.replace( @@ -1255,7 +1275,7 @@ class SHACLGenerator: ) p["sh:datatype"] = {"@id": dt} if ps.class_: - p["sh:class"] = {"@id": f"{graph.base_uri}{ps.class_}"} + p["sh:class"] = {"@id": self._term_iri(graph, ps.class_, "class")} if ps.min_count is not None: p["sh:minCount"] = ps.min_count if ps.max_count is not None: @@ -1288,7 +1308,7 @@ class SHACLGenerator: for i, node_shape in enumerate(graph.node_shapes): shape_uri = f"<{graph.base_uri}{node_shape.target_class}Shape>" - class_uri = f"<{graph.base_uri}{node_shape.target_class}>" + class_uri = f'<{self._term_iri(graph, node_shape.target_class, "class")}>' t(shape_uri, f"<{RDF}type>", f"<{SHACL}NodeShape>") t(shape_uri, f"<{SHACL}targetClass>", class_uri) if node_shape.name: @@ -1303,13 +1323,13 @@ class SHACLGenerator: for j, ps in enumerate(node_shape.property_shapes): bnode = f"_:ps{i}_{j}" t(shape_uri, f"<{SHACL}property>", bnode) - prop_uri = f"<{graph.base_uri}{ps.path}>" + prop_uri = f'<{self._term_iri(graph, ps.path, "property")}>' t(bnode, f"<{SHACL}path>", prop_uri) if ps.datatype: dt_uri = ps.datatype.replace("xsd:", XSD) t(bnode, f"<{SHACL}datatype>", f"<{dt_uri}>") if ps.class_: - t(bnode, f"<{SHACL}class>", f"<{graph.base_uri}{ps.class_}>") + t(bnode, f"<{SHACL}class>", f'<{self._term_iri(graph, ps.class_, "class")}>') if ps.min_count is not None: t(bnode, f"<{SHACL}minCount>", f'"{ps.min_count}"^^<{XSD}integer>') if ps.max_count is not None: diff --git a/tests/ontology/test_shacl_target_namespace.py b/tests/ontology/test_shacl_target_namespace.py index 74449490..5799e86a 100644 --- a/tests/ontology/test_shacl_target_namespace.py +++ b/tests/ontology/test_shacl_target_namespace.py @@ -214,3 +214,117 @@ def test_domainless_attachment_is_available_as_an_explicit_opt_in(): if prop.path.endswith("sourceDocument") } assert len(carriers) == len(graph.node_shapes) + + +# ── Review findings on the first revision of this fix ──────────────────────── + +@pytest.mark.parametrize("fmt,parse_as", [ + ("turtle", "turtle"), ("n-triples", "nt"), ("json-ld", "json-ld"), +]) +def test_every_format_targets_the_ontology_namespace(fmt, parse_as): + """ + The first revision fixed Turtle alone. JSON-LD and N-Triples went on + pasting names onto the shapes namespace, so two of the three formats still + produced shapes that matched nothing. + """ + generator = SHACLGenerator() + graph = generator.generate(_ontology(declare_namespace=False, carry_class_uris=True)) + + shapes = Graph() + shapes.parse(data=generator.serialize(graph, fmt), format=parse_as) + targets = {str(o) for o in shapes.objects(None, SH.targetClass)} + + assert targets == {ONTOLOGY_NS + "Person", ONTOLOGY_NS + "Organization"}, targets + assert not any(t.startswith(SHAPES_NS) for t in targets), targets + + +@pytest.mark.parametrize("fmt,parse_as", [ + ("turtle", "turtle"), ("n-triples", "nt"), ("json-ld", "json-ld"), +]) +def test_every_format_reports_a_real_violation(fmt, parse_as): + generator = SHACLGenerator() + graph = generator.generate(_ontology(declare_namespace=False, carry_class_uris=True)) + + shapes = Graph() + shapes.parse(data=generator.serialize(graph, fmt), format=parse_as) + conforms, text = _validate(VIOLATING_DATA, shapes) + + assert not conforms, f"{fmt} shapes matched no focus nodes:\n{text}" + + +def test_a_property_sharing_a_class_name_keeps_its_own_iri(): + """One name-keyed map gave the property the class's IRI, so sh:path validated + the wrong predicate.""" + ontology = { + "classes": [{"name": "Account", "uri": ONTOLOGY_NS + "Account"}], + "properties": [ + { + "name": "Account", + "uri": ONTOLOGY_NS + "accountNumber", + "type": "datatype", + "range": "string", + "domain": "Account", + "required": True, + } + ], + } + shapes = _shapes_graph(ontology) + + paths = {str(o) for o in shapes.objects(None, SH.path)} + targets = {str(o) for o in shapes.objects(None, SH.targetClass)} + + assert paths == {ONTOLOGY_NS + "accountNumber"}, paths + assert targets == {ONTOLOGY_NS + "Account"}, targets + + +def test_sh_class_resolves_to_the_class_namespace(): + ontology = { + "classes": [ + {"name": "Person", "uri": ONTOLOGY_NS + "Person"}, + {"name": "Organization", "uri": ONTOLOGY_NS + "Organization"}, + ], + "properties": [ + { + "name": "worksAt", + "uri": ONTOLOGY_NS + "worksAt", + "type": "object", + "range": "Organization", + "domain": "Person", + } + ], + } + shapes = _shapes_graph(ontology) + classes = {str(o) for o in shapes.objects(None, SH["class"])} + + assert classes == {ONTOLOGY_NS + "Organization"}, classes + + +def test_the_engine_forwards_the_new_options(): + """to_shacl passed them through generate(**options), which never reads them.""" + from semantica.ontology.engine import OntologyEngine + + ontology = _ontology(declare_namespace=False, carry_class_uris=False) + + turtle = OntologyEngine().to_shacl(ontology, target_namespace="https://forwarded.example/ns#") + shapes = Graph() + shapes.parse(data=turtle, format="turtle") + targets = {str(o) for o in shapes.objects(None, SH.targetClass)} + assert all(t.startswith("https://forwarded.example/ns#") for t in targets), targets + + attached = OntologyEngine().to_shacl(ontology, attach_domainless_properties=True) + assert "sourceDocument" in attached + + default = OntologyEngine().to_shacl(ontology) + assert "sourceDocument" not in default + + +def test_the_opt_in_warns_rather_than_whispering(caplog): + import logging + + with caplog.at_level(logging.WARNING): + SHACLGenerator(attach_domainless_properties=True).generate( + _ontology(declare_namespace=True, carry_class_uris=True) + ) + + messages = " ".join(record.getMessage() for record in caplog.records) + assert "sourceDocument" in messages